rubyforgood
Unauthorized requests should result in 403/404 responses, not 500s, and basic login switching between bank/partner should direct to respectively authorized dashboards
Summary
If people go to a URL that is the partner dashboard, but log in as a bank, they are getting a 500 error. We'd like to gently steer them where they should go, instead.
Details
Due to the security concerns below, this is definitely a core team onlhy thing, and we'd like to take care of it without exposing it to the world at large.
See support thread
I'm not sure how often this can occur other than in switching between partner and bank.
It's concerning that requests that seem to require 403/404 responses are resulting in server errors, but solving that wouldn't entirely resolve the users' error.
When they're logging in, would it be possible to auto-detect that we're about to redirect them to a URL not valid for their currently active role and instead take them to the default dashboard for the active role?
Security
There's also a security implication here in that the Logout button we show when you're at the wrong page doesn't log you out (and doesn't show a warning). See https://rubyforgood.slack.com/archives/C029TFH5873/p1704333543464299?thread_ts=1704325981.838449&cid=C029TFH5873
Note: something that seems pretty problematic security-wise is that, if you're on the 500 error and click "Log out", you proceed to a blank screen with a 'logout'/'signout' URL, but you haven't been logged out (maybe because we tried to log you out as a partner?). You're still logged in as a bank. Pretty bad to present a logout button that you can click and seems to take you away from the logged-in app but which has left you logged in as a bank.
Additional info
This is showing up in bugsnag as "NoMethodError partners/dashboards#show undefined method `requests' for nil:NilClass @partner_requests = @partner.requests.order(created_at: :desc).limit(10) ^^^^^^^^^" -- we got about 10 of those in the period from Jan 2 -9, from 3 different users, so it's something people are doing
Criteria for completion
- [ ] If users go to a screen they shouldn't, they are redirected to the dashboard for their role
- [ ] Automated tests to support the above functionality.
I was trying to ensure I could replicate the error before committing to working on this one, but I think it's possible that this issue has already been resolved. I tried the following workflow:
Log in as a partner, navigate to the partner dashboard, and save the dashboard as a bookmark. Log out and navigate to the saved bookmark. When prompted to log in, log in as a bank.
On the current main branch, this workflow just redirected the user to the bank dashboard with a flashed message explaining they're not a 'partner'.
Logging in to partner dash as bank
I then reverted to the state of the code around when this issue was posted with
git checkout 69e40e65dd1999b3196ae03cd5b9587e3ea7effb
and doing the exact same workflow resulted in the 500 response described in the issue.
So, may I work on this one and would it be sufficient to just create the tests to verify the functionality?
Try this sequence with a user who is both a bank and a partner. I don't know that we have one set up in the seed, so you might have to do that.
log in as bank. Copy the page address (i.e. localhost:3000/dashboard). Switch to being a partner (in the top-right menu) Then go (not navigate, but paste the copied address) to the bank dashboard
This gives me a 500. It's probably ultimately because the current_organization isn't set -- but we should catch that the user isn't performing that role at the moment and direct them to the appropriate place.
Hope that makes things clearer?
Yes, following the sequence you provided produced a 500 error on the current main branch. Should we consider adding a user that has both a bank and partner role to the seed?
I also see Help Wanted was removed, so I'll work on other things.
We should, add a user that has both a bank and partner role to the seed, indeed -- a lot of banks have a partner (for their direct distribution purposes).
Yeah -- someone in the planning meeting said this one will require core team -- I don't recall why? - @dorner?
OK, I think this was addressed enough until we have further observations. Thank you!
