feat: New granular save and edit permissions by lukaskleinschmidt · Pull Request #7896 · getkirby/kirby

lukaskleinschmidt · 2026-01-22T09:01:39Z

Description

Adds granular more save and edit permissions for versioned content.
There are still a few concerns regarding the implementation and possible side effects.

I'm not completely happy with the current Vue implementation as it requires passing down the permissions to the FormControls component.
But I haven't found a better way to do it yet.

Second the way form fields currently get disabled in the Kirby\Form\Fields class is tied to a specific action name.
Which kinda works for the use case here, but it clashes for example with the permissions for langugages which only have a update permission and don't use versioned content. Which is not an issue as far as I can tell, but still not ideal.

lukaskleinschmidt · 2026-01-22T09:12:59Z

src/Cms/Permissions.php

 		}
-	}

-	public function for(


I noticed that the bool return type is expected but not really enforced and could lead to errors.
Maybe it should be more strict and always return false if a check does not yield the expected return type?

Example:
$permissions->for('pages') would throw an exception.
$extendedActions might deviate from the convention.

You mean that an $extendedAction could be defined with a non-boolean value, which will make return $this->actions[$category][$action]; try to return it, causing a TypeError?

To be honest I prefer a TypeError over silent conversion to false (makes it easier to spot issues in the code). However I think PHP sometimes tries to be smart about the conversion and doesn't always trigger a TypeError. So an alternative could be to throw a LogicException for any values that aren't booleans.

Throwing a exception would make the intended behaviour consistent. So a LogicException for non false values works for me.

Now that I'm looking at it again the main headache came from the imposed bool $default = false.
I would assume it to return the default if no matching or expected permission was found.

Hm, but I think there's a difference between "no matching permission" (which is the use case for the default) and "invalid permission" (which points to a bug).

fa1acb0 I could also add the exception in the translations but not sure when/why to pick one over the other.

throw new LogicException( key: 'permission.type', data: ['key' => $key, 'type' => gettype($permission)] );

In an ideal world, all exceptions would be translated. However we sometimes omit that for exceptions that are unlikely to be thrown to the user and/or unlikely to occur at all. Because every translation key needs to be translated by all our community translators, but it doesn't really seem worth it for all exceptions.

lukasbestle

General comment: The alias solution for the old update permission (with callback and array support) looks very useful and clean. 👍 At first glance I think it perfectly covers the migration matrix from the concept, we should test the different variants at the end of review before the PR gets merged though.

Three more uses of the update permission we IMO need to adapt:

config/areas/languages/views.php:35
Panel\File::isFocusable()
Panel\Ui\Buttons\LanguageSettingsButton

src/Cms/PageRules.php

panel/src/components/Views/Files/FileView.vue

src/Cms/Blueprint.php

panel/src/components/Forms/FormControls.vue

lukasbestle · 2026-01-22T20:22:49Z

src/Cms/Permissions.php

+		];
+
+		// normalize core actions
+		$this->actions = $this->normalize(


Question: As the update permissions will be deprecated and aliased, do we need to set them in the actions array? I honestly don't know how the aliases in individual model blueprint classes interact with the Permissions class.

Could you elaborate a bit more. Don't quite get the implication right now.

I don't either TBH. We will need to look into this in more detail.

lukasbestle · 2026-01-22T20:31:36Z

src/Cms/Permissions.php


-		if (is_array($settings) === true) {
-			return $this->setCategories($settings);
+	protected function normalize(


Thought: TBH I think this combined normalize() method is quite hard to read. We are actually trying to get away from long methods with a lot of logic. The old separate methods were a lot more readable and easier to follow.

Problem was that I need the context of what was passed in initially to not overwrite already set values with an alias (L200). The seperate methods would make it way harder to keep that context.

I also dont like the big nested foreach + if list but it was actually the most sane way I could think of with all the back referencing and only setting not yet set values during the normalization.

I will have another stab at this and see if I can make it more maintainable

I think it also depends on the other question above. Maybe @bastianallgeier can comment on that.

Just added a refactored version for the normalization 0c0b390 that makes it a bit more easy to read

I like the new refactored version for its readability. However I'm still not 100% sure the aliasing should happen in this class. We will discuss this in our team meeting today.

lukaskleinschmidt · 2026-01-23T10:27:21Z

I think the languages do not use content versions/drafts at all. So not sure how we would tie that in or if it makes sense to add the distinction.

lukasbestle · 2026-01-24T20:51:26Z

I think the languages do not use content versions/drafts at all. So not sure how we would tie that in or if it makes sense to add the distinction.

Yes, language settings and language variables are saved directly to the PHP files, there are currently no versions for those. We could add version support later and we also have plans for separate permissions for language variables. But that doesn't need to happen in this PR. I think it's fine to keep languages.update in the current setup.

panel/src/components/Views/Files/FileView.vue

lukasbestle · 2026-02-25T07:49:07Z

src/Cms/Permissions.php


 	public function for(
-		string|null $category = null,
+		string $category,


Question: Is there a specific reason why we need to remove the option to pass $category = null? This would be a breaking change.

No. Just figured there would be no use for this method without at least specificing a category. But could be rolled back easily

You are right, that doesn't make any sense. The old implementation requires $category to be string as all called methods and the array access cannot deal with null.

Suggestion: For this PR, keep the nullable argument but check for it and call:

Helpers::deprecated('Passing `$category = null` to `Permissions::for()` is not supported', 'permissions-for-category-null')

We could then switch it over in 6.0, which is technically a breaking change then but probably not relevant in practice. But with the deprecation warning, we could find any remaining uses of this early.

lukaskleinschmidt added 2 commits January 15, 2026 11:10

feat: New granular save and edit permissions

44e2fb1

chore: Cleanup

5c295e0

lukaskleinschmidt commented Jan 22, 2026

View reviewed changes

lukaskleinschmidt marked this pull request as draft January 22, 2026 09:18

lukaskleinschmidt changed the title ~~[WIP] feat: New granular save and edit permissions~~ feat: New granular save and edit permissions Jan 22, 2026

Merge branch 'develop-minor' into feat/save-and-edit-permissions

d1e571f

lukasbestle reviewed Jan 22, 2026

View reviewed changes

chore: Fix indentation

b70d3ed

distantnative reviewed Jan 25, 2026

View reviewed changes

panel/src/components/Views/Files/FileView.vue Outdated Show resolved Hide resolved

lukaskleinschmidt added 8 commits January 26, 2026 14:24

fix: Improve error reporting

fa1acb0

fix: Add canSave prop to FormControls

0b5d1a6

fix: Fix button disabled state logic in FormControls

8ca20fb

fix: Update permission checks to include edit capability

1b5a7c2

fix: Reduce normalization complexity

0c0b390

fix: Update allowed defaults types

a67b0aa

chore: Cleanup

23e64fd

refact: Remove try catch due to unintended problems in tests

395481c

lukasbestle reviewed Feb 25, 2026

View reviewed changes

Uh oh!

Conversation

lukaskleinschmidt commented Jan 22, 2026

Description

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukaskleinschmidt Jan 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukasbestle left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukaskleinschmidt commented Jan 23, 2026

Uh oh!

lukasbestle commented Jan 24, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukasbestle Feb 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

lukaskleinschmidt Jan 23, 2026 •

edited

Loading

lukasbestle Feb 28, 2026 •

edited

Loading