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

Unify logic of e_user_model::checkAdminPerms() and getperms() #5070

Merged
merged 1 commit into from Sep 12, 2023

Conversation

Deltik
Copy link
Member

@Deltik Deltik commented Sep 9, 2023

Motivation and Context

I had some concerns about 44526b4 and 001799c:

The modified method signature of e_user_model::checkAdminPerms() defeats the encapsulation offered by moving the global getperms() to e_user_model::checkAdminPerms() per user because $ap would override the object's internal state for the purposes of simulating permissions.

To remedy this, I suggest that we extract getperms() into a third static method that doesn't depend on state―perhaps called e_userperms::checkAdminPerms()―and have both getperms() and e_user_model::checkAdminPerms() call it to check the admin permissions. This way, we can have both backwards compatibility and encapsulation for the two different styles we support.


I have another concern, which is the extra logic for plugins. I would move this into a new method called e_user_model::checkPluginAdminPerms($plugin_name) which then calls e_user_model::checkAdminPerms() once it has figured out the $perm_str.

Description

Along with extensive documentation, getperms() is now deprecated and its replacements now have first-class support:

  • e_user_model::checkAdminPerms() and getperms() both use e_userperms::simulateHasAdminPerms().
  • e_user_model::checkPluginAdminPerms() and getperms('P', …, …) both use e_userperms::simulateHasPluginAdminPerms().

Partially reverts: 44526b43

Reverts: 001799cb

Fixes: #5064

How Has This Been Tested?

As this is a refactoring only, all existing tests of getperms() pass.

I also added some tests for the misuse cases of whitespace in the second argument or an integer in the first argument of getperms(0, ' ').

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (a change to man pages or other documentation)

Checklist

Along with extensive documentation, `getperms()` is now deprecated and
its replacements now have first-class support:
* `e_user_model::checkAdminPerms()` and `getperms()` both use
  `e_userperms::simulateHasAdminPerms()`.
* `e_user_model::checkPluginAdminPerms()` and `getperms('P', …, …)`
  both use `e_userperms::simulateHasPluginAdminPerms()`.

----

Partially reverts: e107inc@44526b43

Reverts: e107inc@001799cb

Fixes: e107inc#5064
@codeclimate
Copy link

codeclimate bot commented Sep 9, 2023

Code Climate has analyzed commit dd36fbd and detected 183 issues on this pull request.

Here's the issue category breakdown:

Category Count
Bug Risk 183

The test coverage on the diff in this pull request is 77.5% (80% is the threshold).

This pull request will bring the total coverage in the repository to 34.4% (-0.2% change).

View more on Code Climate.

@CaMer0n
Copy link
Member

CaMer0n commented Sep 12, 2023

Thank you!! 🥇 👍

@CaMer0n CaMer0n merged commit 86ff4d6 into e107inc:master Sep 12, 2023
99 of 101 checks passed
@Deltik Deltik deleted the fix/5064 branch September 12, 2023 15:23
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.

Admin impersonation of user broken when permission check fixed
2 participants