Repository navigation
docs(mypolitics): sign-in with social providers - doc, spec and tasks - #120
gieniek-ai wants to merge 9 commits into
Conversation
Social providers only (Google, Facebook, X), one door for signing in and signing up, the account saved only after the completing step, no merging on e-mail, and a universal dialog that asks for an account before any action that needs one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Eight tasks: the session layer, the sign-in card, the completing step, the flow, the universal auth dialog, the sign-in page, the header, and the API adapter, which waits for the API. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 533c7c2c2a
ℹ️ 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".
| | Identity | Provider + the provider's user identifier, unique. E-mail optional, verified only - X often gives none | | ||
| | Session | A cookie the page cannot read (`HttpOnly`, `Secure`, `SameSite=Lax`), 30 days, extended on use; it records when it started | | ||
| | Read the session | The account or none | | ||
| | Provider sign-in | Through a provider window (OAuth with PKCE), answering with one of the six outcomes; falls back to a full-page redirect that returns to `returnTo` when the window is blocked | |
There was a problem hiding this comment.
Resume pending outcomes after full-page provider redirects
When a popup is blocked during a first-time or account-collision sign-in, replacing the page destroys the pending AuthClient.signIn() promise and the SignInFlow reducer. After the provider redirects back, the only startup primitive is getSession(): AuthAccount | null (cycle-1/auth-session.md:66-72), but a pending sign-up or pending link deliberately has no session, and there is no method to consume the redirected outcome. Those users therefore return to the default signed-out card and can never reach the setup or linking step; define a durable transaction/callback-resume contract for these non-session outcomes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right - a pending sign-up or link has no session, so a redirect lost it. Fixed in 2c61c9a: AuthClient gains resumeSignIn() (the outcome of a sign-in that left the page, or null), the redirect always returns to the sign-in page with next, and that page resumes from the outcome - signed in, "Prawie gotowe", account exists or the error - before going on to next. Spec ("The auth dialog", "The sign-in page", "Failure modes") and tasks #204, #207, #208, #209, #210 updated; the interface is in gi-org-pl/mypolitics-app#212 at 7cd8810.
| | | Terms accepted | The date and the version of the terms and of the privacy policy | Recorded on "Gotowe" | | ||
| | Linked provider | Provider and the provider's user identifier | A way to sign in to the account | At least one per account. The pair is unique across all accounts | | ||
| | Session | The account and when it was started | That this browser is signed in | Held by the API in a cookie the page cannot read. 30 days, extended on use | | ||
| | Pending sign-up | Provider, provider's user identifier, suggested name, photo, e-mail | A provider sign-in that has no account yet | Kept by the API for 15 minutes. Becomes an account on "Gotowe" or is dropped | |
There was a problem hiding this comment.
Gate pending profile storage on the age declaration
For a visitor under 16 who presses a provider and then cancels at Prawie gotowe, this design has already retained their provider identifier, name, photo, and e-mail in the API for up to 15 minutes before asking their age. That directly conflicts with the existing product rule in mypolitics/docs/platform/privacy-and-legal.md:21 that no personal data is processed below that age, and cancellation cannot undo the period in which it was already stored. Collect the declaration before copying the provider profile, or explicitly revise the privacy/legal basis for this pre-declaration processing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed that storing the profile before the declaration conflicts with privacy-and-legal. Changed in 2c61c9a rather than reordering the screens (the design asks for the age after the provider): the pending sign-up and the pending link are held only in the person's own browser, in an encrypted HttpOnly cookie that expires in 15 minutes and is deleted by Anuluj, by closing the dialog and by Wybierz inny sposób; the API writes nothing about the person - table, log or cache - before Gotowe. The spec states this as the reading of the under-16 rule (what was given to the provider passes through the sign-in and is never kept). The legal reading is listed as an open point for the president in the description; asking the age before the provider buttons is the alternative if it is not accepted.
| | The app starts | `session` is `unknown`, then the client's answer: `signed-in` with the account, or `signed-out` | | ||
| | `getSession` rejects | `signed-out` - the API being unreachable is treated as signed out | | ||
| | `setSignedIn(account)` | `signed-in` with that account at once, without asking the client | | ||
| | `signOut()` | The client's `signOut`, then `signed-out` - also when the client rejects | |
There was a problem hiding this comment.
Confirm server sign-out before clearing the local session
If client.signOut() rejects because the request never reached the API, the page cannot clear the HttpOnly session cookie, yet this row still presents a successful signed-out state. On a shared browser the next reload will restore the still-valid 30-day session, despite the user having seen the signed-out header; this also contradicts spec/platform/sign-in.md:270, which requires sign-out to end the API session. Preserve/report an uncertain session and allow retry rather than silently treating every rejection as success.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right. Fixed in 2c61c9a and implemented in gi-org-pl/mypolitics-app#212 (7cd8810): signOut() answers false when the client rejects and the session stays signed in; the header keeps the menu open with "Nie udało się wylogować. Spróbuj jeszcze raz." and the button can be pressed again (spec "The header", task #211). The page never shows a person signed out while the API may still hold the session.
| | `completeSignUp` → `failed` or rejects | The step with the save error | | ||
| | `completeSignUp` → `expired` | The choose view with the `failed` error | | ||
| | "Anuluj" | `client.cancelPending()` (its failure ignored), then `onCancel` | | ||
| | The flow is unmounted while waiting | Whatever the client answers later changes nothing and calls nothing; a `new-account` or `account-exists` answer that arrives late is dropped with `cancelPending()` | |
There was a problem hiding this comment.
Cancel in-flight authentication when closing the dialog
If the dialog is closed while a provider window is open and that window subsequently completes an existing-account sign-in, the API has already started the cookie session before returning the signed-in outcome. Merely ignoring the late result changes no local callbacks but does not undo that server-side session; likewise, this cleanup only calls cancelPending() for two pending outcomes and specifies no cancellation when a pending step is already displayed. This violates the close semantics in spec/platform/sign-in.md:173-174, so the API/client contract needs a flow-scoped cancellation or acknowledgement mechanism that runs on every dialog close.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right, ignoring the late answer left a server session behind. Fixed in 2c61c9a: signIn takes an abort signal; closing the dialog unmounts the flow, which aborts a running sign-in - the client closes the window and undoes whatever the sign-in started (ends a session, drops a pending sign-up or link). Closing while the step or the account-exists view is shown calls cancelPending(). While Gotowe is saving the dialog cannot be closed, so there is no half-saved account to undo. Spec "The auth dialog", tasks #204, #207, #208, #209.
| | The app starts | `session` is `unknown`, then the client's answer: `signed-in` with the account, or `signed-out` | | ||
| | `getSession` rejects | `signed-out` - the API being unreachable is treated as signed out | | ||
| | `setSignedIn(account)` | `signed-in` with that account at once, without asking the client | |
There was a problem hiding this comment.
Invalidate startup reads after local session transitions
While the initial getSession() is pending, the auth dialog is explicitly usable in the unknown state (cycle-4/auth-dialog.md:65), so a user can finish provider sign-in and call setSignedIn before that older request resolves. If the startup request then returns its pre-login null result or rejects, these transitions allow it to overwrite the new account with signed-out. Require a request generation/cancellation check so startup or refresh() responses are ignored after a newer sign-in or sign-out transition.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. The implementation in gi-org-pl/mypolitics-app#212 already counts versions and drops an answer asked for before a later sign-in or sign-out (tested: "keeps the sign-in when the late answer says signed out"); 2c61c9a writes the rule and its test into task #204 so it cannot be lost.
| | A second `requireAuth` while open | Ignored | | ||
| | Open | Focus moves into the dialog and stays there (Tab and Shift+Tab wrap, also after the content changes from the card to the step); the page behind does not scroll | | ||
| | `RequireAuth` signed in | Its children | | ||
| | `RequireAuth` signed out | Nothing of the page; the dialog with the intent's line. Signing in shows the children; closing goes back in history, or to the home page when there is no earlier page in the app | |
There was a problem hiding this comment.
Route guarded pages away immediately after sign-out
When the header signs out on a guarded page, the session transitions to signed-out, so this row instructs RequireAuth to hide the page and open another sign-in dialog; it only navigates away after that dialog is closed. The header task (header-account.md:36) and the sign-in spec (spec/platform/sign-in.md:201) instead require an immediate move to the home page after explicit sign-out. Distinguish a signed-in-to-signed-out transition from initially visiting a guarded route while signed out, and test the former.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right. Fixed in 2c61c9a: RequireAuth tells a change from signed in to signed out apart from arriving signed out, and on sign-out goes straight to the home page with no dialog (spec "Guarded pages", task #209 with its test).
…othing stored before the age Answers the review: a full-page provider sign-in comes back to the sign-in page, which resumes it; closing the dialog aborts a running sign-in and undoes it; the pending sign-up stays in the person's browser until Gotowe; a failed sign-out leaves the person signed in; a guarded page goes home on sign-out. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…out that can fail (#204) Follows the review of the spec (gi-org-pl/product#120): AuthClient gains resumeSignIn and an abort signal on signIn, and a sign-out the client could not complete leaves the person signed in. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c61c9ad56
ℹ️ 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".
| | Linked provider | Provider and the provider's user identifier | A way to sign in to the account | At least one per account. The pair is unique across all accounts | | ||
| | Session | The account and when it was started | That this browser is signed in | Held by the API in a cookie the page cannot read. 30 days, extended on use | | ||
| | Pending sign-up | Provider, provider's user identifier, suggested name, photo, e-mail | A provider sign-in that has no account yet | Held only in the person's browser, in an encrypted cookie the page cannot read, for 15 minutes. The API stores none of it until "Gotowe", see "Privacy and data handling". Becomes an account on "Gotowe" or is dropped | | ||
| | Pending link | Provider and provider's user identifier | A provider waiting to be linked to an account made with another one | Held the same way, for 15 minutes. Linked after the owning provider signs in | |
There was a problem hiding this comment.
Bind pending links to the exact owning account
When a person has multiple myPolitics accounts backed by different identities at the named owner provider and authenticates the wrong one, this pending state contains only the new provider/id, while the follow-up AuthClient.signIn(owner, ...) carries no target account. The API therefore cannot verify that the returned identity maps to the account whose e-mail caused account-exists; following the instruction to link after the owner provider signs in can attach the new credential to a different account and let that credential access it later. Include the target account identity in the signed pending state and reject a mismatched owner sign-in.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right - the pending link named only the new provider, so a person with two accounts at the owning provider could link it to the wrong one. Fixed in b8e51b4: the pending link names the account the address belongs to, and only a sign-in meant to link (link: true) whose identity belongs to that account links; any other answers the new outcome link-failed and starts no session. Rule added under "Rules and constraints"; tasks #204, #207 and #208 updated, and the stand-in in gi-org-pl/mypolitics-app#212 (717c683) behaves this way.
| | `signed-in` | `setSignedIn(account)` on the session, then `onSignedIn(account)` | | ||
| | `new-account` | The step, with the provider and the suggested name | | ||
| | `account-exists` | The panel's account-exists view | | ||
| | "Zaloguj się przez {owner}" | `client.signIn(owner, ...)`, the owner's button waits; `signed-in` finishes as above | |
There was a problem hiding this comment.
Surface an expired pending link instead of signing in
If the account-exists screen is left open past the pending link's 15-minute lifetime, or expiry occurs during the owner-provider authentication, this ordinary signIn(owner, ...) still returns signed-in for the existing account even though there is no longer anything to link. The flow then finishes and implies that the new provider was added; unlike completeSignUp, the contract has no expired-link outcome with which to restart the flow. Use a link-specific transaction/outcome and show the restart error when it expires.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right. Fixed in b8e51b4 with the same mechanism: the owner's sign-in from "To konto już istnieje" is a sign-in meant to link, and when the pending link has expired it answers link-failed instead of signing in. The card then returns with "Coś poszło nie tak" and the person starts over with the provider they wanted to add, which opens a fresh pending link (spec "The sign-in card", task #208).
| | `completeSignUp` → `signed-in` | `setSignedIn`, `onSignedIn` | | ||
| | `completeSignUp` → `failed` or rejects | The step with the save error | | ||
| | `completeSignUp` → `expired` | The choose view with the `failed` error | | ||
| | "Anuluj" | `client.cancelPending()` (its failure ignored), then `onCancel` | |
There was a problem hiding this comment.
Keep cancellation open until the pending cookie is cleared
In the revised contract, this row explicitly ignores a cancelPending() rejection and still invokes onCancel. When the API is unreachable, the pending profile/link remains in the encrypted HttpOnly cookie for up to 15 minutes because the page cannot clear that cookie itself, even though the sign-in doc and privacy section promise that Anuluj deletes it and keeps nothing. Keep the cancellation UI available until the API acknowledges an idempotent clear, or use browser-held state that can be erased offline.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changed, with the reason now written into the spec (b8e51b4, "Privacy and data handling"). The pending profile lives only in the person's own browser, in an encrypted HttpOnly cookie that expires after 15 minutes; our servers never hold it. When the API cannot be reached, keeping the step open until it answers would trap a person who wants to leave in a dialog they cannot close, for data that is not on our side anyway; so the step closes, and the cookie expires in their browser. The promise the step makes is about our servers ("nigdy nie będą powiązane"), and that holds. If the president prefers the stricter behaviour, the cancel button can stay busy with a retry, at the cost described.
… cancelling offline Answers the second review: the pending link names the account the address belongs to and only a sign-in meant to link, by an identity of that account, links; anything else answers link-failed and starts no session. Cancelling while the API is unreachable closes the step; the cookie expires in the person's browser. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8e51b47eb
ℹ️ 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".
| signIn(provider: IdentityProvider, options?: SignInOptions): Promise<SignInOutcome>; | ||
| resumeSignIn(): Promise<SignInOutcome | null>; // how a sign-in that left the page ended; null when none | ||
| completeSignUp(setup: AccountSetup): Promise<CompleteSignUpOutcome>; | ||
| cancelPending(): Promise<void>; // drops a pending sign-up or a pending link |
There was a problem hiding this comment.
Bind each pending operation to its authentication flow
When two tabs run auth flows (or two people share one browser profile), the encrypted pending cookie is shared, but these methods carry no flow handle: a second flow can replace the pending sign-up/link and make the first tab's completeSignUp create the second provider's account, or make its cancel/link operation affect the other flow. Even storing several pending entries in the cookie cannot select one from these calls; return a transaction identifier with each outcome and require it for resume, completion, linking, and cancellation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right - with one pending cookie and no handle, two tabs could take each other's place. Fixed in f51564b: every pending sign-up and pending link has its own identifier, returned with the outcome that created it (signUp.id, pendingId), and completion, cancellation and the owner's sign-in (link: pendingId) name it. Spec "The account" and "Interface", tasks #204, #207, #208; the stand-in in gi-org-pl/mypolitics-app#212 implements it (448afae).
| | `signIn` rejects | As `failed` | | ||
| | "Gotowe" | `client.completeSignUp(setup)`; the step is saving and `onBusyChange(true)`, then `onBusyChange(false)` when it settles | | ||
| | `completeSignUp` → `signed-in` | `setSignedIn`, `onSignedIn` | | ||
| | `completeSignUp` → `failed` or rejects | The step with the save error | |
There was a problem hiding this comment.
Reconcile completion when its response is lost
If completeSignUp commits the account and session but the HTTP response times out or is lost, this branch reports a save failure even though the provider is already linked and the HttpOnly session may already be installed. The user can then press "Anuluj" or close believing nothing was saved, and a retry can return expired after the pending cookie was cleared; make completion idempotent with a transaction key or reconcile the session before presenting a retryable failure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right. Fixed in f51564b on both sides: completion is repeatable - completing a sign-up already completed answers with the account it made (named by its identifier, so it is safe) - and before the flow shows a failed save it asks for the session; if the account was saved and only the answer was lost, the step finishes. Spec "The 'Prawie gotowe' step", tasks #207 and #208.
| | `resume` and the client has an outcome | The flow starts from that outcome, as if a provider window had just answered | | ||
| | `resume` and the client has none | The choose view | |
There was a problem hiding this comment.
Block new attempts until redirect resumption settles
On the /login callback, resumeSignIn() is asynchronous, but the flow defines only the eventual outcome/none cases and no resuming state. With a slow resume request, the initial choose view can accept another provider click; the old redirect outcome can then replace that new waiting state or call onSignedIn while the second popup is still open. Keep the card hidden or disabled until resume settles and invalidate a late resume result after any newer attempt or unmount.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right. Fixed in f51564b: while the sign-in page asks how a full-page sign-in ended, it shows nothing in place of the card, so no second sign-in can start; a late answer after unmount was already ignored. Spec "The sign-in page", tasks #208 and #210.
…n, resuming first Answers the third review: every pending sign-up and link is named by its own identifier, completing a sign-up again answers with its account and the flow checks the session before showing a failed save, and the sign-in page shows nothing until a resumed sign-in is known. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f51564b88b
ℹ️ 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".
| | `signIn` rejects | As `failed` | | ||
| | "Gotowe" | `client.completeSignUp(signUp.id, setup)`; the step is saving and `onBusyChange(true)`, then `onBusyChange(false)` when it settles | | ||
| | `completeSignUp` → `signed-in` | `setSignedIn`, `onSignedIn` | | ||
| | `completeSignUp` → `failed` or rejects | The flow asks for the session: signed in, it finishes; otherwise the step with the save error | |
There was a problem hiding this comment.
Reconcile completion with the pending identifier
When the API commits a sign-up but the response is lost before the browser receives the session Set-Cookie, getSession() returns signed out even though the account now exists. The flow then exposes the save error and Anuluj, allowing the user to leave believing nothing was saved; the new idempotence is never used unless they happen to retry manually. The fresh evidence relative to the prior lost-response finding is that this recovery checks ambient session state rather than replaying or status-checking signUp.id before cancellation becomes available.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right - the session check could miss an account whose cookie never arrived. Fixed in ec47aaf: a failed save repeats the completion of the same sign-up once (the API answers a repeated completion with the account it made), and only then shows the error. Implemented in gi-org-pl/mypolitics-app#215 (6dca753).
| | "Wybierz inny sposób" | `client.cancelPending(pendingId)`, then the choose view without an error | | ||
| | `window-closed`, `provider-failed`, `failed` | The choose view with that error | | ||
| | `signIn` rejects | As `failed` | | ||
| | "Gotowe" | `client.completeSignUp(signUp.id, setup)`; the step is saving and `onBusyChange(true)`, then `onBusyChange(false)` when it settles | |
There was a problem hiding this comment.
Keep the dialog locked through completion recovery
When completeSignUp settles as failed after its Set-Cookie was already installed, this resets onBusyChange before the session reconciliation on the next rows finishes. Escape or the close button can therefore unmount the flow during that read, after which the completed account/session cannot be undone and the eventual successful reconciliation is ignored. The fresh evidence is the newly added recovery that now runs after this explicit busy reset; keep the host uncancellable until that recovery settles.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right. Fixed in ec47aaf: the step stays in saving and the dialog cannot be closed until the repeat has answered; onBusyChange(false) comes after it. Implemented in gi-org-pl/mypolitics-app#215 (6dca753), with a test of the busy calls.
| | `window-closed`, `provider-failed`, `failed` | The choose view with that error | | ||
| | `signIn` rejects | As `failed` | |
There was a problem hiding this comment.
Reconcile uncertain provider sign-in outcomes
When an existing-account or owner-link OAuth callback has installed the HttpOnly session but the popup's final response/message is lost, these branches only show the generic failure view. The abort/undo contract applies only while the flow is still waiting, so closing this error view can leave the API signed in (and possibly the provider linked) although the user was told sign-in failed. Preserve a transaction until the outcome is acknowledged, or reconcile/undo it before exposing the failure state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right. Fixed in ec47aaf: a failed outcome (or a rejection) of a provider sign-in is checked against the session before the error is shown; if the session was started, the flow finishes signed in. A linked provider then stays linked, which is what the person asked for. Implemented in gi-org-pl/mypolitics-app#215 (6dca753).
| | Signed in on the page | Goes to `next`, or home | | ||
| | "Anuluj" on the step | Back to the card with three providers | | ||
| | `next` missing | Home | | ||
| | `next` to another site, `//host`, a scheme, or not starting with `/` | Ignored: home | |
There was a problem hiding this comment.
Reject backslashes in the post-login destination
A next value such as /\evil.example starts with /, is not //, and has no explicit scheme, so it passes every invalid case listed here. Browsers normalize backslashes as slashes when resolving special-scheme URLs, which can turn this into a cross-origin destination (or make router navigation throw) after login. Validate with a same-origin URL parser or explicitly reject backslashes, and add this encoded query case to toSafeNextPath tests.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The implementation already rejects it (gi-org-pl/mypolitics-app#217: toSafeNextPath refuses /\ and checks the result with a URL parser against the app's own origin, which also catches a tab or line break that turns the path into another host; the encoded form arrives decoded from the query). The task and the spec did not say so; they do in ec47aaf.
| | Case | Behaviour | | ||
| |---|---| | ||
| | The app starts | `session` is `unknown`, then the client's answer: `signed-in` with the account, or `signed-out` | | ||
| | `getSession` rejects | `signed-out` - the API being unreachable is treated as signed out | |
There was a problem hiding this comment.
Preserve an established session when refresh is unreachable
Because refresh() also calls getSession(), this unconditional rejection rule turns an already signed-in session into signed-out during any transient API outage. The valid HttpOnly cookie may still exist, but the header switches to login and guarded pages navigate home until a reload restores it; only the initial startup check is specified to fail closed this way. On a refresh rejection, retain the last known signed-in state and report uncertainty instead of asserting that the session ended.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right. Fixed in ec47aaf (spec "Failure modes", task #204) and in gi-org-pl/mypolitics-app#212 (2ef5b7e): only the first check fails closed; a later refresh that cannot be answered leaves a signed-in session as it is. Test: "given a signed-in session and a refresh that cannot be answered - stays signed in".
…he navigation The frames draw it light, and filled only on the sign-in page. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… keep a known session Answers the fourth review: a failed save is repeated once and the dialog stays locked until it answers, a failed sign-in is checked against the session, a later outage keeps a known session, and next rejects /\. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec47aaf280
ℹ️ 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".
| | Need | Rule | | ||
| |---|---| | ||
| | Identity | Provider + the provider's user identifier, unique. E-mail optional, verified only - X often gives none | | ||
| | Session | A cookie the page cannot read (`HttpOnly`, `Secure`, `SameSite=Lax`), 30 days, extended on use; it records when it started | |
There was a problem hiding this comment.
Send cookies with cross-origin auth API requests
For production, the app is served from https://mypolitics.pl while the shared client targets https://api.mypolitics.pl/api (survey-api.md:137), so these are cross-origin browser requests. Axios defaults withCredentials to false, and neither this cookie contract nor the later instruction to reuse apiClient requires credentialed CORS; implemented as written, getSession, pending completion, and sign-out will not send these HttpOnly cookies, and auth responses cannot reliably establish them. Require credentialed requests plus the corresponding API CORS policy and cover this in an integration test.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right: the app and the API are two origins of one site, and Axios sends and accepts cookies across origins only with withCredentials. Fixed in 09beadd: the API needs gain "Cross-origin cookies" (CORS for the app's own origins by name and Access-Control-Allow-Credentials: true, never *), and the behaviour gains "Every auth request": credentials on each auth request, not on the shared apiClient. The survey and result requests stay without credentials on purpose, so the API cannot join an account to answers ("Never" in the same table). A unit case checks both, and the hand check in the Definition of Done is run from the app's own origin. Issue #207 updated to match.
| | `account-exists` | The panel's account-exists view | | ||
| | "Zaloguj się przez {owner}" | `client.signIn(owner, { link: pendingId, ... })`, the owner's button waits; `signed-in` finishes as above | | ||
| | `link-failed` | The choose view with the `failed` error | | ||
| | "Wybierz inny sposób" | `client.cancelPending(pendingId)`, then the choose view without an error | |
There was a problem hiding this comment.
Continue after an offline pending-link cancellation
If the network drops after the account-exists view is shown, cancelPending can reject and this row only returns to the provider choice after it resolves. Unlike Anuluj at line 57, the rejection is not ignored, so pressing Wybierz inny sposób can leave the person trapped on the owner-provider screen even though the accepted offline policy lets the browser cookie expire; transition in a finally/failure path as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right: the row said "then", so read literally the view waited on the request. Fixed in 09beadd: "Wybierz inny sposób" and "Anuluj" both drop the pending link or sign-up without waiting and ignore a failure (the cookie expires in the browser, as the offline policy says), with a unit case for the offline press. The flow in gi-org-pl/mypolitics-app#215 already behaves this way (chooseAnother and cancelSetup, test "given the pending sign-up cannot be dropped - still walks away"). Issue #208 updated to match.
| | "Wybierz inny sposób" | `client.cancelPending(pendingId)`, then the choose view without an error | | ||
| | `window-closed`, `provider-failed`, `failed` | The choose view with that error | | ||
| | `signIn` rejects | As `failed` | | ||
| | `failed` (or a rejection) | The flow asks for the session first: signed in, it finishes as `signed-in`; otherwise the choose view with the `failed` error | |
There was a problem hiding this comment.
Reconcile failed links using their pending transaction
The newly added ambient-session fallback is the fresh evidence: when the owner-provider request rejects before linking while another tab has established a session, getSession() returns that tab's account and this row finishes the flow. The pending provider is still unlinked—and the protected action can run under the wrong account—despite the exact-link invariant in spec/platform/sign-in.md:249; reconcile the specific pendingId/OAuth transaction rather than accepting any session.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right about the fact: a failed owner's sign-in is checked with getSession(), which answers whatever session the browser holds, so if another tab signed in meanwhile the flow finishes under that account and the link is not made. Nothing is linked wrongly (the API linked nothing, so the rule at sign-in.md:249 holds), but the protected action then runs under the other tab's account.
Not changed in this round, because each remedy changes the contract the spec and gi-org-pl/mypolitics-app#212 implement: (a) a new API need and AuthClient call that asks how the named pending link ended, by pendingId, which the check would use instead of the session; or (b) a lost owner's sign-in is reported as failed without the session check, so a link that did happen shows an error once and the session appears on the next read. The choice is put to the owner; until then the flow (#215) keeps the session check as written.
…without waiting auth-api: the app and the API are two origins of one site, so the auth requests carry credentials and the API answers them with CORS for the app's origins and credentials allowed; the survey and result requests stay without them, so no account is joined to answers. sign-in-flow: "Wybierz inny sposób" and "Anuluj" drop the pending link or sign-up without waiting and ignore a failure, as the flow already does and as the offline policy of the spec allows. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What
The sign-in of myPolitics 2.0 - social providers only - written down before it is built, as approved with the design on 2026-10-15.
docs/platform/sign-in.md(🟢 GO) - one door for signing in and signing up with Google, Facebook or X; the account saved only after the "Prawie gotowe" step; no merging on e-mail; the card on a page of its own and as a dialog over any action that needs an account.spec/platform/sign-in.md(mixed) - the card and its states, the completing step, the/loginpage, the universal auth dialog (modal on a computer, bottom sheet on a phone, a line per action, the action carries on after signing in, closing changes nothing), guarded pages, the header, and the contract the API has to keep.tasks/epics/accounts/, each published as a sub-issue of the new epic [Epic] Accounts mypolitics-app#203:auth-sessionAuthClientinterface, a stand-in client, the session at the rootsign-in-panelaccount-setup-formauth-apiblocked)sign-in-flowauth-dialoguseRequireAuth,RequireAuth; first consumer "Stwórz własny"sign-in-page/loginwith a safenextheader-accountaccounts.md, and the identity line ofprivacy-and-legal.md(it named a password, which no longer exists).This pull request comes from the bot's fork (
gieniek-ai/product): a ruleset of this repository lets only admins create or update branches, and that was not worked around.The universal dialog
The design drew the dialog over the editor start. It is specified and cut as a component of the app, not of the editor: a provider at the root, and any part of the app calls
requireAuth("<action>", action). Signed in, the action runs at once; signed out, the dialog opens with the action's line and the action runs once after signing in (including the "Prawie gotowe" step); closing runs nothing and returns focus. A page that needs an account wraps itself inRequireAuth. A new action that needs an account costs one entry in the list of actions (its line) and one call - nothing in the dialog changes. "Stwórz własny" on the home page is the first consumer; its action stays empty until the editor exists.Decisions taken (the president can override any)
/loginpage (as the frames draw it), the dialog is for actions.nextaccepts only paths inside the app.ec47aaf): a failed sign-in meant to link is checked with the session, which answers whatever account the browser holds; if another tab signed in meanwhile, the flow finishes under that account and the link is not made (nothing is linked wrongly). Either (a) the API gains a way to ask how the named pending link ended, bypendingId, and the check uses it, or (b) a lost owner's sign-in is reported as failed without the session check. For the president to choose. Blocks: nothing now; the flow (feat: SignInFlow - the sign-in from the first press to the account (#208) mypolitics-app#215) keeps the session check until then.Review
Codex reviewed 533c7c2 with six comments, all accepted and answered in their threads; fixed in 2c61c9a:
resumeSignIn) - before, a pending sign-up was lost on a redirect;Later rounds, all accepted and answered in their threads:
b8e51b4- a pending link names the account the address belongs to; only a sign-in meant to link, by an identity of that account, links (link-failedotherwise); cancelling while the API is unreachable closes the step and the cookie expires in the person's browser (the one point answered without the change Codex proposed - see decision 13).f51564b- every pending sign-up and link has its own identifier, so two tabs never mix; completion is repeatable; the sign-in page shows nothing until a resumed sign-in is known.ec47aaf- a failed save is repeated once and the dialog stays locked until it answers; a failed provider sign-in is checked against the session; a later outage keeps a known session;nextrejects/\.c4559e5- the header's "Zaloguj się" is drawn like the navigation pills, filled on the sign-in page, as the frames draw it (the spec had said "filled main pill").ec47aafon 2026-10-18 (three comments):09beadd- the auth requests carry credentials and the API answers them with credentialed CORS for the app's origins, while the survey and result requests stay without them; "Wybierz inny sposób" and "Anuluj" drop the pending link or sign-up without waiting. The third comment is decision 14, answered in its thread and left open.Verification
mkdocs buildonc4559e5: no warnings. Not rerun on09beadd(two table rows and a test line in two task files;mkdocsdoes not start on the machine this round).09beadd(#207 and #208), with branch names filled in; each is a sub-issue of #203 (attached through the sub-issues API, asghhere has no--parent).mainand do not resolve until this pull request is merged.Not reviewed yet:
09beadd. Codex reviewedec47aafon 2026-10-18 and then reached its usage limit again on the other pull requests of the sign-in work, so it was not asked here. To be asked once it resets.🤖 Generated by Gieniek with Claude Code