backdrop-contrib / paragraphs

Paragraphs module to control your content flow
https://backdropcms.org/project/paragraphs
GNU General Public License v2.0
5 stars 11 forks source link

Enable default permissions for new paragraph types #143

Closed yorkshire-pudding closed 1 year ago

yorkshire-pudding commented 2 years ago

As a site builder I want by default anonymous to have view access to paragraphs So that I don't need to add that permission each time

laryn commented 2 years ago

@yorkshire-pudding I assume this is with the Paragraphs Bundle Permissions submodule?

yorkshire-pudding commented 2 years ago

@laryn - Yes, that is correct. I think I started creating paragraphs before I enabled it. When I enabled it all those paragraphs had view permission for paragraphs but any created subsequently had no permissions by default. I know content types expose permissions in the Configure screen; perhaps replicating that would be the best approach?

stpaultim commented 2 years ago

I agree that this would be really helpful.

yorkshire-pudding commented 1 year ago

@laryn @stpaultim I've added a PR for this enhancement:

Questions

Once the approach for this enhancement is agreed, I will implement similar in #149 although that will probably clone the permissions rather than set default.

yorkshire-pudding commented 1 year ago

@laryn @stpaultim Do you have any thoughts on the way I've implemented this? It is easy enough to change once agreed what the default behaviour will be.

laryn commented 1 year ago

@yorkshire-pudding Thanks for working on this. It's a definite improvement. I have some follow up questions, which may change how we handle this, or could end up being followups, perhaps.

  1. Right now the approach is to choose some reasonable defaults and use them all the time. Would it make sense to have a configuration page for the Paragraphs Bundle Permissions submodule which allows someone to specify default permissions, or is that just extra clutter for little gain?
  2. I think you called this out earlier, but are we at a point where we should flesh out the Add Paragraphs type page to look more like its core counterpart for Content types? (e.g. begin to implement vertical tabs at the bottom, including a "Permissions" block here which could be prefilled with the defaults when creating the Paragraphs type): Screenshot 2022-12-19 at 19-47-21 Add content type Paragraphs Sandbox
yorkshire-pudding commented 1 year ago

@laryn - thanks for getting back to me.

Does this PR overwrite permissions for existing Paragraphs types, or just provide defaults for new Paragraphs types? If the former that may be problematic in the case that someone has configured their Paragraphs type differently than these defaults, and then the permissions get overwritten by a default setting. (I haven't tested yet but I'm wondering about the changes in the install file.)

At the moment, yes. I went backwards and forwards on this and decided to stick it in and ask for feedback. I agree that someone might have done permissions differently. With your feedback, and the benefit of fresh eyes after not looking at it for 2 weeks, I'm minded to do:

What do you think?

Right now the approach is to choose some reasonable defaults and use them all the time. Would it make sense to have a configuration page for the Paragraphs Bundle Permissions submodule which allows someone to specify default permissions, or is that just extra clutter for little gain?

I think that is a nice to have rather than a must have or should have. We could always do that in a follow up if there is demand for it and/or someone has time to do it.

I think you called this out earlier, but are we at a point where we should flesh out the Add Paragraphs type page to look more like its core counterpart for Content types? (e.g. begin to implement vertical tabs at the bottom, including a "Permissions" block here which could be prefilled with the defaults when creating the Paragraphs type):

I think this is also a nice to have and could be added later. What might be a nice but sensible half-way house is to add a link to Permissions in the drop menu and take them to the relevant permissions on the permissions screen.

laryn commented 1 year ago

@yorkshire-pudding I'm okay with the revamped add/edit Paragraphs type page being a follow up. I'm still not sure on the need for a config page to set defaults and am currently leaning against (but could probably be swayed).

I think your latest proposal sounds good -- but would probably also add an info message saying that a change had been made when updating the admin permissions on existing types and recommending they review (perhaps with a link to the permissions section). I'd also lean towards new types to allow editors to edit them, I think, but no change on existing types there.

