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

Policies: treat true as allow, and false as deny #2525

Closed
askvortsov1 opened this issue Jan 4, 2021 · 3 comments · Fixed by #2534
Closed

Policies: treat true as allow, and false as deny #2525

askvortsov1 opened this issue Jan 4, 2021 · 3 comments · Fixed by #2534
Milestone

Comments

@askvortsov1
Copy link
Sponsor Member

Feature Request

I think the new tiered levels of response in the policies system is well worth it, but is also somewhat inconvenient: I have already forgotten to return one of those 2 variables multiple times. Returning a boolean expression is much more than returning boolean expression ? $this->allow() : $this->deny(). Perhaps AbstractPolicy should treat false as $this->deny() and true as $this->allow()?

@SychO9
Copy link
Member

SychO9 commented Jan 6, 2021

I would be in favor, mainly because it would allow us to directly return expressions like before, which is more straightforward and simple, and it just seems to be the way any developer would expect it to be. Also, since we now have the ALLOW and DENY constants, the change would be very simple.

// Before
return $expression;

// Currently
return $expression ? $this->allow() : $this->deny();

@luceos
Copy link
Member

luceos commented Jan 7, 2021

if $expression is "foo", the string, your conditional would be treated as true too..

It would make more sense to do an is_boolean($expression) conditional and then return it:

if(is_bool($expression)) {
    return $expression ? $this->allow() : $this->deny();
}

return <the rest>;

@SychO9
Copy link
Member

SychO9 commented Jan 7, 2021

I was referring to a boolean expression :P
something like:

return $actor->hasPermission('perm') && $actor->id === $model->id;

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 a pull request may close this issue.

3 participants