Skip to content

Separate OpenID Connect Authorization Request and Handoff Logging#8798

Merged
mitchellhenke merged 4 commits intomainfrom
mitchellhenke/separate-oidc-request-handoff-events
Jul 18, 2023
Merged

Separate OpenID Connect Authorization Request and Handoff Logging#8798
mitchellhenke merged 4 commits intomainfrom
mitchellhenke/separate-oidc-request-handoff-events

Conversation

@mitchellhenke
Copy link
Contributor

🛠 Summary of changes

Currently we do not log successful OIDC auth requests when they are not ultimately going to redirect. This was changed in #6942, so this maintains that by splitting the events and ensuring we always log the request when we get it.

I'm not 100% happy with this, but want to get CI going

@zachmargolis
Copy link
Contributor

What if we added a success: boolean to the existing single event instead of splitting into two?

@mitchellhenke
Copy link
Contributor Author

What if we added a success: boolean to the existing single event instead of splitting into two?

I think the issue is it loses the code_digest if it's not logged where it is now

changelog: Improvements, Logging, Separate OpenID Connect Authorization Request and Handoff Logging
@mitchellhenke mitchellhenke force-pushed the mitchellhenke/separate-oidc-request-handoff-events branch from d44e93d to da7ff19 Compare July 18, 2023 17:10
@zachmargolis
Copy link
Contributor

What if we added a success: boolean to the existing single event instead of splitting into two?

I think the issue is it loses the code_digest if it's not logged where it is now

chatted in Slack, I agree things would be much simpler overall with two events, especially considering the mess we had to go through to log code_digest in that previous PR

Mitchell Henke and others added 2 commits July 18, 2023 12:30
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
@mitchellhenke mitchellhenke marked this pull request as ready for review July 18, 2023 17:48
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mitchellhenke mitchellhenke merged commit d2f53d5 into main Jul 18, 2023
@mitchellhenke mitchellhenke deleted the mitchellhenke/separate-oidc-request-handoff-events branch July 18, 2023 18:20
@aduth aduth mentioned this pull request Jul 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants