Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable default permissions for new paragraph types #143

Closed
yorkshire-pudding opened this issue Aug 27, 2022 · 20 comments · Fixed by #151
Closed

Enable default permissions for new paragraph types #143

yorkshire-pudding opened this issue Aug 27, 2022 · 20 comments · Fixed by #151
Labels
enhancement New feature or request

Comments

@yorkshire-pudding
Copy link
Contributor

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

@yorkshire-pudding yorkshire-pudding added the enhancement New feature or request label Aug 27, 2022
@laryn
Copy link
Member

laryn commented Aug 28, 2022

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

@yorkshire-pudding
Copy link
Contributor Author

@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
Copy link
Member

I agree that this would be really helpful.

@yorkshire-pudding
Copy link
Contributor Author

yorkshire-pudding commented Dec 6, 2022

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

  • On install and also on update this will grant view permission to anonymous for each paragraph type (administrator gets full permissions by default at this time)
  • On save of new paragraph type, the full permissions are added to administrator and the view permission is added to anonymous.

Questions

  • Should editor role get any permissions by default?
  • Should this action produce any messages or watchdog logs?

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
Copy link
Contributor Author

@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
Copy link
Member

laryn commented Dec 20, 2022

@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
Copy link
Contributor Author

yorkshire-pudding commented Dec 20, 2022

@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:

  • Default for new & install(?)
    • All permissions for admin
    • View permission for anonymous
  • Update
    • leave anonymous be
    • Add all to admin if not already there - ignore any others

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
Copy link
Member

laryn commented Dec 20, 2022

@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
Copy link
Contributor Author

yorkshire-pudding commented Dec 21, 2022

@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
Copy link
Contributor Author

@laryn - any thoughts?

1 similar comment
@yorkshire-pudding
Copy link
Contributor Author

@laryn - any thoughts?

@laryn
Copy link
Member

laryn commented Jan 16, 2023

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

@laryn
Copy link
Member

laryn commented Jan 31, 2023

@yorkshire-pudding I left a thought last week regarding the new core permissions filter -- what do you think?
#151 (review)

@yorkshire-pudding
Copy link
Contributor Author

yorkshire-pudding commented Jan 31, 2023

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

@laryn. Please see updated PR and comment here.

@yorkshire-pudding
Copy link
Contributor Author

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

@laryn
Copy link
Member

laryn commented Feb 24, 2023

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

  • First created a Paragraph type on 1.x-1.x
  • Activated the Paragraphs Permissions module and checked: Admin already had permissions Question: (Does this indicate we don't actually need paragraphs_bundle_permissions_update_1100 or am I misunderstanding?)
  • Pulled your PR down
  • Ran database updates and checked: Admin still had permissions; no other changes
  • Created a new Paragraph type, got info notice, and checked: Admin has all permissions, Anonymous and up can view, nobody else has any permissions. Question: (I think we had suggested that editors should have edit access, do you have thoughts on that? What about delete/create for editors?)

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
Copy link
Contributor Author

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:

  • Enabled the permissions sub module
  • Created additional paragraph types since then
  • Not checked the permissions

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
Copy link
Member

laryn commented Feb 25, 2023

@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!

@laryn laryn removed the has pr Has a PR or branch for testing. label Feb 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants