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

fix: user permission is now checked for api key authorizer #3875

Merged
merged 8 commits into from
Mar 17, 2020
Merged

fix: user permission is now checked for api key authorizer #3875

merged 8 commits into from
Mar 17, 2020

Conversation

webong
Copy link
Contributor

@webong webong commented Mar 9, 2020

This pull request makes the following changes:

  • Adds ACL Manager Settings permission check on users
    References Can't access General Settings with "Manage Settings" permissions #3261
    Test checklist:
    With an administrator user

  • Create a role with the Manage Settings permission only

  • Assign the role to a user
    With a user who has the new role assigned

  • login with that user, access the settings page (you should see this page instead of access denied)

  • access the general settings

  • change a general setting - site name and regenerate API keys

  • save

  • the settings should save

  • log out

  • the site name should change to what you assigned to it (check the browser tab and the map view)

  • You can check the API key changed in the database, too, or simply reload the settings admin page to see it

  • I certify that I ran my checklist

Fixes ushahidi/platform# .

Ping @ushahidi/platform

@ushbot
Copy link
Collaborator

ushbot commented Mar 9, 2020

Hey @webong,
thank you for your Pull Request.

@rjmackay It looks like this brave person signed our Contributor License Agreement. 👍

Always at your service,

clabot

@coveralls
Copy link

coveralls commented Mar 9, 2020

Coverage Status

Coverage decreased (-0.004%) to 20.495% when pulling 54358be on webong:fix-general-settings-access-denied-for-manage-settings-permission into b8e701d on ushahidi:develop.

@Angamanga
Copy link
Contributor

@rowasc @tuxpiper Could one of you review please?

@rowasc
Copy link
Contributor

rowasc commented Mar 10, 2020

Reviewing and testing @Angamanga

@rowasc
Copy link
Contributor

rowasc commented Mar 10, 2020

@webong this looks good and seems to work just fine, thank you so much 🎉
Just an extra request, if you're willing: would you be able to add an integration test for it?
Roughly would involve

  • creating a new role with the "Manage Settings" permission in tests/datasets/ushahidi/Base.yml
  • create a user specifically for this role in the same file (users key)
  • assign the user with that role in the same file (roles_permissions key in Base.yml)
  • checking that the user has access to apikeys and other general settings (the acl.feature file may offer some guidance on this workflow)
    The api.apikeys.feature file in the integration tests may be another place to add a role/permission check if you'd like too. \

Also: I added some test steps to the description so that we can easily ask @Obadha2 to give us a hand and validate when we merge :) . @Obadha2 please make sure we add this to our regression suite/verify it's in there 💯

@webong
Copy link
Contributor Author

webong commented Mar 11, 2020

@rowasc thanks for the review ... my latest push now implements an integration test.

Copy link
Contributor

@rowasc rowasc left a comment

Choose a reason for hiding this comment

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

thanks @webong , looks great . I'm going to hold on the merge for a bit due to some m other prep we're doing, but this should be considered approved if the tests pass.

@Angamanga
Copy link
Contributor

@rowasc is this one ok to merge now?

@rowasc
Copy link
Contributor

rowasc commented Mar 16, 2020

yes!

@Angamanga Angamanga merged commit 49bb8d9 into ushahidi:develop Mar 17, 2020
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.

5 participants