Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat/generic-oidc-claims: add the ability to set email and id_from_idp from separate OIDC claims in the generic OIDC idp #1153

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

feat/generic-oidc-claims: add the ability to set email and id_from_idp from separate OIDC claims in the generic OIDC idp #1153

Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e7ee66f
generic oidc: add initial support for user_email_field and id_from_id…
davidmonro Jun 26, 2024
e106706
generic oidc: refactor extracting claims to avoid duplication
davidmonro Jun 27, 2024
e8043d6
remove unnecessary use of set()
davidmonro Oct 3, 2024
7607287
Rename return_dict to something more descriptive
davidmonro Oct 3, 2024
80485a9
Merge branch 'master' into feat/generic-oidc-claims
davidmonro Oct 3, 2024
b4ae1a0
generic oidc: don't return an error if the field is missing from the …
davidmonro Oct 3, 2024
7951b84
generic oidc: update config-default.yaml with new options
davidmonro Oct 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 30 additions & 11 deletions fence/resources/openid/idp_oauth2.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,23 +161,42 @@ def get_auth_info(self, code):
user OR "error" field with details of the error.
"""
user_id_field = self.settings.get("user_id_field", "sub")
user_email_field = self.settings.get("user_email_field", "email")
id_from_idp_field = self.settings.get("id_from_idp_field", "sub")
try:
token_endpoint = self.get_value_from_discovery_doc("token_endpoint", "")
jwks_endpoint = self.get_value_from_discovery_doc("jwks_uri", "")
claims = self.get_jwt_claims_identity(token_endpoint, jwks_endpoint, code)

if claims.get(user_id_field):
if user_id_field == "email" and not claims.get("email_verified"):
fields = set([user_id_field, user_email_field, id_from_idp_field])
davidmonro marked this conversation as resolved.
Show resolved Hide resolved

return_dict = {}
davidmonro marked this conversation as resolved.
Show resolved Hide resolved

for field in fields:
# Field missing from claims
if not claims.get(field):
self.logger.exception(
f"Can't get {field} from claims: {claims}"
)
return {"error": f"Can't get {field} from claims"}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't return an error if the email field is missing. The claims come from id_token. Throwing an error because we get an id_token would effectively require the email claim in the id_token. This would take fence out of spec with OIDC standard. See here https://openid.net/specs/openid-connect-core-1_0.html#IDToken

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reworked this, but the new version doesn't return an error if all the fields are missing, which probably isn't right either. I'm not sure what the right answer is - should I check if the requested fields are mandatory, or throw an error if the field requested for user_id_field doesn't exist but ignore the others?


# Field is email, but isn't verified and we aren't assuming all emails are verified
if field == "email" and not (claims.get("email_verified") or self.settings.get("assume_emails_verified")):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test_idp_oauth2 need to be updated to test the branch conditions here to ensure that the error is returned as expected depending on the use of "assume_emails_verified"

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This turns out to be rather difficult, at least with the code as it is now, because get_auth_info() takes code as an argument that it wants to extract the claims from, and having done some experiments I'm not sure the mock idp can handle that?

I could refactor the claims parsing out to another function that would then be more easily testable if you'd like.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assumed_email_verified needs to be added to config-default.yaml under OPENID_CONNECT with documentation explaining its usage for the respective IDPs of interest.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documented in 7951b84

return {"error": "Email is not verified"}
return {
user_id_field: claims[user_id_field],
"mfa": self.has_mfa_claim(claims),
}
else:
self.logger.exception(
f"Can't get {user_id_field} from claims: {claims}"
)
return {"error": f"Can't get {user_id_field} from claims"}

# We got the field, so append it to our dictionary
return_dict[field] = claims[field]

# Append the mfa field
return_dict["mfa"] = self.has_mfa_claim(claims)

# Debug
self.logger.debug(
f"Oauth2ClientBase get_auth_info returning {return_dict}"
)

# Return what we have assembled
return return_dict

except Exception as e:
self.logger.exception(f"Can't get user info from {self.idp}: {e}")
Expand Down
Loading