[LG-7433] Replace id_token_hint with client_id in OIDC Logout by orenyk · Pull Request #6936 · 18F/identity-idp

orenyk · 2022-09-09T20:45:29Z

Resolves LG-7433

Why: We don't want partners sending us ID tokens as query
parameters. We initially permit both client_id and id_token_hint, but
also include a feature flag so that we can extend the rollout of the
deprecation through the sandbox.

changelog: Bug Fixes, Authentication, Replace id_token_hint with
client_id in OIDC logout

app/forms/openid_connect_logout_form.rb

zachmargolis · 2022-09-09T22:48:50Z

app/forms/openid_connect_logout_form.rb

I think we still want to find a way to deactivate the identity, remove the UUID etc if possible

Could we find the identity like:

current_user.identities.find_by(service_provider: client_id)&.deactivate

@zachmargolis good call - that only works if the user already has an active session, but honestly that's the only case where it matters if we deactivate the identity. I'm assuming we end up cleaning up that session_uuid if the session times out anyway?

app/controllers/openid_connect/logout_controller.rb

app/forms/openid_connect_authorize_form.rb

orenyk · 2022-09-12T05:43:51Z

app/validators/redirect_uri_validator.rb

Added this since the extra redirect_uri error message when you don't have a valid SP has caused significant confusion in the past.

app/controllers/openid_connect/logout_controller.rb

app/forms/openid_connect_authorize_form.rb

zachmargolis

small last few comments, LGTM

app/forms/openid_connect_logout_form.rb

orenyk · 2022-09-16T16:35:58Z

@zachmargolis @mitchellhenke added the additional feature flag and some tests - please take another look and let me know what you think. Thanks!

mitchellhenke

👍🏼

zachmargolis

LGTM, one round of small comments, no blockers

spec/features/openid_connect/openid_connect_spec.rb

Resolves LG-7433 **Why:** We don't want partners sending us ID tokens as query parameters. We initially permit both client_id and id_token_hint, but also include two feature flags so that we can extend the rollout of both support for client_id as well as the deprecation of id_token_hint through the sandbox. changelog: Bug Fixes, Authentication, Replace id_token_hint with client_id in OIDC logout

orenyk · 2022-09-20T01:26:16Z

Merging! cc @pauldoomgov @mitchellhenke.

