Change gate for attach mutation #176
Conversation
WalkthroughThe authorization logic within the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PerformMutation
participant Resource
User->>PerformMutation: applyMutation(mutation, attributes)
PerformMutation->>Resource: authorizeTo('attach' + model, model)
alt attach authorization passes
PerformMutation-->>User: Proceed with mutation
else attach authorization fails
PerformMutation->>Resource: authorizeTo('view', model)
alt view authorization passes
PerformMutation-->>User: Proceed with mutation
else view authorization fails
PerformMutation-->>User: Authorization denied
end
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (18)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Query/Traits/PerformMutation.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/Query/Traits/PerformMutation.php (1)
src/Concerns/Authorizable.php (1)
authorizeTo(24-53)
src/Query/Traits/PerformMutation.php
Outdated
| if (!$this->resource->authorizeTo('attach' . $model, $model)) { | ||
| $this->resource->authorizeTo('view', $model); | ||
| } |
There was a problem hiding this comment.
Fix the authorization logic - authorizeTo doesn't return a boolean.
The conditional check if (!$this->resource->authorizeTo(...)) is incorrect because the authorizeTo method doesn't return a boolean. Based on the relevant code snippet from src/Concerns/Authorizable.php, this method either:
- Succeeds and returns void (which is falsy)
- Fails and calls
Response::deny()->authorize()(presumably throwing an exception)
This means the code will always execute the fallback $this->resource->authorizeTo('view', $model) when the first authorization succeeds, which defeats the purpose.
Additionally, 'attach' . $model will likely produce an unintended ability name since $model is a Model instance, not a string.
Consider this approach using a try-catch pattern:
- if (!$this->resource->authorizeTo('attach' . $model, $model)) {
- $this->resource->authorizeTo('view', $model);
- }
+ try {
+ $this->resource->authorizeTo('attach' . class_basename($model), $model);
+ } catch (\Exception $e) {
+ $this->resource->authorizeTo('view', $model);
+ }Or modify the authorizeTo method to return a boolean if this pattern is needed elsewhere.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!$this->resource->authorizeTo('attach' . $model, $model)) { | |
| $this->resource->authorizeTo('view', $model); | |
| } | |
| try { | |
| $this->resource->authorizeTo('attach' . class_basename($model), $model); | |
| } catch (\Exception $e) { | |
| $this->resource->authorizeTo('view', $model); | |
| } |
🤖 Prompt for AI Agents
In src/Query/Traits/PerformMutation.php around lines 67 to 69, the current
authorization check incorrectly treats authorizeTo as returning a boolean, but
it actually throws an exception on failure and returns void on success. To fix
this, replace the if condition with a try-catch block: try calling
authorizeTo('attach' . AbilityName, $model) where AbilityName is a string
representing the ability (not the model object), and if it throws an exception,
catch it and then call authorizeTo('view', $model). This ensures proper
authorization flow without misusing the return value or concatenating the model
object to the ability string.
|
Oh ! but there are no unit tests on this ? I'll add some with it |
close #162
I've added the possibility of using the
attachModelgate when attaching to thePerformMutate, as indicated in the documentation.I've left the possibility of using
viewso as not to create a breaking change.Summary by CodeRabbit