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 create & changeRole permissions/options handling in Panel UI #6612

Closed

Conversation

distantnative
Copy link
Member

@distantnative distantnative commented Aug 11, 2024

Description

⚠️ Trying to split this into smaller, more modular PRs.

Summary of changes

  • Cms\User::roles():
    • So far, this only handled admin vs. anybody else: admins would get all roles, everyone else just their own role. This is not what the method promises to return.
    • Changed it to return the actual list of available roles for any users.
    • Also checks permissions and user options: User::roles($context) and App::roles($context) which apply ->canBeChanged() or ->canBeCreated() accordingly to the context.
  • Panel dialogs: user.create and user.changeRole
    • Pass $roles to Panel\Field::role() using the right $context to retrieve the roles depending on their action
    • Show a radio input, even if only one option is available (though to make it transparent what gets created)
  • Resorted UserPermissions and UserRules
    • UserPermssions::canChangeRole() can be true even if only one role is available - as this isn't a permissions restriction. Instead this check is performed separately where the UI needs it.

Reasoning

The main issue of #5146 why the permission wasn't reflected in the UI, was UserPermissions::canChangeRole only checking if User::roles() returns more than one role. However, User::roles() would always only return the current role for non-admins. That's why the permission itself never was acknowledged. Fixing these now really relies on the permission.

Moreover, in the refactoring to Panel backend dialogs etc. it got lost to filter available roles further by applying ->canBeChanged() or ->canBeCreated().

Additional context

As UserRules::changeRole() was fully restrictive in place, this is no security fix - the permission was always fully enforced. Even more, the Panel UI was falsely more restrictive than it should've been based on the permissions.

Changelog

Fixes

Enhancements

  • Role always shown when creating a new user, even if only one role available

Docs

  • Consistent listing of user.changeRole AND users.changeRole

Ready?

  • In-code documentation (wherever needed)
  • Unit tests for fixed bug/feature
  • Tests and CI checks all pass

For review team

  • Add changes & docs to release notes draft in Notion

@distantnative distantnative added the type: bug 🐛 Is a bug; fixes a bug label Aug 11, 2024
@distantnative distantnative self-assigned this Aug 11, 2024
@distantnative distantnative linked an issue Aug 11, 2024 that may be closed by this pull request
@distantnative distantnative force-pushed the fix/5146-user-changeRole-permission branch from 0358f68 to 188c76e Compare August 11, 2024 21:16
@distantnative distantnative linked an issue Aug 11, 2024 that may be closed by this pull request
@distantnative distantnative force-pushed the fix/5146-user-changeRole-permission branch from 188c76e to abce099 Compare August 31, 2024 10:32
@distantnative distantnative force-pushed the fix/5146-user-changeRole-permission branch from abce099 to 1e5a2f9 Compare August 31, 2024 11:09
@distantnative distantnative marked this pull request as ready for review August 31, 2024 11:12
@distantnative distantnative requested review from a team August 31, 2024 11:13
@distantnative distantnative added this to the 4.5.0 milestone Aug 31, 2024
@distantnative distantnative force-pushed the fix/5146-user-changeRole-permission branch from 1e5a2f9 to 87213bc Compare September 7, 2024 13:48
@distantnative distantnative added the needs: two-person review 🧑‍🤝‍🧑 PR must only be merged with two approvals label Sep 7, 2024
@distantnative distantnative marked this pull request as draft September 7, 2024 13:53
@distantnative distantnative linked an issue Sep 7, 2024 that may be closed by this pull request
@distantnative distantnative changed the title Fix changeRole permission handling Fix user create & changeRole permissions/options handling in Panel UI Sep 7, 2024
@distantnative distantnative marked this pull request as ready for review September 7, 2024 19:17
@distantnative distantnative marked this pull request as draft September 7, 2024 19:21
@distantnative distantnative marked this pull request as ready for review September 7, 2024 19:50
@distantnative distantnative marked this pull request as draft September 7, 2024 19:57
@distantnative distantnative marked this pull request as ready for review September 7, 2024 19:59
@distantnative distantnative marked this pull request as draft September 7, 2024 20:14
@distantnative distantnative removed the request for review from a team September 7, 2024 21:15
@distantnative distantnative removed the request for review from a team September 7, 2024 21:15
@distantnative distantnative removed the needs: two-person review 🧑‍🤝‍🧑 PR must only be merged with two approvals label Sep 7, 2024
@distantnative
Copy link
Member Author

Closing this as we now have all parts from this PR as smaller PRs ready.

@distantnative distantnative deleted the fix/5146-user-changeRole-permission branch September 22, 2024 11:33
@distantnative distantnative removed this from the 4.5.0 milestone Sep 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug 🐛 Is a bug; fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

user permissions: support for options broken user permissions: changeRole has no effect
1 participant