Skip to content

Improve Logging of OIDC Token Event#5485

Merged
mitchellhenke merged 4 commits intomainfrom
mitchellhenke/ahoy-there
Oct 8, 2021
Merged

Improve Logging of OIDC Token Event#5485
mitchellhenke merged 4 commits intomainfrom
mitchellhenke/ahoy-there

Conversation

@mitchellhenke
Copy link
Contributor

@mitchellhenke mitchellhenke commented Oct 8, 2021

The OIDC Token request is almost always server to server, but our event log tries to prevent bots from logging events. We should allow "bots" since they are able to make valid requests in some situations.

I've also split up some token validation logic in cases where the token is expired. This will give us slightly more insight when troubleshooting token request issues with partners because we have more specific errors, and will include the service_provider in the metadata if we can find it, even if the token is invalid/expired.

@mitchellhenke mitchellhenke force-pushed the mitchellhenke/ahoy-there branch from 3edb8e0 to dcd9574 Compare October 8, 2021 14:42
@mitchellhenke mitchellhenke force-pushed the mitchellhenke/ahoy-there branch 3 times, most recently from 223622c to 8443045 Compare October 8, 2021 15:19
Copy link
Contributor

Choose a reason for hiding this comment

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

Can probably remove this @session_expiration assignment and just inline it in the expiration checking method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was worried about changing the expiration to be dependent on the order of the validations. Though, if it's going to be an attribute, it probably makes sense to move it up into initialize?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved in a7116fa

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO it's not even worth saving as an attribute? It's fairly cheap to calculate, and the start of validation and the finish of validation are not more than a few ms apart I would guess

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was close to adding it as a parameter with a default because it felt like something that could be more explicit and exposed to the caller, but I wasn't 100% sure.

@mitchellhenke mitchellhenke force-pushed the mitchellhenke/ahoy-there branch from 8443045 to ccd06d0 Compare October 8, 2021 15:24
@mitchellhenke mitchellhenke marked this pull request as ready for review October 8, 2021 15:32
invalid_code: is invalid either because it expired, or it doesn’t match any
user. Please see our documentation at
https://developers.login.gov/oidc/#token
invalid_code: is invalid because doesn’t match any user. Please see our
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a translation request out for the separated error messages

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added in 88122d3

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 just confirming we have a test later in the file that checks for invalid_code

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 [pending translations]

@mitchellhenke mitchellhenke force-pushed the mitchellhenke/ahoy-there branch from 88122d3 to cdf4524 Compare October 8, 2021 16:31
@mitchellhenke mitchellhenke merged commit 5e70916 into main Oct 8, 2021
@mitchellhenke mitchellhenke deleted the mitchellhenke/ahoy-there branch October 8, 2021 18:05
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