SHS-5907: Implementation: Editors can easily set "view" access for Private Pages

[codechefmarc]

READY FOR REVIEW

Summary

• Adds a new contrib module content_access_simple that adds a simpler interface for settings view permissions per node.

Need Review By (Date)

2024-11-06

Urgency

low

Steps to Test

1. Login to the site as an admin
2. Create a Private Page
3. Title it
4. Verify there is a new area below all of the fields called "Access and Permissions"

6. Open it up and verify that there is a list of roles under "Visibility" but there is not "Developer", "Anonymous", or "Authenticated".
7. Choose a role, like "Site Manager" and enable view access
8. Save the node
9. Visit the "Access control" tab for this node
10. Verify that under "View any hs_private_page content", the "Site Manager" is checked in addition to "Developer" and "Authenticated user" (which were the defaults originally but they are hidden as per config)
11. Uncheck "Authenticated user" under view any here, and save
12. Attempt to view the page on a logged in user who is not "Site Manager" role and verify that you cannot access it
13. Attempt to view the page on a logged in user who is "Site Manager" and verify that you can access it
14. Edit the node and verify that under "Access and Permissions" you get a "complex" warning and no roles are listed

16. To fix, visit the content type permissions at /admin/structure/types/manage/hs_private_page/access and also uncheck "Authenticated user" and save
17. Edit the node again and this time, and this time, mark the node as unpublished and save
18. Edit the node and verify that under "Access and Permissions" you get a "unpublished" warning but the roles are still listed

20. Visit /admin/structure/types/manage/hs_private_page/form-display and drag the "Content Access Simple" "field" to another place in the form and save
21. Edit the node and verify that the Access and Permissions has moved.

PR Checklist

• PR Checklist
• Humsci Basic PR Checklist

---

• To see the specific tasks where the Asana app for GitHub is being used, see below:
  • https://app.asana.com/0/0/1208890594589119

[dalin-]

Phewph, that's a lot to review.

First up, the the code in the other repo:

---

https://github.com/codechefmarc/content_access_simple/blob/9b9135ad1b89dcaebe908673c102fa7063fd7deb/content_access_simple.info.yml#L5
We might be able to get a jump and call it 11 compatible?

---

https://github.com/codechefmarc/content_access_simple/blob/9b9135ad1b89dcaebe908673c102fa7063fd7deb/content_access_simple.module#L46
We also need to check if the extra field has actually been enabled. Something like

$fieldIsEnabled = $display->getComponent('content_access_simple');

---

https://github.com/codechefmarc/content_access_simple/blob/9b9135ad1b89dcaebe908673c102fa7063fd7deb/src/AccessManager.php#L72

I'm not sure it's a good idea to include "own access" here. In the spec it talks about mostly ignoring the whole concept (i.e. if you're doing stuff with "own access", then it's no longer "simple").

---

https://github.com/codechefmarc/content_access_simple/blob/9b9135ad1b89dcaebe908673c102fa7063fd7deb/content_access_simple.module#L92

How does Content Access work without our custom module installed? If you create a new node, then change the defaults for that bundle, does the node use the old defaults, or the new defaults? I suspect the former (and our new module will do the same), but if it's the latter, then we'll need to update our code to match.

---

And then from functional testing:

• Need to rename "Authenticated" as "All logged in users" (just on this form, not globally)
• Need help text "Only the chosen roles will be able to view this content..." this needs to be configurable.
• Unpublished message doesn't match the spec', and needs to be configurable.
• I'm not sure that your plan for not disabling checkboxes will work. Site Manager needs to be visible but disabled.

[codechefmarc]

@dalin- Updated the code and here are my comments on yours:

