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

Add http endpoint to list permissions #5571

Merged
merged 7 commits into from
Feb 15, 2023
Merged

Conversation

butonic
Copy link
Member

@butonic butonic commented Feb 14, 2023

This PR adds an http endpoint to list permission names.

Can be tested with curl:

curl -X POST 'https://cloud.ocis.test/api/v0/settings/permissions-list' -u admin:admin -k --data '{"account_uuid":"some-admin-user-id-0000-000000000000"}' | jq

will return a 200 Ok:

{
  "permissions": [
    "create-space",
    "delete-all-spaces",
    "settings-management",
    "account-management",
    "group-management",
    "set-space-quota",
    "change-logo",
    "role-management",
    "language-readwrite",
    "list-all-spaces",
    "delete-all-home-spaces"
  ]
}
curl -X POST 'https://cloud.ocis.test/api/v0/settings/permissions-list' -u marie:radioactivity -k --data '{"account_uuid":"f7fbf8c8-139b-4376-b307-cf0a8c2d0d9c"}' | jq

will return a 200 Ok like:

{
  "permissions": [
    "language-readwrite",
    "self-management",
    "create-space"
  ]
}

Trying to look up the permissons of a user other than the currently logged in user:

curl -X POST 'https://cloud.ocis.test/api/v0/settings/permissions-list' -u marie:radioactivity -k --data '{"account_uuid":"some-admin-user-id-0000-000000000000"}' | jq

will give a 404 with json body

{
  "code": 404,
  "detail": "user not found: some-admin-user-id-0000-000000000000",
  "status": "Not Found"
}

even for administrators, as I want to keep this minimal.

Now, we could polish this, eg:

  • return a parseable json error ... but none of the settings apis seem to do that anyway ...
  • allow admins to look up othe users permissions, but currently there is no reason to do that not needed
  • return permissions as {"permissionid": "name"} map, but I think that does not any value, especially since permissions might be added dynamically not needed
  • extract the handler into the permissions package
  • tests?

Good so far?

@update-docs
Copy link

update-docs bot commented Feb 14, 2023

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 marked this pull request as draft February 14, 2023 11:23
@butonic butonic requested review from C0rby and removed request for kulmann February 14, 2023 11:23
@butonic
Copy link
Member Author

butonic commented Feb 14, 2023

we agreed to implement listing permissions instead ...

@butonic butonic force-pushed the http-permissions-check branch 2 times, most recently from 8aa81eb to 25fc362 Compare February 14, 2023 15:59
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
@butonic butonic changed the title Add http endpoint to check permissions Add http endpoint to list permissions Feb 14, 2023
@kulmann
Copy link
Member

kulmann commented Feb 14, 2023

{
  "permissions": [
    "create-space",
    "delete-all-spaces",
    "settings-management",
    "account-management",
    "group-management",
    "set-space-quota",
    "change-logo",
    "role-management",
    "language-readwrite",
    "list-all-spaces",
    "delete-all-home-spaces"
  ]
}

Perfect! That's exactly what we need for web.

Regarding some of your optional todos:

  • allow admins to look up othe users permissions, but currently there is no reason to do that

Agree to not do this. Web ui needs the role in the admin settings app. Permissions are only needed for the authenticated user to do permission checks. No need to do more at the moment.

  • return permissions as {"permissionid": "name"} map, but I think that does not any value, especially since permissions might be added dynamically

Permission ids are not relevant for web. Checking permissions by name is sufficient and also provides better developer experience (code readability wise).

Good so far?

YES! Thank you! ❤️

cc @JammingBen

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
@butonic butonic marked this pull request as ready for review February 15, 2023 11:41
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
@sonarcloud
Copy link

sonarcloud bot commented Feb 15, 2023

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 6 Code Smells

66.7% 66.7% Coverage
0.8% 0.8% Duplication

@butonic butonic merged commit 92923f6 into master Feb 15, 2023
@delete-merged-branch delete-merged-branch bot deleted the http-permissions-check branch February 15, 2023 13:24
ownclouders pushed a commit that referenced this pull request Feb 15, 2023
Author: Jörn Friedrich Dreyer <jfd@owncloud.com>
Date:   Wed Feb 15 14:24:19 2023 +0100

    Add http endpoint to list permissions (#5571)

    * Add http endpoint to list permissions

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

    * extract handler registration

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

    * use generated protobuf

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

    * update permissions mock in graph service

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

    * add unit test

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

    * return correct userid

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

    * assert error message type in tests

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

    ---------

    Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
ownclouders pushed a commit that referenced this pull request Feb 16, 2023
Author: Jörn Friedrich Dreyer <jfd@owncloud.com>
Date:   Wed Feb 15 14:24:19 2023 +0100

    Add http endpoint to list permissions (#5571)

    * Add http endpoint to list permissions

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

    * extract handler registration

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

    * use generated protobuf

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

    * update permissions mock in graph service

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

    * add unit test

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

    * return correct userid

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

    * assert error message type in tests

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

    ---------

    Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
@micbar micbar mentioned this pull request May 3, 2023
89 tasks
@ScharfViktor ScharfViktor mentioned this pull request May 4, 2023
86 tasks
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