feat/generic-oidc-claims: add the ability to set email and id_from_idp from separate OIDC claims in the generic OIDC idp #1153
Conversation
fence/resources/openid/idp_oauth2.py
Outdated
| self.logger.exception( | ||
| f"Can't get {field} from claims: {claims}" | ||
| ) | ||
| return {"error": f"Can't get {field} from claims"} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
We should only throw an error if the fields required by the OIDC spec are missing. I believe this is already covered by Fence so there should be no new errors that are thrown unless there's specific configuration that requires email verification (if I understand this PR correctly).
fence/resources/openid/idp_oauth2.py
Outdated
| return {"error": f"Can't get {field} from claims"} | ||
|
|
||
| # 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")): |
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
You should be able to mock the response of get_jwt_claims_identity, so its not reliant on code. Either way, this is a pretty critical part of our authentication service so tests that ensure that this change doesn't break the default case and works as expected in your case are valuable.
fence/resources/openid/idp_oauth2.py
Outdated
| return {"error": f"Can't get {field} from claims"} | ||
|
|
||
| # 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")): |
There was a problem hiding this comment.
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.
Co-authored-by: burtonk <[email protected]>
| email_field: '' # optional (default "email"); claims field to get the user email from | ||
| # If your IDP doesn't support the email_verified claim, set this to True to | ||
| # accept the email address claim anyway | ||
| assume_emails_verified: False |
There was a problem hiding this comment.
required_email_verified makes more sense here. It should default to false so we don't throw this new requirement unexpectedly on existing fence deployments and disrupt login.
There was a problem hiding this comment.
I reused the existing setting from the cognito code. Happy to refactor this if you'd like.
Also, see #1153 (comment)
| if claims.get(field): | ||
|
|
||
| # 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")): |
There was a problem hiding this comment.
We should, be default, assume that email verification isn't a requirement. There is not a requirement in the OAuth2 or OIDC spec that requires email verification so this will break standards in our authentication service.
There was a problem hiding this comment.
The original code would always return an error in the case that the email_verified claim wasn't present, and didn't give you a way to disable that, so I haven't changed the default behaviour here.
Again, happy to tweak that if desired.
fence/resources/openid/idp_oauth2.py
Outdated
| self.logger.exception( | ||
| f"Can't get {field} from claims: {claims}" | ||
| ) | ||
| return {"error": f"Can't get {field} from claims"} |
There was a problem hiding this comment.
We should only throw an error if the fields required by the OIDC spec are missing. I believe this is already covered by Fence so there should be no new errors that are thrown unless there's specific configuration that requires email verification (if I understand this PR correctly).
fence/resources/openid/idp_oauth2.py
Outdated
| return {"error": f"Can't get {field} from claims"} | ||
|
|
||
| # 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")): |
There was a problem hiding this comment.
You should be able to mock the response of get_jwt_claims_identity, so its not reliant on code. Either way, this is a pretty critical part of our authentication service so tests that ensure that this change doesn't break the default case and works as expected in your case are valuable.
Link to JIRA ticket if there is one:
New Features
Allow returning email and id_from_idp claims from the generic OIDC get_auth_info() code, as well as the user_id.
Breaking Changes
Bug Fixes
Improvements
Dependency updates
Deployment changes
Notes:
This is the first time I've attempted to contribute to gen3, so I may well be doing it wrong!
I have tested this locally using the dex idp https://github.com/dexidp/dex as the OIDC server