Repository navigation
feat(kotlin-oidc): landing pages for the JVM sign-in flow - #674
mattcreaser wants to merge 13 commits into
Conversation
The padlock favicon was unrelated to what the page showed. Each page now pairs its in-page glyph with a tab icon drawing the same shape, held together in one type so the two cannot drift apart.
|
Hand-rolled string parsing split the scheme and host by hand, including a helper to step over userinfo and IPv6 brackets. URI does both by the spec, and rejects malformed input that the manual split accepted.
Replaces a hand-written escaper with io.ktor.util.escapeHTML, which covers the same five characters. Ktor is already a dependency of this module.
Replaces hand-rolled query splitting, percent-encoding, and query-string appending with parseQueryString and URLBuilder. URLBuilder also places an appended query before an existing fragment, which the manual version got wrong; a test now covers that.
The loopback handler inferred success from a `code` in the callback query, so a redirect that later failed state or CSRF validation, or whose exchange was rejected, still showed the success page while signIn threw. OidcRedirectSession gains reportOutcome, which signIn calls once it knows. The JVM session hands the callback over first and then waits for that outcome before answering the browser, so nothing that goes wrong while responding can strand a callback that already arrived, and the handler no longer has to guess the outcome from the query keys. Android and iOS do not wait on anything, so they no-op.
- Reject an out-of-range port, which URI parses without range-checking and which cannot be followed or appended to later. - Treat every spelling of a loopback address as loopback, so 127.0.0.2 and an expanded ::1 are accepted over http, rather than only two literals. - Say the host is unusable, and point at punycode, instead of claiming a URL with an underscore or a non-ASCII host has no host at all. - Drop xmlns from the inline SVG, which the HTML parser does not need, and assert the pages fetch nothing by looking for src=, url(, @import and href="http rather than for particular hostnames. - Document signIn, and correct the README: a clean URL is only guaranteed for Redirect. BuiltIn and Html answer the callback itself, so its query stays in the address bar and supplied markup can read the code.
…ncher FakeLauncher records what it receives, so a test can tell a forwarded argument from a dropped one: because the interface defaults options, omitting it at the call site would compile and silently discard every app's configured pages. Also covers the outcome signIn reports, including the cases the handler used to get wrong on its own — a callback carrying a code that fails state validation, and one carrying both code and error.
…dc-landing-pages # Conflicts: # native/kotlin/runtime/src/commonMain/kotlin/com/aws/blocks/kotlin/oidc/OidcClient.kt # native/kotlin/runtime/src/commonTest/kotlin/com/aws/blocks/kotlin/oidc/OidcClientTest.kt
| ## Signing In with OIDC | ||
|
|
||
| ```kotlin | ||
| val auth = AuthApi() |
There was a problem hiding this comment.
This doesn't compile after the merge with main. Since #704, generated API classes take a BlocksClient and are reached through Blocks, so AuthApi() has no no-arg constructor. The quick start above uses blocks.authApi, and OidcE2ETest uses e2eBlocks().oidcAuthApi. This should probably be:
val blocks = Blocks()
val client = blocks.authApi.getClient()
val user = client.signIn("google")| return if (idpError != null) { | ||
| OidcSignInOutcome.Failed(idpError, resultParams["error_description"]) | ||
| } else { | ||
| val code = if (failure is OidcExchangeException) "exchange_failed" else "invalid_callback" |
There was a problem hiding this comment.
This maps everything except OidcExchangeException to invalid_callback, but the try at L104 also covers step 6. Several failures reach the browser as error=invalid_callback even though the callback was fine:
- a network failure on
/auth/exchange, such as aConnectExceptionor OkHttp's read timeout - a parse failure on the response body
- the
BlocksExceptionfromensureOpen()
catch (failure: Throwable) also catches CancellationException. If the caller's scope is cancelled mid-exchange, for example when a Compose screen leaves composition, the page says invalid_callback with "StandaloneCoroutine was cancelled". In each case the description is the raw exception message, which is shown on the built-in page and appended to a Redirect URL, so it ends up in the app's access logs.
An app whose Redirect error page branches on error would tell the user their sign-in link was bad when the backend was unreachable. Classifying by step would fix it:
invalid_callbackonly for steps 4–5exchange_failedfor anything thrown from step 6- its own code (e.g.
cancelled) forCancellationException
Using failure.message only for OidcExceptions, with a fixed description otherwise, would keep third-party exception text out of the page. No test drives exchange_failed through signIn. signIn closes the session when the exchange fails could also assert reportedOutcome.
| this.outcome.complete(outcome) | ||
| // The handler is still holding the browser's connection open; stop(0) in close() | ||
| // would drop it mid-response. | ||
| responded.await(RESPOND_TIMEOUT_SECONDS, TimeUnit.SECONDS) |
There was a problem hiding this comment.
reportOutcome isn't suspend, so this wait (up to 10 s) runs on whatever thread signIn is on. On Compose Desktop that's usually Dispatchers.Main, the Swing event thread. The wait is normally a few milliseconds, but it can last the full 10 s if the write stalls, and coroutine cancellation can't interrupt it. The tests all wrap the call in withContext(Dispatchers.IO), which signIn doesn't do. The interface is @InternalBlocksApi, so it costs little to make reportOutcome suspend and replace the latch with a CompletableDeferred<Unit> awaited under withTimeoutOrNull. The Android and iOS no-ops stay as they are.
|
|
||
| // Long enough for a token exchange, short enough that a caller that stopped running | ||
| // does not hold the browser indefinitely. | ||
| const val OUTCOME_TIMEOUT_SECONDS = 30L |
There was a problem hiding this comment.
nit: there's no test for the fallback when no outcome arrives, and there can't be one without waiting 30 s. Making this a constructor parameter like timeout would let a test cover "no outcome → failure page". The fallback can also mislead the other way: if the exchange outlasts it, the browser shows a bare failure page while signIn goes on to succeed. That's worth a line in outcomeForDisplay's KDoc.
Problem
After an OIDC sign-in on the JVM target, the loopback server served a single line of unstyled HTML on
127.0.0.1. A failed sign-in rendered the success page, becauseerrorin the callback was treated the same ascode. Apps had no way to brand or localize what the user saw.Issue #, if available:
Changes
signInnow accepts anOidcSignInOptions. ItsplatformOptionsis anexpect classwith a per-target shape; on JVM it carries asuccessPageand anerrorPage, each anOidcLandingPage:BuiltIn(default) — new styled, self-contained pages: inline CSS and SVG, light and dark, no network fetches. The failure page shows the provider'serroranderror_description, HTML-escaped, and uses its own glyph and tab icon.Redirect(url)— a 302 to the app's own page. Validated at construction:http/httpsonly, andhttpsunless the host is loopback.Html(document)— the app's own document, served verbatim, for apps with nothing to host.Also in the handler:
stateare not included in the redirect. On failure onlyerroranderror_descriptionare forwarded.The Android and iOS actuals are empty, since their in-app browser dismisses itself.
Validation
OidcLandingPageTest(URL validation) andOidcLoopbackPagesTest(document structure, escaping, tab icon matches the page glyph), plus 8 cases inJvmLoopbackSessionTestdriving each arm against a live loopback server../gradlew build, the runtime JVM / Android-host / iOS-simulator suites, plugin and codegen tests, thee2ebuild, and both example apps.Redirectlands the browser on the external page.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.