-
Notifications
You must be signed in to change notification settings - Fork 6
Propagate error info to caller #1174
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
base: rc-v0.6.7
Are you sure you want to change the base?
Changes from 2 commits
2112668
41313db
408f312
b83bb52
e991e15
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,61 +1,5 @@ | ||||||||||||||||
| package co.worklytics.psoxy.gateway.impl; | ||||||||||||||||
|
|
||||||||||||||||
| import java.io.IOException; | ||||||||||||||||
| import java.net.ConnectException; | ||||||||||||||||
| import java.net.URL; | ||||||||||||||||
| import java.nio.charset.StandardCharsets; | ||||||||||||||||
| import java.time.Instant; | ||||||||||||||||
| import java.util.Collection; | ||||||||||||||||
| import java.util.Collections; | ||||||||||||||||
| import java.util.HashMap; | ||||||||||||||||
| import java.util.List; | ||||||||||||||||
| import java.util.Locale; | ||||||||||||||||
| import java.util.Map; | ||||||||||||||||
| import java.util.Objects; | ||||||||||||||||
| import java.util.Optional; | ||||||||||||||||
| import java.util.Set; | ||||||||||||||||
| import java.util.UUID; | ||||||||||||||||
| import java.util.function.Function; | ||||||||||||||||
| import java.util.function.Supplier; | ||||||||||||||||
| import java.util.logging.Level; | ||||||||||||||||
| import java.util.regex.Pattern; | ||||||||||||||||
| import java.util.stream.Collectors; | ||||||||||||||||
| import javax.inject.Inject; | ||||||||||||||||
| import javax.inject.Named; | ||||||||||||||||
| import javax.inject.Provider; | ||||||||||||||||
| import org.apache.commons.lang3.StringUtils; | ||||||||||||||||
| import org.apache.commons.lang3.tuple.Pair; | ||||||||||||||||
| import org.apache.hc.client5.http.classic.methods.HttpGet; | ||||||||||||||||
| import org.apache.hc.client5.http.classic.methods.HttpHead; | ||||||||||||||||
| import org.apache.hc.core5.http.ContentType; | ||||||||||||||||
| import org.apache.hc.core5.http.HttpHeaders; | ||||||||||||||||
| import org.apache.hc.core5.http.NameValuePair; | ||||||||||||||||
| import org.apache.hc.core5.http.message.BasicNameValuePair; | ||||||||||||||||
| import org.apache.hc.core5.net.WWWFormCodec; | ||||||||||||||||
| import org.apache.http.HttpStatus; | ||||||||||||||||
| import org.apache.http.client.utils.URIBuilder; | ||||||||||||||||
| import com.avaulta.gateway.pseudonyms.PseudonymImplementation; | ||||||||||||||||
| import com.avaulta.gateway.pseudonyms.impl.UrlSafeTokenPseudonymEncoder; | ||||||||||||||||
| import com.avaulta.gateway.tokens.ReversibleTokenizationStrategy; | ||||||||||||||||
| import com.fasterxml.jackson.annotation.JsonInclude; | ||||||||||||||||
| import com.fasterxml.jackson.databind.JsonNode; | ||||||||||||||||
| import com.fasterxml.jackson.databind.ObjectMapper; | ||||||||||||||||
| import com.fasterxml.jackson.databind.node.ArrayNode; | ||||||||||||||||
| import com.fasterxml.jackson.databind.node.ObjectNode; | ||||||||||||||||
| import com.google.api.client.http.ByteArrayContent; | ||||||||||||||||
| import com.google.api.client.http.GenericUrl; | ||||||||||||||||
| import com.google.api.client.http.HttpContent; | ||||||||||||||||
| import com.google.api.client.http.HttpRequest; | ||||||||||||||||
| import com.google.api.client.http.HttpRequestFactory; | ||||||||||||||||
| import com.google.api.client.http.HttpResponse; | ||||||||||||||||
| import com.google.api.client.http.HttpTransport; | ||||||||||||||||
| import com.google.auth.Credentials; | ||||||||||||||||
| import com.google.auth.http.HttpCredentialsAdapter; | ||||||||||||||||
| import com.google.auth.http.HttpTransportFactory; | ||||||||||||||||
| import com.google.common.annotations.VisibleForTesting; | ||||||||||||||||
| import com.google.common.base.Joiner; | ||||||||||||||||
| import com.google.common.collect.ImmutableSet; | ||||||||||||||||
| import com.google.common.collect.Sets; | ||||||||||||||||
| import co.worklytics.psoxy.ControlHeader; | ||||||||||||||||
| import co.worklytics.psoxy.ErrorCauses; | ||||||||||||||||
| import co.worklytics.psoxy.ProcessedDataMetadataFields; | ||||||||||||||||
|
|
@@ -69,6 +13,7 @@ | |||||||||||||||
| import co.worklytics.psoxy.gateway.HttpEventRequest; | ||||||||||||||||
| import co.worklytics.psoxy.gateway.HttpEventResponse; | ||||||||||||||||
| import co.worklytics.psoxy.gateway.ProcessedContent; | ||||||||||||||||
| import co.worklytics.psoxy.gateway.ProxyConstants; | ||||||||||||||||
| import co.worklytics.psoxy.gateway.SecretStore; | ||||||||||||||||
| import co.worklytics.psoxy.gateway.SourceAuthStrategy; | ||||||||||||||||
| import co.worklytics.psoxy.gateway.impl.output.OutputUtils; | ||||||||||||||||
|
|
@@ -81,6 +26,28 @@ | |||||||||||||||
| import co.worklytics.psoxy.utils.ComposedHttpRequestInitializer; | ||||||||||||||||
| import co.worklytics.psoxy.utils.GzipedContentHttpRequestInitializer; | ||||||||||||||||
| import co.worklytics.psoxy.utils.URLUtils; | ||||||||||||||||
| import com.avaulta.gateway.pseudonyms.PseudonymImplementation; | ||||||||||||||||
| import com.avaulta.gateway.pseudonyms.impl.UrlSafeTokenPseudonymEncoder; | ||||||||||||||||
| import com.avaulta.gateway.tokens.ReversibleTokenizationStrategy; | ||||||||||||||||
| import com.fasterxml.jackson.annotation.JsonInclude; | ||||||||||||||||
| import com.fasterxml.jackson.databind.JsonNode; | ||||||||||||||||
| import com.fasterxml.jackson.databind.ObjectMapper; | ||||||||||||||||
| import com.fasterxml.jackson.databind.node.ArrayNode; | ||||||||||||||||
| import com.fasterxml.jackson.databind.node.ObjectNode; | ||||||||||||||||
| import com.google.api.client.http.ByteArrayContent; | ||||||||||||||||
| import com.google.api.client.http.GenericUrl; | ||||||||||||||||
| import com.google.api.client.http.HttpContent; | ||||||||||||||||
| import com.google.api.client.http.HttpRequest; | ||||||||||||||||
| import com.google.api.client.http.HttpRequestFactory; | ||||||||||||||||
| import com.google.api.client.http.HttpResponse; | ||||||||||||||||
| import com.google.api.client.http.HttpTransport; | ||||||||||||||||
| import com.google.auth.Credentials; | ||||||||||||||||
| import com.google.auth.http.HttpCredentialsAdapter; | ||||||||||||||||
| import com.google.auth.http.HttpTransportFactory; | ||||||||||||||||
| import com.google.common.annotations.VisibleForTesting; | ||||||||||||||||
| import com.google.common.base.Joiner; | ||||||||||||||||
| import com.google.common.collect.ImmutableSet; | ||||||||||||||||
| import com.google.common.collect.Sets; | ||||||||||||||||
| import dagger.Lazy; | ||||||||||||||||
| import lombok.AllArgsConstructor; | ||||||||||||||||
| import lombok.Builder; | ||||||||||||||||
|
|
@@ -90,7 +57,32 @@ | |||||||||||||||
| import lombok.SneakyThrows; | ||||||||||||||||
| import lombok.Value; | ||||||||||||||||
| import lombok.extern.java.Log; | ||||||||||||||||
| import co.worklytics.psoxy.gateway.ProxyConstants; | ||||||||||||||||
| import org.apache.commons.lang3.StringUtils; | ||||||||||||||||
| import org.apache.commons.lang3.tuple.Pair; | ||||||||||||||||
| import org.apache.hc.client5.http.classic.methods.HttpGet; | ||||||||||||||||
| import org.apache.hc.client5.http.classic.methods.HttpHead; | ||||||||||||||||
| import org.apache.hc.core5.http.ContentType; | ||||||||||||||||
| import org.apache.hc.core5.http.HttpHeaders; | ||||||||||||||||
| import org.apache.hc.core5.http.NameValuePair; | ||||||||||||||||
| import org.apache.hc.core5.http.message.BasicNameValuePair; | ||||||||||||||||
| import org.apache.hc.core5.net.WWWFormCodec; | ||||||||||||||||
| import org.apache.http.HttpStatus; | ||||||||||||||||
| import org.apache.http.client.utils.URIBuilder; | ||||||||||||||||
|
|
||||||||||||||||
| import javax.inject.Inject; | ||||||||||||||||
| import javax.inject.Named; | ||||||||||||||||
| import javax.inject.Provider; | ||||||||||||||||
| import java.io.IOException; | ||||||||||||||||
| import java.net.ConnectException; | ||||||||||||||||
| import java.net.URL; | ||||||||||||||||
| import java.nio.charset.StandardCharsets; | ||||||||||||||||
| import java.time.Instant; | ||||||||||||||||
| import java.util.*; | ||||||||||||||||
| import java.util.function.Function; | ||||||||||||||||
| import java.util.function.Supplier; | ||||||||||||||||
| import java.util.logging.Level; | ||||||||||||||||
| import java.util.regex.Pattern; | ||||||||||||||||
| import java.util.stream.Collectors; | ||||||||||||||||
|
|
||||||||||||||||
| @NoArgsConstructor(onConstructor_ = @Inject) | ||||||||||||||||
| @Log | ||||||||||||||||
|
|
@@ -241,6 +233,17 @@ public HttpEventResponse handle(HttpEventRequest requestToProxy, | |||||||||||||||
| // our canonicalization code for this to go wrong | ||||||||||||||||
| log.log(Level.WARNING, "Error parsing / building request URL", e); | ||||||||||||||||
|
|
||||||||||||||||
| // InvalidTokenException extends RuntimeException | ||||||||||||||||
| if (e instanceof ReversibleTokenizationStrategy.InvalidTokenException ite) { | ||||||||||||||||
| return HttpEventResponse.builder() | ||||||||||||||||
| // should this be a 500? Maybe 422 Unprocessable Content? | ||||||||||||||||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 500 will make us retry, and same call would yield same results. |
||||||||||||||||
| .statusCode(HttpStatus.SC_INTERNAL_SERVER_ERROR) | ||||||||||||||||
| .header(ProcessedDataMetadataFields.ERROR.getHttpHeader(), | ||||||||||||||||
| ErrorCauses.FAILED_TO_BUILD_URL.name()) | ||||||||||||||||
|
||||||||||||||||
| // should this be a 500? Maybe 422 Unprocessable Content? | |
| .statusCode(HttpStatus.SC_INTERNAL_SERVER_ERROR) | |
| .header(ProcessedDataMetadataFields.ERROR.getHttpHeader(), | |
| ErrorCauses.FAILED_TO_BUILD_URL.name()) | |
| .statusCode(HttpStatus.SC_CONFLICT) | |
| .header(ProcessedDataMetadataFields.ERROR.getHttpHeader(), | |
| ErrorCauses.TOKENIZED_REQUEST_PARAMETER_INVALID.name()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I agree not a 500 ... whatever 4xx seems most semantically correct for the case.
Copilot
AI
Apr 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The newly added behavior that surfaces InvalidTokenException details during URL building (and the associated status/header/body mapping) doesn’t appear to be covered by tests. Please add/extend ApiDataRequestHandlerTest to assert the response status and X-Psoxy-Error header (and any new detail header/body) for this failure mode, so future refactors don’t silently change the contract.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,7 @@ | ||
| package com.avaulta.gateway.tokens; | ||
|
|
||
| import lombok.Getter; | ||
|
|
||
| import java.util.function.Function; | ||
|
|
||
| /** | ||
|
|
@@ -41,11 +43,32 @@ default byte[] getReversibleToken(String originalDatum) { | |
|
|
||
| /** | ||
| * Indicates that the token could not be reversed, likely because invalid | ||
|
eschultink marked this conversation as resolved.
Outdated
|
||
| * | ||
| */ | ||
| class InvalidTokenException extends RuntimeException { | ||
| public InvalidTokenException(String message, Throwable cause) { | ||
| super(message, cause); | ||
|
|
||
| @Getter | ||
| public enum ErrorCode { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe not the best approach, but for certain cases (rules exceptions, this, and access errors) could be helpful to exactly know what's going on rather than a generic error |
||
| ALGORITHM_PARAMETER_ERROR("ITE01", "Failed to decrypt token; some algorithm parameter, such as iv, is wrong"), | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no need to be so clever here; just return whole enum names ... don't care about a few extra chars in error case |
||
| BAD_PADDING("ITE02", "Failed to decrypt token; token appears to be corrupted or invalid, due to mismatch between token's padding and the padding expected by the cipher mode"), | ||
|
eschultink marked this conversation as resolved.
Outdated
|
||
| ILLEGAL_BLOCK_SIZE("ITE003", "Failed to decrypt token; token appears to be corrupted or invalid, as block size seems to differ from expected"), | ||
| DECRYPTION_FAILED("ITE004", "Failed to decrypt token; most likely because encryption key has been rotated"); | ||
|
|
||
| private final String code; | ||
| private final String message; | ||
|
|
||
| ErrorCode(String code, String message) { | ||
| this.code = code; | ||
| this.message = message; | ||
| } | ||
|
|
||
| } | ||
|
|
||
| @Getter | ||
| private final ErrorCode errorCode; | ||
|
|
||
| public InvalidTokenException(ErrorCode errorCode, Throwable cause) { | ||
| super("[" + errorCode.getCode() + "] " + errorCode.getMessage(), cause); | ||
| this.errorCode = errorCode; | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think Error is a value from an enum ... so is specific
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so if Error is a strict code -- I think eliminate this, just use
Error; but if it's freeform message text, OK.