Convention for permissions

[luceos]

Before stable we need to:

• decide on a permission convention
• decide whether we want to enact this convention
• if so, apply the changes to core and bundled extensions.

Some options in the conventions:

• using dot namespaced permission, eg user.edit
• using camelcase userEdit

What we have to consider is that permissions are usually made an attribute on a model. So it's pretty confusing if different conventions are used for model attributes, permissions and permission grid keys, for instance:

camelCase used in core for viewDiscussions: https://github.com/flarum/core/blob/88366fe8af3baa566ad625743016acb85a0cf345/js/src/admin/components/PermissionGrid.js#L104-L113

camelCase and dot mixed viewLastSeenAt and user.viewLastSeenAt: https://github.com/flarum/core/blob/88366fe8af3baa566ad625743016acb85a0cf345/js/src/admin/components/PermissionGrid.js#L153-L157

[dsevillamartin]

For reference, this discussion was initially brought up in #2113 (comment) and later in an internal meeting.

[tankerkiller125]

Personally I'm a huge fan of dot notated permissions, it gives a easy way to break permission down if you need to (such as was done in #2113). The attribute names sent via JSON will have to be camelCase since JSON attribute names can't have dots in them. But in the backend the dot notated makes the most sense for easy break down.

Further some core extension and even core itself requires dot notated namespaces in order to function properly.

[askvortsov1]

I would prefer dot notated as well, as it would open up possibilities for more robust namespacing of permissions

[askvortsov1]

Related to #2092

[askvortsov1]

If we go with dots, the following changes need to be made:

CORE:

viewHiddenGroups => groups.view.hidden
user.viewLastSeenAt => user.view.lastSeenAt
startDiscussion => discussion.create
discussion.reply => post.create
discussion.viewIpsPost => post.view.Ips
postWithoutThrottle => post.create.withoutThrottle
discussion.editPosts => post.edit
discussion.hidePosts => post.hide
discussion.deletePosts => post.delete

Also, for the changes in #2212:

viewForum => forum.view
searchUsers => user.search

Approve:

discussion.startWithoutApproval => discusion.create.withoutApproval
discussion.replyWithoutApproval => post.create.withoutApproval
discussion.approvePosts => post.approve

Tags:

discussion.tag => discussion.edit.tags

Likes:

discussion.likePosts => post.like

Flags

discussion.viewFlags => post.view.flagged
discussion.flagPosts => flag.create OR post.flag

Scopes

In addition to the permission itself (dot-separated), we also have per-scope permissions, such as per-tag. I have 2 proposals for doing this:

1. tag-id:permission_name, e.g. meta:discussion.create
2. Storing the scope and permission name in separate columns of the DB, and providing the scope as a separate argument to user->can()

[matteocontrini]

postWithoutThrottle => post.create.withoutThrottled

Is "throttled" a typo here?

[luceos]

I'm okay with this proposal (#2202 (comment)), but I'd rather see dash-es than camelCasing where permissions are combined into one string, for instance:

post.create.without-throttling

What do you think?

[clarkwinkelmann]

I do like the idea of having an additional column for the extension ID that's separated from the permission name in the database, and they could be combined into a single value at run time.

That column could be used to clear permissions when an extension is uninstalled permanently, just like how migrations work.

[askvortsov1]

additional column for the extension ID

That's a good idea. Do you mean this in addition to the scope column / prefix for which tag to use?

[clarkwinkelmann]

This would be my proposed solution.

Split the permission into 3 big parts:

• scope
• extension
• permission

And have permission split into two parts:

• namespace/domain/model
• ability

If possible, I would actually suggest to have those 4 items as separate columns/parameters/attrs, so we wouldn't need to choose an actual separator.

$user->hasPermission($extension, $namespace, $ability, $scope = null)

Possibly allow nullable namespace for global permissions.

This would however not work well with $user->can(), but should it? If we refactor the naming, we can ask people to use hasPermission instead of using the magic of can.

Or if we go with separator, my preference would be column for scope, then dots between the others. Which would be stored very similarly to what we do now I believe.

like scope:extension.namespace.ability
ex tag1:core.post.createWithoutThrottle

As to namespace and ability casing, I would be in favor of camelCase

[askvortsov1]

I don't think we should move away from ->can, because that removes the ability for policies to butt in and apply custom logic. IMO, the only places where hasPermission should be used directly are within policies and the gate.

I would be in favor of storing extension ID as a database column, but I don't think it should actually be provided or used when checking the permission, since its biggest use case is being able to clear out permission settings for an old extension.

