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

Permissions to view banner and the entity page fixes #161

Merged
merged 15 commits into from
Feb 21, 2022

Conversation

andybroomfield
Copy link
Contributor

Adds view all and seperate view entity page permissions
Adds default view all to the anon and authenticated user role
Re-adds removed routesubscirber and access subcriber taken out in #135

@andybroomfield
Copy link
Contributor Author

andybroomfield commented Jul 20, 2021

I'm going to remove the permissions generator here and only have global permissions for all banner types.
Is that ok with everyone?

To allow blocking of the alert banner revision path
- Use route subscriber instead of custom operation for the revision.
- Add route match to access route subscriber
- Use the banner type to check permission to view the route.
@andybroomfield andybroomfield marked this pull request as ready for review January 31, 2022 09:41
@andybroomfield
Copy link
Contributor Author

andybroomfield commented Jan 31, 2022

Moving this from draft to review.

This adds seperate permissions for viewing an alert banner, and accessing the alert banner entity page (for preview / revisions).

An update / install hook will grant anonymous and authenticated user permission to view all alert banners, however they will not be /admin/content/alert-banner/{id} or /admin/content/alert-banner/{id}/revisions/{revision_id}/view.
Only those with the emergency publisher role should be able to access this path (restored the old route subscriber).

Additional permissions for per alert banner type are still included, this is to cover a scenario whereby a banner would be intended for internal use only (notifiing communications team of upcoming changes) which would be a seperate banner type. In such a case the permissions for each banner type would need to be changed by site builders to grant the appropritate access.
(Above could also be handled by adding a user role condition to the banner, but that isn't enabled by default on our condition field for banners).

Tests where already testing access to banners and routes, but no access control on the banners themselves meant they always passed (anon users never had view permission, so they couldnt access the pages, but we also never checked if they could view the banner when displayed in a block. Now that we do check we had to give them permission to view the banner but still stop them accessing the route). Thats the happy path tests working, should really add a test where a user does not have access to a banner and check it can't be viewed (manual testing shows this is the case).

Test fails are due to other Localgov Drupal modules.

@finnlewis
Copy link
Member

Discussing on our Merge Monday call, @ekes would like a trivial test for this to confirm that the correct user can see it or not, (and not cached).

… banner

This will demonstrate that a user (or the anon user) can have permissions
to view an alert banner revoked, and it will not display to them.

Users and anon get the view all banners and view each banner type entity
permission granted by default, so the test applies by revoking permission
- This will match the permissions with fresh install
@andybroomfield
Copy link
Contributor Author

Finally looks like I have fixed this, so it is now ready for review again.

Copy link
Member

@ekes ekes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

\o/ tests

@finnlewis
Copy link
Member

Just discussing this at merge Monday meeting.

Noting that the tests are failing on the 1.x branch, but this is not a blocker to merging this as we would like to remove support for the 1.x branch.

@andybroomfield will create a separate branch to remove the testing against 1.x

https://github.com/localgovdrupal/localgov_alert_banner/blob/1.x/.github/workflows/test.yml#L21-L23

Otherwise we're happy to merge this now that this has the tests to confirm the permissions are working.

@andybroomfield andybroomfield merged commit 76bae67 into 1.x Feb 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants