Feat: handle OAuth errors #2227

thekaveman · 2024-07-17T16:00:46Z

Closes #2019

Implements a new error screen in the oauth app
Handles errors on creation/registration of the OAuth client
Handles errors for all OAuth client method calls
Sends a new analytics event oauth error in all cases
- operation property indicates the specific client call: init, authorize_redirect, authorize_access_token, and load_server_metadata
- message property describes the exception
Captures an exception in Sentry in all cases

How to test

Checkout the branch locally
Run bin/init.sh to get the PO file updates
Launch the app with F5
Select an agency or enter via their slug URL
Manually enter the URL in the browser: /oauth/error: see the error page
Attempt a Login.gov flow for the agency (without configuring the AuthProvider): see the error page
Misconfigure an EligibilityVerifier to not specify an AuthProvider and without an Eligibility API base URL or form class and attempt to sign in with this verifier selected: see the error page

github-actions · 2024-07-17T16:01:51Z

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
benefits/core
middleware.py
benefits/oauth
analytics.py
middleware.py
redirects.py
views.py					179
Project Total

_{This report was generated by python-coverage-comment-action}

angela-tran

These code changes look good to me!

I'm not sure if this is just me, but when I try to log out, the IdG doesn't redirect me back to Benefits. I end up at this screen:

When I try log out on https://dev-benefits.calitp.org, it redirects me back to Benefits.

Does this happen for anyone else?

thekaveman · 2024-07-19T20:00:44Z

Hmm I'm seeing an error page now (both locally and for dev) just trying to sign in with IdG, so I wonder if they are working on some changes in dev on their side?

lalver1 · 2024-07-22T20:12:02Z

The code looks good and I did see the new error screen when going through a flow that uses a misconfigured AuthProvider (for example using an invalid Authority) 👍
As I was testing I did notice these 2 use cases that maybe can show the new error screen too:

Not selecting an AuthProvider for a flow that requires one. The middleware seems to catch this case, DEBUG benefits.oauth.middleware:22 Session not configured with eligibility verifier that uses auth verification, so the user sees 200-user-error.html. Perhaps we can instead show the new error screen system_error.html in this case.
Wrong scope/claim configured. If a wrong scope/claim has been configured, attempting a Login.gov flow will return an error screen generated by the Authority, for example, Sorry, there was an error : Invalid scope. Perhaps we can instead show the new error screen system_error.html in this case.

thekaveman · 2024-07-22T21:13:21Z

These code changes look good to me!

I'm not sure if this is just me, but when I try to log out, the IdG doesn't redirect me back to Benefits. I end up at this screen:

[image]

When I try log out on https://dev-benefits.calitp.org, it redirects me back to Benefits.

Does this happen for anyone else?

@angela-tran I'm seeing this behavior now locally, but not in dev -- I can't see how it would be due to any changes in this PR. I'll follow up in Slack.

thekaveman · 2024-07-22T21:31:43Z

These code changes look good to me!
I'm not sure if this is just me, but when I try to log out, the IdG doesn't redirect me back to Benefits. I end up at this screen:
[image]
When I try log out on https://dev-benefits.calitp.org, it redirects me back to Benefits.
Does this happen for anyone else?

@angela-tran I'm seeing this behavior now locally, but not in dev -- I can't see how it would be due to any changes in this PR. I'll follow up in Slack.

I found the issue, it was a change from this PR 😞 I'm going to revert it and rebase... stand by.

Fixed now, I dropped out the commit that changed the URL of our post_logout endpoint, which is registered with IdG 🤦

angela-tran · 2024-07-22T22:02:17Z

Fixed now, I dropped out the commit that changed the URL of our post_logout endpoint, which is registered with IdG

Ah that makes sense

logs exception to Sentry

- operation references the specific call that failed - message gives more information about the exception or response code

operation: authorize_redirect

operation: authorize_access_token

operation: load_server_metadata

align all test function signatures to have this parameter first

angela-tran

Looks good!

lalver1

The logic looks good @thekaveman for the misconfigured verifier case, but there may be a small typo related to checking for is None and == "" that prevents the new error page from displaying.

benefits/oauth/middleware.py

the case when the session verifier has neither Eligibility API nor IDP config is a system error / configuration error

lalver1

Looks good! 👍

thekaveman self-assigned this Jul 17, 2024

thekaveman force-pushed the feat/oauth-error branch 6 times, most recently from faa7598 to a675967 Compare July 17, 2024 20:58

