-
Notifications
You must be signed in to change notification settings - Fork 52
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
fix: overwriting what projects users have been assigned every time you login via oauth2 #1583
base: main
Are you sure you want to change the base?
Conversation
…u login via oauth2
…are currently assigned to the current user.
Hello @neoReuters First, thanks for your PR. It took us some time to understand why this was done like it is. So to give you some context:
Now if I understand your need:
(for reference see also the refactor made after the previous PR #1395 ) Is that correct? |
so would this story fix your issue? |
Hi @bamthomas - your assumptions are correct. Just for further brevity (not sure that it matters); we are using To my understanding now after you've broken it down - the way the system works; it expects the list of projects to essentially be part of the identity provider's responsibility. So, in a sense, we must send through something like As a solution then, I guess it would be helpful if we could have a custom key, kind of like what we did with The current solution proposed assumed that it was Datashare's responsibility to map projects to users hence why we're adjusting this inside the |
Ah just replied before seeing your reply, I guess something like this would work fine mostly. That means essentially the identity provider, in our case Cognito, is in charge of enriching the token with projects that user is responsible for, correct? Funnily enough, before quite recently it was quite tough to do this, but AWS have finally provided a way to enrich tokens with a Lambda trigger (https://aws.amazon.com/about-aws/whats-new/2023/12/amazon-cognito-user-pools-customize-access-tokens/). So I can imagine it working something like this - we have something like a DynamoDB or even a relational database which holds the projects the users are mapped to. Then we fire the lambda to go and check and enrich the token with this user's projects. So just some more observations. This works well but would also be interested in seeing whether you can also possibly provide a solution similar to the code this PR has, for people who might not have the ability to manipulate their oauth tokens? Thoughts @bamthomas? |
sorry for the late answer.
Personally I'd avoid hacking the goups by adding them each time the connection is made on the Identity Provider. It introduces side effects, that is hard to follow and understand afterward (why this user has this group BTW?). To me, either you can't configure projects in the Identity Provider and you can store them in the redis/postgres database as a project ACL (datashare should not touch the information stored that is going to be changed with #1585), either you can do it in your Identity Provider and then datashare should be configurable to pick the list in a given field. Finally if we want to use the information as a cache, then it should be all overridden at each login to have fresh information. The principle is that the information is coming from one source, and it is not modified in the login/caching process. |
Overview
This pull request addresses an issue where logging in via the OAuth2 filter would overwrite the projects a user is assigned to, replacing them with the default project. This behavior was particularly undesirable for users with multiple project assignments, as it caused them to lose their existing projects. The fix ensures that existing projects are preserved, and the default project is only appended if it is not already present in the datashare list.
Changes
Motivation
The issue was identified when logging in via the OAuth2 filter, where the user’s assigned projects were being overwritten with the default project. This was especially problematic for users with multiple project assignments, as they would lose their previously assigned projects each time they logged in. This fix provides a more flexible and reliable approach, ensuring that projects are appended rather than overwritten, preserving the existing assignments.
Impact
These changes are backward compatible with existing configurations and will not alter the current behavior unless there is an issue with project overwrites. It improves the system’s data integrity, ensuring that multiple projects can be tracked without losing existing assignments.
Request for Feedback
I would appreciate feedback on the overall approach for preserving and appending to datashare. Specifically, I’d welcome any suggestions for further optimizations or concerns about edge cases where existing project data could still be affected.