Add collaborators and demo users to allowlist

[danielnaab]

No description provided.

[changeset-bot]

⚠️ No Changeset found

Latest commit: e0450df

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[claude]

Code Review: Add collaborators and demo users to allowlist

Summary

This PR updates the sandbox authentication allowlist from a simple .gov domain check to an explicit allowlist of authorized users. The changes are well-structured and follow the existing pattern used in server-doj.

---

✅ Strengths

1. Security improvement: Moving from a broad domain-based check (.gov) to an explicit allowlist provides better access control
2. Clear documentation: Email addresses are organized by organization with helpful comments
3. Consistent pattern: Follows the same allowlist approach used in apps/server-doj/src/server.ts:16-36
4. Case-insensitive matching: Proper use of .toLowerCase() prevents case-sensitivity issues

---

🔍 Areas for Consideration

1. Security: Hardcoded Credentials

Severity: Medium

The allowlist is hardcoded in the source file, which means:

• Email addresses are visible in version control history
• Changes require code deployment
• No audit trail for allowlist modifications

Recommendation: Consider moving the allowlist to:

• Environment variables (e.g., AUTHORIZED_EMAILS as a comma-separated list)
• A database table with admin UI for management
• A configuration file that's gitignored or encrypted

Example approach:

isUserAuthorized: async (email: string) => {
  const flexionDomain = email.endsWith('@flexion.us');
  const envAllowlist = process.env.AUTHORIZED_EMAILS?.split(',').map(e => e.trim().toLowerCase()) || [];
  return flexionDomain || envAllowlist.includes(email.toLowerCase());
}

2. Inconsistent Authorization Logic

Severity: Low

The sandbox now uses a hybrid approach (domain + allowlist), while server-doj uses only an allowlist. Consider:

• Should .gov addresses still have access to sandbox?
• Is the Flexion domain check (@flexion.us) sufficient, or should specific Flexion employees be listed?

Location: apps/sandbox/src/server.ts:15-33

3. Missing Test Coverage

Severity: Medium

The isUserAuthorized function is not tested. The existing test at apps/sandbox/tests/integration.test.ts:9-15 only verifies the home page renders.

Recommendation: Add tests for authorization logic:

describe('isUserAuthorized', () => {
  test('allows Flexion email addresses', async () => {
    const db = await createInMemoryDatabaseContext();
    const server = await createCustomServer(db);
    // Test authorization logic
  });
  
  test('allows allowlisted email addresses', async () => {
    // Test specific allowlist entries
  });
  
  test('rejects unauthorized email addresses', async () => {
    // Test rejection
  });
  
  test('handles case-insensitive emails', async () => {
    // Test case handling
  });
});

4. Code Quality: Magic Strings

Severity: Low

Consider extracting the allowlist to a constant for better maintainability:

const AUTHORIZED_DOMAINS = ['@flexion.us'];
const AUTHORIZED_EMAILS = [
  // Digital Public Ventures
  'jim@digitalpublic.ventures',
  'mike@digitalpublic.ventures',
  // Maryland Digital Service
  'syed.azeem@maryland.gov',
  'lauren.george@maryland.gov',
  'emil.leong@maryland.gov',
  'paul.roberts@maryland.gov',
] as const;

// In createCustomServer:
isUserAuthorized: async (email: string) => {
  const emailLower = email.toLowerCase();
  if (AUTHORIZED_DOMAINS.some(domain => emailLower.endsWith(domain))) {
    return true;
  }
  return AUTHORIZED_EMAILS.includes(emailLower);
}

5. Documentation

Severity: Low

Consider adding a comment explaining why the old .gov check was removed and the criteria for adding users to the allowlist.

---

🎯 Recommendations

Must address before merge:

• Add test coverage for the authorization logic

Should consider:

• Document the allowlist management process (who can add users, approval process)
• Evaluate if hardcoded emails are acceptable for this environment or if externalized config is needed
• Align authorization strategy between sandbox and server-doj (both domain-based, both allowlist, or hybrid)

Nice to have:

• Extract allowlist to constants for better maintainability
• Add JSDoc comment explaining the authorization logic

---

📝 Overall Assessment

This is a functional change that improves security by moving from broad domain-based access to explicit authorization. The code quality is good and follows existing patterns. The main concerns are around test coverage and the long-term maintainability of hardcoded allowlists.

Status: ✅ Approve with recommendations for test coverage

🤖 This review was generated using Claude Code