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.

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

VADC-845 #1131

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

VADC-845 #1131

wants to merge 1 commit into from

Conversation

tianj7
Copy link
Contributor

@tianj7 tianj7 commented Dec 20, 2023

https://ctds-planx.atlassian.net/browse/VADC-845

New Features

Add ability to sync ADFS custom groups from Cognito Login

Copy link
Contributor

@pieterlukasse pieterlukasse left a comment

Choose a reason for hiding this comment

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

thanks @tianj7 ! I added some comments and questions. Please take a look.

email, groups, db_session=current_app.scoped_session()
)

super(CognitoCallback, self).post_login(id_from_idp=id_from_idp)
Copy link
Contributor

Choose a reason for hiding this comment

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

why not just super(CognitoCallback, self).post_login()?

@@ -16,3 +24,27 @@ def __init__(self):
super(CognitoCallback, self).__init__(
idp_name=IdentityProvider.cognito, client=flask.current_app.cognito_client
)

def post_login(self, user=None, token_result=None, id_from_idp=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

token_result is actually used in super.post_login(). Why is it not passed?

# for each group, assign current user the following resources:
# /cohort-middleware/{group}
# with both role_ids: 'cohort_middleware_admin' and 'cohort_middleware_outputs_admin_reader'
db_session = db_session or current_session
Copy link
Contributor

Choose a reason for hiding this comment

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

db_session is not part of this method's arguments

they are in on the ADFS server.
Args:
groups (list): list of groups to sync
db_session (flask_sqlalchemy_session.SQLAlchemySession): db session to use
Copy link
Contributor

Choose a reason for hiding this comment

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

db_session is not part of this method's arguments



def _sync_adfs_groups(gen3_user, groups, current_session, db_session=None):
db_session = db_session or current_session
Copy link
Contributor

Choose a reason for hiding this comment

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

does not seem to be used?

Args:
userinfo (dict): userinfo response
Return:
str: list of groups
Copy link
Contributor

Choose a reason for hiding this comment

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

is this a single string (e.g. a comma-separated list)? If yes, where is is split into a real list?

self.logger.info("Synchronizing arborist with authorization info...")
success = self._update_authz_in_arborist(
sess,
user_projects,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is for another feature in Gen3, not related to "team projects" and not reusable as such. Can you please check?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants