Refactor: IdG sign out to use client_id

[lalver1]

Part of #2678

Due to sign out changes in the IdG, in this PR we're replacing id_token_hint with client_id in the deauthorize_redirect helper that is used as part of the sign out process.

We can review this PR but we should hold off from merging it to main since this functionality has been deployed to IdG dev, but we should coordinate with the IdG team before deploying this to further environments.

Reviewing

Set up a login.gov flow, such as Older Adult, and go through the flow until eligibility is confirmed

Click on Sign out of Login.gov and confirm that you can successfully sign out

[github-actions]

Coverage report

Click to see where and how coverage changed

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

This report was generated by python-coverage-comment-action

[thekaveman]

@lalver1 for this PR, I think we should make sure there isn't any other token stuff laying around the codebase, since it was purely used for this sign out.

E.g. there is no need to keep this in the session anymore: https://github.com/cal-itp/benefits/blob/fix/idg-signout/benefits/oauth/views.py#L147

This clean up may have wider implications, but I think its more important to do it all at once vs. leaving some of it for later (e.g. potentially a different release).

[lalver1]

Ah thanks for pointing that out @thekaveman! I agree, it makes sense to clean it up all at once in this PR so I'll work on updating it.

[lalver1]

This is ready for another review (mostly session cleanups in 4b1a6a5 and f8c26bb) but if it's ok I'll leave it as Draft so that I don't accidentally merge it.

[thekaveman]

@lalver1 overall this is great! Exactly the kind of low-impact change we talked about 👍

One small request: the terminology in this PR now mixes the concepts of authentication with authorization.

IMO the role of this session variable is closer to authorization; i.e. the user has already proven their identity (authentication) by logging in. We are then (via e.g. the middleware) giving access (or not) to additional resources (e.g. views, the other parts of the flow).

So I think just a simple rename from authentication to authorization and this will be normalized.

[lalver1]

Thanks @thekaveman! That's a good point, I'll change the session variable name to oauth_authorized and update any docstrings or comments to make sure we normalize to authorization 👍
Normalized terminology in bf1665d

[angela-tran]

Exactly the kind of low-impact change we talked about

I take it from this that there is some reason for keeping the session variable as opposed to removing it. It's not clear to me what that reason would be though since we're not using session.oauth_authorized anywhere other than tests.

[thekaveman]

Exactly the kind of low-impact change we talked about

I take it from this that there is some reason for keeping the session variable as opposed to removing it. It's not clear to me what that reason would be though since we're not using session.oauth_authorized anywhere other than tests.

session.logged_in is used in a lot of places (middleware, views, templates, etc.), and that previously worked by converting bool(session.oauth_token).

Although we're removing the token, we still need the concept of logged_in.

[angela-tran]

I see, it is used in session.logged_in. I missed that, thanks