Repository navigation
Optionally fetch userinfo from OIDC IdP - #1358
Jasper51297 wants to merge 4 commits into
Conversation
|
Good afternoon, are you able to address the three failing pytests? |
|
Sorry, did not get notified some tests were failing. This should have been resolved now. |
|
Thank you for the update! |
| raise Exception(e) | ||
|
|
||
| if claims.get(user_id_field): | ||
| if user_id_field == "email" and not claims.get("email_verified"): |
There was a problem hiding this comment.
You cannot remove this check, it is required when the email field is found and the verified field is found. Google does this to protect against unverified emails
| return {"error": "Email is not verified"} | ||
| user_id_value = self._get_claim_value(user_id_field, claims, userinfo) | ||
|
|
||
| if user_id_value: |
There was a problem hiding this comment.
why is this whole return gated behind finding that user_id_value now? Before I believe it just prompted the additional email check?
| userinfo_endpoint = self.get_value_from_discovery_doc("userinfo_endpoint", "") | ||
| if userinfo_endpoint and raw_access_token: | ||
| header = {"Authorization": "Bearer " + raw_access_token} | ||
| res = requests.get(userinfo_endpoint, headers=header, proxies=self.get_proxies()) |
There was a problem hiding this comment.
I would recommend adding exponential backoff and error handling explicitly since this is an external call.
| return res.json() | ||
| else: | ||
| self.logger.error("Unable to get userinfo: status_code: {}, message: {}".format(res.status_code, res.text)) | ||
| return {} |
There was a problem hiding this comment.
failure case return pattern should be avoided. Reverse this logic, fail fast on errors and the default return should be the positive case
There was a problem hiding this comment.
We need new unit tests to test the new functionality. Change the config and test that what you added achieves the intended results
| "firstname": claims.get(firstname_claim_field), | ||
| "lastname": claims.get(lastname_claim_field), | ||
| "email": claims.get(email_claim_field), | ||
| organization_claim_field: self._get_claim_value(organization_claim_field, claims, userinfo), |
There was a problem hiding this comment.
This could cause implicit overriding of dictionary fields and confuses purpose. organization_claim_field is intended to be the field within the IdP claims to get the organization information from, we are standardizing the location in Gen3's response to "org". Now you are mirroring and duplicating the parsing if the "org" field is in fact "org", e.g. this conflicts with the line explicitly standardizing on "org" below.
Can you please explain more context around why this is required? Can you not simply provide this existing configuration to get the org information from the right claim and then parse the "org" from the "org" field?
3 participants
This pull request enables Fence to propagate specific attributes from upstream OIDC Identity Providers (IdPs) into the ID tokens issued to clients. Previously, certain attributes returned by external providers during the OIDC flow were captured by Fence but not made accessible to the downstream applications.
This change introduces a configurable mapping that allows administrators to specify which fields from the upstream
userinforesponse should be included as claims in the Fence-generated ID token.https://openid.net/specs/openid-connect-core-1_0.html#UserInfo
Minimal Fence (helm) example:
New Features
idp_attr_mappingconfiguration toOPENID_CONNECT.Breaking Changes
None
Bug Fixes
None
Improvements
userinfo.Dependency updates
None
Deployment changes
None