test: run the login walkthrough against Identity Server 7.3.0, and describe a deployment in a file - #34
Conversation
A second deployment made the shape of the problem obvious. The variables that name a deployment were exported by hand into whichever shell was running the live targets, so moving between Asgardeo and a local Identity Server meant retyping five of them, and the one that differs between the two is not the one anyone would guess: the audience is the client ID on Asgardeo and the API resource identifier on Identity Server. A leftover export from the last deployment then outranks whatever you meant to run against, silently. So the live targets now source a file. Go has no dotenv convention and this module carries four direct dependencies, all load-bearing, so nothing here parses anything: the file is an ordinary shell fragment, `make` sources it, and sourcing it yourself does exactly the same thing when running `go test` directly. One file per deployment, named with SMOKE_ENV, and the file overwrites what the shell already exported rather than deferring to it — which is the direction that makes switching deployments work without a fresh terminal. env.example writes the Asgardeo audience as a reference to the client ID rather than as the same string twice, because two identical placeholders read as a copy-paste slip and invite someone to "fix" one of them. *.env joins the ignore list. The existing .env and .env.* patterns did not cover a per-deployment file named for its deployment, which is the natural thing to write and would have been committed. Claude-Session: https://claude.ai/code/session_01RojiAgW9hi3b9f9G6ZXBVp
The walkthrough carried Identity Server as the product it had not been run against. Section 3.5 said what lands in an access token's `aud` claim there was unmeasured, section 3.3 cited the 6.0.0 documentation for loopback port flexibility, and section 3.6 pointed at the TLS problem without saying what to do about it. All three are now answered against wso2/wso2is:7.3.0. The `aud` finding is the one that matters, because it is not what the Asgardeo run implied. Asgardeo binds the claim to the client ID and offers no way to change it, and this document had recorded that as a platform fact with Identity Server flagged as an open question. It is not a platform fact. Identity Server 7.3.0 adds every entry of the application's audience list to the access token beside the client ID, so the API resource identifier is the right value there — but only once it is registered, and the field that registers it is the one that on Asgardeo reaches the ID token alone. The same-looking control does different work on the two products, which is precisely the kind of difference a walkthrough exists to state. So the shell's audience check can carry product-level meaning on Identity Server and cannot on Asgardeo. Nothing in the broker changes: it already verifies rather than assumes, which is correct under either behavior. What changes is what a context document should say, and that carrying one product's `audience` value to the other costs a browser sign-in and ends in auth.narrowing_unavailable. Loopback flexibility turned out stronger than the documentation implies. The application registers its callbacks as a regexp enumerating four ports, and a login through a fifth completed anyway, so the waiver is applied ahead of the registered pattern rather than as a fallback when none matches. Section 3.6 now writes the TLS step out, including what it costs. Go ignores SSL_CERT_FILE on macOS and the shell exposes no custom-CA option, so the keychain is the only way in — and the certificate being trusted is CA:TRUE with a private key that ships in every download and every copy of the public image behind a published password. That is worth trusting for a local test and worth removing afterwards, and a reader should be told both. The research document records the Identity Server verdicts in a table of their own. The Asgardeo cells stay as they are: these are two deployments, not two readings of one. Claude-Session: https://claude.ai/code/session_01RojiAgW9hi3b9f9G6ZXBVp
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Updates the login smoke walkthrough and research docs with measured results from running against WSO2 Identity Server 7.3.0, and improves the smoke test ergonomics by allowing deployments to be described in a sourced env file (without adding any new Go dependencies or changing shell runtime behavior).
Changes:
- Record Identity Server 7.3.0 measurements (notably
audbehavior when the application Audience list is populated) and reflect the product-specific difference vs Asgardeo in the docs. - Add
test/smoke/env.exampleand update smoke run docs to encourage per-deployment env files. - Update
make smoke-login/make empirical-asgardeoto sourceSMOKE_ENVwhen present and print which file was read; ignore*.envin git.
Standards (repo conventions)
No apparent convention breaks in the changed Makefile/docs structure; the new Makefile macro is kept local and the recipes remain POSIX-sh-compatible.
Spec (PR description / issue intent)
Matches the stated scope: documentation + test tooling only, and the changes align with the PR’s stated findings (Identity Server’s audience list affecting access-token aud) and the goal of avoiding brittle per-shell exports.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
test/smoke/RUNNING.md |
Documents using a per-deployment env file and records the Identity Server vs Asgardeo audience difference. |
test/smoke/env.example |
Adds a template env file for smoke runs (Asgardeo + Identity Server blocks). |
Makefile |
Sources SMOKE_ENV for live smoke targets and prints which env file was read. |
docs/research/asgardeo-redirect-uri-and-scope-narrowing.md |
Adds Identity Server 7.3.0 measurements and updates conclusions. |
docs/guides/login.md |
Expands Identity Server registration guidance (container-first) and documents measured aud behavior + TLS trust steps. |
.gitignore |
Ignores *.env to prevent committing per-deployment smoke env files. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Every one of the plan's sixty-seven checkboxes was still open, including the definition of done, while all twelve tasks were implemented, reviewed and merged. Anyone reading the plan as a status report would conclude the slice had not been started. Sixty-six are now ticked. The definition-of-done entries carry what closed them rather than only a mark: which deployment and date each live check ran against, and which acceptance test pins each deterministic one. One stays open, and the note under it says exactly why. The live smoke run acquires through the broker asking for every scope the session already holds, so its scope comparison is a set against itself and cannot fail — a deployment that ignored narrowing entirely would still report a grant. The audience half of that check is real; the scope half is not. config.NarrowTarget() exists for this and the smoke run does not use it. The module half of that same check needs nothing further: a module never learns which issuer minted its token, so running it against a live deployment would prove nothing the deterministic chain test does not already prove. The closing note records the finding that outran the plan. The spec assumed an access token's audience could name the product resource; Asgardeo binds it to the client ID with no way to change that, and Identity Server 7.3.0 does not. That makes a definition-of-done clause satisfiable on one supported product and structurally not on the other, which is a decision for the issue rather than a task here. Claude-Session: https://claude.ai/code/session_01RojiAgW9hi3b9f9G6ZXBVp
|
Added a third commit: 66 are now ticked, with each definition-of-done entry carrying what closed it — deployment and date for the live checks, the acceptance test name for the deterministic ones. One stays open on purpose. The live smoke run acquires through the broker asking for The module half of the same clause needs nothing further: a module never learns which issuer minted its token, so running it against a live deployment proves nothing Left as a finding rather than fixed here, since it changes what the smoke run asserts rather than what this PR is about. |
**env.example promised a password it does not contain.** The note explaining why nothing secret belongs in these files pointed at "the password below" — reasoning carried over from a real per-deployment file, which does carry a test account's password, and left in the template, which carries none. A reader searching for it finds only the sentence claiming it exists. Rewritten to say what is actually true of these variables: none of them is a credential, and the CI client secret deliberately lives in an environment variable a context names rather than anywhere on disk. The gitignore pattern in that sentence was stale too — it is `*.env` now, not `.env` and `.env.*`. **Section 3.6 read a keystore the container-first path does not have.** The section now opens by recommending a container, then exported the certificate with `keytool` from `repository/resources/security/wso2carbon.p12` — a path that exists only in an unpacked distribution. Rather than adding a `docker cp` beside it with a version-pinned in-container path, take the certificate from the port: one command, correct for either deployment shape, no keystore password, and it captures exactly what the deployment serves rather than what its keystore happens to hold. Verified against the 7.3.0 container to produce bytes identical to the keytool export, which is still named for the case where the certificate is wanted before the deployment is running. **The Makefile described sourcing with `source`.** Harmless where it stood — that sentence is about the reader's own interactive shell, not a recipe, so the `/bin/sh` the recipes use never sees it — but RUNNING.md and env.example both write `.` for the same instruction, and one file disagreeing with the other two is the kind of thing a reader stops to wonder about. Aligned on `.`. env.example was re-checked by sourcing it under plain `/bin/sh`: it reads cleanly and its audience-as-a-reference line still resolves. Claude-Session: https://claude.ai/code/session_01RojiAgW9hi3b9f9G6ZXBVp
Runs the live smoke walkthrough from #17 against the second product it names — a local Identity Server 7.3.0 — and records what that measured. Documentation and test tooling only; no shell behavior changes.
The finding that changes something
The Asgardeo run recorded that a JWT access token's
audclaim is bound to the client ID, never to the API resource identifier whose scopes the token carries, with no setting that changes it. That was written down as a platform fact, with Identity Server flagged as unmeasured.It is not a platform fact. Measured against
wso2/wso2is:7.3.0:aud"<client id>"reference-status["<client id>", "reference-status"]So the API resource identifier is the right value for
products.<namespace>.audienceon Identity Server — once it is registered as an audience. The field that registers it sits under the application's ID token settings on both consoles, and does different work: on Asgardeo it reaches the ID token alone, on Identity Server it reaches both. A same-looking control with different behavior across the two supported products is exactly what a walkthrough exists to state.Confirmed end to end, not only through a token-endpoint probe: a passing
make smoke-loginthrough the browser code flow, whose brokered acquisition was granted againstWSO2_SMOKE_AUDIENCE=reference-status.Nothing in the broker changes. It already verifies the audience rather than assuming it, which is correct under either behavior. What changes is what a context document should say, and that carrying one product's
audiencevalue to the other costs a browser sign-in and ends inauth.narrowing_unavailable.This leaves #17 a design question rather than an open measurement: what should the broker's policy say when one supported product can satisfy a clause and another structurally cannot? That belongs on the issue, not here.
The other two verdicts
Loopback flexibility is stronger than the 6.0.0 documentation implies. The application registers its callbacks as
regexp=(…10425…|…10428…)— four ports, enumerated — and a login through127.0.0.1:16000completed anyway. The waiver is applied ahead of the registered pattern, not as a fallback when none matches.Both are recorded in a table of their own in the research document. The Asgardeo cells are untouched: these are two deployments, not two readings of one.
Describing a deployment in a file
The second deployment made the problem obvious. Five variables were exported by hand into whichever shell ran the live targets, and the one that differs between Asgardeo and Identity Server is not the one anyone would guess — it is the audience, per the finding above. A leftover export from the last deployment then silently outranks whatever you meant to run against.
make smoke-loginandmake empirical-asgardeonow sourcetest/smoke/.env, orSMOKE_ENV=test/smoke/is.envfor a named one, and print which file they read.Nothing parses these files. Go has no dotenv convention and this module carries four direct dependencies, all load-bearing, so adding one for test convenience would be the tail wagging the dog. The file is an ordinary shell fragment:
makesources it, and. test/smoke/is.envin your own shell does exactly the same thing when runninggo testdirectly. Sourcing means the file overwrites what the shell already exported rather than deferring to it, which is the direction that makes switching deployments work without a fresh terminal.test/smoke/env.examplewrites the Asgardeo audience as"$WSO2_SMOKE_CLIENT_ID"rather than repeating the placeholder, because two identical placeholders read as a copy-paste slip and invite someone to "fix" one of them.*.envjoins.gitignore. The existing.envand.env.*patterns did not cover a per-deployment file named for its deployment, which is the natural thing to write and would have been committed.Section 3.6, written out
The TLS step was previously a pointer. It is now the commands, plus what they cost:
SSL_CERT_FILEon macOS —crypto/x509honors it on every Unix except Darwin — and the shell exposes no custom-CA option, so the keychain is the only way in.CA:TRUE, and its private key ships inside every Identity Server download and every copy of the public container image behind the published passwordwso2carbon. The zip and the image serve a byte-identical certificate. Trusting it as a root means trusting a signing key anyone can obtain, for any hostname.-p ssl, the login keychain rather than the system one, and the removal command alongside the setup command.A reader deciding whether to run that should be told both halves.
Verification
The live runs were against
wso2/wso2is:7.3.0in a container.make smoke-loginwas also re-run on this exact base after rebasing onto the #33 merge.Based on
feature/login, on top of the #33 merge rather than beside it.Relates to #17.
https://claude.ai/code/session_01RojiAgW9hi3b9f9G6ZXBVp