In general, I would prefer if we can store permission data separated in the database, because that makes changing it with SQL much easier. That doesn't in any way affect the APIs for can() and hasPermission(), because we can always parse a provided string into components as long as they are clearly delimited.

For global namespaces, we'd need to choose between null (empty string) or some constant. Empty string is probably more intuitive.

And that brings us to the ability itself. I think there's consensus that the namespace should at least be dot-separated out. But do we want to have dot-separation within the ability itself to separate out sub-abilities (e.g. #2202 (comment)). This is the convention used in #2113, and it could make policy evaluation more powerful. For instance, we could try the following implementation:

Policies are tied to a namespace (e.g. (new Extend\Authorization)->policy('post', PostPolicy)

class PostPolicy extends AbstractPolicy
{
    protected function permissionHandlers() {
        return [
            "*" => "can",
            "post.create" => "create",
            "post.create.without.throttle" => "createWithoutThrottle"
        ];
    }

    public function create($actor, $target) {
        return $this->allow();
    }

    public function createWithoutThrottle($actor, $target) {
        return $this->deny();
    }
}

When evaluating post.create.withoutThrottle, we first run through all policies that handle post.create.withoutThrottle. If those don't return anything, then we run through all policies that handle post.create. If there were more levels, and so on.

We could use either camel case or dashes for separating words, but sub-permissions would be separated by dots.

Implementation-aside, a whole other issue is how we can provide migration from one system to the other, or a backwards compatibility layer.

[clarkwinkelmann]

I don't feel like we need to add more complexity to policies/AbstractPolicy. I have never felt limited by the current solution.

The permission namespacing by extension somehow has to be part of the call, because an extension might want to access/set permissions for another extension as well. We can't assume all calls will be for permissions from that very extension. If the extension ID is only added in the database, then this means we need some kind of permission registration function. Which might be a good idea, but I also like the current solution of having flexible permissions.

One aspect of free-name permissions is that extension can dynamically create permissions as well. It's something I plan to use in Formulaire to have form-specific permissions, which might be in addition to the existing tag-scoping.

How are we going to decide on this? There are many good proposals. Do we want to make some kind of vote?

[askvortsov1]

I don't feel like we need to add more complexity to policies/AbstractPolicy. I have never felt limited by the current solution.

Even without adding logic to run through "layers" of permissions, I feel like denoting subpermissions with dots would be clearer to understand and work with. Also, decoupling permission names from method names (and having each policy specify that relationship) would make the structure of policies a lot more intuitive.

I don't think permissions should be "namespaced" by extension ID, I would prefer if the extension ID was considered metadata. It could be used for clearing out DB tables on uninstall and deciding which extension page to display those permissions on with #2409. I don't think it should be considered at all in the get/set process for extensions, other than to add that tag / label. Actually we might not even need to store it in the DB, we could just use an extender to tie permissions to extensions.

For proceeding, I think we should have a tiny bit more discussion on options, and when we settle on the exact differences between the proposal, we can vote. Would also be good to get input from the other developers @luceos @tankerkiller125 @datitisev @SychO9

[SychO9]

Not sure I understand why we'd need to namespace by extension, isn't it to avoid possible problems with extensions using already used permission names ? If so, then perhaps we can find a different solution to that since this feels like adding more complexity. And I also agree it'd be best not to move away from using ->can().

But do we want to have dot-separation within the ability itself to separate out sub-abilities

I feel like the current system is simple and powerful enough, so I don't think we need to go for sub-abilities unless really necessary. While it could be very useful, I don't really see a benefit right now apart from maybe nicer permission names. Could be wrong depending on how implementation actually goes.

For global namespaces, we'd need to choose between null (empty string) or some constant. Empty string is probably more intuitive.

I'd go for empty string.

[clarkwinkelmann]

I think the discussion kind of deviated a bit with many suggestions for radical changes. I'm also very happy with the current solution. I don't have much to say regarding the suggestions for core, I think all the proposals make sense and would work.

My only concern are extension prefixes and magic namespaces. Those were not covered in the comments higher up.

The main example for this is the discussion. prefix that's stripped in the DiscussionPolicy and powers tag-scoped permissions, which is a super nice feature, but creates some difficulties with permission naming.

My usual standard is to start the permission name with the extension ID. But I need to switch things to benefit from the magic namespaces, example:

clarkwinkelmann-my-extension.moderateReactions
discussion.clarkwinkelmann-my-extension.createReaction

The fix could be:

• Somehow have a fixed number of dots for core and extensions so we always know which part represents the model namespace (probably very impractical, unless we force the prefix to be present and add core. to all core permissions)
• Define the permission as a "POJO" where the order doesn't matter (my suggestion from above, would require lots of refactoring, and I'm still not sure how we would call can with such a setup)
• Different separator between extension ID and the part that starts with a model namespace. This would probably be the easiest of all solutions since it doesn't constraint the number of parts separated by dots, nor does it force extensions to use a prefix/ID in their permission names

