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

[8.x] Allows authorizeResource method to receive arrays of models and parameters #40516

Merged
merged 4 commits into from Jan 20, 2022
Merged

[8.x] Allows authorizeResource method to receive arrays of models and parameters #40516

merged 4 commits into from Jan 20, 2022

Conversation

Drewdan
Copy link
Contributor

@Drewdan Drewdan commented Jan 20, 2022

Hey Guys,

This PR adds some niceities with adding policies to controllers using the authorizeResource method in the AuthorizesRequest trait.

This came as a result of wanting to access other models which were bound on the route in a policy. So for example, if I wanted to access a model unrelated to the policy on the create method, I might do something like this in my controller constructor:

class FirewallController extends Controller {

	public function __construct() {
		$this->middleware('auth');
		$this->authorizeResource('App\Firewall,interaction', 'firewall,interaction');
	}

which would give me access to the interaction model in the policy in the create method, for example:

class FirewallPolicy {

	use HandlesAuthorization;
	
	public function create(User $user, Interaction $interaction) {
		
	}

This approach means I can still use authorizeResource instead of $this->authorize() in the method on the controller instead.

This PR adds some niceties around adding multiple models and parameters to the authorizeResource

class FirewallController extends Controller {

	public function __construct() {
		$this->middleware('auth');
		$this->authorizeResource([Firewall::class, 'interaction'], ['firewall','interaction']);
	}

I can instead pass in an array of models and parameters. Which then get imploded and added in this authorizeResource.

I have also added some tests to this to make sure the middleware is applied correctly.

Please let me know of any thoughts or concerns. Happy to make any changes to it!

Thanks guys :)

@Drewdan Drewdan changed the title Allows authorizeResource method to receive arrays of models and parameters [8.x] Allows authorizeResource method to receive arrays of models and parameters Jan 20, 2022
@taylorotwell
Copy link
Member

I'm pretty confused. Is Interaction the parent model of Firewall or the child?

@Drewdan
Copy link
Contributor Author

Drewdan commented Jan 20, 2022

Thank you for taking a look :)

A Firewall belongsTo an Interaction. So the interaction is the parent.

The flow here is that a user might block a customer via an interaction (interactions/{interaction}/firewalls), so we link the firewall rule to the interaction, so we can track which interaction caused the need to block the interaction.

However, we use policies to determine if the user has the rights to block an interaction, so admins can block agents interactions, but agents cannot block other agents interactions. We also only allow blocking of interactions if they are in a certain state.

With that, we need access to the interaction in the firewall policy, so we can do some checks on it to make sure it can be blocked.

I hope that makes sense, but I can try to provide further info if required.

@taylorotwell
Copy link
Member

OK - I think I found it most confusing that firewall is listed before interaction in the second argument to the authorizeResource method. Can you explain that?

$this->authorizeResource([Firewall::class, 'interaction'], ['firewall','interaction']);

@Drewdan
Copy link
Contributor Author

Drewdan commented Jan 20, 2022

My understanding here, and I may be wrong with this, but the first parameter passed in decides which policy gets applied. So in this case, I want the firewall policy to be the one that is applied, and I want to be able to access the interaction from within that policy. If I reverse these, then the Interaction Policy is applied.

@taylorotwell taylorotwell merged commit d11d431 into laravel:8.x Jan 20, 2022
@taylorotwell
Copy link
Member

Ah, yeah, you're correct I believe.

@Drewdan
Copy link
Contributor Author

Drewdan commented Jan 20, 2022

Amazing, thanks @taylorotwell

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.

None yet

2 participants