Need a 'graceful landing' if someone goes to a screen they don't have access to while logged in.

[cielf]

Summary

If someone goes to a screen they don't have access to while logged in (usually through a link they have saved), take them to their own dashboard screen, with an error message.

Why

There are several people who have multiple roles. We're frequently seeing cases in bugsnag where people are trying to go to a screen they can't access in their current role. We'd like this to be gentler than a 500 error.

Details

If someone is logged in, but attempting to access a screen they can't, redirect them to the dashboard for their current role and show an error "That screen is not available. Please switch to the correct role and try again."

Recreation

Create a new user that is both a org admin and a partner.
Log in as that user
As the org admin, go to a report, and grab the link for that.
Switch to the partner role
Go to that link
Current: you get a 500 error
Desired: redirect to the partner dashboard with the above message.

(Similarly for the opposite case where you are going to a partner page from the org admin)

Criteria for completion

• above behaviour across all screens
• automated tests to demonstrate the behaviour

[elasticspoon]

@cielf to clarify you mean something like:

I log in a partner and go to http://localhost:3000/reports/annual_reports/2023 and get hit with

?

[cielf]

I think the particular error that's coming up in that case is because they don't have a 'current organization" because they are signed in as a partner. I think that's going to be the kind of thing that currently happens on most of the relevant cases, but/and they shouldn't be able to access any of the views that require org_user or org_admin status when logged in as a partner. Does that help?

[therufs]

Hey all,

With apologies for forging rashly ahead (I took a look at this to see if I could figure it out, and by the time I figured out I could, well, it was mostly done) I'm working on a draft for this one: #4650

I still have some tests to write, and per the reqs need to bubble up a slightly more helpful message than Access Denied, but put up a draft bc I find it it helpful to see in diff view (and so folks can chime in if I've totally misunderstood something here!)

[cielf]

It might be a day or two before anyone has a chance to take a look at what you've done, but I've assigned it to you.

[therufs]

thanks @cielf! I'll try to get the rest of my ducks in a row here meanwhile :)

[dorner]

@therufs not sure I understand what's changed in that PR. As far as I can tell you renamed a method, but I don't really see how that fixes this problem?

[github-actions]

This issue is marked as stale due to no activity within 30 days. If no further activity is detected within 7 days, it will be unassigned.

[therufs]

Hey @dorner -- sorry I missed this when it was fresher! The renamed method is now being called as a before_action in the annual_reports_controller and reports_controller (which other reports inherit from). As in the screenshot elasticspoon and cielf were discussing above, the 500 error we're currently seeing is due to those reports trying to look things up on nil orgs, so the before methods should ascertain that the user persona who's trying to access the report has the necessary information to successfully find an org.

[dorner]

I'm still a bit confused.

ApplicationController has this as a filter:

  def authorize_user
    return unless params[:controller] # part of omniauth controller flow
    verboten! unless params[:controller].include?("devise") ||
      current_user.has_role?(Role::SUPER_ADMIN) ||
      current_user.has_role?(Role::ORG_USER, current_organization) ||
      current_user.has_role?(Role::ORG_ADMIN, current_organization) ||
      current_user.has_role?(Role::PARTNER, current_partner)
  end

Are you saying that in the case of the 500 error, some of the other checks are stopping this from doing verboten!? If so, which? I can't see the logic that this would help with.

[therufs]

tl;dr: No, I think verboten! still runs, but since the conditionals we're checking before we run it still permit users who currently have the PARTNER role, and users who currently have the PARTNER role have a current_organization of nil, we still end up having problems. Or, put another way, the "user" that's being authorized in this authorize_user is too broad a net for these specific cases, so we need to check specifically that the user is an ORG_USER or admin.

Deeper exploration, following through the stack trace above from the bottom:

1. The user is in the PARTNER role, so the current_organization helper doesn't find anything (and does not raise an error).
2. the annual_reports_controller asks Reports to retrieve_report, passing organization: current_organization (nil).
3. AnnualReport.find_or_create_by is called with report_attributes that include the nil organization. (On the topic of not raising errors, AnnualReport does not seem to immediately care that its organization is nil, which I would have guessed belongs_to would validate -- possibly the error ends up getting bubbled up before the validation occurs?)
4. Reports#all_reports generates an array of new different types of Report, with organization: organization (nil)
5. the new report in question finally tries to ask something of the nil organization, specifically in the example for the total_disposable_diapers_distributed in the time specified
6. Nil cannot respond to the inquiry, NoMethodErrors (500s to prod users) ensue

As a meta observation, while this issue is specifically about reports, the same error is generated when a partner visits any page that is only accessible to org users (e.g. /purchases). This problem doesn't go the other way, because the BaseController that partner-related controllers inherit from already handles a redirect for users without appropriate credentials. If we wanted to commit to the bit, we could refactor the controllers that handle org actions to inherit from a similar specialized class that could implement and call a require_org?

[dorner]

Yep, that's what I think we need to do. Now I get why this is happening, but your current fix doesn't go far enough. Your suggestion is what I would go with. Rather than inheriting, maybe we could create a module that all the org controllers include (because otherwise I think we'd have to mess with all the routes to make it make sense).

[therufs]

🤦🏻 amazing job me, I picked the obvious test case and then in the interim forgot there were other cases. I will keep working on it!