Make NavBarPolicy allow Feedback only for Admins #996
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
solebared
reviewed
Nov 30, 2021
There was a problem hiding this comment.
Thanks for working on this @bobmazanec ! 🙏🏾 .
Looks good, just one thought below.
app/policies/nav_bar_policy.rb
Outdated
Comment on lines
14
to
16
| def is_admin?(user) | ||
| user && (user.admin_role? || user.sys_admin_role?) | ||
| end |
There was a problem hiding this comment.
The ApplicationPolicy base class provides a can_admin? method that serves the same purpose. In fact, looking at line 8 above, you can probably add Feedback to the array there instead.
There was a problem hiding this comment.
Indeed—thanks for pointing that out (plus a little 🤦 😄)
Updated/simplified (e139258)
Anything else?
Thanks!
Per comments on #996, use ApplicationPolicy#can_admin? rather than duplicating it in NavBarPolicy#is_admin?
solebared
approved these changes
Nov 30, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why
To fix #992, show the
FeedbackNavBar button only to Admins & Sys-AdminsI chose those approach after seeing that the route triggered by the
Feedbackbutton uses theSoftwareFeedbacksControlleris derived fromAdminController, which allows actions only by users with either theadminorsys_adminrole. That is, I inferred the UI was incorrectly offering the button.What
Change
NavBarPolicy#visible_buttonsto includeFeedbackonly forUsers with either theadminorsys_admin.How
Moved adding
Feedbackto the allow-list from the line withNavBarPolicy#visible_buttons's simple non-nilcheck to the line guarded with the call toApplicationPolicy#can_admin?Testing
The PR modifies existing examples, removing the expectation for
Feedback's presence forneighborcases.With modified examples, without modified code - 2 failures
With modified examples & code - 0 failures
Rubocop - no change
Outstanding Questions, Concerns and Other Notes
More about the specs than the bug: in my dev/repro environment, I noticed that my new
Userhad theunsetrole. This was different fromneighbor, but both are "neitheradminnorsys_admin", so I simply modified the existingcontexts.Accessibility
None that I'm aware of.
Security
Very slight improvement—now the UI "advertises" one fewer route to non-{,sys}admin users.
Meta
This is my first PR here.
I hope/believe I have read and understood the various guides, policies, etc. (humble apologies if not...)
Constructive feedback welcome, of course.
Pre-Merge Checklist