Fix #3624: fix manager permission within groups #3754

matlink · 2023-08-04T19:27:54Z

Fix #3624 by checking if the user is member of a group that has access to the collection.

stefan0xC · 2023-08-04T22:30:15Z

What if a Manager is member of a group with access to all collections?

matlink · 2023-08-05T09:56:16Z

What if a Manager is member of a group with access to all collections?

you are right, we lack something since in this case the manager cannot edit the collection. I'm looking into it, thanks =)

matlink · 2023-08-05T10:22:17Z

@stefan0xC This d282b32 should do the job.

farodin91 · 2023-09-08T07:53:35Z

Any progress?

BlackDex · 2023-09-08T08:02:12Z

It has to be checked first. Did not had time yet to validate. Working on other issues when i have the time.

stefan0xC

Sorry, it took me a bit but I've finally had time to review your changes.

I think the comments and the naming of some variables/functions could be improved to make it more obvious why something is done so the changes are easier to review. I've added some suggestions for your changes but this probably applies to other variables and comments as well.

With the renamed variables I think it is easier to see how your approach could be improved upon. I.e. instead of returning the two lists of member ids and later checking if they contain the uuid of the current user, we really only care if user_org has full access via a group or if not, if they have access to a given collection via a group. So we could make two functions that check

if the given user_org has full access to an organization (either directly or via a group)
if the given user_org is a member of a group assigned to the given collection.

edit: also we might need to add similar changes to get_org_collection_details

src/api/core/organizations.rs

src/db/models/group.rs

src/api/core/organizations.rs

BlackDex · 2023-12-31T14:43:44Z

@matlink, could you rebase and address the items mentioned by @stefan0xC?
Looks ok for the rest of it i think.

matlink · 2023-12-31T15:25:10Z

@BlackDex will do ;)

matlink · 2024-01-01T19:36:49Z

@BlackDex @stefan0xC I've made the changes you requested and rebased. Needs further testing ;)

src/api/core/organizations.rs

stefan0xC

@BlackDex Can you review/test the changes? 🙈

src/api/core/organizations.rs

BlackDex · 2024-01-27T22:49:36Z

And, it needs a rebase of course.
I tested it btw, including my suggestion, that seems to work fine.

fthobe · 2024-02-15T19:57:01Z

Hey Guys, I don't want to put pressure on you people, but do you think we can nudge this one over the finish line? I would literally send you pizza :))))

matlink · 2024-02-16T05:42:13Z

So I just need to rebase?

BlackDex · 2024-02-16T06:00:19Z

So I just need to rebase?

@matlink no see my request.
And a rebase too of course.

…llections

Co-authored-by: Stefan Melmuk <[email protected]>

instead of returning the two lists of member ids and later checking if they contain the uuid of the current user, we really only care if the current user has full access via a group or if they have access to a given collection via a group

Co-authored-by: Mathijs van Veluw <[email protected]>

matlink · 2024-02-17T07:25:38Z

@BlackDex I've applied you suggestion.
it looks like the rebase went well, no conflict.
Keep me in touch if needed.
Cheers,

BlackDex · 2024-02-17T07:30:03Z

@BlackDex I've applied you suggestion. it looks like the rebase went well, no conflict. Keep me in touch if needed. Cheers,

@matlink well, a small item. The code isn't formatted correctly.
Run a cargo fmt and that should be fixed.

matlink · 2024-02-17T12:52:58Z

@matlink well, a small item. The code isn't formatted correctly. Run a cargo fmt and that should be fixed.

Done.

fthobe · 2024-02-27T14:29:16Z

Hey, can this be merged now? The issue seems to be resolved to everybody's satisfaction. If testing is needed I would volunteer.

…ia#3754) * Fix dani-garcia#3624: fix manager permission within groups * Query returns UUID only * Fix issue when user is manager and in a group having access to all collections * optimize condition check * fix(groups): renaming and optimizations * fix: wrong organization group membership detection * Simplify group membership check Co-authored-by: Stefan Melmuk <[email protected]> * Remove unused statement * improve check if the user has access via groups instead of returning the two lists of member ids and later checking if they contain the uuid of the current user, we really only care if the current user has full access via a group or if they have access to a given collection via a group * improve comments for get_org_collections_details * small refactor to make it easier to review * fix(groups): query full access via group only when necessary Co-authored-by: Mathijs van Veluw <[email protected]> * chore(fmt): apply rustfmt --------- Co-authored-by: Stefan Melmuk <[email protected]> Co-authored-by: Stefan Melmuk <[email protected]> Co-authored-by: Mathijs van Veluw <[email protected]>