Something like

clarkwinkelmann-my-extension:moderateReactions
clarkwinkelmann-my-extension:discusison.createReaction

We could then know that the part after : (or at the start of the string if : isn't present) is the model namespace that's used for some of the magic operations in policies.

This still creates a problem, because now we need to patch the permission name back together. Would clarkwinkelmann-my-extension:discusison.createReaction become $user->can('clarkwinkelmann-my-extension:createReaction', $discussion) ?

Or should we choose to put the extension ID as a suffix in extensions (?)

Whatever we decide, I think permission namespacing by extension ID needs to be part of what we choose as standard. Core extensions might not use them, but we need this to be standardized across community extensions.

[askvortsov1]

The main example for this is the discussion. prefix that's stripped in the DiscussionPolicy and powers tag-scoped permissions, which is a super nice feature, but creates some difficulties with permission naming.

One of my goals in the policy refactor is to eliminate the magicness of this namespacing, and explicitly have policies assigned to subject namespaces. The subject namespaces here would be post, discussion, user, etc. So based on subject. From there, the policy would internally route the check to a method based on the permission name. Regardless of whether that's dot separated or camel cased.

Now, another important feature of the permission is thescope. By this I mean the tag (or other conceivable scope). This makes sense to go BEFORE the subject namespace with a colon separating it. So: meta:discussion.create. IMO, this should be passed to the policy as an argument, NOT used to determine which policy/method should be used.

I'm still not sure about the extension ID. What's its purpose, really? It doesn't factor in deciding which policy to use, and is unlikely to matter TO a policy. It's metadata that can be used for cleaning up permissions from the DB, but that isn't actually used in evaluation. So I think we might as well chuck it in the front or the end of policies:

clarkwinkelmann-cool-extension:meta:discussion.create OR meta:discussion.create:clarkwinkelmann-cool-extension?

[clarkwinkelmann]

I think the need for an extension ID/prefix/suffix is to prevent conflict. Many extensions will do similar things, and some permission names would clash. For example two extensions could allow reacting to posts in different ways, and both shouldn't just call their permission discussion.react.

Even if two extensions with identical permission naming are technically incompatible, uninstalling one to install another could pick up leftover permissions and cause all sort of problems. Because of this, I think rigorous namespacing is a must.

Using the extension ID as prefix or suffix is a nice way to have something consistent across extensions.

The alternative would be to have the prefix part of the ability name, but it's not always easy to find something that writes well with the established casing.

I'm not a big fan of this syntax, but it's also a valid option:

discussion.clarkWinkelmannReactionsReact
discussion.fofReactionsReact

EDIT: something I didn't mention. Having the namespace in front is incredibly useful when editing permissions in the database since they get grouped with the default ordering

[tankerkiller125]

I think having extension namespaces in front makes sense as well, I also think using the : character to separate the namespace from the permission is valid and probably better IMO.

[SychO9]

What if when registering a policy through the extender, we set the extension ID in the policy class, once the policy class is aware of its extension ID we could use that to properly determine a permission of the form: scope:extension-id.model.ability

[askvortsov1]

Since extension IDs are currently "optional", I don't think it should be dot separated: scope:extension-id:model.ability should be easier to parse (or extension-id:scope:model.ability so we can query by having extension ID first in the DB)

[luceos]

I agree with the majority that not changing too much is the best way forward. I also think that the last proposal makes the most sense extension-id:scope:model.ability.

Having said that, I do think that this issue should impact the logic of tags and simplify it. It sounds "nice", but the fact that the discussion prefix is used to scope permission by tag doesn't make a lot of sense. So, looking at the proposal how would tags be modified to register an ability and core understand it needs to horizontally scale the permission grid? Because if we understand that, we can also understand how an alternative tags extension (eg categories which is standalone from tags) should create permissions...

I hope this makes sense :)

[askvortsov1]

What if we did extension-id:model.ability:scope? That way, permissions are namespaced within an (optional) extension id, and (optionally) restricted to a scope. It's also easier to parse with a bit less ambiguity.

