chore: explicitly opt-in to allow all users#502
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds an explicit Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthCallback
participant AccessControl
participant Env (Vercel/Cloudflare)
Client->>AuthCallback: User signs in (OAuth)
AuthCallback->>Env: Read UNSAFE_ALLOW_ALL_USERS (env var)
AuthCallback->>AccessControl: checkAccessAllowed({allowedUsers, allowedDomains, unsafeAllowAllUsers})
AccessControl-->>AuthCallback: allow / deny
AuthCallback-->>Client: Complete sign-in or reject
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@terraform/environments/production/checks.tf`:
- Around line 15-24: The Terraform check "access_controls_configured" can be
bypassed by strings that contain only commas/whitespace; update the assertion to
mirror the app parsing in packages/web/src/lib/access-control.ts
(parseAllowlist/checkAccessAllowed) by treating var.allowed_users and
var.allowed_email_domains as comma-separated lists: split on ",", trim each
element, filter out empty entries, and require at least one non-empty entry (or
unsafe_allow_all_users true). Concretely, replace the current
length(trimspace(...)) checks with logic that counts non-empty items after
splitting and trimming each element so the Terraform check fails when the
runtime would produce an empty allowlist.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c41d64e-c5da-480a-ae61-683bc3ae9e16
📒 Files selected for processing (10)
docs/GETTING_STARTED.mdpackages/web/README.mdpackages/web/src/lib/access-control.test.tspackages/web/src/lib/access-control.tspackages/web/src/lib/auth.tsterraform/environments/production/checks.tfterraform/environments/production/terraform.tfvars.exampleterraform/environments/production/variables.tfterraform/environments/production/web-cloudflare.tfterraform/environments/production/web-vercel.tf
Replace the advisory `check` block with a `terraform_data` precondition that actually blocks `terraform plan`/`apply` when no access control is configured. Add missing test for unsafeAllowAllUsers with populated allowlists. Follow-up to #502.
## Summary Follow-up to #502 — strengthens the access control guardrails: - **Replace advisory `check` block with `terraform_data` precondition** — Terraform `check` blocks only produce warnings and don't block `apply`. The new `precondition` on a `terraform_data` resource makes this a hard error that blocks `terraform plan`/`apply` when neither allowlist nor `unsafe_allow_all_users` is configured. - **Add missing test for `unsafeAllowAllUsers: true` with populated allowlists** — verifies that when both the flag and allowlists are set, the allowlist is still enforced (non-matching users are denied). - **Combine duplicated destructuring** in `checkAccessAllowed`. ## Test plan - [x] `npm test -w @open-inspect/web` — 27 tests pass (2 new) - [x] `terraform fmt -check` — clean - [ ] Verify `terraform plan` fails when both allowlists are empty and `unsafe_allow_all_users = false` - [ ] Verify `terraform plan` succeeds when `unsafe_allow_all_users = true` or an allowlist is set <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Expanded test coverage for access control scenarios, including allowlist enforcement behavior with various configuration combinations. * **Refactor** * Optimized access control implementation code structure for improved maintainability. * **Chores** * Updated production infrastructure access control validation to be enforced at deployment-time via infrastructure preconditions. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary by CodeRabbit
New Features
Behavior Change
Deployment / Validation
Documentation