* Replace id_token_hint with client_id in OIDC Logout (#6936) Resolves LG-7433 **Why:** We don't want partners sending us ID tokens as query parameters. We initially permit both client_id and id_token_hint, but also include two feature flags so that we can extend the rollout of both support for client_id as well as the deprecation of id_token_hint through the sandbox. changelog: Bug Fixes, Authentication, Replace id_token_hint with client_id in OIDC logout * Fix a typo in the step indicator constants for inherited proofing (#6985) [skip changelog] * Implement basic Please Verify page UI for Inherited Proofing (#6988) Why: Inherited Proofing users will need to verify that the information we receive from the partner organization is correct changelog: Internal, Inherited Proofing, Adding basic Please Verify UI * LG-7152: A/B testing native camera only (#6915) * LG-7152: Setting up A/B testing for native camera vs Acuant SDK changelog: Internal, Document Capture, Set up A/B testing for native camera vs Acuant SDK * include a feature flag to enable/disable test completely * first cut at AbTestBucket * flesh out AbTestBucket * apply the AbTestBucket to the DocumentCaptureStep * Pull the specifics around this A/B test into its own class. * Log the bucket in the image upload vendor submitted event. * use a fully deterministic spec to test bucket distribution * check for nativeCameraOnly as part of shouldStartAcuantCapture * adds the name of the experiment to the percent generator * better logic on when to block SDK for A/B test * adds a spec for the native camera A/B test * LG-7123 Normalize arguments for enrollments (#6987) **Why:** - We were sometimes passing Pii::Attribute structs and other times passing hashes to this function. While it wasn't causing a problem now it is confusing changelog: Upcoming Features, In-person proofing, Normalize arguments for creating an enrollment * LG-7195 | log_reproof_event is now reachable (#6982) changelog: Internal, Attempts API, Fixes log_reproof_event * Scope the NativeCameraABTest to the Idv module (#6989) [skip changelog] * LG-7364 Return specific attributes that fail from LexisNexis proofer (#6956) * LG-7364 Return specific attributes that fail from LexisNexis proofer This commit aims to refactor the LexisNexis proofer to user a plain old ruby object and to have it return specifically which attributes fail if only certain attributes fail * i can't even write psuedocode * still cannot code * add failing specs for the proofer * put resolution job back the way we found it for now * [skip changelog] * make the lexisnexis proofer look like the phonefinder one * get started on the mock proofer * get mock proofer resolution client passing * start mapping checks to attributes * User proofer_result directly * Punt on merging with state_id_proofer result * Punt on mutating the callback_log_data result * Punt on context field and other proofer results * Group transaction_id and reference with other fields and mark TODO field * Test expected fields in turn * Group fields and mark TODO context field * Test fields in turn * Group and mark TODO * Group and mark TODO * Test fields in turn * Test fields in turn * Consolidate different result hash logic * use match instead of eq * some things passing and some things failing * example of how to fix nomethoderror * Defer when resolution result is only a proofer result * Implement methods on proofing result class * Test result fields in turn * Rename local variable name * Rename threatmetrix entities * Add back resolution tests * Test result fields in turn * Test expected value directly * Test threatmetrix disabled * Test lexisnexis failure response * Restore threatmetrix nil response test * Consolidate logic * Improve format * Improve format * Test against result hash methods * delint * spec cleanup * state id result is not quite ready * clean up agent spec * Define first_name on pii test object * delint Co-authored-by: Kimball Bighorse <kbighorse@yahoo.com> * Log timing info for TMX inside ResolutionProofingJob (#6991) [skip changelog] * LG-7469 Standardize naming conventions (#6992) changelog: Internal, Attempts API, Standardize events name * Allow logging of emailage fields including confidence scores (#6993) * Allow logging of emailage fields including confidence scores * changelog: Internal, ThreatMetrix API, allow non-PII fields * Only fetch all email addresses when requested for OIDC user info (#6999) changelog: Internal, Performance, Only fetch all email addresses when requested for OIDC user info * Add ESLint enforcement of awaited userEvent interaction (#6995) * Add ESLint enforcement of awaited userEvent **Why**: Avoid developer confusion associated with race conditions caused by not properly awaiting the completion of a userEvent interaction. changelog: Internal, Automated Testing, Improve developer experience for writing interaction tests * Refactor password reset button spec to avoid Mocha "done" API * Refactor PasswordResetButton spec to use Chai promise helprs * Clean up some AB Test bucket code (#6994) - Move to initializer so we're not constantly re-allocating and checking - Remove ActiveModel::Model, it was only half-used - Update DocAuthRouter to use buckets * Update document_capture_step spec and create new FakeAbTestBucket [skip changelog] Co-authored-by: Doug Price <douglas.price@gsa.gov> * Update Rails (#7000) * Update Rails changelog: Internal, Dependencies, Update Rails * Fix patched behavior for redirects and unsafe redirects * Cache phone_configuration queries during OTP authentication (#6998) changelog: Internal, Performance, Cache phone_configuration queries during OTP authentication * Handle zip+0 at GPO verification letter export (#6970) * Handle zip+0 at GPO verification letter export [skip changelog] * Fix short-circuiting in OTP confirmation (#7002) [skip changelog] * Revert strscan version upgrade (#7003) [skip changelog] Co-authored-by: Oren Kanner <oren.kanner@gsa.gov> Co-authored-by: Jonathan Hooper <jonathan.hooper@gsa.gov> Co-authored-by: Melissa Miller <melissa.miller@gsa.gov> Co-authored-by: Doug Price <douglas.price@gsa.gov> Co-authored-by: Sheldon Bachstein <sheldon.bachstein@gsa.gov> Co-authored-by: Matt Wagner <mattwagner@navapbc.com> Co-authored-by: Kimball Bighorse <kbighorse@yahoo.com> Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com> Co-authored-by: olatifflexion <109746710+olatifflexion@users.noreply.github.com> Co-authored-by: John Skiles Skinner <john.skinner@gsa.gov> Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov> Co-authored-by: John Maxwell <john.maxwell@gsa.gov>

orenyk added the status - do not merge label Sep 9, 2022

orenyk requested review from a team, jmhooper and mitchellhenke September 9, 2022 20:45

orenyk force-pushed the oyk-oidc-logout-change branch from 8c2c315 to b10fcdb Compare September 9, 2022 20:51

orenyk marked this pull request as draft September 9, 2022 20:52

orenyk force-pushed the oyk-oidc-logout-change branch 2 times, most recently from 4e764bd to caca7e9 Compare September 9, 2022 21:49

zachmargolis reviewed Sep 9, 2022

View reviewed changes

app/forms/openid_connect_logout_form.rb Outdated Show resolved Hide resolved

zachmargolis reviewed Sep 9, 2022

View reviewed changes

app/forms/openid_connect_logout_form.rb Outdated Show resolved Hide resolved

orenyk force-pushed the oyk-oidc-logout-change branch from caca7e9 to f2c25ff Compare September 9, 2022 22:40

zachmargolis reviewed Sep 9, 2022

View reviewed changes

orenyk force-pushed the oyk-oidc-logout-change branch 5 times, most recently from 4005c5d to 336feea Compare September 12, 2022 05:23

zachmargolis reviewed Sep 12, 2022

View reviewed changes

app/controllers/openid_connect/logout_controller.rb Outdated Show resolved Hide resolved

zachmargolis reviewed Sep 12, 2022

View reviewed changes

app/forms/openid_connect_authorize_form.rb Outdated Show resolved Hide resolved

zachmargolis reviewed Sep 12, 2022

View reviewed changes

app/forms/openid_connect_authorize_form.rb Outdated Show resolved Hide resolved

orenyk force-pushed the oyk-oidc-logout-change branch 3 times, most recently from 130b8a4 to aad2a24 Compare September 13, 2022 04:27

orenyk commented Sep 13, 2022

View reviewed changes

orenyk requested a review from zachmargolis September 13, 2022 04:29

orenyk marked this pull request as ready for review September 13, 2022 04:29

orenyk removed the status - do not merge label Sep 13, 2022

zachmargolis approved these changes Sep 13, 2022

View reviewed changes

app/forms/openid_connect_logout_form.rb Outdated Show resolved Hide resolved

app/forms/openid_connect_logout_form.rb Outdated Show resolved Hide resolved

orenyk force-pushed the oyk-oidc-logout-change branch from aad2a24 to 079a863 Compare September 16, 2022 02:45

orenyk requested a review from zachmargolis September 16, 2022 16:36

mitchellhenke approved these changes Sep 19, 2022

View reviewed changes

zachmargolis approved these changes Sep 19, 2022

View reviewed changes

spec/features/openid_connect/openid_connect_spec.rb Outdated Show resolved Hide resolved

spec/features/openid_connect/openid_connect_spec.rb Outdated Show resolved Hide resolved

spec/features/openid_connect/openid_connect_spec.rb Outdated Show resolved Hide resolved

orenyk force-pushed the oyk-oidc-logout-change branch from 079a863 to 8f30dd0 Compare September 20, 2022 01:08

orenyk merged commit 5c280f3 into main Sep 20, 2022

orenyk deleted the oyk-oidc-logout-change branch September 20, 2022 01:26

mitchellhenke mentioned this pull request Sep 22, 2022

Deploy RC 211 to Production #6997

Merged

mitchellhenke mentioned this pull request Sep 28, 2022

Confirm OIDC logout when using client_id parameter (LG-7629) #7017

Merged

3 tasks

mitchellhenke mentioned this pull request Apr 2, 2025

Always require confirmation of user logout in OIDC Logout #12048

Merged

Conversation

orenyk commented Sep 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zachmargolis Sep 9, 2022

Choose a reason for hiding this comment

Uh oh!

orenyk Sep 12, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

orenyk Sep 12, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zachmargolis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

orenyk commented Sep 16, 2022

Uh oh!

mitchellhenke left a comment

Choose a reason for hiding this comment

Uh oh!

zachmargolis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

orenyk commented Sep 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

orenyk commented Sep 9, 2022 •

edited

Loading