AP595: Stack Pass Request admin form showing Forbidden when not logged in

[yzhoubk]

Add authenticate before checking user role in StackPass form.

[awilfox]

This is good, but it's needed at app/controllers/reference_card_forms_controller.rb:87 as well.

[yzhoubk]

Yes, there maybe other forms.

[anarchivist]

I'm in agreement with @awilfox. Does this need to be added to everything that inherits from AuthenticatedFormController that also has before_action :require_admin!? I think that includes ProxyBorrowerAdminController, ReferenceCardFormController, and StackPassAdminController.

In addition, AP-595 also says we should do something about framework/app/views/home/forbidden.html.erb. The text on that page says "Only Library IT developers can access this page." What would be the impact of deleting that view file? Edit: I'll make a separate issue to deal with this - we have a lot of forbidden pages that are fairly inconsistent.

[awilfox]

Honestly, I think the entire concept of "require a user and a role" should probably be its own helper; this was a foot-gun I tripped over.

Something like authenticate_with_role!, which takes the role as a parameter, would be good - it would replace the authenticate! call here and internally just perform the logic we're duplicating everywhere.

[anarchivist]

Maybe we all can chat after standup tomorrow to confirm a path forward?

[yzhoubk]

Many other forms use this role check method; only a couple of forms are missing the authentication step. As I mentioned in the ticket, we could consolidate all of these role-check methods into a single place. However, we need to carefully review the roles during the update, as they may be implemented differently across forms. Also, we may also add a CalNet error page, when missing CalNet attributes.

[awilfox]

I agree that sounds like the best way forward. Bring the logic into one place, and verify that the roles are being checked correctly on all forms.

Adding a CalNet error page for when attributes are missing sounds like a good idea but should probably be in a separate ticket / MR IMO.

[yzhoubk]

Controllers like ProxyBorrowerAdminController and ReferenceCardFormController inherit from AuthenticatedFormController, which already includes before_action :authenticate!, so they do not need to add authenticate before checking role status.

Although some of the other controllers inherit from ApplicationController, they have included before_action :authenticate! on their own.

We only need to add authenticate to ReferenceCardFormController. I don’t see any other forms that need adding it.

[anarchivist]

thanks for following up on this. r+; looks good to me.

[awilfox]

Looks good! Thank you for your thorough investigation of this. r+