Attempting to utilize this module for a project. Installed via composer, all permissions setup right, the following behat test fails:
Scenario: Support can create sitewide alert, has required fields, and is visible on primary and admin themes, and not visible to anonymous.
Given I am logged in as a user with the "support" role
And I am at "admin/content/sitewide_alert/add"
Then I should see the heading "Add sitewide alert"
And I fill in "Name" with "Behat Test Sitewide Alert"
Then I fill in the wysiwyg on field "Alert Message" with "This is a behat test alert 1234"
And I select "Default" from "Alert Style"
And I should see an enabled "Schedule Alert" field
And the "Limit by Page" checkbox should be unchecked
And the "Active" checkbox should be checked
And I press the "Save" button
Then I should see the text "Created the Behat Test Sitewide Alert Sitewide Alert." in the "messages" region
# Alert banners are late loaded via javascript to get around any page cacheing that may be going on.
And I wait 3 seconds
Then I should see the text "This is a behat test alert 1234" in the "sitewide_alert" region
When I am at "user/dashboard"
And I wait 3 seconds
Then I should see the text "This is a behat test alert 1234" in the "sitewide_alert" region
When I am not logged in
And I wait 3 seconds
Then I should not see the text "This is a behat test alert 1234" in the "sitewide_alert" regionAnonymous has NO permissions relating to sitewide banner. Specifically they do no possess "View published Sitewide Alert entities" or "View unpublished Sitewide Alert entities"
| Comment | File | Size | Author |
|---|---|---|---|
| #11 | sitewide_alert-require-view-perm-on-route-3133546-11.patch | 524 bytes | gngn |
| #2 | require-view-perm-on-route-3133546-1.patch | 543 bytes | jnicola |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
jnicola commentedTested this by stripping down the controller code to just return JSON that says "1234" and have the various cacheing dependencies. Even adding dependencies for user and role fails though. Not sure what's up, but it's just not working with any quick fixes I can see. I'd possibly rebuild it so the controller always returns a value, and instead of having the return value cached, just generate the value to return and cache that value itself?
Note regarding my testing as well, all local, with no cacheing from varnish affecting the results I am getting.
Here's a simple patch that works for the time being. This alters the controller for loading route to require the permission 'view published sitewide alert entities'
This works for my purposes, and may for others.
Comment #3
chrissnyderHmm... We would also need to consider, not attaching the JS/HTML to the pages if they don't have that permission to avoid excess JS calls that are not needed if the user does not have that permission. In addition, we would need to change the downstream caching strategy to avoid allowing downstream caches to cache the responses for anonymous users. This may cause a large performance impact on the server if every request has to be authenticated.This will require more than just that route permission change.
Comment #4
jnicola commentedYeah, there's a lot to consider here. My solution was just one for now to get it done, and it works. It's certainly not ideal though.
I'm game to experiment with some different methods or help out. I do think it needs to happen though, as this seems like a critical piece of the functionality that isn't working.
Comment #5
chrissnyderThis solution does work, and I agree it should be added, to give at least some respect to the permission setting. The only thing missing from this codebase is an update hook to make sure that when people update the module the permissions are adjusted to give both anonymous and authenticated users this view permission to ensure that sites are not all of a sudden not working as they had when the module is updated.
Comment #6
chrissnyderComment #7
jnicola commentedOkay, so there's a junior developers with our entity and this seems like a great opportunity for her. If she can update the patch and include the update hook to give anonymous and authenticated those permissions by default when UPDATING the module, you'd accept this patch?
Any thoughts on default values? I am personally not into setting permissions by default, as there are possible implications there. However if someone takes and runs the update, we need to ensure they at least retain the experience they had.
Comment #8
chrissnyderYes, If the patch sets the permissions needed to preserve existing functionality when updating, it should be good to go.
As far as your second question on default permission values... you have a good point that generally we should not be setting permissions in install. However, I am wondering if this falls into a gray area because just like Drupal core's node module (which sets the permission for anonymous and authenticated user to "View published content"), this module is mostly useless without the "View published Sitewide Alert entities" permission set. So either we set those permissions on install or we make sure that the documentation/readme is is very clear that which permissions need be set as part of the installation process. I can see an argument both ways. Further thoughts on this?
Comment #9
chrissnyderAlso.. have your junior developer who is working on the patch join this conversation so I can make sure to give her credit when it is accepted!
Comment #10
gngn commentedI also experienced the described behaviour.
I created an alert with "Limit by Page" and permission 'view published sitewide alert entities' given only to certain (non-anonymous) roles.
Visiting the given page as anonymous I am shown the alert.
I think this should be considered "major" - showing potentially dangerous information to unwanted roles while offering a permission to prevent this.
The permission 'view published sitewide alert entities' is only used in SitewideAlertAccessControlHandler which I think is only called if viewing the "normal" entity view (like /admin/content/sitewide_alert/1) and not if displayed via JS.
So maybe we need to check the permission also in SitewideAlertsController?
The patch proposed in [#3149684 comment #2] does something similar (the patch offers a per-alert access check for roles).
Nice behat test by the way.
Comment #11
gngn commentedWhile looking forward to a more complete solution I agree with chrissnyder and jnicola that this should added.
I am using 2.x (because I like the alerts to be fieldable).
So I adjusted jnicola's patch from #2 to the current 2.x-version (2.0.0-alpha4 and 2.x-dev are currently the same).
So I am setting to need-review again and version to 2.x-dev, hope that's OK.
Current development and usage seems to focus on 8.x though...
Comment #12
chrissnyderThe only thing missing from this change is an update hook to make sure that when people update the module the permissions are adjusted to give both anonymous and authenticated users this view permission to ensure that sites are not all of a sudden not working as they had when the module is updated. Thoughts on this?
Or should we just rely on a note in the module's release notes to inform those updating the module that they need to confirm their permissions are correct?
Comment #13
jnicola commentedHmmm. Would that run the risk of a site getting this update possibly exposing alerts unintentionally to unintended users?
If it's at all possible, I think we opt out of it and leave it to site administrators to notice that certain users are no longer seeing alerts. The alternative is that users who shouldn't see alerts are getting them, which I think is the worse of the two outcomes.
I don't have a strong opinion though on this, and could be swayed to agree otherwise :)
Comment #14
chrissnyderAfter some further testing, I found that when an anonymous user does not have permission to view alerts, that endpoint returns a 403 status ( which it should). However, the javascript loads the alerts is still on the page, causing browser errors in the console. See screenshot.
Possible approaches:
In addition to the route access change included in the patch. I think we should not attach the sitewide alert javascript to the page if the user does not have enough permission to prevent these errors.
OR
Alternatively, we could keep the route access permission as it is and check the current user's permissions in \Drupal\sitewide_alert\Controller\SitewideAlertsController::load() and prevent the alerts from loading if they don't have enough permissions, returning an empty JSON object.
Comment #15
chrissnyderComment #17
chrissnyderThis PR also includes some bugfixes found while testing the permissions.
Comment #18
chrissnyder