Feature/merge remove binary target#9
Conversation
…for service-layers; add testing dependency on mini-sdk and unit/integrations tests
… action rejection
There was a problem hiding this comment.
Pull request overview
Ports the service-layer mutator pattern and HTTP message signing support to the Volley integration, updates initialization semantics, and adds a mini-SDK based test harness with refreshed GitHub Actions workflows for CI validation.
Changes:
- Introduces
ApproovServiceMutatorhooks across request processing, secure-string substitution, and direct-fetch helpers, plus new status-based exception types. - Adds HTTP message signing for Volley via
ApproovDefaultMessageSigningand supporting Structured Field Values / signature utilities. - Integrates mini-SDK + Robolectric/Mockito test coverage and updates CI workflows/build configuration.
Reviewed changes
Copilot reviewed 62 out of 62 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| USAGE.md | Adds integration/customization guidance, including mutators and message signing usage. |
| settings.gradle | Adds mini-SDK/testing modules as sibling-project includes. |
| REFERENCE.md | Adds public API reference for new mutator/signing APIs and initialization semantics. |
| README.md | Links to changelog/usage/reference and documents included third-party sources. |
| CHANGELOG.md | Introduces 3.5.4 changelog entries describing new features/behavior changes. |
| approov-service/src/test/java/org/apache/http/StatusLine.java | Test stub for Apache HTTP types used by tests. |
| approov-service/src/test/java/org/apache/http/ProtocolVersion.java | Test stub for Apache HTTP protocol version. |
| approov-service/src/test/java/org/apache/http/message/BasicStatusLine.java | Test stub for Apache HTTP status line implementation. |
| approov-service/src/test/java/org/apache/http/message/BasicHttpResponse.java | Test stub for Apache HTTP response implementation. |
| approov-service/src/test/java/org/apache/http/message/BasicHeader.java | Test stub for Apache HTTP header implementation. |
| approov-service/src/test/java/org/apache/http/HttpResponse.java | Test stub for Apache HTTP response interface. |
| approov-service/src/test/java/org/apache/http/HttpEntity.java | Test stub for Apache HTTP entity interface. |
| approov-service/src/test/java/org/apache/http/Header.java | Test stub for Apache HTTP header interface. |
| approov-service/src/test/java/org/apache/http/entity/BasicHttpEntity.java | Test stub for Apache HTTP entity implementation. |
| approov-service/src/test/java/io/approov/util/sig/TestComponentProvider.java | Test component provider implementation for signature-base tests. |
| approov-service/src/test/java/io/approov/util/sig/SignatureParametersTest.java | Unit tests for signature-parameter serialization. |
| approov-service/src/test/java/io/approov/util/sig/SignatureBaseBuilderTest.java | Unit tests for signature base construction. |
| approov-service/src/test/java/io/approov/service/volley/ApproovTestSupport.java | Test utilities for resetting static state and building fixtures. |
| approov-service/src/test/java/io/approov/service/volley/ApproovServiceMiniSdkTest.java | Robolectric + mini-SDK contract tests exercising real request flow. |
| approov-service/src/test/java/io/approov/service/volley/ApproovServiceContractTest.java | Unit-level contract tests for ApproovService APIs and behaviors. |
| approov-service/src/test/java/io/approov/service/volley/ApproovHurlStackContractTest.java | Contract tests for header mutation / mutator interactions in stack. |
| approov-service/src/test/java/io/approov/service/volley/ApproovDefaultMessageSigningContractTest.java | Contract tests for signing behavior and header replacement. |
| approov-service/src/test/java/android/util/Base64.java | Test-only shim for android.util.Base64 on JVM. |
| approov-service/src/main/java/io/approov/util/sig/SignatureParameters.java | Adds signature-parameter carrier/serialization logic. |
| approov-service/src/main/java/io/approov/util/sig/SignatureBaseBuilder.java | Adds signature-base construction logic. |
| approov-service/src/main/java/io/approov/util/sig/LICENSE | Adds MIT license for adapted signature utilities. |
| approov-service/src/main/java/io/approov/util/sig/ComponentProvider.java | Adds component-provider interface for signature inputs. |
| approov-service/src/main/java/io/approov/util/http/sfv/Utils.java | Adds structured-field helper utilities. |
| approov-service/src/main/java/io/approov/util/http/sfv/Type.java | Adds SFV type interface. |
| approov-service/src/main/java/io/approov/util/http/sfv/TokenItem.java | Adds SFV token item. |
| approov-service/src/main/java/io/approov/util/http/sfv/StringItem.java | Adds SFV string item. |
| approov-service/src/main/java/io/approov/util/http/sfv/ParseException.java | Adds SFV parse exception with diagnostics. |
| approov-service/src/main/java/io/approov/util/http/sfv/Parameters.java | Adds SFV parameters container implementation. |
| approov-service/src/main/java/io/approov/util/http/sfv/Parameterizable.java | Adds SFV parameterizable contract. |
| approov-service/src/main/java/io/approov/util/http/sfv/package-info.java | Adds SFV package docs and minimal example. |
| approov-service/src/main/java/io/approov/util/http/sfv/OuterList.java | Adds SFV outer list type. |
| approov-service/src/main/java/io/approov/util/http/sfv/NumberItem.java | Adds SFV number item contract. |
| approov-service/src/main/java/io/approov/util/http/sfv/ListElement.java | Adds SFV list-element marker. |
| approov-service/src/main/java/io/approov/util/http/sfv/LICENSE | Adds Apache 2.0 license for adapted SFV utilities. |
| approov-service/src/main/java/io/approov/util/http/sfv/Item.java | Adds SFV item interface and type conversion helpers. |
| approov-service/src/main/java/io/approov/util/http/sfv/IntegerItem.java | Adds SFV integer item. |
| approov-service/src/main/java/io/approov/util/http/sfv/InnerList.java | Adds SFV inner list type. |
| approov-service/src/main/java/io/approov/util/http/sfv/DisplayStringItem.java | Adds SFV display-string item. |
| approov-service/src/main/java/io/approov/util/http/sfv/Dictionary.java | Adds SFV dictionary type. |
| approov-service/src/main/java/io/approov/util/http/sfv/DecimalItem.java | Adds SFV decimal item. |
| approov-service/src/main/java/io/approov/util/http/sfv/DateItem.java | Adds SFV date item. |
| approov-service/src/main/java/io/approov/util/http/sfv/ByteSequenceItem.java | Adds SFV byte-sequence item (binary). |
| approov-service/src/main/java/io/approov/util/http/sfv/BooleanItem.java | Adds SFV boolean item. |
| approov-service/src/main/java/io/approov/service/volley/ApproovServiceMutator.java | Introduces mutator interface + default behaviors for statuses/substitutions. |
| approov-service/src/main/java/io/approov/service/volley/ApproovService.java | Refactors init/state, adds TraceID header support, mutator pipeline, and new helpers. |
| approov-service/src/main/java/io/approov/service/volley/ApproovRequestMutations.java | Adds mutation-tracking carrier for composed mutators (e.g., signing). |
| approov-service/src/main/java/io/approov/service/volley/ApproovNetworkException.java | Deprecates network exception in favor of fetch-status exception (compat kept). |
| approov-service/src/main/java/io/approov/service/volley/ApproovFetchStatusException.java | Adds typed exception carrying SDK fetch status. |
| approov-service/src/main/java/io/approov/service/volley/ApproovException.java | Improves exception wrapping/constructors for underlying causes. |
| approov-service/src/main/java/io/approov/service/volley/ApproovDefaultMessageSigning.java | Adds Volley message signing mutator (digest + signature headers). |
| approov-service/pom.xml | Adds BouncyCastle runtime dependency for message signing publishing. |
| approov-service/build.gradle | Adds test configuration, mini-SDK test deps, and signing-related dependencies. |
| .vscode/settings.json | Adds local VS Code Java build config setting. |
| .github/workflows/build_only.yml | Removes old “build only” workflow. |
| .github/workflows/build_and_test.yml | Adds new build+test workflow with mini-SDK checkout and worker probing/redeploy. |
| .github/workflows/build_and_publish.yml | Updates publish workflow to run tests and use repo subdirectory checkout. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…o support empty-config bypass
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 62 out of 62 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 62 out of 62 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 62 out of 62 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try { | ||
| if (config.length() != 0) | ||
| Approov.initialize(context, config, "auto", null); | ||
| Approov.setUserProperty("approov-service-volley"); | ||
| Approov.initialize(context.getApplicationContext(), config, "auto", comment); | ||
| } catch (IllegalArgumentException e) { | ||
| Log.e(TAG, "Approov initialization failed: " + e.getMessage()); | ||
| return; | ||
| throw e; | ||
| } catch (IllegalStateException e) { | ||
| Log.e(TAG, "Approov already initialized: Ignoring native layer exception " + e.getMessage()); | ||
| } | ||
|
|
||
| Approov.setUserProperty("approov-service-volley"); | ||
|
|
||
| // create an alternative hurlstack to use | ||
| hurlStack = new ApproovHurlStack(); | ||
| isInitialized = true; |
There was a problem hiding this comment.
initialize(..., config, comment) calls Approov.setUserProperty(...) unconditionally, even when config is empty (the documented bypass mode that skips SDK initialization). If the native Approov SDK is not already initialized, this can still trigger an IllegalStateException and defeats the purpose of empty-config mode. Consider guarding setUserProperty behind config.length() != 0 (or wrapping it in a try/catch and only calling it when the SDK is known to be initialized).
| public static synchronized String getApproovTokenHeaderValue(Approov.TokenFetchResult approovResults) { | ||
| if (approovResults == null) { | ||
| return null; | ||
| } | ||
| String token = approovResults.getToken(); | ||
| if (token == null) { | ||
| return null; | ||
| } | ||
| return formatApproovTokenHeaderValue(token); |
There was a problem hiding this comment.
getApproovTokenHeaderValue(...) returns the configured prefix even when approovResults.getToken() is the empty string. This contradicts the method’s documented behavior ("Returns null if there is no token") and can result in sending an empty/placeholder token header (e.g., "Bearer " or "") on otherwise-successful fetches. Consider treating an empty token the same as null and returning null unless the caller explicitly opts into status fallback via getApproovTokenHeaderValueOrStatus(...).
… and update documentation
- Guarded against null comment parameter in ApproovService.initialize - Guarded Approov.setUserProperty execution to match bypass config state - Normalized empty string Approov tokens to return null in header mapping - Synced bcprov artifact generation path in pom.xml to match build.gradle
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 62 out of 62 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
approov-service/src/main/java/io/approov/util/sig/SignatureParameters.java:1
setCustomParameteris declared to returnObjectbut always returnsthis(aSignatureParameters). This is confusing for callers and breaks fluent usage typing (forces casts / loses chaining in IDE autocomplete). Additionally, the method does unchecked casts like(String) value/(Long) valuebased solely on the key, which will throwClassCastExceptionat runtime without a clear error message. Consider changing the return type toSignatureParametersand adding explicit type checks per key (throwingIllegalArgumentExceptionwith a clear expected/actual type) to make misuse fail fast and predictably.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public static synchronized void initialize(Context context, String config, String comment) { | ||
| boolean allowEnableAfterEmptyInitialization = isInitialized && (configString != null) && configString.isEmpty() && !config.isEmpty(); | ||
| if (isInitialized && (comment == null || !comment.startsWith("reinit")) && !allowEnableAfterEmptyInitialization) { | ||
| if (!config.equals(configString)) { | ||
| throw new IllegalStateException("ApproovService layer is already initialized"); | ||
| } | ||
| Log.d(TAG, "Ignoring multiple ApproovService layer initializations with the same config"); | ||
| return; | ||
| } | ||
|
|
||
| // initialize the Approov SDK | ||
| hurlStack = null; | ||
| isInitialized = false; | ||
| configString = null; |
There was a problem hiding this comment.
initialize(...) clears isInitialized/configString before attempting native Approov.initialize(...), but then treats IllegalStateException ("already initialized") as success and overwrites configString with the newly supplied config. If the native SDK is already initialized with a different config (e.g., another service layer initialized it first), the service layer can end up reporting isInitialized=true and configString=<new> while the underlying SDK continues operating with the original config, creating an internal inconsistency that can change runtime behavior and invalidate later "same config vs different config" checks. A safer approach is to preserve the previous configString until native initialization succeeds, and on IllegalStateException either (a) validate the requested config matches the previously active config (if known) or (b) refuse different-config "reinit" attempts when the SDK reports it's already initialized.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 62 out of 62 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Wrapped fetchApproovTokenAndWait calls inside formatting methods with try-catch blocks to securely wrap and rethrow IllegalArgument/IllegalStateExceptions as ApproovExceptions.
adriantuk
left a comment
There was a problem hiding this comment.
ApproovService.initialize(...) now allows reinit... calls to bypass the existing config consistency check. If a caller reinitializes with a different non-empty config, the native SDK can remain on the original config while the Volley wrapper updates its own configString/isInitialized state to the new one. That leaves the wrapper and native layer out of sync.
ApproovDefaultMessageSigning.SignatureParametersFactory can now be used through the public API without setting baseParameters, and the current docs show exactly that pattern. In that case, the first signed request will hit new SignatureParameters(baseParameters) with baseParameters == null and fail with an unhandled NullPointerException.
.github/workflows/build_and_test.yml now runs on every pull_request but unconditionally checks out the private approov/core-service-layers-testing repository using CORE_SERVICE_LAYERS_TESTING_PAT. Forked PRs do not receive repository secrets, so those CI runs will fail before build/test execution, removing PR validation for external contributors.
Recommended fixes are to keep the same-config guard for reinit... flows, make SignatureParametersFactory either supply safe defaults or fail early with a clear error, and gate the private checkout path in CI so forked PRs still get a usable workflow.
…s and safely catch Throwable during reflection
Port mutators and message signing to Volley for 3.5.4
Align Volley initialization behavior with other service layers
chore: integrate mini-sdk testing and update github workflows
Obsoleted setProceedOnNetworkFailure