refactor the auth for user role editing#2228
Conversation
… the roles we protect Signed-off-by: Nicolas Höning <nicolas@seita.nl>
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
| can_modify_roles.append(can_modify_this_role) | ||
|
|
||
| return False | ||
| return bool(can_modify_roles) and all(can_modify_roles) |
There was a problem hiding this comment.
Q: This implementation returns False if there were no roles to check (which could be more explicit, I guess).
Should it return True, rather?
There was a problem hiding this comment.
It doesn't matter as long as we're calling this function one role at a time. If there were no roles modified, users.py never even calls this method.
That said, I say True. In case we do start calling this method in the future with a possibly empty list of roles, then that signals the question is "can we do no changes to roles" and the answer should be "yes" (because "no" might lead to upstream code raising).
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
| can_modify_roles.append(can_modify_this_role) | ||
|
|
||
| return False | ||
| return bool(can_modify_roles) and all(can_modify_roles) |
There was a problem hiding this comment.
What is bool(can_modify_roles) meant to do? This condition seems to evaluate to True for any non-empty list of booleans, which is always the case unless roles is an empty list. If that is on purpose, then if roles seems more to the point.
| ) | ||
| # if flexmeasures_roles is not empty, check if the user can modify the role | ||
| if k == "flexmeasures_roles" and (v or len(v) == 0): | ||
| from flexmeasures.auth.policy import can_modify_role |
There was a problem hiding this comment.
This should be movable to the top without circular import errors (i.e. auth package should be importable from api package).
| if not can_modify_role(current_user, [role], user): | ||
| raise Forbidden( | ||
| f"You are not allowed to remove ({role.name}) role from this user." | ||
| ) |
There was a problem hiding this comment.
Good thing these calls were being made for each role separately. It now seems we could safely pass the whole list at once, but the error message mentioning the (first) role to be forbidden is actually nice to keep.
| ), | ||
| ], | ||
| ) | ||
| def test_can_modify_role_denies_unexplicit_role_changes( |
There was a problem hiding this comment.
I recommend adding a regression test case where the first new role is forbidden and the last new role is allowed. It should still be denied. I suspect that on main this test case would fail, but since in users.py we loop over roles and call can_modify_role one role at a time the bug was never exposed to users.
| ( | ||
| make_mock_user(19, ["admin"], 1, []), | ||
| make_mock_user(23, ["consultant"], 1, []), | ||
| [4, SimpleNamespace(name="unsupported-role")], |
There was a problem hiding this comment.
How can I be sure that 4 is an allowed new role (which is what this test implicitly claims)?
| can_modify_roles.append(can_modify_this_role) | ||
| continue | ||
| if role.name == ADMIN_ROLE: |
There was a problem hiding this comment.
| can_modify_roles.append(can_modify_this_role) | |
| continue | |
| if role.name == ADMIN_ROLE: | |
| pass | |
| elif role.name == ADMIN_ROLE: |
or if you want to be explicit:
| can_modify_roles.append(can_modify_this_role) | |
| continue | |
| if role.name == ADMIN_ROLE: | |
| can_modify_this_role = False | |
| elif role.name == ADMIN_ROLE: |
| continue | ||
| if role.name == ADMIN_ROLE: | ||
| # Nobody can do this here, only in CLI or directly in the DB. | ||
| can_modify_this_role = False |
| can_modify_this_role = user.has_role( | ||
| ADMIN_ROLE | ||
| ) # only admins can change admin-reader status |
There was a problem hiding this comment.
| can_modify_this_role = user.has_role( | |
| ADMIN_ROLE | |
| ) # only admins can change admin-reader status | |
| # only admins can change admin-reader status | |
| can_modify_this_role = user.has_role(ADMIN_ROLE) |
| can_modify_this_role = user.has_role( | ||
| ADMIN_ROLE | ||
| ) # only admins can change admin-reader status | ||
| elif role.name == ACCOUNT_ADMIN_ROLE: # admins and consultants can do this |
There was a problem hiding this comment.
| elif role.name == ACCOUNT_ADMIN_ROLE: # admins and consultants can do this | |
| elif role.name == ACCOUNT_ADMIN_ROLE: | |
| # admins and consultants can do this |
| and user.has_role(CONSULTANT_ROLE) | ||
| and user.account.id == modified_user.account.consultancy_account.id | ||
| ) | ||
| elif role.name == CONSULTANT_ROLE: # admins and account-admins can do this |
There was a problem hiding this comment.
| elif role.name == CONSULTANT_ROLE: # admins and account-admins can do this | |
| elif role.name == CONSULTANT_ROLE: | |
| # admins and account-admins can do this |
2 participants
Description
Clearer and explicitly about the roles we protect
documentation/changelog.rstHow to test
Log in as the toy user and see which user roles you can edit. Make him a consultant and the toy account the consultancy of another account, then edit a user there.