fix: make access control terraform check a hard error

[ColeMurray]

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

• npm test -w @open-inspect/web — 27 tests pass (2 new)
• 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

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.

[coderabbitai]

📝 Walkthrough

Walkthrough

The changes add test coverage for access control behavior when unsafeAllowAllUsers is enabled, refactor parameter destructuring in the access-control function, and replace a Terraform check advisory with a hard precondition to enforce access-control configuration.

Changes

Cohort / File(s)	Summary
Access Control Tests packages/web/src/lib/access-control.test.ts	Added test suite for checkAccessAllowed verifying that allowlist enforcement applies when unsafeAllowAllUsers is true; tests confirm username and email domain matching behavior returns expected boolean values.
Access Control Logic packages/web/src/lib/access-control.ts	Refactored parameter destructuring to extract unsafeAllowAllUsers directly from config in a single declaration; control flow and return logic remain unchanged.
Terraform Enforcement terraform/environments/production/checks.tf	Replaced check "access_controls_configured" advisory block with terraform_data.access_control_gate resource using lifecycle.precondition; changes failure mode from warning to hard plan/apply-time error when access-control validation fails.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

• chore: explicitly opt-in to allow all users #502: Touches the same access-control code, tests, and Terraform checks to implement/enforce the unsafe_allow_all_users allowlist behavior.

Suggested reviewers

• open-inspect

Poem

🐰 A refactor hops through three neat files,
Tests now bark where safeguards pile,
Terraform stands guard with preconditions tight,
Access controls enforced—all systems right! ✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: converting a non-blocking Terraform check to a hard error precondition for access control validation.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch chore/access-control-followups

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

Terraform Validation Results

Step	Status
Format	✅
Init	✅
Validate	✅

Note: Terraform plan was skipped because secrets are not configured. This is expected for external contributors. See docs/GETTING_STARTED.md for setup instructions.

Pushed by: @ColeMurray, Action: pull_request

[open-inspect]

Summary

This PR replaces a non-blocking Terraform check with a hard-fail terraform_data precondition, adds coverage for the unsafeAllowAllUsers plus populated allowlist case, and simplifies a small destructuring in checkAccessAllowed. I reviewed the 3 changed files (+31/-10) and did not find any blocking correctness, security, performance, or maintainability issues in the diff.

Critical Issues

None.

Suggestions

• terraform/environments/production/checks.tf:15 - The new guard looks appropriate for turning this into a hard error. The remaining gap is operational verification: it would still be valuable to run the manual terraform plan scenarios from the PR description in an environment with Terraform installed so we confirm the failure/success paths end to end.

Nitpicks

None.

Positive Feedback

• The new access-control tests cover the important edge case where unsafeAllowAllUsers is true but allowlists are still populated, which protects the intended precedence rule.
• Replacing the advisory Terraform validation with a blocking precondition matches the stated goal and makes the safety check materially stronger.
• The code change in checkAccessAllowed stays minimal and easy to follow.

Questions

None.

Verdict

Approve: Ready to merge based on the diff review and local test results. I also ran npm test -w @open-inspect/web, and all 184 tests passed. I could not run the Terraform plan checks locally because the Terraform CLI is not installed in this environment.

[coderabbitai]

🧹 Nitpick comments (1)

terraform/environments/production/checks.tf (1)

20-24: Optional: extract the allowlist-non-empty computation to a local.

The split + trimspace + filter pattern is duplicated for allowed_users and allowed_email_domains, and the same parsing likely exists where these values are consumed downstream. Lifting these into locals (e.g., local.allowed_users_list, local.allowed_email_domains_list) would keep the precondition readable and ensure the gate uses the exact same parsing as the actual consumers — eliminating drift risk if one side changes normalization later.

♻️ Suggested refactor

+locals {
+  allowed_users_list         = [for item in split(",", var.allowed_users) : trimspace(item) if trimspace(item) != ""]
+  allowed_email_domains_list = [for item in split(",", var.allowed_email_domains) : trimspace(item) if trimspace(item) != ""]
+}
+
 resource "terraform_data" "access_control_gate" {
   lifecycle {
     precondition {
       condition = (
         var.unsafe_allow_all_users ||
-        length([for item in split(",", var.allowed_users) : trimspace(item) if trimspace(item) != ""]) > 0 ||
-        length([for item in split(",", var.allowed_email_domains) : trimspace(item) if trimspace(item) != ""]) > 0
+        length(local.allowed_users_list) > 0 ||
+        length(local.allowed_email_domains_list) > 0
       )
       error_message = "At least one access control allowlist must be configured. Set allowed_users or allowed_email_domains, or set unsafe_allow_all_users = true to explicitly allow all authenticated GitHub users."
     }
   }
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@terraform/environments/production/checks.tf` around lines 20 - 24, Extract
the duplicated split/trimspace/filter logic into locals (e.g.,
local.allowed_users_list and local.allowed_email_domains_list) and use those
locals in the existing condition instead of repeating the comprehension;
specifically, create locals that compute the parsed non-empty lists from
var.allowed_users and var.allowed_email_domains, then replace the two
length([...]) expressions in the condition that references
var.unsafe_allow_all_users with checks like length(local.allowed_users_list) > 0
and length(local.allowed_email_domains_list) > 0 so the gate and downstream
consumers share identical normalization.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@terraform/environments/production/checks.tf`:
- Around line 20-24: Extract the duplicated split/trimspace/filter logic into
locals (e.g., local.allowed_users_list and local.allowed_email_domains_list) and
use those locals in the existing condition instead of repeating the
comprehension; specifically, create locals that compute the parsed non-empty
lists from var.allowed_users and var.allowed_email_domains, then replace the two
length([...]) expressions in the condition that references
var.unsafe_allow_all_users with checks like length(local.allowed_users_list) > 0
and length(local.allowed_email_domains_list) > 0 so the gate and downstream
consumers share identical normalization.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f733240c-699a-478a-9c3f-c0339243d63a

📥 Commits

Reviewing files that changed from the base of the PR and between 13c3104 and 3da77d8.

📒 Files selected for processing (3)

• packages/web/src/lib/access-control.test.ts
• packages/web/src/lib/access-control.ts
• terraform/environments/production/checks.tf