From d92be4e69b4fddcff487c1c5a7a289e8bc75a366 Mon Sep 17 00:00:00 2001 From: Darin Kelkhoff Date: Thu, 1 Aug 2024 15:00:36 -0500 Subject: [PATCH] don't duplicate apikey=value in re-tries; mask api key in outboundApiLog urls --- .../module/api/actions/BaseAPIActionUtil.java | 114 +++++++++++------- .../api/actions/BaseAPIActionUtilTest.java | 68 +++++++++++ 2 files changed, 138 insertions(+), 44 deletions(-) diff --git a/qqq-backend-module-api/src/main/java/com/kingsrook/qqq/backend/module/api/actions/BaseAPIActionUtil.java b/qqq-backend-module-api/src/main/java/com/kingsrook/qqq/backend/module/api/actions/BaseAPIActionUtil.java index 57cef42d..ae9fe8d0 100644 --- a/qqq-backend-module-api/src/main/java/com/kingsrook/qqq/backend/module/api/actions/BaseAPIActionUtil.java +++ b/qqq-backend-module-api/src/main/java/com/kingsrook/qqq/backend/module/api/actions/BaseAPIActionUtil.java @@ -734,20 +734,7 @@ public class BaseAPIActionUtil case API_KEY_HEADER -> request.setHeader("API-Key", backendMetaData.getApiKey()); case API_TOKEN -> request.setHeader("Authorization", "Token " + backendMetaData.getApiKey()); case OAUTH2 -> request.setHeader("Authorization", "Bearer " + getOAuth2Token()); - case API_KEY_QUERY_PARAM -> - { - try - { - String uri = request.getURI().toString(); - uri += (uri.contains("?") ? "&" : "?"); - uri += backendMetaData.getApiKeyQueryParamName() + "=" + backendMetaData.getApiKey(); - request.setURI(new URI(uri)); - } - catch(URISyntaxException e) - { - throw (new QException("Error setting authorization query parameter", e)); - } - } + case API_KEY_QUERY_PARAM -> addApiKeyQueryParamToRequest(request); case CUSTOM -> { handleCustomAuthorization(request); @@ -758,6 +745,35 @@ public class BaseAPIActionUtil + /*************************************************************************** + ** + ***************************************************************************/ + protected void addApiKeyQueryParamToRequest(HttpRequestBase request) throws QException + { + try + { + String uri = request.getURI().toString(); + String pair = backendMetaData.getApiKeyQueryParamName() + "=" + backendMetaData.getApiKey(); + + /////////////////////////////////////////////////////////////////////////////////// + // avoid re-adding the name=value pair if it's already there (e.g., for a retry) // + /////////////////////////////////////////////////////////////////////////////////// + if(!uri.contains(pair)) + { + uri += (uri.contains("?") ? "&" : "?"); + uri += pair; + } + + request.setURI(new URI(uri)); + } + catch(URISyntaxException e) + { + throw (new QException("Error setting authorization query parameter", e)); + } + } + + + /******************************************************************************* ** *******************************************************************************/ @@ -1207,39 +1223,11 @@ public class BaseAPIActionUtil return; } - String requestBody = null; - if(request instanceof HttpEntityEnclosingRequest entityRequest) - { - try - { - requestBody = StringUtils.join("\n", IOUtils.readLines(entityRequest.getEntity().getContent())); - } - catch(Exception e) - { - // leave it null... - } - } - - //////////////////////////////////// - // mask api keys in query strings // - //////////////////////////////////// - String url = request.getURI().toString(); - if(backendMetaData.getAuthorizationType().equals(AuthorizationType.API_KEY_QUERY_PARAM)) - { - url = url.replaceFirst(backendMetaData.getApiKey(), "******"); - } + OutboundAPILog outboundAPILog = generateOutboundApiLogRecord(request, response); InsertInput insertInput = new InsertInput(); insertInput.setTableName(table.getName()); - insertInput.setRecords(List.of(new OutboundAPILog() - .withMethod(request.getMethod()) - .withUrl(url) - .withTimestamp(Instant.now()) - .withRequestBody(requestBody) - .withStatusCode(response.getStatusCode()) - .withResponseBody(response.getContent()) - .toQRecord() - )); + insertInput.setRecords(List.of(outboundAPILog.toQRecord())); new InsertAction().executeAsync(insertInput); } catch(Exception e) @@ -1250,6 +1238,44 @@ public class BaseAPIActionUtil + /*************************************************************************** + ** + ***************************************************************************/ + public OutboundAPILog generateOutboundApiLogRecord(HttpRequestBase request, QHttpResponse response) + { + String requestBody = null; + if(request instanceof HttpEntityEnclosingRequest entityRequest) + { + try + { + requestBody = StringUtils.join("\n", IOUtils.readLines(entityRequest.getEntity().getContent(), StandardCharsets.UTF_8)); + } + catch(Exception e) + { + // leave it null... + } + } + + //////////////////////////////////// + // mask api keys in query strings // + //////////////////////////////////// + String url = request.getURI().toString(); + if(backendMetaData.getAuthorizationType().equals(AuthorizationType.API_KEY_QUERY_PARAM)) + { + url = url.replaceAll(backendMetaData.getApiKey(), "******"); + } + + return new OutboundAPILog() + .withMethod(request.getMethod()) + .withUrl(url) + .withTimestamp(Instant.now()) + .withRequestBody(requestBody) + .withStatusCode(response.getStatusCode()) + .withResponseBody(response.getContent()); + } + + + /******************************************************************************* ** *******************************************************************************/ diff --git a/qqq-backend-module-api/src/test/java/com/kingsrook/qqq/backend/module/api/actions/BaseAPIActionUtilTest.java b/qqq-backend-module-api/src/test/java/com/kingsrook/qqq/backend/module/api/actions/BaseAPIActionUtilTest.java index 60923ea5..ad985295 100644 --- a/qqq-backend-module-api/src/test/java/com/kingsrook/qqq/backend/module/api/actions/BaseAPIActionUtilTest.java +++ b/qqq-backend-module-api/src/test/java/com/kingsrook/qqq/backend/module/api/actions/BaseAPIActionUtilTest.java @@ -61,6 +61,7 @@ import com.kingsrook.qqq.backend.core.model.metadata.QInstance; import com.kingsrook.qqq.backend.core.model.metadata.tables.UniqueKey; import com.kingsrook.qqq.backend.core.utils.CollectionUtils; import com.kingsrook.qqq.backend.core.utils.SleepUtils; +import com.kingsrook.qqq.backend.core.utils.lambdas.UnsafeConsumer; import com.kingsrook.qqq.backend.module.api.BaseTest; import com.kingsrook.qqq.backend.module.api.TestUtils; import com.kingsrook.qqq.backend.module.api.exceptions.RateLimitException; @@ -74,6 +75,7 @@ import org.apache.http.Header; import org.apache.http.HttpEntity; import org.apache.http.client.methods.HttpEntityEnclosingRequestBase; import org.apache.http.client.methods.HttpGet; +import org.apache.http.client.methods.HttpRequestBase; import org.json.JSONArray; import org.json.JSONObject; import org.junit.jupiter.api.BeforeEach; @@ -868,6 +870,72 @@ class BaseAPIActionUtilTest extends BaseTest + /******************************************************************************* + ** + *******************************************************************************/ + @Test + void testAuthorizationApiKeyQueryParam() throws QException + { + APIBackendMetaData backend = (APIBackendMetaData) QContext.getQInstance().getBackend(TestUtils.MOCK_BACKEND_NAME); + backend.setAuthorizationType(AuthorizationType.API_KEY_QUERY_PARAM); + backend.setApiKeyQueryParamName("apikey"); + backend.setApiKey("9876-WXYZ"); + + //////////////////////////////////////////////////////////////////////////////////////////// + // this will make it not use the mock makeRequest method, // + // but instead the mock executeHttpRequest, so we can test code from the base makeRequest // + //////////////////////////////////////////////////////////////////////////////////////////// + mockApiUtilsHelper.setUseMock(false); + + ////////////////////////////////////////////////////////////////////////////// + // we'll want to assert that the URL has the api query string - and just // + // one copy of it (as we once had a bug where it got duplicated upon retry) // + ////////////////////////////////////////////////////////////////////////////// + UnsafeConsumer asserter = request -> assertThat(request.getURI().toString()) + .contains("?apikey=9876-WXYZ") + .doesNotContain("?apikey=9876-WXYZ&apikey=9876-WXYZ"); + + //////////////////////////////////////// + // queue up a 429, so we'll try-again // + //////////////////////////////////////// + mockApiUtilsHelper.setMockRequestAsserter(asserter); + mockApiUtilsHelper.enqueueMockResponse(new QHttpResponse().withStatusCode(429).withContent("")); + + ////////////////////// + // queue a response // + ////////////////////// + mockApiUtilsHelper.setMockRequestAsserter(asserter); + mockApiUtilsHelper.enqueueMockResponse(""" + {"id": 3, "name": "Bart"}, + """); + + GetOutput getOutput = runSimpleGetAction(); + } + + + + /******************************************************************************* + ** + *******************************************************************************/ + @Test + void testGenerateOutboundApiLogRecord() throws QException + { + APIBackendMetaData backend = (APIBackendMetaData) QContext.getQInstance().getBackend(TestUtils.MOCK_BACKEND_NAME); + backend.setAuthorizationType(AuthorizationType.API_KEY_QUERY_PARAM); + backend.setApiKeyQueryParamName("apikey"); + backend.setApiKey("9876-WXYZ"); + + MockApiActionUtils mockApiActionUtils = new MockApiActionUtils(); + mockApiActionUtils.setBackendMetaData(backend); + OutboundAPILog outboundAPILog = mockApiActionUtils.generateOutboundApiLogRecord(new HttpGet("...?apikey=9876-WXYZ"), new QHttpResponse()); + + assertThat(outboundAPILog.getUrl()) + .doesNotContain("9876-WXYZ") + .contains("?apikey=*****"); + } + + + /******************************************************************************* ** *******************************************************************************/