Log digest of OIDC "code" param (LG-7440) by zachmargolis · Pull Request #6942 · 18F/identity-idp

zachmargolis · 2022-09-12T20:43:22Z

Why: to assist with debugging requests from partners

changelog: Internal, Logging, Log hash of OpenID Connect "code" param

**Why**: to assist with debugging requests from partners changelog: Internal, Logging, Log hash of OpenID Connect "code" param

zachmargolis · 2022-09-12T20:45:35Z

spec/controllers/openid_connect/authorization_controller_spec.rb

                 acr_values: 'http://idmanagement.gov/ns/assurance/ial/1',
-                 scope: 'openid')
+                 scope: 'openid',
+                 code_digest: anything)


in an ideal world, this should be kind_of(String) however, due to the mess of redirects, side effects, what-not, this ends up being nil in these controller specs, which is super unfortunate.

I would really like to reorganize the interactions between:

The OIDC::AuthorizationController

The IdentityLinker class

and the OpenIdConnectAuthorizeForm

since there are some side effecty, ordering, inter-dependencies that I am not a fan of right now

🤠 😭 😎 turns out this is nil in real life too... time for some gross code changes

I'm not proud of this but 33bf61e seems to cover this and I tried to add explanatory comments

**How**: By submitting the form a second time after the identity has been linked, because we need the session uuid from that linkage for the code, for the param. Somehow it was getting into the success_redirect_uri before

app/controllers/openid_connect/authorization_controller.rb

aduth · 2022-09-13T13:03:58Z

app/controllers/openid_connect/authorization_controller.rb

      return redirect_to(sign_up_completed_url) if needs_completion_screen_reason
      link_identity_to_service_provider
+
+      result = @authorize_form.submit


Would it be worth changing this up so that pre_validate_authorize_form is either merged into this method or called from the method, such that the same result could be shared by both?

I don't think the logic for validating is especially heavy, but could be worth optimizing since I imagine it's a hot path, and there's quite a few validations in OpenidConnectAuthorizeForm.

The problem is, we need to run link_identity_to_service_provider, but only if the the form is successful and so if we want to get the right code in the result payload from the form, we need to run that after it links... so that's why I went with this double submit/sandwich approach.

I think a right thing to do would be to fund a way to untangle, but I feel like given time constraints, now is not that time? I did file LG-7544 so we could keep track of this

aduth · 2022-09-13T13:13:23Z

app/forms/openid_connect_authorize_form.rb

      scope: scope&.sort&.join(' '),
      acr_values: acr_values&.sort&.join(' '),
      unauthorized_scope: @unauthorized_scope,
+      code_digest: code ? Digest::SHA256.hexdigest(code) : nil,


Rather than reading code as a byproduct of success_redirect_uri, could we reference it the same way that success_redirect_uri is assigning it?

Suggested change

code_digest: code ? Digest::SHA256.hexdigest(code) : nil,

code_digest: identity&.session_uuid ? Digest::SHA256.hexdigest(identity&.session_uuid) : nil,

hmmm I like that idea, maybe we make code into a getter method and share that in both places? I gave that a shot in 711bb00

Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>

aduth

LGTM 👍