thekaveman marked this pull request as ready for review July 17, 2024 21:08

thekaveman requested a review from a team as a code owner July 17, 2024 21:08

thekaveman added this to the Improved UX for application error states milestone Jul 17, 2024

thekaveman force-pushed the feat/oauth-error branch 8 times, most recently from b2b6bf9 to 06aaca5 Compare July 18, 2024 21:11

angela-tran reviewed Jul 19, 2024

View reviewed changes

thekaveman force-pushed the feat/oauth-error branch from 06aaca5 to 872773e Compare July 19, 2024 20:00

thekaveman marked this pull request as draft July 22, 2024 17:59

thekaveman requested a review from lalver1 July 22, 2024 21:25

thekaveman force-pushed the feat/oauth-error branch from f5c7820 to 06fdbb8 Compare July 22, 2024 21:35

thekaveman added 16 commits July 22, 2024 15:30

feat(oauth): error template and copy

c98549a

feat(oauth): error url and view function

059fca7

feat(oauth): view helper gets client or error redirect

b6ed94b

logs exception to Sentry

feat(oauth): handle client init errors in login

5917f12

feat(oauth): handle client init errors in authorize

1f1a521

feat(oauth): handle client init errors in logout

f4878bb

feat(oauth): handle authorize_redirect errors

3a992da

feat(oauth): handle authorize_access_token errors

4909958

feat(oauth): handle load_server_metadata errors

effc92f

feat(oauth): define analytics event for client error

f0c0600

- operation references the specific call that failed - message gives more information about the exception or response code

feat(oauth): send analytics error event for client init

94658cb

feat(oauth): send analytics event for login errors

9edea09

operation: authorize_redirect

feat(oauth): send analytics event for authorize errors

a0b5e6c

operation: authorize_access_token

feat(oauth): send analytics event for logout errors

c19ebfa

operation: load_server_metadata

refactor(tests/oauth): move app_request to first parameter

417d775

align all test function signatures to have this parameter first

refactor(oauth): centralize analytics, sentry, error redirect

651ff11

thekaveman force-pushed the feat/oauth-error branch from 06fdbb8 to 664afd3 Compare July 22, 2024 22:30

angela-tran previously approved these changes Jul 23, 2024

View reviewed changes

lalver1 reviewed Jul 23, 2024

View reviewed changes

benefits/oauth/middleware.py Outdated Show resolved Hide resolved

feat(oauth): handle misconfigured session verifier

5bce79d

the case when the session verifier has neither Eligibility API nor IDP config is a system error / configuration error

thekaveman dismissed angela-tran’s stale review via 5bce79d July 23, 2024 20:17

thekaveman force-pushed the feat/oauth-error branch from 664afd3 to 5bce79d Compare July 23, 2024 20:17

thekaveman requested a review from lalver1 July 23, 2024 20:18

lalver1 approved these changes Jul 23, 2024

View reviewed changes

thekaveman merged commit 8979fd9 into main Jul 23, 2024
15 checks passed

thekaveman deleted the feat/oauth-error branch July 23, 2024 21:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: handle OAuth errors #2227

Feat: handle OAuth errors #2227

thekaveman commented Jul 17, 2024 •

edited

Loading

github-actions bot commented Jul 17, 2024 •

edited

Loading

angela-tran left a comment

thekaveman commented Jul 19, 2024

lalver1 commented Jul 22, 2024

thekaveman commented Jul 22, 2024

thekaveman commented Jul 22, 2024 •

edited

Loading

angela-tran commented Jul 22, 2024

angela-tran left a comment

lalver1 left a comment

lalver1 left a comment

Feat: handle OAuth errors #2227

Feat: handle OAuth errors #2227

Conversation

thekaveman commented Jul 17, 2024 • edited Loading

How to test

github-actions bot commented Jul 17, 2024 • edited Loading

Coverage report

angela-tran left a comment

Choose a reason for hiding this comment

thekaveman commented Jul 19, 2024

lalver1 commented Jul 22, 2024

thekaveman commented Jul 22, 2024

thekaveman commented Jul 22, 2024 • edited Loading

angela-tran commented Jul 22, 2024

angela-tran left a comment

Choose a reason for hiding this comment

lalver1 left a comment

Choose a reason for hiding this comment

lalver1 left a comment

Choose a reason for hiding this comment

thekaveman commented Jul 17, 2024 •

edited

Loading

github-actions bot commented Jul 17, 2024 •

edited

Loading

thekaveman commented Jul 22, 2024 •

edited

Loading