feat: New Guards classes - #8364
Conversation
| public function isSameAs(BaseFile $file): bool | ||
| { | ||
| if ($this->exists() === false) { | ||
| return false; | ||
| } | ||
|
|
||
| // the model is based on the props of the new file, | ||
| // to compare templates, we need to get the props of | ||
| // the already existing file from meta content file | ||
| $existing = $this->parent()->file($this->filename()); | ||
|
|
||
| return | ||
| $this->sha1() === $file->sha1() && | ||
| $this->template() === $existing->template(); | ||
| } | ||
|
|
There was a problem hiding this comment.
I would think there will be questions in the future what's the difference between this and ::is() - and when to use which.
There was a problem hiding this comment.
Would you fold it into ::is or change the name?
There was a problem hiding this comment.
I am not sure, I think :is() works also for virtual files etc. while ::sha1() would want to read an actual file to do this check. Wondering if this doesn't belong rather into FileValidators.
There was a problem hiding this comment.
Otherwise maybe ::isIdentical(). ::is() = actually it is the same object (well kinda, actually just the same id), ::isIdentical() - could be different objects, but they are identical.
| { | ||
| // never publish risky files (e.g. HTML, PHP or Apache config files) | ||
| FileRules::validFile($file, false); | ||
| $file->guards()->validators()->validateFile(false); |
There was a problem hiding this comment.
Suggestion: Use named parameter to know what false refers to.
| 'delete' => true, | ||
| 'deleteAvatar' => true, | ||
| 'list' => true, | ||
| 'replaceAvatar' => true, |
There was a problem hiding this comment.
Discussion: Seeing these, I am wondering if long term it really makes sense to have these three avatar permissions. Are there use cases to allow creating and deleting but not replacing? Could this be tied to the user.update/users.update permissions - is the avatar here special vs. any user content field? Doesn't need to be solved by this PR but maybe good to think about and maybe add a TODO.
There was a problem hiding this comment.
Or maybe has to be solved here - just realized this PR introduces them :D Why do you think we need these?
| } | ||
|
|
||
| // `UserRules` enforces a minimum length of 8 characters, | ||
| // `UserValidators` enforces a minimum length of 8 characters, |
There was a problem hiding this comment.
Suggestion: Shouldn't a lot of this code migrate to UserValidators?
| * @param bool $default Used if no rule is defined for the action | ||
| * @throws PermissionException | ||
| */ | ||
| protected function trySetting(string $action, bool $default = false): void |
There was a problem hiding this comment.
I think here ensureSetting would Athen also become a stronger name.
| ## The dry run | ||
|
|
||
| The dry run is everything that can be checked without the arguments of an action. It runs the ability check first and the permission check second, and throws for the first one that fails: |
There was a problem hiding this comment.
Nitpick: I think "dry run" is a wrong analogy - these methods are not actually trying to execute the action, whether it fails, what the results would be... they just check the preconditions.
|
|
||
| The `error.` prefix is added by the exception classes, so no key in the guards ever contains it. Every key needs a matching entry in the translations, otherwise the generic fallback message of the exception is shown. | ||
|
|
||
| ## Adding a new action |
There was a problem hiding this comment.
Maybe good to mention that these are all optional? Otherwise it could seem quite laborious to add a new action if I have to implement all of these.
| } | ||
| ``` | ||
|
|
||
| That difference is worth remembering: replacing one of the three check classes changes one step in every action that uses it, adding a method to the guards class changes a single action completely. |
There was a problem hiding this comment.
Question: Those all show custom X for specific models. What's our take on how to implement custom permission that isn't tied to a specific model?
| 'changeName' => true, | ||
| 'changePassword' => true, | ||
| 'changeRole' => true, | ||
| 'changeSecret' => true, |
There was a problem hiding this comment.
Question: Wondering if changing secrets should be its own permission. Or if this should be folded with changePassword somehow.
f861d4e to
9fb6e20
Compare
9fb6e20 to
c8299a3
Compare
Review
Timing: No pressure.
Description & Docs
See https://github.com/getkirby/kirby/tree/v6/feat/guards/src/Guards#readme
Changelog
🎉 Features
guards()method to ask those questions directly.✨ Enhancements
🐛 Bug fixes
beforehook returned, not the original one.♻️ Refactored
☠️ Deprecated
PageRules,FileRules,UserRules,SiteRulesandLanguageRules. Use$model->guards()instead.$model->permissions()and the permission classes behind it. Use$model->guards()instead.🚨 Breaking changes
error.page.changeStatus.toDraft.invalidwas replaced by separate keys for the home page and the error page.