-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix(auth): derive the Google hd hint from an address-only allow list #213
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -33,7 +33,19 @@ export function workspaceDomains(): readonly string[] { | |||||
| } | ||||||
|
|
||||||
| export function primaryWorkspaceDomain(): string | undefined { | ||||||
| return allowList().domains[0]; | ||||||
| const { domains, addresses } = allowList(); | ||||||
|
|
||||||
| if (domains[0]) return domains[0]; | ||||||
|
|
||||||
| // An address-only allow list still names a workspace domain, and a solo | ||||||
| // self-hoster on `me@acme.com` wants the same account chooser as one on | ||||||
| // `acme.com`. Only when every allowed address shares a domain: `hd` narrows | ||||||
| // the chooser to one, so sending it for one of several would hide the rest. | ||||||
| const hosts = new Set( | ||||||
| addresses.map((address) => address.split("@")[1]).filter(Boolean), | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: When an address entry has no host, Prompt for AI agents
Suggested change
|
||||||
| ); | ||||||
|
|
||||||
| return hosts.size === 1 ? [...hosts][0] : undefined; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: When an address-only allow-list rejects Prompt for AI agents |
||||||
| } | ||||||
|
|
||||||
| export function hasSignInAllowList(): boolean { | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| import { afterEach, describe, expect, it } from "bun:test"; | ||
| import { isWorkspaceEmail, primaryWorkspaceDomain } from "../src/workspace"; | ||
|
|
||
| const original = process.env.ALLOWED_SIGN_IN; | ||
|
|
||
| afterEach(() => { | ||
| if (original === undefined) delete process.env.ALLOWED_SIGN_IN; | ||
| else process.env.ALLOWED_SIGN_IN = original; | ||
| }); | ||
|
|
||
| describe("the domain behind the account chooser", () => { | ||
| it("is the configured domain when one is configured", () => { | ||
| process.env.ALLOWED_SIGN_IN = "acme.com"; | ||
| expect(primaryWorkspaceDomain()).toBe("acme.com"); | ||
| }); | ||
|
|
||
| it("is derived from a single address, so a solo self-hoster gets the hint too", () => { | ||
| process.env.ALLOWED_SIGN_IN = "rep@acme.com"; | ||
| expect(primaryWorkspaceDomain()).toBe("acme.com"); | ||
| }); | ||
|
|
||
| it("is withheld when the addresses span more than one domain", () => { | ||
| // `hd` narrows the chooser to one domain, so sending it for one of two | ||
| // would hide the other rather than help. | ||
| process.env.ALLOWED_SIGN_IN = "rep@acme.com,other@beta.com"; | ||
| expect(primaryWorkspaceDomain()).toBeUndefined(); | ||
| }); | ||
|
|
||
| it("still prefers a configured domain over an address", () => { | ||
| process.env.ALLOWED_SIGN_IN = "rep@beta.com,acme.com"; | ||
| expect(primaryWorkspaceDomain()).toBe("acme.com"); | ||
| }); | ||
|
|
||
| it("is nothing when the list is empty, which fails closed", () => { | ||
| process.env.ALLOWED_SIGN_IN = ""; | ||
| expect(primaryWorkspaceDomain()).toBeUndefined(); | ||
| }); | ||
|
|
||
| it("does not widen who may sign in", () => { | ||
| process.env.ALLOWED_SIGN_IN = "rep@acme.com"; | ||
| expect(isWorkspaceEmail("rep@acme.com")).toBe(true); | ||
| expect(isWorkspaceEmail("someone-else@acme.com")).toBe(false); | ||
| }); | ||
| }); |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: AGENTS.md forbids all code comments ("Never add code comments. Not to new code, not to code you edit."), and this change adds a four-line comment block in production code. Remove the comment; the branch logic is self-explanatory. The added comment in packages/auth/test/workspace.spec.ts (
// hd narrows the chooser to one domain...) violates the same rule.Prompt for AI agents