Skip to content

Conversation

@max-zilla
Copy link
Contributor

Fixes a situation where a group could have multiple roles on a dataset.

@max-zilla max-zilla requested review from longshuicy and tcnichol April 5, 2023 16:35
@tcnichol
Copy link
Contributor

tcnichol commented Apr 5, 2023

I merged main in so it would be easier to test.
Something I noticed (after merge, so the merge might have caused this)

  1. I added Group1 to a dataset as a viewer.
  2. I see a new entry in authorization where the group_id is the id for that group.
  3. I use 'add group' to add Group1 as an uploader.
  4. In the db, I see a new authorization entry with that group_id and the role 'uploader.'
  5. The old entry was still there, but the group_ids were empty (so the permission has no effect.)

@tcnichol
Copy link
Contributor

tcnichol commented Apr 5, 2023

Max, I did the following:

  1. Added Group1 to a dataset as 'viewer.'
  2. I see a new authorization object in the db with authorization 'viewer'.
  3. Then I did 'share with group' and added Group1 as an 'uploader.'
  4. I see a new entry for that dataset and group with 'uploader', but the old one that has 'viewer' is still there, but just with no group_ids or user_ids attached.

Not sure if this was happening before or if I accidentally messed something up when I merged main.

@max-zilla
Copy link
Contributor Author

that is intended behavior - it's ok for those lists to be empty, they aren't created until they are needed but they will be re-used if any user or group gets that role in the future.

Copy link
Contributor

@tcnichol tcnichol left a comment

Choose a reason for hiding this comment

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

Tested and will mark approved. Works as intended.

@max-zilla max-zilla merged commit 6b3f51a into main Apr 5, 2023
@max-zilla max-zilla deleted the auth-bugfixes branch April 5, 2023 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants