feat: add Android support for captive signing with URL - #3
Conversation
IronTony
added
documentation
IronTony
left a comment
IronTony
left a comment
There was a problem hiding this comment.
I verified the premise against the shipped androidsdk:2.1.4 AAR rather than the docs. DSSigningDelegate.launchCaptiveSigning(Context, String signingURL, String, String, DSCaptiveSigningListener) is there. Its body calls CaptiveSigningActivity.getIntent(context, envelopeId, null, recipientId, signingURL) and sets setAuthNeeded(false) before startActivity. So the argument order here is right, params 3 and 4 really are nullable (only context, signingURL and listener get checkNotNullParameter), and the "no loginWithAccessToken required" claim holds. The README line this PR deletes was wrong.
Two items I want fixed before merge: the URL validation and the duplicate error event. The rest can land as follow-ups.
For the description: onCancel(envelopeId, recipientId) maps to handleSigningCancelled(envelopeId, null), so reason is always null on Android cancels, while iOS forwards DSMSigningExitReasonKey. That matches the existing session flow, so it is not a blocker, but the parity claim is a little stronger than what ships. Version bumps in package.json and android/build.gradle (both still on 1.0.5) I'll handle at release time.
IronTony
left a comment
IronTony
left a comment
There was a problem hiding this comment.
Switching this to changes requested so the state is explicit. The approach is right and I verified it against the shipped SDK, so this is about two specific items before merge, both already threaded inline:
DocuSignManager.kt:338-341: reject a blank (and ideally non-https)signingUrl. The SDK's URL overload validates nothing and will launch an empty signing activity, leaving the promise unsettled.DocuSignModule.kt:171: drop the secondonSigningErroremit. It fires a duplicate event and flattensrecipient_signing_failedintosigning_failed.
The rest of the review can land as follow-ups. Ping me once those two are in and I'll re-review.
Re-triaged. Most of what I flagged was maintainer work on pre-existing code and has moved to #4. Replacing this with a smaller review.
IronTony
left a comment
IronTony
left a comment
There was a problem hiding this comment.
Trimmed this down. Five of my seven comments were really maintainer work on code that predates your PR, so I have deleted those threads and moved the items to #4. Sorry for the noise.
For the record, what moved: the listener and launch-helper extraction, the currentEnvelopeId catch in presentCaptiveSigning, the duplicate onSigningError emit, the CodedException code forwarding, and the README Throws bullet. They are tracked as a checklist on #4 and will land there after this merges.
Two things left, both in code this PR adds:
DocuSignManager.kt:341: reject a blank and ideally non-httpssigningUrl. The 5-arg overload validates nothing and callsstartActivityunconditionally, so a blank URL launches an empty signing activity and the promise never settles.src/api.ts: a small delegation test forpresentCaptiveSigningWithUrl.
The approach is right and I verified it against the shipped 2.1.4 AAR. Ping me once those two are in.
Heads up on #4: it adds an opt-in launchStrategy to the session flow that mints a recipient view on device and launches it through the same overload you exposed here. Different entry point, same SDK path, so it will consume your entrypoint rather than duplicate it. It is a draft and waits for this one to merge first, so there is no rush on your side and no rebase coming your way.
There was a problem hiding this comment.
🟡 Changes recommended
The Android implementation introduces androidx.core.net.toUri() usage without declaring core-ktx, which can break compilation depending on transitive dependencies.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds Android support for the URL-based captive signing flow (presentCaptiveSigningWithUrl) so consumers can launch captive signing from a pre-minted recipient-view URL on both iOS and Android, aligning the JS API surface and documentation across platforms.
Changes:
- Remove the JS-side iOS-only guard for
presentCaptiveSigningWithUrland delegate directly to the native module on both platforms. - Implement Android
presentCaptiveSigningWithUrlviaDocuSign.getInstance().getSigningDelegate().launchCaptiveSigning(...), including basic HTTPS URL validation and in-flight session guarding. - Update README/CHANGELOG and add a Jest unit test plus native-module mocking for
presentCaptiveSigningWithUrl.
File summaries
| File | Description |
|---|---|
| src/api.ts | Removes iOS-only restriction and exposes URL flow on Android via native module. |
| src/api.test.ts | Adds a unit test asserting JS delegation to the native module. |
| README.md | Updates flow matrix and URL-flow docs to reflect iOS+Android support and initialize requirement. |
| jest.setup.js | Extends the native-module Jest stub to include presentCaptiveSigningWithUrl. |
| CHANGELOG.md | Documents upcoming Android URL-flow support under “Next”. |
| android/src/main/java/expo/modules/docusign/DocuSignModule.kt | Adds Android async function bridging for presentCaptiveSigningWithUrl. |
| android/src/main/java/expo/modules/docusign/DocuSignManager.kt | Implements URL-based captive signing on Android using the DocuSign SDK delegate. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🔵 Needs a closer look
The Android URL validation can crash on malformed URLs, and not_initialized failures are currently surfaced as signing_failed instead of a distinct error code.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
android/src/main/java/expo/modules/docusign/DocuSignManager.kt:350
- The HTTPS validation can crash for malformed/relative URLs because
Uri.parse(signingUrl).schememay be null at runtime; calling.equals(...)on a null scheme will throw instead of returning a cleanSigningFailedException.
val signingUri = android.net.Uri.parse(signingUrl)
if (
!signingUri.scheme.equals("https", ignoreCase = true) ||
signingUri.host.isNullOrBlank()
) {
android/src/main/java/expo/modules/docusign/DocuSignModule.kt:173
presentCaptiveSigningWithUrlcan fail withNotInitializedException, but the module currently rejects it with codesigning_failed. This makes it hard for callers to distinguish a missinginitialize()call (and it contradicts the documentednot_initializedcode in the README error table).
onFailure = { error ->
emitSigningError(params.envelopeId, "signing_failed", error.message ?: "Unknown error")
promise.reject("signing_failed", error.message ?: "Unknown error", error as? Exception)
}
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
The protect-main ruleset requires status checks named build, lint and test, and also CodeQL results. The repo had no workflows at all, so none of those were ever reported and every PR sat permanently blocked on checks that had no producer. #3 is currently stuck exactly this way. Job ids are build / lint / test deliberately: with no name override, the job id becomes the status check context, which is what the ruleset matches on. Renaming a job here silently makes its required check unsatisfiable again. Three separate jobs rather than one with three steps, so each context reports independently and a lint failure does not mask a test failure. permissions is contents: read, there are no secrets, and no untrusted context values reach any run step. Actions are tag-pinned rather than SHA-pinned, which is worth revisiting if the repo standardises on stricter supply-chain rules.
|
CI is wired up now and running on this PR.
import { expect, it, jest } from '@jest/globals';
+
import DocuSignModule from './DocuSignModule';
import { presentCaptiveSigningWithUrl } from './api';
One heads up on the CI itself: this PR is from a fork, so GitHub holds the workflow runs until I approve them. I approved this round and will keep doing so on each push, but that is why the checks sat on "Waiting for status to be reported" for a while rather than starting. Once lint is green I will re-approve and merge. The two things I asked for are both in and both look right: the https/host guard sits ahead of the |
|
Thanks for the review and for merging 🙌 |
Everything here touches code that predates #3. It sat on that PR's review as requested changes, which was wrong of me: asking a contributor to refactor the maintainer's own code as a condition of landing their feature is scope creep. Pulled off #3 and landed here instead. Both entrypoints carried their own copy of a 33-line DSCaptiveSigningListener, so any callback fix had to be made twice and the copies could drift. captiveSigningListener() is now the single source, and presentCaptiveSigningWithUrl folds onto the same launchWithSigningUrl helper the signing-URL strategy uses. #3's https/host guard becomes isHttpsUrl and now covers every URL this object hands to the SDK or sends credentials to. It stays ahead of the compareAndSet in presentCaptiveSigningWithUrl: a rejected URL must not claim the pending slot, or the next valid call is refused as already in progress. That guard also now covers the recipient-view request itself. `host` arrives from JS unvalidated and that request carries the session bearer token, so an http:// host would have sent it in cleartext. The signing-URL strategy is the first code path to treat `host` as an endpoint rather than passing it to the SDK, so the exposure comes in with this feature. Throwing routes to the fetch fallback, meaning a misconfigured host degrades instead of failing outright. launchViaEnvelopeFetch's catch now clears currentEnvelopeId. Only the URL path did, and #3 had the correct shape. restApiRoot is lifted out of the request builder. Its ordering is load-bearing, since an already-versioned host would otherwise fall through and get a second /restapi/v2.1 appended, and that reads better as a named function than buried in a connection setup. Both branches are now case-insensitive; an uppercase RESTAPI segment previously fell through and built a doubled path. Not unit tested: android/build.gradle declares implementation project(':expo-modules-core'), which only resolves inside a host app, so there is no standalone gradle build and CI runs jest only. Error codes are the behaviour change. CodedException infers a code from the class name when none is given, so forwarding it verbatim would have surfaced NotInitializedException as ERR_NOT_INITIALIZED rather than the not_initialized the README error table has always documented. The exceptions now carry explicit codes and the module forwards them, so callers can tell a missing initialize() from a missing login from a real signing failure instead of receiving signing_failed for all three. The module also stops emitting onSigningError itself. handleSigningError already emits, so every failure delivered two events, and the module's copy flattened recipient_signing_failed into signing_failed. The emit in loginWithAccessToken stays: that path never reaches handleSigningError, so it is the only event.
Everything here touches code that predates #3. It sat on that PR's review as requested changes, which was wrong of me: asking a contributor to refactor the maintainer's own code as a condition of landing their feature is scope creep. Pulled off #3 and landed here instead. Both entrypoints carried their own copy of a 33-line DSCaptiveSigningListener, so any callback fix had to be made twice and the copies could drift. captiveSigningListener() is now the single source, and presentCaptiveSigningWithUrl folds onto the same launchWithSigningUrl helper the signing-URL strategy uses. #3's https/host guard becomes isHttpsUrl and now covers every URL this object hands to the SDK or sends credentials to. It stays ahead of the compareAndSet in presentCaptiveSigningWithUrl: a rejected URL must not claim the pending slot, or the next valid call is refused as already in progress. That guard also now covers the recipient-view request itself. host arrives from JS unvalidated and that request carries the session bearer token, so an http:// host would have sent it in cleartext. The signing-URL strategy is the first code path to treat host as an endpoint rather than passing it to the SDK, so the exposure comes in with this feature. The check runs before the connection is opened and before the Authorization header is set, and throwing routes to the fetch fallback, so a misconfigured host degrades instead of failing outright. launchViaEnvelopeFetch's catch now clears currentEnvelopeId. Only the URL path did, and #3 had the correct shape. restApiRoot is lifted out of the request builder. Its ordering is load-bearing, since an already-versioned host would otherwise fall through and get a second /restapi/v2.1 appended, and that reads better as a named function than buried in a connection setup. Both branches are now case-insensitive; an uppercase RESTAPI segment previously fell through and built a doubled path. Not unit tested: android/build.gradle declares implementation project(':expo-modules-core'), which only resolves inside a host app, so there is no standalone gradle build and CI runs jest only. Error codes are the behaviour change. CodedException infers a code from the class name when none is given, so forwarding it verbatim would have surfaced NotInitializedException as ERR_NOT_INITIALIZED rather than the not_initialized the README error table has always documented. The exceptions now carry explicit codes and the module forwards them, so callers can tell a missing initialize() from a missing login from a real signing failure instead of receiving signing_failed for all three. The module also stops emitting onSigningError itself. handleSigningError already emits, so every failure delivered two events, and the module's copy flattened recipient_signing_failed into signing_failed. The emit in loginWithAccessToken stays: that path never reaches handleSigningError, so it is the only event.
Everything here touches code that predates #3. It sat on that PR's review as requested changes, which was wrong of me: asking a contributor to refactor the maintainer's own code as a condition of landing their feature is scope creep. Pulled off #3 and landed here instead. Both entrypoints carried their own copy of a 33-line DSCaptiveSigningListener, so any callback fix had to be made twice and the copies could drift. captiveSigningListener() is now the single source, and presentCaptiveSigningWithUrl folds onto the same launchWithSigningUrl helper the signing-URL strategy uses. #3's https/host guard becomes isHttpsUrl and now covers every URL this object hands to the SDK or sends credentials to. It stays ahead of the compareAndSet in presentCaptiveSigningWithUrl: a rejected URL must not claim the pending slot, or the next valid call is refused as already in progress. That guard also now covers the recipient-view request itself. host arrives from JS unvalidated and that request carries the session bearer token, so an http:// host would have sent it in cleartext. The signing-URL strategy is the first code path to treat host as an endpoint rather than passing it to the SDK, so the exposure comes in with this feature. The check runs before the connection is opened and before the Authorization header is set, and throwing routes to the fetch fallback, so a misconfigured host degrades instead of failing outright. launchViaEnvelopeFetch's catch now clears currentEnvelopeId. Only the URL path did, and #3 had the correct shape. restApiRoot is lifted out of the request builder. Its ordering is load-bearing, since an already-versioned host would otherwise fall through and get a second /restapi/v2.1 appended, and that reads better as a named function than buried in a connection setup. Both branches are now case-insensitive; an uppercase RESTAPI segment previously fell through and built a doubled path. Not unit tested: android/build.gradle declares implementation project(':expo-modules-core'), which only resolves inside a host app, so there is no standalone gradle build and CI runs jest only. Error codes are the behaviour change. CodedException infers a code from the class name when none is given, so forwarding it verbatim would have surfaced NotInitializedException as ERR_NOT_INITIALIZED rather than the not_initialized the README error table has always documented. The exceptions now carry explicit codes and the module forwards them, so callers can tell a missing initialize() from a missing login from a real signing failure instead of receiving signing_failed for all three. The module also stops emitting onSigningError itself. handleSigningError already emits, so every failure delivered two events, and the module's copy flattened recipient_signing_failed into signing_failed. The emit in loginWithAccessToken stays: that path never reaches handleSigningError, so it is the only event.
Add Android support for
presentCaptiveSigningWithUrlusing the URL basedlaunchCaptiveSigning, which is supported by the SDK (doc). README updated 👍 .