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

Instances of models should not matter when checking permissions #2186

Merged
merged 1 commit into from
May 27, 2020

Conversation

askvortsov1
Copy link
Sponsor Member

**Ref #2092 **

Changes proposed in this pull request:

So right now, the permission system has, essentially 4 layers, checked in the following order.

  1. Run through all relevant policies. If non-null returned, return that. Else, continue.
  2. If the user is in a group that has the given permission and the permsission is not being evaluated on an instance of a model, return true. Else, continue.
  3. If the user is in the admin group, return true. Else, continue.
  4. Return false.

My proposed changes to the actual process are:

  1. Have an order of precedence for step (1), that being forceDeny, forceAllow, deny, allow. This solves the issue of dependence on extension boot order,
  2. Don't disqualify step 2 if the permission is being evaluated on an instance of a model. I'm not really sure why that was there in the first place.

This PR does (2) above, by not disqualifying a user's group having a permission if the permission check is called on an instance. This system isn't intuitive, and was set up in a commit titled "fix permissions being incorrectly granted" (which tells us nothing) back from 2015. Looking at the history at the time, this logic was called BEFORE policies were called, which might explain the changes. Now that policies are considered first, this is no longer necessary.
90def3f

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).

Copy link
Member

@luceos luceos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at the git history, but see no reason why this check was added in the first place. As I cannot come up with a scenario why this should matter I think it's okay to merge.

@askvortsov1 askvortsov1 merged commit 3c87f80 into master May 27, 2020
@askvortsov1 askvortsov1 deleted the as/remove_model_check_in_permissions_system branch May 27, 2020 16:22
@askvortsov1 askvortsov1 added this to the 0.1.0-beta.14 milestone May 27, 2020
@franzliedke
Copy link
Contributor

Nice digging. 👏

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.

4 participants