Automated copy feature/OIDC oauth groups #1199
Conversation
remove trailing space Co-authored-by: Alexander VanTol <[email protected]>
...to make user creation more uniform, reflecting what is also done elsewhere in fence/sync/sync_users.py _upsert_userinfo() for example
* fix use real bucket name * fix * update comment * use regex * update version
Update documentation link in setup.md
Pull Request Test Coverage Report for Build 12416371639Details
💛 - Coveralls |
Please find the detailed integration test report here Please find the ci env pod logs here |
|
@Avantol13 -- Will you be able to give me access to the detailed integration test report? I get 401 when I click on the link. |
Please find the detailed integration test report here Please find the ci env pod logs here |
| @@ -27,6 +28,11 @@ def get_error_response(error): | |||
| ) | |||
| ) | |||
|
|
|||
| # TODO: Issue: Error messages are obfuscated, the line below needs be | |||
There was a problem hiding this comment.
Can we get this fixed before we merge?
| self.app = app | ||
|
|
||
| # This block of code probably need to be made more concise |
There was a problem hiding this comment.
Can we do this cleanup before we merge?
| @@ -68,7 +96,8 @@ async def update_tokens(self, db_session): | |||
|
|
|||
| """ | |||
| start_time = time.time() | |||
| self.logger.info("Initializing Visa Update Cronjob . . .") | |||
| # Change this line to reflect we are refreshing tokens, not just visas | |||
There was a problem hiding this comment.
can we make this change?
| # Initialize visa clients: | ||
| oidc = config.get("OPENID_CONNECT", {}) | ||
|
|
||
| if not isinstance(oidc, dict): |
There was a problem hiding this comment.
we try not to do type-checking explicitly, the Pythonic way is to treat things as they are expected to be and then error when it behaves unexpectedly.
Do you need this here? Can we move this to be a post-configuration check rather than runtime?
| @@ -72,18 +94,101 @@ def get_jwt_keys(self, jwks_uri): | |||
| return None | |||
| return resp.json()["keys"] | |||
|
|
|||
| def get_raw_token_claims(self, token_id): | |||
There was a problem hiding this comment.
if this is unused, we should remove it
| @@ -187,14 +315,15 @@ def get_access_token(self, user, token_endpoint, db_session=None): | |||
| """ | |||
| Get access_token using a refresh_token and store new refresh in upstream_refresh_token table. | |||
| """ | |||
| # this function is not correct. use self.session.fetch_access_token, | |||
There was a problem hiding this comment.
can you expand on this or fix the underlying issue? I'm not sure I'm following
There was a problem hiding this comment.
The function is not retrieving the expires_at from token_response. I'm thinking we should be storing the new expires retrieved from token_response.
| @@ -274,3 +403,150 @@ def store_refresh_token(self, user, refresh_token, expires, db_session=None): | |||
| current_db_session = db_session.object_session(upstream_refresh_token) | |||
| current_db_session.add(upstream_refresh_token) | |||
| db_session.commit() | |||
|
|
|||
| def get_groups_from_token(self, decoded_id_token, group_prefix=""): | |||
| """Retrieve and format groups from the decoded token.""" | |||
There was a problem hiding this comment.
need vars in the docstring
|
|
||
| def get_groups_from_token(self, decoded_id_token, group_prefix=""): | ||
| """Retrieve and format groups from the decoded token.""" | ||
| authz_groups_from_idp = decoded_id_token.get("groups", []) |
There was a problem hiding this comment.
See previous comment re: this being a configuration per IdP
| self.arborist.add_user_to_group( | ||
| username=user.username, | ||
| group_name=arborist_group["name"], | ||
| expires_at=exp, |
There was a problem hiding this comment.
The expiration here is the access_token expiration, was that intentional? Usually those are very short lived, like on the order of minutes, not hours. I wonder if we want to make the expiration configurable per IdP (so I could set it to be 8 hours for example) up to the maximum allowed in the refresh_token (which could be like 30 days?)?
There was a problem hiding this comment.
yeah, setting per IdP will be better
| expires_at=exp, | ||
| ) | ||
|
|
||
| # Remove user from groups in Arborist that they are not part of in IDP |
There was a problem hiding this comment.
Unfortunately this design will not work generally. IdP's are not the only source of user membership, so you can't remove users from groups that don't match simply by querying for all groups and diffing with what's in a token, b/c there are other, external sources of other group memberships (admin user.yamls, RAS Passports) that need to be able to maintain their own policies. This logic would wipe users from those groups.
There was a problem hiding this comment.
Either we rely on policy expiration to cleanup things, or you need a smarter comparison by knowing the list of groups managed by and IdP
duplicated from here: #1188
Link to JIRA ticket if there is one:
New Features
Breaking Changes
Bug Fixes
Improvements
Dependency updates
Deployment changes