Skip to content

Commit

Permalink
User: Fixes various issues in OIDC authentication provider
Browse files Browse the repository at this point in the history
TYPE: Bugfix
LINK: OGC-1767
  • Loading branch information
Daverball authored Dec 19, 2024
1 parent 7bff3ed commit bef4762
Show file tree
Hide file tree
Showing 8 changed files with 23 additions and 20 deletions.
4 changes: 2 additions & 2 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ repos:
exclude: .pre-commit-config.yaml
- id: pt_structure
- repo: https://github.com/astral-sh/ruff-pre-commit
rev: v0.8.3
rev: v0.8.4
hooks:
- id: ruff
args: [ "--fix" ]
Expand All @@ -32,7 +32,7 @@ repos:
- id: sass-lint
files: '^src/.*\.scss'
- repo: https://github.com/pre-commit/mirrors-eslint
rev: v9.16.0
rev: v9.17.0
hooks:
- id: eslint
files: '^src/.*\.jsx?$'
Expand Down
2 changes: 1 addition & 1 deletion src/onegov/core/csv.py
Original file line number Diff line number Diff line change
Expand Up @@ -755,7 +755,7 @@ def has_duplicates(a_list: 'Sequence[Any]') -> bool:

def list_duplicates_index(a: 'Sequence[Any]') -> list[int]:
"""
returns a list of indexes of duplicates in a list.
Returns a list of indexes of duplicates in a list.
for example::
a = [1, 2, 3, 2, 1, 5, 6, 5, 5, 5]
Expand Down
2 changes: 1 addition & 1 deletion src/onegov/election_day/path.py
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,7 @@ def get_data_source_item(

@ElectionDayApp.path(
model=ArchivedResultCollection,
path='/archive/{date}' # noqa: RUF027
path='/archive/{date}'
)
def get_archive_by_year(
app: ElectionDayApp,
Expand Down
4 changes: 2 additions & 2 deletions src/onegov/landsgemeinde/path.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ def get_assemblies(app: LandsgemeindeApp) -> AssemblyCollection:

@LandsgemeindeApp.path(
model=Assembly,
path='/landsgemeinde/{date}', # noqa: RUF027
path='/landsgemeinde/{date}',
converters={'date': extended_date_converter}
)
def get_assembly(app: LandsgemeindeApp, date: 'date') -> Assembly | None:
Expand All @@ -36,7 +36,7 @@ def get_assembly(app: LandsgemeindeApp, date: 'date') -> Assembly | None:

@LandsgemeindeApp.path(
model=AgendaItemCollection,
path='/traktanden/{date}', # noqa: RUF027
path='/traktanden/{date}',
converters={'date': extended_date_converter}
)
def get_agenda_items(
Expand Down
2 changes: 1 addition & 1 deletion src/onegov/org/cronjobs.py
Original file line number Diff line number Diff line change
Expand Up @@ -672,7 +672,7 @@ def send_monthly_mtan_statistics(request: 'OrgRequest') -> None:

@OrgApp.cronjob(hour=4, minute=0, timezone='Europe/Zurich')
def delete_content_marked_deletable(request: 'OrgRequest') -> None:
""" find all models inheriting from DeletableContentExtension, iterate
""" Find all models inheriting from DeletableContentExtension, iterate
over objects marked as `deletable` and delete them if expired.
Currently extended directory entries, news, events and occurrences.
Expand Down
2 changes: 1 addition & 1 deletion src/onegov/pay/models/payment_providers/stripe.py
Original file line number Diff line number Diff line change
Expand Up @@ -474,7 +474,7 @@ def payments(ids: 'Collection[UUID]') -> 'Query[StripePayment]':

def sync_payouts(self, session: 'Session') -> None:
"""
see https://stripe.com/docs/api/balance_transactions/list
See https://stripe.com/docs/api/balance_transactions/list
and https://stripe.com/docs/api/payouts
"""

Expand Down
11 changes: 7 additions & 4 deletions src/onegov/user/auth/clients/oidc.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import requests
from attr import attrs, attrib
from base64 import urlsafe_b64encode
from jwt import PyJWKClient, decode_complete
from jwt import PyJWKClient, decode_complete, get_algorithm_by_name
from jwt.exceptions import InvalidIssuerError, InvalidSignatureError
from oauthlib.oauth2.rfc6749.endpoints import AuthorizationEndpoint
from oauthlib.oauth2.rfc6749.endpoints import MetadataEndpoint
Expand Down Expand Up @@ -84,7 +84,9 @@ class OIDCClient:
def session(
self,
provider: 'OIDCProvider',
request: 'CoreRequest'
request: 'CoreRequest',
*,
with_openid_scope: bool = False,
) -> OAuth2Session:
""" Returns a requests session tied to a OAuth2 client """
assert isinstance(self.scope, list), 'Invalid scope, expected list'
Expand All @@ -93,7 +95,7 @@ def session(
provider_cls, {'name': provider.name}, name='redirect')
return OAuth2Session(
self.client_id,
scope=['openid', *self.scope],
scope=['openid', *self.scope] if with_openid_scope else self.scope,
redirect_uri=redirect_url,
)

Expand Down Expand Up @@ -180,7 +182,8 @@ def validate_token(

if access_token:
# validate the access_token using at_hash
digest = header['alg'].compute_hash_digest(access_token)
alg = get_algorithm_by_name(header['alg'])
digest = alg.compute_hash_digest(access_token.encode('utf-8'))
at_hash = urlsafe_b64encode(digest[:len(digest) // 2])
given_at_hash = payload.get('at_hash', '').encode('utf-8')
if not compare_digest(at_hash, given_at_hash):
Expand Down
16 changes: 8 additions & 8 deletions src/onegov/user/auth/provider.py
Original file line number Diff line number Diff line change
Expand Up @@ -1395,7 +1395,7 @@ def authenticate_request(
f'{app.application_id}: {error}')
return Failure(_('Authorisation failed due to an error'))

session = client.session(self, request)
session = client.session(self, request, with_openid_scope=True)
auth_url, _state = session.authorization_url(
metadata['authorization_endpoint'],
request.new_url_safe_token(
Expand Down Expand Up @@ -1490,14 +1490,14 @@ def request_authorisation(
username = payload.get(client.attributes.username, None)
first_name = payload.get(client.attributes.first_name, None)
last_name = payload.get(client.attributes.last_name, None)
group = payload.get(client.attributes.group, None)
groups = payload.get(client.attributes.group, None)
if first_name and last_name:
realname = f'{first_name} {last_name}'
else:
realname = payload.get(client.attributes.preferred_username, None)

# try to retrieve any missing claims from the userinfo endpoint
if not (source_id and username and group and realname) and (
if not (source_id and username and groups and realname) and (
user_url := metadata.get('userinfo_endpoint')
):
try:
Expand All @@ -1513,7 +1513,7 @@ def request_authorisation(
client.attributes.first_name, None)
last_name = last_name or claims.get(
client.attributes.last_name, None)
group = group or claims.get(client.attributes.group, None)
groups = groups or claims.get(client.attributes.group, None)

if realname:
# already set
Expand All @@ -1535,16 +1535,16 @@ def request_authorisation(
log.info(f'No source_id found for {username}')
return Failure(_('Authorisation failed due to an error'))

if not group:
log.info(f'No group found for {username}')
if not groups:
log.info(f'No groups found for {username}')
return Failure(_("Can't login because your user has no group. "
"Contact your OpenID system administrator"))

role = self.roles.match(roles, [group])
role = self.roles.match(roles, groups)

if not role:
log.info(f'No authorized group for {username}, '
f'having group: {group}')
f'having groups: {", ".join(groups)}')
return Failure(_('Authorisation failed due to an error'))

user = ensure_user(
Expand Down

0 comments on commit bef4762

Please sign in to comment.