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

Make permission and role ids unique #5051

Merged
merged 2 commits into from
Nov 15, 2022

Conversation

butonic
Copy link
Member

@butonic butonic commented Nov 14, 2022

We've fixed the duplicate assignment of 79e13b30-3e22-11eb-bc51-0b9f0bad9a58 for both: create-space and settings-management permissions. We also deduplicate role ids after listing them internally to make permission retrieval more efficient.

fixes #5033

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
@update-docs
Copy link

update-docs bot commented Nov 14, 2022

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@butonic butonic self-assigned this Nov 14, 2022
@butonic butonic added the bug label Nov 14, 2022
@butonic butonic added this to the 2.0.0 General Availability milestone Nov 14, 2022
@sonarcloud
Copy link

sonarcloud bot commented Nov 14, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@butonic butonic requested a review from rhafer November 14, 2022 12:14
@micbar
Copy link
Contributor

micbar commented Nov 15, 2022

@rhafer this may need some upgrade considerations

@rhafer
Copy link
Contributor

rhafer commented Nov 15, 2022

@rhafer this may need some upgrade considerations

Yes. We'd need to update the roles that get persisted upon startup of the service accordingly. What makes things even worse is that we're still suffering from: #3432 (which mean the default assignements are duplicated with every restart of ocis 😿 )

@rhafer
Copy link
Contributor

rhafer commented Nov 15, 2022

Yes. We'd need to update the roles that get persisted upon startup of the service accordingly. What makes things even worse is that we're still suffering from: #3432 (which mean the default assignements are duplicated with every restart of ocis crying_cat_face )

Hm, thinking again about this, we might actually be lucky. The default roles will be re-created with the new permissions at startup of the settings service. So I think we won't need a migration.

And #3432, while ugly, is unrelated to this.

@@ -563,9 +563,14 @@ func (g Service) hasStaticPermission(ctx context.Context, permissionID string) b
return false
}

roleIDs = make([]string, 0, len(assignments))
// deduplicate roleids
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this deduplication is mainly needed because of #3432 ?

I am fine merging this. But it seems to paper over a couple of issues:

  1. AFAIK for now we wanted to have a restriction so that every user can only have a single role assigned. This does not seem to be enforced anywhere.
  2. It is possible to create the same assignment multiple times. The server will happily create that over and over again.

@butonic butonic merged commit 7443a9a into master Nov 15, 2022
@delete-merged-branch delete-merged-branch bot deleted the make-permission-and-role-ids-unique branch November 15, 2022 15:51
ownclouders pushed a commit that referenced this pull request Nov 15, 2022
Author: Jörn Friedrich Dreyer <jfd@owncloud.com>
Date:   Tue Nov 15 16:51:49 2022 +0100

    Make permission and role ids unique (#5051)

    * make permission ids unique

    Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

    * deduplicate roleids after listing permissions

    Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

    Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Settings service normal user can edit existing bundles
3 participants