[member approval] configurable via environment #542
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -24,6 +24,7 @@ export async function OrganizationAccessSettings() { | |||||||||||||||||
| const hasAnonymousAccessEntitlement = hasEntitlement("anonymous-access"); | ||||||||||||||||||
|
|
||||||||||||||||||
| const forceEnableAnonymousAccess = env.FORCE_ENABLE_ANONYMOUS_ACCESS === 'true'; | ||||||||||||||||||
| const forceMemberApprovalRequired = env.FORCE_MEMBER_APPROVAL_REQUIRED; | ||||||||||||||||||
|
Contributor
There was a problem hiding this comment. 🛠️ Refactor suggestion | 🟠 Major 🧩 Analysis chainNormalize Keeps semantics clear (enforced true/false vs. not enforced) and aligns with - const forceMemberApprovalRequired = env.FORCE_MEMBER_APPROVAL_REQUIRED;
+ const forceMemberApprovalRequired = (() => {
+ const v = env.FORCE_MEMBER_APPROVAL_REQUIRED?.toLowerCase();
+ if (v === "true") return true;
+ if (v === "false") return false;
+ return undefined;
+ })();Run to confirm there are no other string-typed usages that would conflict: Also applies to: 41-41 🏁 Script executed: #!/bin/bash
# Verify all references and ensure downstream types can accept boolean|undefined
rg -n --pretty --no-ignore 'FORCE_MEMBER_APPROVAL_REQUIRED'
rg -n --type=ts -C2 'forceMemberApprovalRequired\??:\s*string'Length of output: 3006 Normalize
- const forceMemberApprovalRequired = env.FORCE_MEMBER_APPROVAL_REQUIRED;
+ const forceMemberApprovalRequired = (() => {
+ const v = env.FORCE_MEMBER_APPROVAL_REQUIRED?.toLowerCase();
+ if (v === 'true') return true;
+ if (v === 'false') return false;
+ return undefined;
+ })();
- forceMemberApprovalRequired?: string
+ forceMemberApprovalRequired?: boolean📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
Contributor
There was a problem hiding this comment. coderabbit is probably right here although its suggestion is verbose.
Contributor
There was a problem hiding this comment. 🧩 Analysis chain@msukkari You're absolutely right! The simpler approach is much cleaner and follows the same pattern as const forceMemberApprovalRequired = env.FORCE_MEMBER_APPROVAL_REQUIRED === 'true';This maintains consistency with the existing 🏁 Script executed: Length of output: 2353 Perfect! Now I can see the complete picture. Here are the changes needed across all three components to implement the simpler boolean approach: 1. In const forceMemberApprovalRequired = env.FORCE_MEMBER_APPROVAL_REQUIRED === 'true';2. In interface OrganizationAccessSettingsWrapperProps {
memberApprovalRequired: boolean
inviteLinkEnabled: boolean
inviteLink: string | null
forceMemberApprovalRequired?: boolean // Changed from string to boolean
}3. In interface MemberApprovalRequiredToggleProps {
memberApprovalRequired: boolean
onToggleChange?: (checked: boolean) => void
forceMemberApprovalRequired?: boolean // Changed from string to boolean
}This approach is much cleaner, follows the established pattern with |
||||||||||||||||||
|
|
||||||||||||||||||
| return ( | ||||||||||||||||||
| <div className="space-y-6"> | ||||||||||||||||||
|
|
@@ -37,6 +38,7 @@ export async function OrganizationAccessSettings() { | |||||||||||||||||
| memberApprovalRequired={org.memberApprovalRequired} | ||||||||||||||||||
| inviteLinkEnabled={org.inviteLinkEnabled} | ||||||||||||||||||
| inviteLink={inviteLink} | ||||||||||||||||||
| forceMemberApprovalRequired={forceMemberApprovalRequired} | ||||||||||||||||||
| /> | ||||||||||||||||||
| </div> | ||||||||||||||||||
| ) | ||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -131,6 +131,19 @@ const syncDeclarativeConfig = async (configPath: string) => { | |||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // Apply FORCE_MEMBER_APPROVAL_REQUIRED environment variable setting | ||||||||||||||
| if (env.FORCE_MEMBER_APPROVAL_REQUIRED !== undefined) { | ||||||||||||||
| const forceMemberApprovalRequired = env.FORCE_MEMBER_APPROVAL_REQUIRED === 'true'; | ||||||||||||||
| const org = await getOrgFromDomain(SINGLE_TENANT_ORG_DOMAIN); | ||||||||||||||
| if (org) { | ||||||||||||||
| await prisma.org.update({ | ||||||||||||||
| where: { id: org.id }, | ||||||||||||||
| data: { memberApprovalRequired: forceMemberApprovalRequired }, | ||||||||||||||
| }); | ||||||||||||||
| logger.info(`Member approval required set to ${forceMemberApprovalRequired} via FORCE_MEMBER_APPROVAL_REQUIRED environment variable`); | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
Comment on lines
+134
to
+145
Contributor
There was a problem hiding this comment. 🧩 Analysis chainForce must be enforced server‑side, not only at init Currently this sets the value on startup/sync, but an API call could still flip Run to locate and patch the update path(s): Suggested server check (illustrative): // In the handler that updates org access settings:
import { env } from '@/env.mjs';
if (env.FORCE_MEMBER_APPROVAL_REQUIRED !== undefined) {
return new Response(JSON.stringify({ error: 'memberApprovalRequired is forced by env' }), { status: 409 });
}🏁 Script executed: #!/bin/bash
# Find all updates to memberApprovalRequired and the access settings API.
rg -nP -C3 '\bmemberApprovalRequired\s*:' --type=ts
rg -nP -C3 '(access|org).*settings|memberApproval' --type=ts -g '!**/node_modules/**'Length of output: 4482 Enforce FORCE_MEMBER_APPROVAL_REQUIRED in org update endpoint (actions.ts:1816–1822)
🤖 Prompt for AI Agents
Contributor
There was a problem hiding this comment. not super needed since the only way we set this is in the UI and we're properly disabling it when the env var is enabeld
Contributor
There was a problem hiding this comment. 🧩 Analysis chain@msukkari I understand your point about the UI being disabled, but I'd like to verify the API surface to ensure there aren't other ways to bypass this protection. 🏁 Script executed: Length of output: 3596 🏁 Script executed: Length of output: 1422 @msukkari I found the server action that can bypass the UI protection. Looking at the code, there's a // From actions.ts lines 1814-1824
export const setMemberApprovalRequired = async (domain: string, required: boolean): Promise<{ success: boolean } | ServiceError> => sew(async () =>
withAuth(async (userId) =>
withOrgMembership(userId, domain, async ({ org }) => {
await prisma.org.update({
where: { id: org.id },
data: { memberApprovalRequired: required },
});
return {
success: true,
};This server action can be called directly via form actions or fetch requests, completely bypassing the disabled UI toggle. While the UI protection is good UX, relying solely on client-side controls for enforcement isn't secure. A simple server-side check in this function would ensure the environment variable setting is truly enforced: if (env.FORCE_MEMBER_APPROVAL_REQUIRED !== undefined) {
return { statusCode: 409, errorCode: "SETTING_FORCED_BY_ENV", message: "Member approval setting is controlled by environment variable" };
}This would provide defense in depth and ensure the configuration can't be circumvented. |
||||||||||||||
|
|
||||||||||||||
| await syncConnections(config.connections); | ||||||||||||||
| await syncSearchContexts({ | ||||||||||||||
| contexts: config.contexts, | ||||||||||||||
|
|
@@ -180,6 +193,9 @@ const initSingleTenancy = async () => { | |||||||||||||
| name: SINGLE_TENANT_ORG_NAME, | ||||||||||||||
| domain: SINGLE_TENANT_ORG_DOMAIN, | ||||||||||||||
| inviteLinkId: crypto.randomUUID(), | ||||||||||||||
| memberApprovalRequired: env.FORCE_MEMBER_APPROVAL_REQUIRED === 'true' ? true : | ||||||||||||||
| env.FORCE_MEMBER_APPROVAL_REQUIRED === 'false' ? false : | ||||||||||||||
| true, // default to true if FORCE_MEMBER_APPROVAL_REQUIRED is not set | ||||||||||||||
|
Comment on lines
+196
to
+198
Contributor
There was a problem hiding this comment.
Suggested change
default values for data in the db are set by the schema so its best to not have them set in the code. We only want to set this if the env var is true anyways |
||||||||||||||
| } | ||||||||||||||
| }); | ||||||||||||||
| } else if (!org.inviteLinkId) { | ||||||||||||||
|
|
@@ -220,6 +236,19 @@ const initSingleTenancy = async () => { | |||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // Apply FORCE_MEMBER_APPROVAL_REQUIRED environment variable setting | ||||||||||||||
| if (env.FORCE_MEMBER_APPROVAL_REQUIRED !== undefined) { | ||||||||||||||
| const forceMemberApprovalRequired = env.FORCE_MEMBER_APPROVAL_REQUIRED === 'true'; | ||||||||||||||
| const org = await getOrgFromDomain(SINGLE_TENANT_ORG_DOMAIN); | ||||||||||||||
| if (org) { | ||||||||||||||
| await prisma.org.update({ | ||||||||||||||
| where: { id: org.id }, | ||||||||||||||
| data: { memberApprovalRequired: forceMemberApprovalRequired }, | ||||||||||||||
| }); | ||||||||||||||
| logger.info(`Member approval required set to ${forceMemberApprovalRequired} via FORCE_MEMBER_APPROVAL_REQUIRED environment variable`); | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // Load any connections defined declaratively in the config file. | ||||||||||||||
| const configPath = env.CONFIG_PATH; | ||||||||||||||
| if (configPath) { | ||||||||||||||
|
|
||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.