Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

differentiate 401 with 500 #159

Merged
merged 6 commits into from
Nov 20, 2023

Conversation

yishi-ttd
Copy link
Contributor

@yishi-ttd yishi-ttd commented Nov 10, 2023

The error handling logic is on core: https://github.com/IABTechLab/uid2-core/blob/c62c6400a50f121996dd00d85957d59940bebd00/src/main/java/com/uid2/core/vertx/CoreVerticle.java#L237

  • If unhandled exception throws, or AyncResult failed, it will return 500
  • If AsyncResult succeeds, but the AttestationResult failed, it will return 401 with reason.
  • Otherwise, if AttestationResult succeeds, it will return 200.

It would be an overkill to define all error enum in AttestationFailure. Hence I added addtional AttestationClientException field in AttestationResult. If it's not null, we will get failure reason from its error message.

This change will impact new GCP attestation provider and new Azure attestation provider. Other attestation provider will not be impacted.

Copy link
Contributor

@thomasm-ttd thomasm-ttd left a comment

Choose a reason for hiding this comment

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

Not finished my review, but some things to look at

Copy link
Contributor

@thomasm-ttd thomasm-ttd left a comment

Choose a reason for hiding this comment

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

One small logging comment, otherwise approved

@yishi-ttd yishi-ttd merged commit 870d629 into master Nov 20, 2023
3 checks passed
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