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

[5.9] Don't set default message when denying policy, move "This action is unauthorized" message to exception. #29178

Merged
merged 1 commit into from
Jul 16, 2019

Conversation

garygreen
Copy link
Contributor

This minor PR brings these improvements:

  • Makes it possible to determine if a policy returned with an error message or not. Currently it's not possible to easily know if a policy came back with a defined message or if it was just set as the default one ("This action is unauthorized") - with this PR the response message will be null by default if no message was supplied.
  • The AuthorizationException still sets the default error message if nothing was explicitly returned from the policy.

@garygreen garygreen changed the title Don't set default message when denying policy, move "This action is unauthorized" message to exception. [5.9] Don't set default message when denying policy, move "This action is unauthorized" message to exception. Jul 15, 2019
@driesvints
Copy link
Member

@garygreen how would you use this in a real-life example?

@garygreen
Copy link
Contributor Author

garygreen commented Jul 16, 2019

Example in code:

class MemberPolicy {

	public function reigsterAsVip(Member $member)
	{
		if ($member->isVip()) {
			// Return specific error message.
			return $this->deny('You cannot register as a VIP because you are already one.');
		}

		if ($member->isUnderSecretReview()) {
			// Don't return specific error, we want to keep this reason hidden/non obvious.
			return false;
		}

		return true;
	}

}

$access = Gate::inspect('register-as-vip', $member);
if ($access->denied()) {
	// Return the message from the policy or set a default error message.
	return redirect('/')->withError($access->message() ?? 'You cannot register as a VIP at this time.');
}

// Happy path...
// ...

Without this PR, it wouldn't be obvious to know if the policy came back with a specific message because even on the return false line above the message would be set as This action is unauthorized which technically isn't true. The default error message is not useful when your inspecting the actual response from the policy.

Also this PR fixes some cases in the framework where you create a AuthorizationException and it doesn't set the default message which is a bit inconsistent.

@taylorotwell taylorotwell merged commit 1e5bf5b into laravel:master Jul 16, 2019
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.

3 participants