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

Refactor administrate authorization to support gem update #1032

Merged
merged 2 commits into from
Aug 29, 2022

Conversation

jazairi
Copy link
Contributor

@jazairi jazairi commented Aug 26, 2022

Why these changes are being introduced:

The most recent version of administrate refactors authorization
in a way that has downstream effects. See #1941
for more info.

Relevant ticket(s):

https://mitlibraries.atlassian.net/browse/ENGX-192

How this addresses that need:

This replaces our override of the show_action? method with
authorized_action? in the administrate application controller,
and it replaces our combined check of valid_action? and
show_action? with accessible_action? in the administrate views.

This also updates the ability so thesis processors can continue to
access the submitter dashboard. This is not currently needed
as we do not have any users with the thesis_processor role, but it
seemed wise to address it sooner than later.

Side effects of this change:

  • Thesis processors can now see links to controller actions in the
    submitter admin dashboard, as well as a link to the admin dashboard
    itself from the main site nav.
  • The admin submitter dashboard test will move to the admin/
    subdirectory with the other individual dashboard tests. This change
    will come in a subsequent commit to make the git history easier
    to read.

Developer

  • All new ENV is documented in README
  • All new ENV has been added to Heroku Pipeline, Staging and Prod
  • ANDI or Wave has been run in accordance to
    our guide and
    all issues introduced by these changes have been resolved or opened as new
    issues (link to those issues in the Pull Request details above)
  • Stakeholder approval has been confirmed (or is not needed)

Code Reviewer

  • The commit message is clear and follows our guidelines
    (not just this pull request message)
  • There are appropriate tests covering any new functionality
  • The documentation has been updated or is unnecessary
  • The changes have been verified
  • New dependencies are appropriate or there were no changes

Requires database migrations?

NO

Includes new or updated dependencies?

YES

Why these changes are being introduced:

The most recent version of administrate refactors authorization
in a way that has downstream effects. See [#1941](thoughtbot/administrate#1941)
for more info.

Relevant ticket(s):

https://mitlibraries.atlassian.net/browse/ENGX-192

How this addresses that need:

This replaces our override of the `show_action?` method with
`authorized_action?` in the administrate application controller,
and it replaces our combined check of `valid_action?` and
`show_action?` with `accessible_action?` in the administrate views.

This also updates the ability so thesis processors can continue to
access the submitter dashboard. This is not currently needed
as we do not have any users with the thesis_processor role, but it
seemed wise to address it sooner than later.

Side effects of this change:

* Thesis processors can now see links to controller actions in the
submitter admin dashboard, as well as a link to the admin dashboard
itself from the main site nav.
* The admin submitter dashboard test will move to the admin/
subdirectory with the other individual dashboard tests. This change
will come in a subsequent commit to make the git history easier
to read.
@mitlib mitlib temporarily deployed to thesis-submit-pr-1032 August 26, 2022 15:33 Inactive
@coveralls
Copy link

Coverage Status

Coverage increased (+0.004%) to 97.408% when pulling a82448f on update-administrate into c23500e on main.

Copy link
Member

@JPrevost JPrevost left a comment

Choose a reason for hiding this comment

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

I confirmed a few roles manually to confirm they still work as expected which along with our automated tests I feel confident this change is solid.

The weirdness in Submitter ability seems like a bug that you are fixing here so I'm not as worried about that as I was before looking at the change and poking at it myself.

Copy link
Member

@matt-bernhardt matt-bernhardt left a comment

Choose a reason for hiding this comment

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

I see the impact of this change when I run the app locally, and think this is good - it should open us up to actually using the processor role in the future, and it also allows us to keep current with updates. There's an aspect I noted in one area that will likely trip someone up, but I don't see a way to resolve this entirely - there seems to be an inconsistency in how arguments are ordered when passed to the gem now.

:shipit:


redirect_to root_path, alert: 'Not authorized.'
end

# Hide links to actions if the user is not allowed to do them.
# This is an override of an Administrate method to work with CanCan
def show_action?(action, resource)
def authorized_action?(resource, action)
Copy link
Member

Choose a reason for hiding this comment

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

Non-blocking: I see here, and in the underlying PR against the gem, that some orders of arguments are changing. This is a bit confusing to me, as the can? call is still in the original order, but now some methods go the other way.

I don't think there's anything to be done about this here - I see that authorized_action? here, and accessible_action? in the views are now consistent in their argument use, which probably hides the complexity well enough - and I don't want to bike-shed this. I'm just sure that this will trip someone up in the future, and I don't look forward to that being me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I hear you. It seemed less confusing to me to match the argument order with what's in the upstream code, but you're absolutely right that this is likely to confuse future us anyway. Fwiw, I tend to think that will unfortunately be the case with any administrate update that involves changing custom controller methods and views.

@jazairi jazairi merged commit dc9aa58 into main Aug 29, 2022
@jazairi jazairi deleted the update-administrate branch August 29, 2022 19:01
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.

5 participants