matlink force-pushed the 3624_manager_groups branch 3 times, most recently from e7beb25 to 7a1458e Compare August 4, 2023 20:29

stefan0xC suggested changes Oct 25, 2023

View reviewed changes

matlink force-pushed the 3624_manager_groups branch from 8311f45 to 50048a9 Compare January 1, 2024 19:36

stefan0xC reviewed Jan 1, 2024

View reviewed changes

src/api/core/organizations.rs Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

stefan0xC approved these changes Jan 6, 2024

View reviewed changes

stefan0xC mentioned this pull request Jan 22, 2024

prevent side effects if groups are disabled #4265

Merged

BlackDex requested changes Jan 27, 2024

View reviewed changes

src/api/core/organizations.rs Outdated Show resolved Hide resolved

matlink and others added 7 commits February 17, 2024 08:23

Fix dani-garcia#3624: fix manager permission within groups

bb100df

Query returns UUID only

f4e9857

Fix issue when user is manager and in a group having access to all co…

67732fe

…llections

optimize condition check

a477a6f

fix(groups): renaming and optimizations

704d9bd

fix: wrong organization group membership detection

7bc45a0

Simplify group membership check

7c93866

Co-authored-by: Stefan Melmuk <[email protected]>

matlink and others added 5 commits February 17, 2024 08:23

Remove unused statement

a41125d

improve check if the user has access via groups

e858b96

instead of returning the two lists of member ids and later checking if they contain the uuid of the current user, we really only care if the current user has full access via a group or if they have access to a given collection via a group

improve comments for get_org_collections_details

92d2df9

small refactor to make it easier to review

e314c4e

fix(groups): query full access via group only when necessary

4d9ff9a

Co-authored-by: Mathijs van Veluw <[email protected]>

matlink force-pushed the 3624_manager_groups branch from 0570532 to 4d9ff9a Compare February 17, 2024 07:23

chore(fmt): apply rustfmt

ef6154c

BlackDex approved these changes Feb 17, 2024

View reviewed changes

dani-garcia merged commit 7c3cad1 into dani-garcia:main Mar 17, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #3624: fix manager permission within groups #3754

Fix #3624: fix manager permission within groups #3754

matlink commented Aug 4, 2023

stefan0xC commented Aug 4, 2023

matlink commented Aug 5, 2023 •

edited

Loading

matlink commented Aug 5, 2023

farodin91 commented Sep 8, 2023

BlackDex commented Sep 8, 2023

stefan0xC left a comment •

edited

Loading

BlackDex commented Dec 31, 2023

matlink commented Dec 31, 2023

matlink commented Jan 1, 2024

This comment was marked as resolved.

This comment was marked as resolved.

stefan0xC left a comment

BlackDex commented Jan 27, 2024

fthobe commented Feb 15, 2024

matlink commented Feb 16, 2024

BlackDex commented Feb 16, 2024

matlink commented Feb 17, 2024

BlackDex commented Feb 17, 2024

matlink commented Feb 17, 2024

fthobe commented Feb 27, 2024

Fix #3624: fix manager permission within groups #3754

Fix #3624: fix manager permission within groups #3754

Conversation

matlink commented Aug 4, 2023

stefan0xC commented Aug 4, 2023

matlink commented Aug 5, 2023 • edited Loading

matlink commented Aug 5, 2023

farodin91 commented Sep 8, 2023

BlackDex commented Sep 8, 2023

stefan0xC left a comment • edited Loading

Choose a reason for hiding this comment

BlackDex commented Dec 31, 2023

matlink commented Dec 31, 2023

matlink commented Jan 1, 2024

This comment was marked as resolved.

This comment was marked as resolved.

stefan0xC left a comment

Choose a reason for hiding this comment

BlackDex commented Jan 27, 2024

fthobe commented Feb 15, 2024

matlink commented Feb 16, 2024

BlackDex commented Feb 16, 2024

matlink commented Feb 17, 2024

BlackDex commented Feb 17, 2024

matlink commented Feb 17, 2024

fthobe commented Feb 27, 2024

matlink commented Aug 5, 2023 •

edited

Loading

stefan0xC left a comment •

edited

Loading