-
Notifications
You must be signed in to change notification settings - Fork 2
feat(init): add audience prompt for admin-provided vs DIY OAuth setup #124
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
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 |
|---|---|---|
|
|
@@ -137,6 +137,7 @@ func TestValidateOAuthJSONRejectsGarbage(t *testing.T) { | |
|
|
||
| // stubPrompter records calls and returns canned values. | ||
| type stubPrompter struct { | ||
| audience string | ||
| credChoice string | ||
| pasteJSON string | ||
| filePath string | ||
|
|
@@ -151,6 +152,14 @@ type stubPrompter struct { | |
| calls []string | ||
| } | ||
|
|
||
| func (s *stubPrompter) SelectAudience() (string, error) { | ||
| s.calls = append(s.calls, "audience") | ||
| if s.audience == "" { | ||
| return "diy", nil | ||
| } | ||
| return s.audience, nil | ||
| } | ||
|
|
||
| func (s *stubPrompter) SelectCredSource(_ bool) (string, error) { | ||
| s.calls = append(s.calls, "select") | ||
| return s.credChoice, nil | ||
|
|
@@ -286,7 +295,8 @@ func TestEnsureCredentialsFlagFile(t *testing.T) { | |
| } | ||
| dst, _ := d.GetCredentialsPath() | ||
|
|
||
| d.Prompter = &stubPrompter{} | ||
| stub := &stubPrompter{} | ||
| d.Prompter = stub | ||
| if err := ensureCredentials(d, &initOptions{credentialsFile: src}, dst); err != nil { | ||
| t.Fatalf("ensureCredentials: %v", err) | ||
| } | ||
|
|
@@ -300,6 +310,10 @@ func TestEnsureCredentialsFlagFile(t *testing.T) { | |
| if perm := fs.perms[dst]; perm != 0600 { | ||
| t.Errorf("expected perms 0600, got %o", perm) | ||
| } | ||
| // --credentials-file bypasses the wizard entirely; audience must not be asked. | ||
| if contains(stub.calls, "audience") { | ||
| t.Errorf("expected --credentials-file bypass; SelectAudience was called: calls=%v", stub.calls) | ||
| } | ||
| } | ||
|
|
||
| func TestEnsureCredentialsRejectsBadJSON(t *testing.T) { | ||
|
|
@@ -397,6 +411,95 @@ func TestEnsureCredentialsShortCircuitsWhenAlreadyPresent(t *testing.T) { | |
| if contains(stub.calls, "select") { | ||
| t.Errorf("expected wizard short-circuit, but SelectCredSource was called: calls=%v", stub.calls) | ||
| } | ||
| // Audience prompt must not be reached before the credentials.json short-circuit. | ||
| if contains(stub.calls, "audience") { | ||
| t.Errorf("expected short-circuit before audience prompt; calls=%v", stub.calls) | ||
| } | ||
| } | ||
|
|
||
| func TestEnsureCredentialsAudienceAdminSkipsDIYSteps(t *testing.T) { | ||
| t.Parallel() | ||
| fs := newFakeFS() | ||
| d := baseDeps(t, fs) | ||
| out := &bytes.Buffer{} | ||
| d.View = view.NewWithWriters(out, &bytes.Buffer{}) | ||
| dst, _ := d.GetCredentialsPath() | ||
| d.Prompter = &stubPrompter{audience: "admin", credChoice: "paste", pasteJSON: validOAuthJSON} | ||
|
|
||
| if err := ensureCredentials(d, &initOptions{}, dst); err != nil { | ||
| t.Fatalf("ensureCredentials: %v", err) | ||
| } | ||
| got := out.String() | ||
| if strings.Contains(got, "Enable APIs: Gmail") { | ||
| t.Errorf("admin audience should not show DIY steps; output:\n%s", got) | ||
| } | ||
| if !strings.Contains(got, "Your admin should have shared credentials.json") { | ||
| t.Errorf("admin audience should show the admin-provisioned one-liner; output:\n%s", got) | ||
| } | ||
| } | ||
|
|
||
| func TestEnsureCredentialsAudienceDIYShowsDIYStepsAndAdminPointer(t *testing.T) { | ||
| t.Parallel() | ||
| fs := newFakeFS() | ||
| d := baseDeps(t, fs) | ||
| out := &bytes.Buffer{} | ||
| d.View = view.NewWithWriters(out, &bytes.Buffer{}) | ||
| dst, _ := d.GetCredentialsPath() | ||
| d.Prompter = &stubPrompter{audience: "diy", credChoice: "paste", pasteJSON: validOAuthJSON} | ||
|
|
||
| if err := ensureCredentials(d, &initOptions{}, dst); err != nil { | ||
| t.Fatalf("ensureCredentials: %v", err) | ||
| } | ||
| got := out.String() | ||
| if !strings.Contains(got, "Enable APIs: Gmail") { | ||
| t.Errorf("diy audience should show DIY steps; output:\n%s", got) | ||
| } | ||
| if !strings.Contains(got, "https://github.com/open-cli-collective/google-readonly/blob/main/WORKSPACE_ADMINS.md") { | ||
| t.Errorf("diy audience should show the fully-qualified Workspace admin doc URL (so Homebrew/Choco users can reach it); output:\n%s", got) | ||
|
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. 🟡 Medium (harness-engineering:harness-self-documenting-code-reviewer): TestEnsureCredentialsAudienceIsAskedOncePerWizard accesses reads[idx] without bounds checking. If ClipboardReadAll is called more than twice (e.g., after a refactor raising the retry cap), the test panics with an index-out-of-range instead of failing cleanly. Use a bounds-checked helper or t.Fatal inside the closure. Reply to this thread when addressed.
Contributor
Author
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. Fixed in 8a40d4d — the ClipboardReadAll closure now checks |
||
| } | ||
| } | ||
|
|
||
| func TestEnsureCredentialsAudienceIsAskedOncePerWizard(t *testing.T) { | ||
| t.Parallel() | ||
| fs := newFakeFS() | ||
| d := baseDeps(t, fs) | ||
| d.ClipboardSupported = func() bool { return true } | ||
| // First clipboard read returns invalid JSON, second returns valid. This | ||
| // forces the wizard's retry loop to run twice while audience must remain | ||
| // sticky. Bounds-check: if the wizard reads more than twice (e.g. a future | ||
| // retry-cap change), fail cleanly instead of panicking. | ||
| reads := []string{"not valid json", validOAuthJSON} | ||
| idx := 0 | ||
| d.ClipboardReadAll = func() (string, error) { | ||
| if idx >= len(reads) { | ||
| t.Fatalf("unexpected extra ClipboardReadAll call #%d; wizard should have succeeded after 2 reads", idx+1) | ||
| } | ||
| s := reads[idx] | ||
| idx++ | ||
| return s, nil | ||
| } | ||
| dst, _ := d.GetCredentialsPath() | ||
| stub := &stubPrompter{audience: "admin", credChoice: "clipboard"} | ||
| d.Prompter = stub | ||
|
|
||
| if err := ensureCredentials(d, &initOptions{}, dst); err != nil { | ||
| t.Fatalf("ensureCredentials: %v", err) | ||
| } | ||
| gotAudience, gotSelect := 0, 0 | ||
| for _, c := range stub.calls { | ||
| switch c { | ||
| case "audience": | ||
| gotAudience++ | ||
| case "select": | ||
| gotSelect++ | ||
| } | ||
| } | ||
| if gotAudience != 1 { | ||
| t.Errorf("expected SelectAudience called exactly once, got %d; calls=%v", gotAudience, stub.calls) | ||
| } | ||
| if gotSelect != 2 { | ||
| t.Errorf("expected SelectCredSource called exactly twice (retry), got %d; calls=%v", gotSelect, stub.calls) | ||
| } | ||
| } | ||
|
|
||
| func TestEnsureCredentialsTightensPermsOnOverwrite(t *testing.T) { | ||
|
|
||
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.
🔵 Low (harness-engineering:harness-self-documenting-code-reviewer): stubPrompter.SelectAudience() silently returns "diy" when the audience field is the zero value (empty string). Any existing or future test that constructs a &stubPrompter{} without setting audience will quietly exercise the diy path rather than failing. A sentinel value or required-non-empty check would surface misconfigured stubs earlier.
Reply to this thread when addressed.
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.
Declining — this was a deliberate design choice flagged in the architect review for this PR. The zero-value default to 'diy' was specifically requested to avoid touching ~20 existing test setups that would otherwise need to set
audience: "diy"to keep compiling. Existing tests that exercise the wizard exercised the DIY path before this PR; defaulting their zero-value to 'diy' preserves that semantic exactly. New tests that care about audience set it explicitly. The two bypass tests (TestEnsureCredentialsFlagFile,TestEnsureCredentialsShortCircuitsWhenAlreadyPresent) now assertaudienceis NOT incalls, which catches the misuse case you're worried about: if a wizard test forgot to think about audience and the audience prompt fires unexpectedly, those tests would surface it.