1. Drupal 11 - updated the info file and did a check with Upgrade Status module so it is ready for D11
2. Good catch on checking to see if the field is enabled in the default display. I added that feature in.
3. The "own access" here (https://github.com/codechefmarc/content_access_simple/blob/9b9135ad1b89dcaebe908673c102fa7063fd7deb/src/AccessManager.php#L72) is actually our own module permissions, not content access. However, I see your point in wanting to make this module more simple, so I removed it and there is only one permission to use this module now.
4. In content access, they do not show the "Access control" tab when you first create a node. They only show it after the fact on the edit page. The defaults come from the content type settings itself via content_access_node_access_records which is a hook that sets permissions on node save. It, in turn calls this line: return $grants[$node->id()][$op] ?? content_access_get_settings($op, $node->getType()); which essentially gets the per node setting, but if that doesn't exist, use the setting from the content type. Since we're showing our form elements on the add page, we need a way to get the defaults first. Unless you want to follow content access and not show our stuff on the add page and instead only show it on the edit page?
5. Renamed "Authenticated" to "All logged in users"
6. Help text added and is part of the config
7. For the unpublished message - I did make this dynamic and list roles who can view unpublished based on the View Unpublished module permissions which Stanford has, but, not all sites will have that module. So, I can also add a config-based unpublished message that will take precedence if set in config. I like the idea of the dynamic one, but it does sort of depend on the view unpublished module. I'm out of hours for this project this week, so I'll make that change next week.
8. Added logic and config for disabled roles. When I first read the spec, I didn't get it and was going to reach out to you, but I understand now.
9. We can certainly hide Stanford specific roles with their config, and I'll do that in this PR to test next week. As part of the module, I only hid the ones that made sense out of the gate for contrib.

[dalin-]

@codechefmarc

Since we're showing our form elements on the add page, we need a way to get the defaults first. Unless you want to follow content access and not show our stuff on the add page and instead only show it on the edit page?

Makes sense. No changes needed.

For the unpublished message... I'm out of hours for this project this week, so I'll make that change next week.

Sounds good.

As part of the module, I only hid the ones that made sense out of the gate for contrib.

Ooooh, smart.

---

On
https://hs-traditional-nibcuwfk1qrcj3cnraoyrgs4360qz81k.tugboatqa.com/
I masqueraded as "Lindsey" who is a Site Manager

When I try to edit an existing private page, our new field is not there

But maybe this is more about generic config vs our config.

---

Lastly, there's a lot of code changed in this PR that seems unrelated. Maybe it needs to merge in the source branch.

[codechefmarc]

Ahh, for "Lindsey", I know why - we have permissions for our module too and that wasn't set as part of this PR. I'll need to make that change for Site Managers in the config and do an update hook as well.

And yup, I had the base incorrect for the merge, so I changed it to the 11.5.2 release branch as the base and now there's only a handful of files changed.

However, I'd like to discuss with you how we do this deploy - if we do want this as a contrib module, once we get everything buttoned up, I'll upload to Drupal.org then change composer.json to point to that version and not my repo.

[dalin-]

@cienvaras

However, I'd like to discuss with you how we do this deploy - if we do want this as a contrib module, once we get everything buttoned up, I'll upload to Drupal.org then change composer.json to point to that version and not my repo.

You might want to bring this up in your weekly call with Albert + SWS. I'm happy to let you handle it, or let me know if you want my opinion.

[cienvaras]

@dalin-

You might want to bring this up in your weekly call with Albert + SWS. I'm happy to let you handle it, or let me know if you want my opinion.

We don't have meeting with SWS, but I can ask Joe on the chat. I'd like your opinion though, I haven't been fully involved on this ticket and there are things I might be missing.

[dalin-]

@cienvaras
I suspect that we'll need to go through a couple of rounds on this:

1. Get SWS review on the generic module.
2. Get H&S review of the UI/UX.
3. Publish it to drupal.org
4. Create a follow-up task to integrate the contrib module into our site.

[joegl]

@ahughes3 @codechefmarc This looks really neat. I haven't reviewed the custom module code in detail yet, but I'm wondering if we may want to roll this out only to a few select sites first to beta test and work out any kinks instead of push it out to every site. Because it affects access I think it's better to be a little more cautious to start.

[joegl]

What exactly does this permissions allow and could it be renamed to fit into the "access, create, edit, delete, etc.," categories used for other permissions?

[codechefmarc]

Hi @joegl, welcome back! This permission grants the ability for the site manager to see the new "Access and Permissions" details pane. See screenshot below. I copied the format from the Content Access module which uses grant content access as one of their permissions. We can change it if you wish as this module is still not widely used yet (Drupal.org says it's got one install and that may be Stanford?). If we changed it, I would say "access" so it would read access content access simple if that works for you. Let me know your preference. Thanks!

[joegl]

I agree with @dalin- on changing this to access content access simple

[ahughes3]

@joegl If you have a way for us to Beta test this feature on stage and then on prod I'm all for it. 4K did a lot of local testing and we did a good amount of Tugboat testing so we feel good about the functionality but we want to test on stage as soon as possible.

If you prefer to only have this enabled on select sites these are the sites we would prefer to roll the feature out to as they are waiting on the functionality - english, MTL, and Anthropology

[dalin-]

I like "access content access simple" rather than "grant content access simple".

I think the easiest way to roll this out to only a few sites is to remove su_humsci_profile_update_9722() which gives the above permission to all 'site_manager' on all sites, and either:

• write a replacement update hook to only add it to these 3 sites
• manually set the permission on these 3 sites after deploy

And create a follow-up ticket to add it everywhere.

[joegl]

I like "access content access simple" rather than "grant content access simple".

I think the easiest way to roll this out to only a few sites is to remove su_humsci_profile_update_9722() which gives the above permission to all 'site_manager' on all sites, and either:

• write a replacement update hook to only add it to these 3 sites
• manually set the permission on these 3 sites after deploy

And create a follow-up ticket to add it everywhere.

@dalin- @ahughes3 @codechefmarc

Either of these works for me, although I think I lean more towards manually enabling it on the 3 sites. We also may need to set up a few temporary config_ignore rules so any subsequent deploys to staging don't uninstall the module or wipe out any configuration. I've used a config_ignore on a specific module like this before core.extension:module:content_access_simple.

With the permission name change and changes to only enable on the 3 sites I'd be ready to merge this and move forward.

Another possible option would be to add an extra "enable" setting into the module itself available on the settings page. That way we could install the module across all sites but only enable it on the 3 sites via the setting (and only have to config_ignore the settings page).

[dalin-]

We also may need to set up a few temporary config_ignore rules...

I wonder if that's more complexity than we really need??? I think we're already ignoring all permissions, so just manually giving the site_manager role the access content access simple permission on those 3 sites could be enough without adding still more config_ignore rules.

[joegl]

We also may need to set up a few temporary config_ignore rules...

I wonder if that's more complexity than we really need??? I think we're already ignoring all permissions, so just manually giving the site_manager role the access content access simple permission on those 3 sites could be enough without adding still more config_ignore rules.

That's a good point. The --partial imports we do won't remove permissions or uninstall modules not listed in core.extension.yml (which is why we always need to do those in update hooks when we do deprecations). I don't think we need any config_ignore settings adding.

[joegl]

@codechefmarc @dalin- Because the module is in a separate repo and not a PR, how would you like me to go about reviewing the code and offering feedback? I did take a cursory review of the code and didn't see anything noteworthy. I wish the content_access module API was more D8+ OOP oriented instead of having to use all those functions but it's unavoidable.

[codechefmarc]

Since this is a contrib module, probably best to create an issue on d.o so that I can fix there and then give you credit at the same time. :)

[codechefmarc]

@dalin- and @joegl - I've made the following changes:

• In Content Access Simple the permission is now access content access simple
• In Humsci repo, I've removed the permission for Site Managers in config and removed the update hook
• I tested on Tugboat and re-added that permission manually to Site Editors and masqueraded as a Site Manager and that worked

Let me know if there are any other changes you need. Thanks!

[joegl]

@dalin- and @joegl - I've made the following changes:

• In Content Access Simple the permission is now access content access simple
• In Humsci repo, I've removed the permission for Site Managers in config and removed the update hook
• I tested on Tugboat and re-added that permission manually to Site Editors and masqueraded as a Site Manager and that worked

Let me know if there are any other changes you need. Thanks!

@codechefmarc Looking good, just one thing left I see

• Remove the module from config/default/core.extension.yml so it doesn't get installed on all sites.

@ahughes3 I know we have a prod deploy next Wednesday -- do you still want to try to get this into that release or wait? Either works for me since we're manually activating it on only 3 sites.

[codechefmarc]

@joegl - Some guidance here - I attempted to remove this from core.extension but it won't work because there is the default config content_access_simple.settings that has an error when it attempts to import saying the module isn't installed. We could put this in the config_ignore.settings? And then on the three sites we want to test, the config would have to be manually set via drush or some other method (there is no UI to set this config yet).

The other path here is we keep it as is, and because we removed permissions to see it, site managers and non-developers won't see that "Access and Permissions" area. Developers will, of course.

Let me know your thoughts. Thanks!

[joegl]

@joegl - Some guidance here - I attempted to remove this from core.extension but it won't work because there is the default config content_access_simple.settings that has an error when it attempts to import saying the module isn't installed. We could put this in the config_ignore.settings? And then on the three sites we want to test, the config would have to be manually set via drush or some other method (there is no UI to set this config yet).

The other path here is we keep it as is, and because we removed permissions to see it, site managers and non-developers won't see that "Access and Permissions" area. Developers will, of course.

Let me know your thoughts. Thanks!

I say we go the route of adding the settings to config_ignore and set them on the 3 sites when we manually install the module.

I'm pasting the current settings here for reference after they get removed from the PR:

role_config:
  hidden_roles:
    - anonymous
    - administrator
    - author
    - intranet_viewer
  disabled_roles:
    - site_manager
debug: false
help_text_view: 'Only the chosen roles will be able to view this content (when published). <i>Site Managers</i> can always view all content.'
unpublished_message: 'These settings only take effect once this page is published. Only <i>Site Managers</i> and <i>Contributors</i> can view unpublished private pages.'

[ahughes3]

Tugboat has finished building the preview for this pull request!

Links:

• https://pr1679-cuxprfpktuq9xrkg56bkmghxea5nizx4.tugboatqa.com
• https://hs-colorful-cuxprfpktuq9xrkg56bkmghxea5nizx4.tugboatqa.com
• https://hs-traditional-cuxprfpktuq9xrkg56bkmghxea5nizx4.tugboatqa.com
• https://english-cuxprfpktuq9xrkg56bkmghxea5nizx4.tugboatqa.com
• https://history-cuxprfpktuq9xrkg56bkmghxea5nizx4.tugboatqa.com
• https://west-cuxprfpktuq9xrkg56bkmghxea5nizx4.tugboatqa.com

Dashboard:

• https://dashboard.tugboatqa.com/679ae5cb1728297d1421ccb7

Visual Diffs:

• https://dashboard.tugboatqa.com/679ae5cb1728297d1421ccb7/visualdiffs

[codechefmarc]

@joegl - OK, removed from core.extension. I added content_access_simple.settings to the config_ignore.settings and tested it locally and it worked fine, but when I pushed to Tugboat, we got errors again. So, I simply removed the settings altogether and we can re-recreate on the 3 sites with what you saved in the previous comment. Build it working just fine now.