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 applicative privilege escalation. #4397

Conversation

corentin-soriano
Copy link
Contributor

@corentin-soriano corentin-soriano commented Oct 3, 2024

Now, a manager can edit existing user and give him admin role.

  • Cast $session_user_manager to int -> to fix issue where managers can't see "users" section (but can access with ?page=users)
  • Better check for remaining admins when demoting a user.
  • Improve permissions check.

We can edit like this:

  • Administrators can edit any user.
  • Managers can edit only basic users and read only users which are administered by their group and can't promote more than basic user.
  • Teampass managers can edit all basic and read only users whatever their group and can't promote more than basic user.
  • Other users can't edit any users.
  • Managers/teampass managers can't edit other managers.

I changed new_user to fit as this to and moved this error before "duplicate login" because it is a good practice.
We redid a series of tests to validate that it is no longer possible to create/modify a user in an illegitimate way.

@corentin-soriano corentin-soriano marked this pull request as draft October 3, 2024 11:24
@corentin-soriano corentin-soriano force-pushed the fix_store_user_changes_privilege_escalation branch from 55d4621 to 979355f Compare October 3, 2024 15:28
@corentin-soriano corentin-soriano marked this pull request as ready for review October 3, 2024 15:36
@nilsteampassnet nilsteampassnet merged commit de5ca17 into nilsteampassnet:master Oct 3, 2024
2 checks passed
@corentin-soriano corentin-soriano deleted the fix_store_user_changes_privilege_escalation branch October 9, 2024 07:32
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.

2 participants