Use RESTful routing for users' API key management #3442
Conversation
This commit refactors how we are using routes to use RESTful actions, moving the non-RESTful endpoint into a separate controller. This has the benefit to have a more organized codebase, but also to improve the granularity of how we can set permissions. For example, now we can have a permission set that does not allow to change or revoke users' API key but allows management of other users things. For this reason, a new ability is required and has been added in the provided UserManagement permission set. This commit also deprecates the old controller actions, printing a message in the log of users that are still using the old actions. The controller tests for the old actions have been deprecated as well, and copied over the new controller specs file but it is only checking controller related stuff now, since the real functionality is already covered by unit and feature specs elsewhere.
kennyadsl
force-pushed
the
kennyadsl/refactor-api-key-actions
branch
from
5769d55 to
4153238
Compare
There was a problem hiding this comment.
@kennyadsl looks good to me, thank you!
Just one question: do you think it may be worth adding integration specs for this?
| end | ||
|
|
||
| context "without ability to manage users and API keys" do | ||
| stub_authorization! do |_user| |
There was a problem hiding this comment.
🤔 is there any reason for having the empty block here?
There was a problem hiding this comment.
Yes, it's weird but if no block is passed a full authorization will be granted, see the method definition.
There was a problem hiding this comment.
interesting, thank you for clarifying it to me!
|
@spaghetticode For the integraton tests question, we already have integration tests for this and I'm satisfied since that they are green without the need to change them anyhow. If you think we should still add more specs, just let me know! |
|
@kennyadsl I agree those specs are all we need, thank you |
Description
This PR refactors how we are using routes and controller actions to handle users' API key. In order to use RESTful actions, this is moving the non-RESTful endpoints into a separate controller.
This changes the default routing structure in this way:
generate_api_key_admin_user_pathPUT /admin/users/:id/generate_api_keyadmin_user_api_key_pathPOST /admin/users/:user_id/api_keyclear_api_key_admin_user_pathPUT /admin/users/:id/clear_api_keyadmin_user_api_key_pathDELETE /admin/users/:user_id/api_keyThis has the benefit to have a more organized codebase, but also to improve the granularity of how we can set permissions. For example, now we can have a permission set that does not allow to change or revoke users' API key but allows management of other users things. For this reason, a new ability is required and has been added in the provided UserManagement permission set.
This PR also deprecates the old controller actions, printing a message in the log of users that are still using the old actions.
The controller tests for the old actions have been deprecated as well, and copied over the new controller specs file but it is only checking controller related stuff now, since the real functionality is already covered by unit and feature specs elsewhere.
Checklist:
I have updated Guides and README accordingly to this change (if needed)I have attached screenshots to this PR for visual changes (if needed)