flarum / framework

Simple forum software for building great communities.
http://flarum.org/
6.3k stars 835 forks source link

Convention for permissions #2202

Closed luceos closed 3 years ago

luceos commented 4 years ago

Before stable we need to:

Some options in the conventions:

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 commented 4 years ago

For reference, this discussion was initially brought up in https://github.com/flarum/core/pull/2113#issuecomment-619099987 and later in an internal meeting.

tankerkiller125 commented 4 years ago

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 commented 4 years ago

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

askvortsov1 commented 4 years ago

Related to https://github.com/flarum/core/issues/2092

askvortsov1 commented 4 years ago

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 https://github.com/flarum/core/pull/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 commented 4 years ago

postWithoutThrottle => post.create.withoutThrottled

Is "throttled" a typo here?

luceos commented 3 years ago

I'm okay with this proposal (https://github.com/flarum/core/issues/2202#issuecomment-650626019), 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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

This would be my proposed solution.

Split the permission into 3 big parts:

And have permission split into two parts:

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 commented 3 years ago

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. https://github.com/flarum/core/issues/2202#issuecomment-650626019). 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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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:

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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:

Downsides:

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.

luceos commented 3 years ago

I also prefer dots over camelCasing. As to the magic happening with resolving abilities; I'd rather not have that magic happen and be explicit.

SychO9 commented 3 years ago

are we sure using sub-abilities (ie: dot notation for abilities) does not present a problem with wanting to shorten model.ability to ability and auto transforming it to model.ability ?

I personally don't have a problem with auto resolving to the method that has the same name as the ability, that's a perk that comes with the language. I'm not against explicitly declaring which methods resolve which abilities either. Maybe we could have both ? first check a map of ability => method, and if there's nothing check if a method with the name of the ability exists and resolve to that.

askvortsov1 commented 3 years ago

As to the magic happening with resolving abilities; I'd rather not have that magic happen and be explicit.

The magic was one of the main points of discussion at the meeting. Essentially, it comes down to this: $actor->can() represents an abstract ability on an (optional) target, which is usually, but not always, determined by whether the user has some permission. The current "magic" system works by making an educated guess: that if there exists a permission governing this ability, it will be called target_model.ability, and so it checks for that.

Quick sidenote to:

are we sure using sub-abilities (ie: dot notation for abilities) does not present a problem with wanting to shorten model.ability to ability and auto transforming it to model.ability ?

It's currently a 1-way transformation, so editGroups maps to user.editGroups just as well as edit.groups maps to user.edit.groups.

Anyways, what's the alternative of doing this without that assumption? Well, we can require that every ability register a policy method that explicitly checks the method. So, the edit method of UserPolicy would explicitly call $actor->hasPermission('user.edit'), the edit.groups method of UserPolicy would explicitly call $actor->hasPermission('user.edit.groups'), and so on. This is quite a bit more verbose, and would require extensions to add these new methods (it's a BC issue that involves security so we'd need to keep around the old system for a bit too), but IMO would be better because we avoid magic assumptions about extension namespacing and model namespacing.

If we combine this with explicitly mapping abilities to methods (but keeping a generic "can" method), we can get rid of almost all the magic.

luceos commented 3 years ago

I've thought long and hard about all this. The magic that happened, happened for a reason. The abstract policy would look for overrides using methods on the implementation, if it does not exist look at the string permission by potentially prefixing it with discussion. in some scenarios.

If we can make this process more transparent, and as you say use object slug by default user. and singular everywhere, it would be viable.

How does everyone feel about this now?

askvortsov1 commented 3 years ago

Honestly I think another meeting is needed here. I would be in favor of pushing this decision to the next release. In the meantime, I think we should adjust #1965 to use camelCase (since that's the current convention), and get that merged for this release. If we switch from camelCase to dot.separated, we'll need migrations to update current permissions anyways, might as well lump that in there.

askvortsov1 commented 3 years ago

A few conclusions from the internal meeting:

We should also consider renaming viewDiscussions to viewForum and viewUserList to searchUsers now before the next breaking release. If we do so, it should be highlighted in big, bold, red at the top of the update guide so that extensions don't try to use the old names.

askvortsov1 commented 3 years ago

Decisions need to be made on the following items:

The 1st one allows a more accurate "namespace", but might cause further confusion. Then again, our policy system is already somewhat confusing, and we could provide a util for this insertion in AbstractPolicy to avoid code duplication.

SychO9 commented 3 years ago

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

I think that makes sense.

  • Prefix the ability with it, colon-separated
  • Recommend moving the vendor namespace to the end

First one is more appropriate as long as it's doable, which seems to be the case.

and we could provide a util for this insertion in AbstractPolicy to avoid code duplication

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.

askvortsov1 commented 3 years ago

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:

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 commented 3 years ago

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 commented 3 years ago

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

@flarum/core

clarkwinkelmann commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

askvortsov1 commented 3 years ago

Which one? Scoping or permision renaming?

SychO9 commented 3 years ago

both

askvortsov1 commented 3 years ago

both

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

SychO9 commented 3 years ago

Yes, that would be harmless

clarkwinkelmann commented 3 years ago

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 commented 3 years ago

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

clarkwinkelmann commented 3 years ago

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 commented 3 years ago

I believe that this is now mostly a documentation issue?

SychO9 commented 3 years ago

Yup