yorkshire-pudding commented 1 year ago

@laryn I've updated as per proposal above. For the save info message, I've added a check for permissions before giving a link as they may have rights to create new paragraph types but not to administer permissions. I've assumed that for updates and installs they will have full admin permissions.

I did wonder about using the anchor to the 'view' permission of the paragraph type, but given this issue that might not be helpful.

Given the above, I haven't done:

What might be a nice but sensible half-way house is to add a link to Permissions in the drop menu and take them to the relevant permissions on the permissions screen.

Perhaps having the link to permissions when adding, installing and updating is enough, or perhaps we should have a global link on the paragraphs overview page that goes to paragraph type permissions?

yorkshire-pudding commented 1 year ago

@laryn - any thoughts?

yorkshire-pudding commented 1 year ago

@laryn - any thoughts?

laryn commented 1 year ago

@yorkshire-pudding Just need to make time for final review!

laryn commented 1 year ago

@yorkshire-pudding I left a thought last week regarding the new core permissions filter -- what do you think? https://github.com/backdrop-contrib/paragraphs/pull/151#pullrequestreview-1264901465

yorkshire-pudding commented 1 year ago

Hi @laryn - Sorry for the delay in getting back to you.

I looked through your suggestions in detail when you made them and I totally agree that we need to make use of the new core permission search. What I'm not 100% sure of is the need to strip everything back and this stems from my maintainer role on Filter Permissions.

Paragraph Type Permissions are one area that can quickly proliferate permissions (i.e. 4 x [count of paragraph types] x [count of roles]) and therefore Filter Permissions is more likely to be needed. I started thinking about the best way to make it work seamlessly with either just the core permission search or with a combination of core and Filter Permissions. This lead to me improving Filter Permissions to allow the new core search filter by URL query to pass through filter permissions , and I'm now ready to get my head around this and come up with a revision.

I also came across another use case today that I need to factor in: authenticated needs to have view by default at the same time as anonymous otherwise you can see it logged out but non admin users can't see once logged in.

Another thing the new core permissions search enables is to add a Permissions link in the operations button of the paragraph type; I'm thinking about the best way to do that so it works across languages.

I hope this makes sense. Happy to discuss.

yorkshire-pudding commented 1 year ago

@laryn

See updated PR (sorry for the commit mess; I forgot that my clone PR adjusted the same part of the overview menu so had to resolve that).

I've moved the common url function in the module file and am using that everywhere. We now use the core search to point to the exact paragraph type permissions (1) on save and (2) from the operations menu; Filter permissions also applies the module filter if enabled. We use the module search or Filter permissions to go to all paragraph type permissions (1) on install (2) on update.

I've added in the authenticated view permission for save new and install

yorkshire-pudding commented 1 year ago

@laryn. Please see updated PR and comment here.

yorkshire-pudding commented 1 year ago

Hi @laryn - Any chance you can review this soon?

laryn commented 1 year ago

@yorkshire-pudding Here's how testing went today:

So two open questions, which I'm interested on your thoughts on!

EDIT: As I think about it, the Editor role is a default role but can be deleted. Maybe we just leave that alone as is...

yorkshire-pudding commented 1 year ago

Hi @laryn

The rationale for paragraphs_bundle_permissions_update_1100 is that paragraphs created after the paragraphs_bundle_permissions is enabled do not get any permissions by default so someone who had:

Will have a number of paragraph types with no permissions allocated. This may not be immediately apparent until they open up to other users or the public as User 1 always has full permissions to do anything.

So between your steps 2 and 3 you would need to create additional paragraph types. If you check them, they will not have any permissions by default until you pull the PR down.

RE: Editors - it was a question I asked early on, but for the reason you have added above, I never pursued it.

Hope that clarifies.

laryn commented 1 year ago

@yorkshire-pudding Thanks for carrying this one through -- clearly I let it drag on too long if I'm rehashing things we've already discussed and decided on. Merged!