* LG-6497: Revert Memorable Date Changes (#6940) * Revert "LG-6497: Minor style/doc fixes related to memorable date change (#6929)" This reverts commit e0e8ad4. * Revert "LG-6497: Create and use new Memorable Date component in State ID step of IPP flow (#6713)" This reverts commit cead2c1. * [skip changelog] * Build frontend analytics method parameters from method signature (#6927) * Build frontend analytics method parameters from method signature **Why**: - To avoid confusion where a developer would expect analytics method parameters to be fulfilled from the frontend payload (#6791) - To ensure that all of the analytics methods are explicit and documented in the parameters they're expecting See: #6918 (comment) changelog: Internal, Analytics, Improve frontend analytics payload documentation * Fix YARDoc parameter name documentation * Restore flow path parameter to front-end mapped events * Add flow_path handling to specs * Destructure, use compact for kwarg aggregation * Extract hash_from_method_kwargs to service class See: #6927 (comment) * Extract FrontendLogger service class from FrontendLogController See: #6927 (comment) * Remove leftover code from earlier iterations * Collapse MethodSignatureHashBuilder to FrontendLogger See: #6927 (comment) * LG-6497: Reintroduce Memorable Date on IPP State ID Page (#6943) * Revert "LG-6497: Revert Memorable Date Changes (#6940)" This reverts commit d2500a8. * [skip changelog] * Try to fix intermittent failure for in-person feature spec (#6933) **Why**: So that our builds pass reliably. changelog: Internal, Automated Testing, Reduce intermittent failures in automated testing * LG-7431 Logged In User Change Password (#6948) * LG-7431 Logged In User Change Password changelog: Internal, Attempts API, Track additional events * Qualify script rails executable properly with bin/ (#6951) changelog: Internal, Upcoming Features, Qualify script rails executable properly with bin/ * LG-6497: Workaround New Relic addEventListener bug and improve error styles (#6938) * LG-6497: Circumvent New Relic event bug * LG-6497: Ensure memorable date correctly assigns errors for error styling * [skip changelog] * Update app/javascript/packages/memorable-date/index.spec.ts Co-authored-by: Sheldon Bachstein <sheldon.bachstein@gsa.gov> * Revert "LG-6497: Revert Memorable Date Changes (#6940)" This reverts commit d2500a8. * [skip changelog] * Update app/javascript/packages/memorable-date/index.ts Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov> Co-authored-by: Sheldon Bachstein <sheldon.bachstein@gsa.gov> Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov> * Refactor PhoneFinder (#6920) This commit refactors the PhoneFinder proofer client so that it does not user the `Proofer::Base` super class any more. It converts the PhoneFinder proofer into a plain old ruby object with its own result object. [skip changelog] Co-authored-by: Jonathan Hooper <jonathan.hooper@gsa.gov> * ThreatMetrix JS verification (LG-7518) (#6937) * Add ThreatMetrix code signing cert to config Cert includes public key used to verify signature on ThreatMetrix Javascript * Add job to verify ThreatMetrix javascript Periodically request TMX JS and validate the signature at the end of the file * Remove stale job stuff * Don't hardcode test certs Generate keys / certs so we don't have scary-looking private keys in repo * Minor tweak to session_id arg changelog: Upcoming Features, ThreatMetrix, Periodically check signature on ThreatMetrix Javascript * Use pack to do hex-to-binary conversion * how do you spell Javascript * Fail if signing certificate is expired * Run ThreatMetrixJsVerificationJob on 1h intervals * Remove cert from application.default.yml will be in config, just not going to include in code * Lint * Trigger devops on merges to main. (#6944) * Trigger devops on merges to main. * typo * testing [skip changelog] * Only run on main * Update total-monthly-auths report to pull from the raw table (#6952) **Why**: We've found some discrepancies with the aggregated monthly table so this helps us have more precise reports changelog: Internal, Reporting, Update billing reports to be more accurate * Log digest of OIDC "code" param (LG-7440) (#6942) * Log digest of OIDC "code" param (LG-7440) **Why**: to assist with debugging requests from partners * Work around nil code **How**: By submitting the form a second time after the identity has been linked, because we need the session uuid from that linkage for the code, for the param. Somehow it was getting into the success_redirect_uri before changelog: Internal, Logging, Log hash of OpenID Connect "code" param Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov> * update ready to verify presenter to use enrollment_established_at (#6950) * update ready to verify presenter to use enrollment_established_at * [skip changelog] * refactor * fix lint errors * added new event for idv phone send link rate limit (#6958) changelog: Internal, Attempts API, Track additional events * Lg 7545 password change reauth (#6954) * LG-7545 Password Change Reauth Submit changelog: Internal, Attempts API, Track additional events * LG-7411 Analytics event for TM Proofing Failure Page (#6953) * added setup errors visited * Add redirect tracking to contact url on sorry page * add changelog changelog: Internal, Analytics, Add analytics to sorry for proofing page * add tracker method for contact page analytics * LG-6497: Set min date for State ID page to January 1, 1900 (#6960) * LG-6497: Set min date for State ID page to January 1, 1900; remove leading zero from day * [skip changelog] * LG-7203: Use DB for service provider IPP feature enablement (#6945) * LG-7203: Add in_person_proofing_enabled field to service_providers; update feature check to use new field * LG-7203: Replace use of hardcoded IPP issuer config and update tests * changelog: Upcoming Features, In-person proofing, Use DB for configuring service providers * LG-7203: Switch to leaner query query clear expected value for in_person_proofing_enabled * LG-7547-Password-Change-Reauthentication-Rate-Limit (#6955) * LG-7547-Password-Change-Reauthentication-Rate-Limit changelog: Internal, Attempts API, Track additional events * LG-7195 | Adds reproof completed event (#6957) changelog: Internal, Attempts API, Adds reproofing complete event * Update Password Change to Profile Change (#6962) changelog: Internal, Attempts API, Track additional events * Enforce YARD parameter documentation for tracker_events.rb, fix errors (#6964) [skip changelog] Co-authored-by: Tim Bradley <90272033+NavaTim@users.noreply.github.com> Co-authored-by: Luis H. Matos <ThatSpaceGuy@users.noreply.github.com> Co-authored-by: Gene M. Angelo, Jr <web.gma@gmail.com> Co-authored-by: Sheldon Bachstein <sheldon.bachstein@gsa.gov> Co-authored-by: Kimball Bighorse <kimball.bighorse@gsa.gov> Co-authored-by: Jonathan Hooper <jonathan.hooper@gsa.gov> Co-authored-by: Matt Hinz <matthinz@gmail.com> Co-authored-by: Alex Kritikos <alex.kritikos@gsa.gov> Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com> Co-authored-by: Shannon A <20867088+svalexander@users.noreply.github.com> Co-authored-by: Rwolfe-Nava <87499456+Rwolfe-Nava@users.noreply.github.com> Co-authored-by: Alex Bradley <alexander.bradley@gsa.gov> Co-authored-by: Matt Wagner <mattwagner@navapbc.com>

Log digest of OIDC "code" param (LG-7440)

c97dc76

**Why**: to assist with debugging requests from partners changelog: Internal, Logging, Log hash of OpenID Connect "code" param

zachmargolis requested review from a team September 12, 2022 20:43

zachmargolis commented Sep 12, 2022

View reviewed changes

Work around nil code

33bf61e

**How**: By submitting the form a second time after the identity has been linked, because we need the session uuid from that linkage for the code, for the param. Somehow it was getting into the success_redirect_uri before

aduth approved these changes Sep 13, 2022

View reviewed changes

zachmargolis and others added 4 commits September 13, 2022 07:57

fix typo

27c84a1

Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>

Refactor code into a method

711bb00

lint fix, useless assignment

bd95292

fix YARD syntax

ec953bc

aduth approved these changes Sep 13, 2022

View reviewed changes

zachmargolis merged commit 7130173 into main Sep 13, 2022

zachmargolis deleted the margolis-code-digest-log branch September 13, 2022 19:55

aduth mentioned this pull request Sep 15, 2022

Deploy RC 209 to Prod #6966

Merged

mitchellhenke mentioned this pull request Jul 18, 2023

Separate OpenID Connect Authorization Request and Handoff Logging #8798

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Log digest of OIDC "code" param (LG-7440)#6942

Log digest of OIDC "code" param (LG-7440)#6942
zachmargolis merged 6 commits intomainfrom
margolis-code-digest-log

zachmargolis commented Sep 12, 2022

Uh oh!

zachmargolis Sep 12, 2022

Uh oh!

zachmargolis Sep 12, 2022

Uh oh!

zachmargolis Sep 12, 2022

Uh oh!

Uh oh!

aduth Sep 13, 2022

Uh oh!

zachmargolis Sep 13, 2022

Uh oh!

aduth Sep 13, 2022

Uh oh!

zachmargolis Sep 13, 2022

Uh oh!

aduth left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	code_digest: code ? Digest::SHA256.hexdigest(code) : nil,
	code_digest: identity&.session_uuid ? Digest::SHA256.hexdigest(identity&.session_uuid) : nil,

Conversation

zachmargolis commented Sep 12, 2022

Uh oh!

zachmargolis Sep 12, 2022

Choose a reason for hiding this comment

Uh oh!

zachmargolis Sep 12, 2022

Choose a reason for hiding this comment

Uh oh!

zachmargolis Sep 12, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aduth Sep 13, 2022

Choose a reason for hiding this comment

Uh oh!

zachmargolis Sep 13, 2022

Choose a reason for hiding this comment

Uh oh!

aduth Sep 13, 2022

Choose a reason for hiding this comment

Uh oh!

zachmargolis Sep 13, 2022

Choose a reason for hiding this comment

Uh oh!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants