Developer guide: two figures for the sign-in stack - #5812
Conversation
Authentication and Identity is 306 lines with no picture in it, and two of the things it explains are structural rather than procedural, so prose has to carry them a sentence at a time. The first figure is the sentence "every provider-specific class is a thin layer on top of these primitives" drawn out: five provider classes over OidcClient over SystemBrowser, with each platform's browser surface named underneath. FirebaseAuth sits outside that column, because it is the one that is not OpenID Connect at all -- the ID token crosses over to it, the code path does not, and that distinction is easy to miss in the running text. The second is the five-step list under the quick start, as a flow across three lanes. It makes the leg the list does not emphasise visible: the token exchange goes straight from the app to the provider, carrying the PKCE verifier and no client secret, with the browser already finished. That is the part of the model that makes a public mobile client safe, and it is worth being able to point at. Flat SVG, presentation attributes only, no filters or CSS variables, so it survives the HTML build, asciidoctor-pdf and the website embed. Both rendered in headless Chrome and read back. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7fedb1bee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The stack figure put every provider class on one bus into OidcClient, and two of them do not belong there. AppleSignIn.signIn() looks up the native interface first and calls signInNative whenever it reports supported, so on iOS 13+ neither OidcClient nor SystemBrowser is involved at all -- the OIDC route is the fallback for everywhere else. The figure now forks it, with the native sheet as its own destination. FirebaseAuth reaches its session by way of signInWithIdpIdToken, which posts id_token=..., and FacebookConnect.signIn returns OidcTokens whose getIdToken() is null because Facebook's flow issues an access token and nothing else. So "an ID token from one of the classes above" was an invitation to build a request Firebase rejects. The handoff now names the four providers that can supply one, and FacebookConnect's box says it issues none. The chapter says a thin-layer-over-the-primitives sentence and then spends 200 lines on the exceptions, so the two exits are now stated in the prose beside the figure rather than only drawn. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cee463bf6b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The entry point is authorize(). OidcClient has no signIn(), the quick start directly above the figure calls authorize(), and a reader translating the picture into code would have gone looking for a method that does not exist. The refresh token is conditional. The quick start asks for openid, email and profile and nothing else, so Google returns no refresh token and refreshIfExpired has nothing to renew -- which is why GoogleConnect.signIn adds access_type=offline and prompt=consent. The figure said the token endpoint returns one, flatly. And "no client secret" was too categorical. It is true of a provider that treats the app as a public client, which is the arrangement PKCE exists to make safe, but AppleSignIn's non-native path sets the webClientSecret and the Apple section says the exchange needs a JWT your server mints. Scoped in the figure and in the sentence beside it, which carried the same claim. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bff3564adf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
JavaSE and the Web port do not use the default browser. OidcBrowserNativeImpl exists only under Ports/iOSPort and Ports/Android, so everywhere else SystemBrowser.authenticate falls back to BrowserWindow -- a JavaFX WebView on the desktop. The figure said "the default browser" because the chapter's own bullet does, three paragraphs above a WARNING about providers refusing embedded views. Both now say what happens, and why a simulator sign-in can be refused where a device one is not. Only the redirect URL crosses back. AppleSignIn's non-iOS path sets response_mode=form_post, so Apple POSTs code and state to the redirect URI, and OidcClient.handleRedirect parses the URL alone -- the sign-in fails with STATE_MISMATCH unless a page at that URI turns the POST back into a redirect. Said in the figure and spelled out beside it. And the nonce check is conditional: postToTokenEndpoint compares only when the ID token actually carries a nonce claim, so a token that omits one is accepted and persisted. The figure promised verification flatly; it now says when it happens. That last one is a weakness in OidcClient rather than in the guide, and worth a look on its own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Cloudflare Preview
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9bd02dcbdd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Developer Guide build artifacts are available for download from this workflow run:
Developer Guide quality checks: |
…needs an App Link on Android Grouping JavaSE and the Web port together was wrong in the direction that matters. HTML5Implementation.createNativeBrowserWindow returns an HTML5BrowserWindow, whose show() calls Window.current().open() -- a real top-level browser window, not an embedded view, so the provider-rejection warning that applies to JavaSE's JavaFX WebView does not apply to it. The two are now separate in the prose and in the figure. And Apple's non-iOS path needs more than the POST-to-redirect bridge on Android: the Services ID callback is an HTTPS URL, and OidcBrowserNativeImpl hands control back by matching the scheme of the redirect it is waiting for against the arriving intent, so the custom scheme the chapter registers earlier is not what receives it. That callback host needs its own verified App Link, which is now said where the Services ID setup is described. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d194b3c90c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…t exists The Web port's BrowserWindow is a window.open() call, and it runs after the discovery request comes back rather than inside the click that started the sign-in, so a browser that has expired the transient activation blocks it -- and HTML5BrowserWindow.show() calls addEventListener on the result with no null check. Said where the Web fallback is described, with the two ways out. The Apple-on-Android note pointed at the deep-link chapter for an App Link recipe that chapter only named. It now carries the filter, so this cross-reference goes somewhere useful, and the note says what to look for -- autoVerify, the https scheme and host, and that the hint takes one string so a second filter shares it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd17691a03
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
HTML5BrowserWindow watches the popup by reading win.getLocation() on every load, and the same-origin policy throws once the popup has navigated to another host. The catch falls back to the URL it already had -- the authorization URL -- so SystemBrowser never sees a redirect that matches and authorize() never completes. Silent, and it applies to the Apple form_post bridge, which the chapter had just told the reader to host somewhere. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Authentication-And-Identity.asciidocis 306 lines with no picture in it, and two of the things it explains are structural rather than procedural — so the prose has to carry them one sentence at a time.The provider stack
The chapter says "every provider-specific class is a thin layer on top of these primitives" and then spends 200 lines proving it one class at a time. The figure is that sentence: five provider classes over
OidcClientoverSystemBrowser, with each platform browser surface named underneath.FirebaseAuthsits outside that column, because it is the one that is not OpenID Connect at all. The ID token crosses over to it; the code path does not. That distinction is a single clause in the running text and is easy to read past.The sign-in flow
The quick start lists five steps. Drawn across three lanes, it shows the leg the list does not emphasise: the token exchange goes straight from the app to the provider, carrying the PKCE verifier and no client secret, with the browser already finished. That is the part of the model that makes a public mobile client safe, and it is worth being able to point at.
A sentence was added beside it saying so.
Conventions
Flat SVG, presentation attributes only, no
<style>, novar(), no filters or masks — so it renders identically in the HTML build, underasciidoctor-pdf(prawn-svg) and in the website embed. Explicitwidth/height/viewBox,font-family="Arial, sans-serif".Both were rendered in headless Chrome and read back, which caught a label sitting on top of
ASWebAuthenticationSessionand a dashed arrow pointing the wrong way (it implied Firebase feedsOidcClient, which is exactly what the caption says it does not).Gates
asciidoctor --failure-level WARNandasciidoctor-pdf— cleanstatus: ok,total: 0