docs: add WORKSPACE_ADMINS.md for Internal OAuth setup - #122
Conversation
Walkthrough for Google Workspace admins to set up an Internal OAuth app once and distribute credentials.json to org users. Internal audience skips Google's app verification (no CASA, no 100-user cap, no unverified-app warning). Linked from README. Rationale that previously lived in local exploration notes is preserved in the new doc. Closes #121
|
Blocker
Major
Minor
Code Quality Lens: scope is otherwise clean, README edits are traceable, and the enforcement language mostly avoids overclaiming. The remaining issues are one false implementation claim, one speculative Google-policy claim, one incomplete non-destructive mapping, and one walkthrough command footgun. |
- Remove PKCE claim (gro doesn't use PKCE; oauth2.VerifierOption is not passed in internal/auth/auth.go:130). Replace with accurate framing: access is bounded by Internal audience + user consent. - Soften future-policy claim about Google's verification rules. - Expand scope-by-scope rows for calendar.events and contacts to explicitly state what gro does NOT do (no event/contact create or delete; no editing of event/contact fields beyond RSVP-color and group-star). - Replace verify-step file-move with `gro init --credentials-file` to avoid mkdir/glob footguns.
|
Findings None. The updated diff adheres to the architectural intent: it documents the Internal-audience admin path, keeps the README change scoped and discoverable, traces the non-destructive promise through scopes + qualified CI guardrails + command-surface absence, and removes the prior PKCE and future-policy overclaims. Code Quality Lens: no drive-by edits, no speculative abstraction, no code behavior changes, and the verification path now uses the existing |
TDD assessmentTL;DR: No new tests needed. Merge as docs-only. This is a pure documentation PR ( On the "doc could go stale" questionThe doc embeds two things that could drift from code:
Both are already covered structurally:
So any code-side drift (adding an 8th scope, swapping one out, adding a Why I'm not recommending a doc-parsing structural testA test that greps
If scope drift vs. docs ever becomes a real problem (e.g. someone ships a scope change without doc updates and it ships to users), revisit then. Minor doc-only nits (non-blocking)
|
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 42861e3
Summary
| Reviewer | Findings |
|---|---|
| documentation:docs-reviewer | 2 |
| harness-engineering:harness-architecture-reviewer | 1 |
| harness-engineering:harness-enforcement-reviewer | 1 |
documentation:docs-reviewer (2 findings)
WORKSPACE_ADMINS.md:137
Step 6 documents
gro init --credentials-file <path>but the PR description states 'No code changes' and that admin-provided credential path support is deferred to a follow-up PR. If this flag is not implemented, every admin following the guide will hit an 'unknown flag' error during the self-verification step, undermining trust in the documentation. Verify the flag exists in the cobra command definition forgro initor replace the step with the workaround: place the file at~/.config/google-readonly/credentials.jsonand rungro initwithout flags.
WORKSPACE_ADMINS.md:148
The expected terminal output 'Token saved to Keychain' is macOS-specific. Per CLAUDE.md, Linux users see libsecret output or file-based storage messages. Admins verifying the setup on Linux may think the process failed. Update the expected output to cover all platforms: macOS ('Token saved to Keychain'), Linux with libsecret, and the file-based fallback ('Token saved to ~/.config/google-readonly/token.json').
harness-engineering:harness-architecture-reviewer (1 findings)
WORKSPACE_ADMINS.md:95
The scope list in Step 4 (including
https://www.googleapis.com/auth/contactsandhttps://www.googleapis.com/auth/drive.metadata) must be verified to exactly match the literal strings ininternal/auth/auth.go:AllScopesandinternal/auth/auth_test.go. A mismatch — either extra or missing scopes — means admins configure the wrong consent screen, causing auth failures for all org users. The PR's own test-plan item requires this verification but it is not evident that it was completed.
harness-engineering:harness-enforcement-reviewer (1 findings)
💡 Suggestion - WORKSPACE_ADMINS.md:76
https://www.googleapis.com/auth/calendar.eventsappears in the scope table but may not be inAllScopes. The PR description's test plan says all 7 scope URLs must matchinternal/auth/auth_test.goassertions — ifcalendar.eventsis absent fromAllScopesit won't appear on the consent screen, making the admin setup guide misleading. Confirm this scope is present inAllScopesor remove it from the doc.
2 info-level observations excluded. Run with --verbose to include.
3 PR discussion threads considered.
Completed in 3m 08s | $0.51 | sonnet | daemon 0.2.116 | Glorfindel
| Field | Value |
|---|---|
| Model | sonnet |
| Reviewers | hybrid-synthesis, documentation:docs-reviewer, harness-engineering:harness-architecture-reviewer, harness-engineering:harness-enforcement-reviewer, harness-engineering:harness-knowledge-reviewer |
| Engine | claude · sonnet |
| Reviewed by | pr-review-daemon · monit-pr-reviewer |
| Duration | 3m 08s wall · 4m 50s compute (Reviewers: 1m 45s · Synthesis: 1m 20s) |
| Cost | $0.51 |
| Tokens | 249.4k in / 20.5k out |
| Turns | 14 |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost |
|---|---|---|---|---|---|---|
| hybrid-synthesis | sonnet | 30.0k | 4.2k | 13.6k | 16.4k (1h) | $0.14 |
| documentation:docs-reviewer | haiku | 9.2k | 11.3k | 0 | 9.2k (1h) | $0.07 |
| harness-engineering:harness-architecture-reviewer | sonnet | 70.1k | 1.7k | 56.3k | 13.8k (1h) | $0.10 |
| harness-engineering:harness-enforcement-reviewer | sonnet | 70.3k | 1.6k | 56.4k | 13.9k (1h) | $0.10 |
| harness-engineering:harness-knowledge-reviewer | sonnet | 69.8k | 1.7k | 56.1k | 13.7k (1h) | $0.10 |
Re-reviews only run when @monit-reviewer is re-requested as a reviewer — push as many commits as you need, then re-request when ready. PRs targeting branches other than main, master are skipped, even when @monit-reviewer is re-requested.
Daemon reviewer correctly flagged the macOS-specific 'Token saved to Keychain' as misleading for Linux users. Replace with the actual 'Token saved to <storage>' template, naming all three backends (Keychain / libsecret / token.json fallback).
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 5e865cc | Previous: 42861e3 (incremental)
Approved with 2 non-blocking suggestions below. Address at your discretion.
Summary
| Reviewer | Findings |
|---|---|
| harness-engineering:harness-enforcement-reviewer | 2 |
harness-engineering:harness-enforcement-reviewer (2 findings)
💡 Suggestion - WORKSPACE_ADMINS.md:97
The scope table rows for
calendar.eventsandcontactsdescribe behavioral restrictions ('No event creation/deletion', 'No contact creation/deletion') without the same caveat thegmail.modifyrow carries — that the restriction is code-enforced, not scope-enforced. Both scopes grant full write access at the API level; compliance reviewers reading the table alone may conclude the scope itself prevents destructive operations. Add a note consistent with thegmail.modifyrow (e.g. 'Behavioral restriction enforced by code, not by scope') for both rows.
💡 Suggestion - WORKSPACE_ADMINS.md:131
The client-secret rotation guidance states 'Active user tokens issued before rotation continue to work until they're revoked or expire.' This is accurate for short-lived access tokens but misleading for refresh tokens — Google invalidates refresh tokens when a client secret is rotated, requiring users to re-authorize. Admins rotating a compromised secret will expect continuity but instead face org-wide re-auth. Clarify that refresh tokens are invalidated on secret rotation.
6 info-level observations excluded. Run with --verbose to include.
7 PR discussion threads considered.
Completed in 2m 55s | $0.62 | sonnet | daemon 0.2.116 | Glorfindel
| Field | Value |
|---|---|
| Model | sonnet |
| Mode | Re-review · Cycle 2 · Session resumed |
| Reviewers | hybrid-synthesis, documentation:docs-reviewer, harness-engineering:harness-architecture-reviewer, harness-engineering:harness-enforcement-reviewer, harness-engineering:harness-knowledge-reviewer |
| Engine | claude · sonnet |
| Reviewed by | pr-review-daemon · monit-pr-reviewer |
| Duration | 2m 55s wall · 4m 28s compute (Reviewers: 1m 12s · Synthesis: 1m 28s) |
| Cost | $0.62 |
| Tokens | 272.6k in / 17.2k out |
| Turns | 13 |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost |
|---|---|---|---|---|---|---|
| hybrid-synthesis | sonnet | 38.9k | 4.6k | 13.6k | 25.3k (1h) | $0.17 |
| documentation:docs-reviewer | haiku | 9.3k | 7.1k | 0 | 9.2k (1h) | $0.05 |
| harness-engineering:harness-architecture-reviewer | sonnet | 52.9k | 1.7k | 39.0k | 13.9k (1h) | $0.10 |
| harness-engineering:harness-enforcement-reviewer | sonnet | 70.1k | 1.7k | 56.3k | 13.7k (1h) | $0.10 |
| harness-engineering:harness-knowledge-reviewer | sonnet | 70.1k | 1.6k | 56.4k | 13.8k (1h) | $0.10 |
| discussion-summarizer | — | 31.4k | 487 | 18.9k | 12.5k (1h) | $0.10 |
Re-reviews only run when @monit-reviewer is re-requested as a reviewer — push as many commits as you need, then re-request when ready. PRs targeting branches other than main, master are skipped, even when @monit-reviewer is re-requested.
| 4. Click **Create**. A dialog will show the **Client ID** and **Client Secret** plus a **Download JSON** button. | ||
| 5. Click **Download JSON** and save the file. This is the `credentials.json` you'll distribute. Treat it as access-controlled (you'll store it where only your users can reach it), but note that Google explicitly documents that desktop-app client secrets are **not** confidential — installed-app OAuth clients are designed to be embedded in software users can inspect. What bounds access for your `gro` deployment is the combination of (a) Internal audience (only your Workspace domain can complete consent) and (b) each user's own OAuth consent step. See the [Google OAuth installed-app documentation](https://developers.google.com/identity/protocols/oauth2/native-app). | ||
|
|
||
| ### 6. Verify by running `gro init` yourself |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-enforcement-reviewer): The scope table rows for calendar.events and contacts describe behavioral restrictions ('No event creation/deletion', 'No contact creation/deletion') without the same caveat the gmail.modify row carries — that the restriction is code-enforced, not scope-enforced. Both scopes grant full write access at the API level; compliance reviewers reading the table alone may conclude the scope itself prevents destructive operations. Add a note consistent with the gmail.modify row (e.g. 'Behavioral restriction enforced by code, not by scope') for both rows.
Reply to this thread when addressed.
There was a problem hiding this comment.
Not addressing — the consistency you're asking for is already provided by the paragraph immediately below the scope table, which explicitly states that 'the non-destructive guarantee comes from the code, not the scope' and details the structural CI guardrails plus absent-command-paths argument. Adding the same caveat inline to two more table rows would be redundant. Low-severity nit, declining.
| Two of these scope descriptions overstate what `gro` does — `gmail.modify` includes "send" and `contacts` includes "permanently delete." These are restrictions Google offers as separate sub-scopes only for sensitive but not restricted scopes, so `gro` has to request the broader scope to get the parts it does use. The non-destructive guarantee comes from the code, not the scope: | ||
|
|
||
| - **Structural CI guardrails**: `internal/architecture/architecture_test.go` runs at every CI build and fails if any source file in the repo contains one of an explicit list of destructive Google API method patterns (`.Send(`, `.Untrash(`, `.BatchDelete(`, and others). This is a guardrail that catches the named patterns; it is not a proof that every conceivable destructive call is impossible. | ||
| - **No destructive command paths**: `gro`'s top-level commands (under `internal/cmd/`) do not expose `send`, `delete`, `trash`, or equivalent operations. Compliance reviewers can audit `internal/cmd/` directly to confirm what the binary surfaces to users. |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-enforcement-reviewer): The client-secret rotation guidance states 'Active user tokens issued before rotation continue to work until they're revoked or expire.' This is accurate for short-lived access tokens but misleading for refresh tokens — Google invalidates refresh tokens when a client secret is rotated, requiring users to re-authorize. Admins rotating a compromised secret will expect continuity but instead face org-wide re-auth. Clarify that refresh tokens are invalidated on secret rotation.
Reply to this thread when addressed.
There was a problem hiding this comment.
Addressed in c8db95f — softened to operational guidance: 'plan for some user re-authorization... test with one user before rotating broadly.' I couldn't find a definitive Google statement on refresh-token invalidation behavior for installed-app clients on secret rotation, so the doc now hedges and tells admins to validate with one user first rather than asserting either way.
Daemon reviewer noted (correctly, with the caveat that Google's documented behavior is unclear) that secret rotation may invalidate refresh tokens. Replace 'tokens continue to work' with practical operational guidance: plan for re-auth, test with one user, distribute the new credentials before rotating.
Summary
WORKSPACE_ADMINS.mdat repo root: ~10-minute walkthrough for Google Workspace admins to set up an Internal OAuth app forgroand distributecredentials.jsonto org users via 1Password (or similar). Internal audience skips Google's app verification (no CASA, no 100-user cap, no "unverified app" warning), which is what makes this practical for orgs usinggro's restricted scopes (gmail.modify,drive.readonly).## Setupplus light copy tweak to the init-flow feature bullet so the admin-provided path is discoverable.gro init.Scope handling
The doc addresses head-on the gap between Google's consent-screen wording (e.g.
gmail.modifyreads as "Read, compose, and send") and whatgroactually does. The non-destructive promise is backed by:internal/auth/auth.go:30,AllScopes)internal/architecture/architecture_test.go:341–383fails the build if.Send(,.Untrash(,.BatchDelete(etc. appear)internal/cmd/Doc states the test is a guardrail covering named patterns, not a proof of impossibility — avoids overclaiming for compliance reviewers.
Test plan
make checkpasses (docs-only change; was green locally)WORKSPACE_ADMINS.mdmatch the literal URLs asserted byinternal/auth/auth_test.goREADME.md,docs/architecture.md,docs/golden-principles.mdsignalft.com)Closes #121