The biggest issue that came up wasn't tags and scopability (we could provide that as a separate parameter to the grid). Instead, it's the rather the "magic" logic in policies, where calling $actor->can('edit', $post) gets transformed into $actor->hasPermission('posts.edit') if $post is a Post instance. That gets messy with namespaces, but Clark suggested that the model namespace could be spliced in between the two: $actor->can('clark-extension:edit', $post) => $actor->hasPermission('clark-extension:posts.edit'), which is doable.

For the actual permission naming, I still prefer dots over camel case, as it's a more versatile format. That being said, I definitely understand the downsides, so if most of @flarum/core prefer camel case, I'd be alright with that.

Pros:

• It's a more versatile format, and could eventually be extended to add logic to evaluation of sub-permissions
• It clearly conveys that some abilities are sub-permissions of others

Downsides:

• parsing out the namespace is a bit tougher, but we're currently only adding the namespace in, not parsing it out, so that shouldn't be an issue
• We'd need to migrate stuff which could be messy
• Automatically associating methods of policies to abilities ceases to work (although I'm not the biggest fan of the automatic conversion in the first place).

One last thing that came up was potentially adding another layer of logic in hasPermission, so that custom logic could be added there, based solely on the current actor. This might supercede the prepareGroups extender, as I believe the whole point of prepareGroups is to accomplish something like this.

[askvortsov1]

Would it be possible to introduce a permission builder class/object, that would be responsible for building the permission name from all these parameters (namespace, model, ability, scope) ? we could even unit test that. I need to refresh my memory and check the code again, but I feel like that would be a lot better, even the "magic" would be less magicky.

Our entrypoint for permission checks is currently the can method, which takes 2 arguments:

• Ability (which includes an optional vendor namespace, permission name, and optional scope (like with tags))
• Subject, which is how model is determined

And of course, the actor is implicitly available.

I suppose some of these could be split out into other params, but which exactly? Permission name would be good to keep together. Scope (e.g. with tags) is non-standard (and might make sense as a separate column tbh, but that breaks some existing things. Vendor is part of the permission, would be odd detaching that.

[luceos]

I dislike the idea of a helper. That would introduce a class to fix something that currently isn't clear? I think it's not wrong or bad to require abilities to be prefixed and it should be no issue that devs have to take that into consideration themselves. To me, this sounds like something that needs to be doc-driven in terms of education of best practices.

[askvortsov1]

Do we want to rename viewDiscussions and viewUserList and/or modify the translations to be clearer?

@flarum/core

[clarkwinkelmann]

It's probably a bit late to change the scopable prefix/suffix thing for the next release now.

For the viewDiscussions and viewUserList I agree with the proposal above. viewForum and searchUsers sound like reasonable alternate names. I assume we will also be renaming their English names in the permissions page.

[askvortsov1]

For the viewDiscussions and viewUserList I agree with the proposal above. viewForum and searchUsers sound like reasonable alternate names. I assume we will also be renaming their English names in the permissions page.

The obvious fix is a migration to rename all permission values containing "viewForum" and "viewUserList". However, I'm concerned about the potential impact to extensions: we can introduce a global policy for the old ability names (and do the same in tags), but any cases using hasPermission would break.

[SychO9]

I think it's a little late to make that change as well tbh.

[askvortsov1]

Which one? Scoping or permision renaming?

[SychO9]

both

[askvortsov1]

both

For now, we could change the translations to be a bit clearer?

[SychO9]

Yes, that would be harmless

[clarkwinkelmann]

For reference. This is a text search on the latest version of all Packagist Flarum extensions in files ending in .php.

For viewDiscussions:

Found in flarum/tags@v0.1.0-beta.16 src/Access/GlobalPolicy.php:36
        if (in_array($ability, ['viewDiscussions', 'startDiscussion'])) {
Found in flarum/tags@v0.1.0-beta.16 src/Access/ScopeDiscussionVisibility.php:30
                        ->whereIn('tag_id', Tag::getIdsWhereCannot($actor, 'viewDiscussions'));
Found in flarum/tags@v0.1.0-beta.16 src/Access/ScopeDiscussionVisibility.php:33
                    $query->whereVisibleTo($actor, 'viewDiscussionsInRestrictedTags');
Found in flarum/tags@v0.1.0-beta.16 src/Access/ScopeDiscussionVisibility.php:39
        if (! $actor->hasPermission('viewDiscussions')) {
Found in flarum/tags@v0.1.0-beta.16 src/Access/ScopeTagVisibility.php:24
        $query->whereNotIn('id', Tag::getIdsWhereCannot($actor, 'viewDiscussions'));
Found in fof/secure-https@0.3.0 src/Api/Controllers/GetImageUrlController.php:39
        $actor->assertCan('viewDiscussions');
Found in askvortsov/flarum-help-tags@v0.4.0 extend.php:23
        ->scope(Access\ScopeDiscussionVisibility::class, 'viewDiscussionsInRestrictedTags'),
Found in archlinux-de/flarum-import-fluxbb@1.1.3 src/Importer/Groups.php:194
            $this->insertPermission($forumPermission, 'read_forum', 'viewDiscussions');
Found in flarum/flarum-ext-tags@v0.1.0-beta.8.2 src/Access/DiscussionPolicy.php:84
                ->whereIn('tag_id', Tag::getIdsWhereCannot($actor, 'viewDiscussions'))
Found in flarum/flarum-ext-tags@v0.1.0-beta.8.2 src/Access/DiscussionPolicy.php:90
        if (! $actor->hasPermission('viewDiscussions')) {
Found in flarum/flarum-ext-tags@v0.1.0-beta.8.2 src/Access/GlobalPolicy.php:34
        if (in_array($event->ability, ['viewDiscussions', 'startDiscussion']) && is_null($event->model)) {
Found in flarum/flarum-ext-tags@v0.1.0-beta.8.2 src/Access/TagPolicy.php:32
        $query->whereNotIn('id', Tag::getIdsWhereCannot($actor, 'viewDiscussions'));
Found in flagrow/subscribed@0.1.0-beta.1 src/Listeners/DiscussionCreated.php:70
                $recipient->can('viewDiscussions', $discussion);
Found in davis/flarum-ext-securehttps@0.1.0-beta6 src/Api/Controllers/GetImageUrlController.php:17
        $this->assertCan($request->getAttribute('actor'), 'viewDiscussions');
Found in sinamics/flarum-ext-tags@v0.1.0-beta.7 src/Access/DiscussionPolicy.php:99
                ->whereIn('tag_id', Tag::getIdsWhereCannot($actor, 'viewDiscussions'))
Found in sinamics/flarum-ext-tags@v0.1.0-beta.7 src/Access/DiscussionPolicy.php:105
        if (! $actor->hasPermission('viewDiscussions')) {
Found in sinamics/flarum-ext-tags@v0.1.0-beta.7 src/Access/GlobalPolicy.php:33
        if (in_array($event->ability, ['viewDiscussions', 'startDiscussion']) && is_null($event->model)) {
Found in sinamics/flarum-ext-tags@v0.1.0-beta.7 src/Access/TagPolicy.php:31
        $query->whereNotIn('id', Tag::getIdsWhereCannot($actor, 'viewDiscussions'));
Found in jammerxd/tags@v0.1.2-beta13 src/Access/DiscussionPolicy.php:82
                ->whereIn('tag_id', Tag::getIdsWhereCannot($actor, 'viewDiscussions'));
Found in jammerxd/tags@v0.1.2-beta13 src/Access/DiscussionPolicy.php:87
        if (! $actor->hasPermission('viewDiscussions')) {
Found in jammerxd/tags@v0.1.2-beta13 src/Access/GlobalPolicy.php:32
        if (in_array($event->ability, ['viewDiscussions', 'startDiscussion']) && is_null($event->model)) {
Found in jammerxd/tags@v0.1.2-beta13 src/Access/TagPolicy.php:30
        $query->whereNotIn('id', Tag::getIdsWhereCannot($actor, 'viewDiscussions'));

For viewUserList:

Found in morgandusty/flarum-category-russian@2.8 extend.php:82
            if ($serializer->getActor()->can('viewUserList')) {

So if we exclude forks and abandoned extensions from this list we're left with almost nothing, with almost all extensions controlled by Flarum core members.

Of course there are all the private extensions out there to keep in mind as well.

EDIT: and for the javascript side, there's no usage of canViewDiscussions and just one usage of canViewUserList:

Found in clarkwinkelmann/flarum-ext-author-change@0.2.3 js/dist/forum.js:1

[tankerkiller125]

@clarkwinkelmann based on that data it would seem that it would be safe to change these then?

[clarkwinkelmann]

based on that data it would seem that it would be safe to change these then?

That's my thought as well. Very limited and manageable impact.

Also whether we make it backward compatible or not, we could create a good error message if anyone tries to use the old ability name so that it doesn't silently fails if someone did manage to use it in a private extension or local extender.

I believe I have two private extensions that use the ability name but those will be very simple changes, they only change client-side display of the permission dropdown.

[askvortsov1]

I believe that this is now mostly a documentation issue?

[SychO9]

Yup