Compare commits

...

7 Commits
1.19.3 ... dev

Author SHA1 Message Date
miloschwartz
7506c0420d properly pass org policy error message in olm register 2026-06-26 17:11:32 -04:00
Owen Schwartz
5572822c4a Merge pull request #3351 from fosrl/copilot/fix-rest-ruleid-changing
fix: preserve rule IDs when saving policy rules via the GUI
2026-06-26 15:05:31 -04:00
Owen
ea3f1c341b Move hashing outside of transaction 2026-06-26 14:40:39 -04:00
Owen
35dffe71cb Make error statement debug 2026-06-26 14:40:31 -04:00
copilot-swe-agent[bot]
5428bf4ed0 fix: preserve rule IDs when saving policy rules through the GUI
The `setResourcePolicyRules` endpoint was deleting all existing rules and
re-inserting them on every save, causing all ruleIDs to change.

Backend: Accept an optional `ruleId` per rule in the request body and
perform an upsert — update existing rules (matched by ruleId), insert
new ones (no ruleId), and delete only rules absent from the incoming list.

Frontend: Include `ruleId` in the rules payload for existing (non-new)
rules so the backend can match and preserve them.
2026-06-26 14:37:34 +00:00
copilot-swe-agent[bot]
9a89579e08 Initial plan 2026-06-26 14:33:35 +00:00
Owen
2e628fe0e4 Make sure the rebuild actually executes 2026-06-26 09:26:43 -04:00
7 changed files with 463 additions and 431 deletions

View File

@@ -172,7 +172,9 @@ export async function applyBlueprint({
} catch (err) { } catch (err) {
blueprintSucceeded = false; blueprintSucceeded = false;
blueprintMessage = `Blueprint applied with errors: ${err}`; blueprintMessage = `Blueprint applied with errors: ${err}`;
logger.error(blueprintMessage); logger.debug(
`Org ${orgId} blueprint apply issues: ${blueprintMessage}`
);
error = err; error = err;
} }

View File

@@ -29,7 +29,7 @@ type ClientRow = typeof clients.$inferSelect;
function runQueuedClientAssociationRebuilds( function runQueuedClientAssociationRebuilds(
userId: string, userId: string,
queuedClients: ClientRow[] queuedClients: ClientRow[]
): void { ) {
if (queuedClients.length === 0) { if (queuedClients.length === 0) {
return; return;
} }
@@ -39,35 +39,29 @@ function runQueuedClientAssociationRebuilds(
uniqueClientsById.set(client.clientId, client); uniqueClientsById.set(client.clientId, client);
} }
void (async () => {
for (const client of uniqueClientsById.values()) { for (const client of uniqueClientsById.values()) {
try { rebuildClientAssociationsFromClient(client).catch((error) => {
await rebuildClientAssociationsFromClient(client);
} catch (error) {
logger.error( logger.error(
`Failed rebuilding associations for client ${client.clientId} (user ${userId}): ${String(error)}` `Error rebuilding client associations for client ${client.clientId} (user ${userId}): ${String(
error
)}`
); );
} });
} }
logger.debug( logger.debug(
`Queued association rebuild completed for ${uniqueClientsById.size} client(s) (user ${userId})` `Queued association rebuild completed for ${uniqueClientsById.size} client(s) (user ${userId})`
); );
})();
} }
export async function calculateUserClientsForOrgs( export async function calculateUserClientsForOrgs(
userId: string userId: string
): Promise<void> { ): Promise<void> {
const trx = primaryDb; const trx = primaryDb;
const queuedAssociationRebuilds: ClientRow[] = [];
const execute = async (transaction: Transaction | typeof db) => { const queuedAssociationRebuilds: ClientRow[] = [];
const orgCache = new Map<string, typeof orgs.$inferSelect | null>(); const orgCache = new Map<string, typeof orgs.$inferSelect | null>();
const adminRoleCache = new Map< const adminRoleCache = new Map<string, typeof roles.$inferSelect | null>();
string,
typeof roles.$inferSelect | null
>();
const exitNodesCache = new Map< const exitNodesCache = new Map<
string, string,
Awaited<ReturnType<typeof listExitNodes>> Awaited<ReturnType<typeof listExitNodes>>
@@ -80,8 +74,7 @@ export async function calculateUserClientsForOrgs(
const roleClientAccessCache = new Map<string, boolean>(); const roleClientAccessCache = new Map<string, boolean>();
const userClientAccessCache = new Map<string, boolean>(); const userClientAccessCache = new Map<string, boolean>();
const getOrgOlmKey = (orgId: string, olmId: string) => const getOrgOlmKey = (orgId: string, olmId: string) => `${orgId}:${olmId}`;
`${orgId}:${olmId}`;
const getRoleClientKey = (roleId: number, clientId: number) => const getRoleClientKey = (roleId: number, clientId: number) =>
`${roleId}:${clientId}`; `${roleId}:${clientId}`;
const getUserClientKey = (cachedUserId: string, clientId: number) => const getUserClientKey = (cachedUserId: string, clientId: number) =>
@@ -92,7 +85,7 @@ export async function calculateUserClientsForOrgs(
return orgCache.get(orgId) ?? null; return orgCache.get(orgId) ?? null;
} }
const [org] = await transaction const [org] = await trx
.select() .select()
.from(orgs) .from(orgs)
.where(eq(orgs.orgId, orgId)); .where(eq(orgs.orgId, orgId));
@@ -106,7 +99,7 @@ export async function calculateUserClientsForOrgs(
return adminRoleCache.get(orgId) ?? null; return adminRoleCache.get(orgId) ?? null;
} }
const [adminRole] = await transaction const [adminRole] = await trx
.select() .select()
.from(roles) .from(roles)
.where(and(eq(roles.isAdmin, true), eq(roles.orgId, orgId))) .where(and(eq(roles.isAdmin, true), eq(roles.orgId, orgId)))
@@ -147,7 +140,7 @@ export async function calculateUserClientsForOrgs(
return existingClientCache.get(key) ?? null; return existingClientCache.get(key) ?? null;
} }
const [existingClient] = await transaction const [existingClient] = await trx
.select() .select()
.from(clients) .from(clients)
.where( .where(
@@ -164,16 +157,13 @@ export async function calculateUserClientsForOrgs(
return existingClient ?? null; return existingClient ?? null;
}; };
const hasRoleClientAccess = async ( const hasRoleClientAccess = async (roleId: number, clientId: number) => {
roleId: number,
clientId: number
) => {
const key = getRoleClientKey(roleId, clientId); const key = getRoleClientKey(roleId, clientId);
if (roleClientAccessCache.has(key)) { if (roleClientAccessCache.has(key)) {
return roleClientAccessCache.get(key)!; return roleClientAccessCache.get(key)!;
} }
const [existingRoleClient] = await transaction const [existingRoleClient] = await trx
.select() .select()
.from(roleClients) .from(roleClients)
.where( .where(
@@ -199,7 +189,7 @@ export async function calculateUserClientsForOrgs(
return userClientAccessCache.get(key)!; return userClientAccessCache.get(key)!;
} }
const [existingUserClient] = await transaction const [existingUserClient] = await trx
.select() .select()
.from(userClients) .from(userClients)
.where( .where(
@@ -217,7 +207,7 @@ export async function calculateUserClientsForOrgs(
}; };
// Get all OLMs for this user // Get all OLMs for this user
const userOlms = await transaction const userOlms = await trx
.select() .select()
.from(olms) .from(olms)
.where(eq(olms.userId, userId)); .where(eq(olms.userId, userId));
@@ -226,7 +216,7 @@ export async function calculateUserClientsForOrgs(
// No OLMs for this user, but we should still clean up any orphaned clients // No OLMs for this user, but we should still clean up any orphaned clients
await cleanupOrphanedClients( await cleanupOrphanedClients(
userId, userId,
transaction, trx,
[], [],
queuedAssociationRebuilds queuedAssociationRebuilds
); );
@@ -234,7 +224,7 @@ export async function calculateUserClientsForOrgs(
} }
// Get all user orgs with all roles (for org list and role-based logic) // Get all user orgs with all roles (for org list and role-based logic)
const userOrgRoleRows = await transaction const userOrgRoleRows = await trx
.select() .select()
.from(userOrgs) .from(userOrgs)
.innerJoin( .innerJoin(
@@ -250,10 +240,7 @@ export async function calculateUserClientsForOrgs(
const userOrgIds = [ const userOrgIds = [
...new Set(userOrgRoleRows.map((r) => r.userOrgs.orgId)) ...new Set(userOrgRoleRows.map((r) => r.userOrgs.orgId))
]; ];
const orgIdToRoleRows = new Map< const orgIdToRoleRows = new Map<string, (typeof userOrgRoleRows)[0][]>();
string,
(typeof userOrgRoleRows)[0][]
>();
for (const r of userOrgRoleRows) { for (const r of userOrgRoleRows) {
const list = orgIdToRoleRows.get(r.userOrgs.orgId) ?? []; const list = orgIdToRoleRows.get(r.userOrgs.orgId) ?? [];
list.push(r); list.push(r);
@@ -300,10 +287,7 @@ export async function calculateUserClientsForOrgs(
} }
// Check if a client already exists for this OLM+user+org combination // Check if a client already exists for this OLM+user+org combination
const existingClient = await getExistingClient( const existingClient = await getExistingClient(orgId, olm.olmId);
orgId,
olm.olmId
);
if (existingClient) { if (existingClient) {
// Ensure admin role has access to the client // Ensure admin role has access to the client
@@ -313,7 +297,7 @@ export async function calculateUserClientsForOrgs(
); );
if (!hasRoleAccess) { if (!hasRoleAccess) {
await transaction.insert(roleClients).values({ await trx.insert(roleClients).values({
roleId: adminRole.roleId, roleId: adminRole.roleId,
clientId: existingClient.clientId clientId: existingClient.clientId
}); });
@@ -336,7 +320,7 @@ export async function calculateUserClientsForOrgs(
); );
if (!hasUserAccess) { if (!hasUserAccess) {
await transaction.insert(userClients).values({ await trx.insert(userClients).values({
userId, userId,
clientId: existingClient.clientId clientId: existingClient.clientId
}); });
@@ -366,13 +350,11 @@ export async function calculateUserClientsForOrgs(
} }
const randomExitNode = const randomExitNode =
exitNodesList[ exitNodesList[Math.floor(Math.random() * exitNodesList.length)];
Math.floor(Math.random() * exitNodesList.length)
];
// Get next available subnet // Get next available subnet
const { value: newSubnet, release: releaseSubnetLock } = const { value: newSubnet, release: releaseSubnetLock } =
await getNextAvailableClientSubnet(orgId, transaction); await getNextAvailableClientSubnet(orgId, trx);
const subnet = newSubnet.split("/")[0]; const subnet = newSubnet.split("/")[0];
const updatedSubnet = `${subnet}/${org.subnet.split("/")[1]}`; const updatedSubnet = `${subnet}/${org.subnet.split("/")[1]}`;
@@ -398,19 +380,16 @@ export async function calculateUserClientsForOrgs(
}; };
// Create the client // Create the client
const [newClient] = await transaction const [newClient] = await trx
.insert(clients) .insert(clients)
.values(newClientData) .values(newClientData)
.returning(); .returning();
await releaseSubnetLock(); await releaseSubnetLock();
existingClientCache.set( existingClientCache.set(getOrgOlmKey(orgId, olm.olmId), newClient);
getOrgOlmKey(orgId, olm.olmId),
newClient
);
// create approval request // create approval request
if (requireApproval) { if (requireApproval) {
await transaction await trx
.insert(approvals) .insert(approvals)
.values({ .values({
timestamp: Math.floor(new Date().getTime() / 1000), timestamp: Math.floor(new Date().getTime() / 1000),
@@ -425,7 +404,7 @@ export async function calculateUserClientsForOrgs(
queuedAssociationRebuilds.push(newClient); queuedAssociationRebuilds.push(newClient);
// Grant admin role access to the client // Grant admin role access to the client
await transaction.insert(roleClients).values({ await trx.insert(roleClients).values({
roleId: adminRole.roleId, roleId: adminRole.roleId,
clientId: newClient.clientId clientId: newClient.clientId
}); });
@@ -435,7 +414,7 @@ export async function calculateUserClientsForOrgs(
); );
// Grant user access to the client // Grant user access to the client
await transaction.insert(userClients).values({ await trx.insert(userClients).values({
userId, userId,
clientId: newClient.clientId clientId: newClient.clientId
}); });
@@ -453,11 +432,10 @@ export async function calculateUserClientsForOrgs(
// Clean up clients in orgs the user is no longer in // Clean up clients in orgs the user is no longer in
await cleanupOrphanedClients( await cleanupOrphanedClients(
userId, userId,
transaction, trx,
userOrgIds, userOrgIds,
queuedAssociationRebuilds queuedAssociationRebuilds
); );
};
runQueuedClientAssociationRebuilds(userId, queuedAssociationRebuilds); runQueuedClientAssociationRebuilds(userId, queuedAssociationRebuilds);
} }
@@ -496,7 +474,7 @@ async function cleanupOrphanedClients(
) )
.returning(); .returning();
// Queue deleted clients for post-transaction association cleanup. // Queue deleted clients for post-trx association cleanup.
for (const deletedClient of deletedClients) { for (const deletedClient of deletedClients) {
queuedAssociationRebuilds.push(deletedClient); queuedAssociationRebuilds.push(deletedClient);

View File

@@ -197,15 +197,6 @@ export const handleOlmRegisterMessage: MessageHandler = async (context) => {
policyCheck policyCheck
}); });
if (policyCheck?.error) {
logger.error(
`[handleOlmRegisterMessage] Error checking access policies for olm user ${olm.userId} in org ${orgId}: ${policyCheck?.error}`,
{ orgId: client.orgId, clientId: client.clientId }
);
sendOlmError(OlmErrorCodes.ORG_ACCESS_POLICY_DENIED, olm.olmId);
return;
}
if (policyCheck.policies?.passwordAge?.compliant === false) { if (policyCheck.policies?.passwordAge?.compliant === false) {
logger.warn( logger.warn(
`[handleOlmRegisterMessage] Olm user ${olm.userId} has non-compliant password age for org ${orgId}`, `[handleOlmRegisterMessage] Olm user ${olm.userId} has non-compliant password age for org ${orgId}`,
@@ -238,7 +229,7 @@ export const handleOlmRegisterMessage: MessageHandler = async (context) => {
olm.olmId olm.olmId
); );
return; return;
} else if (!policyCheck.allowed) { } else if (!policyCheck.allowed || policyCheck.error) {
logger.warn( logger.warn(
`[handleOlmRegisterMessage] Olm user ${olm.userId} does not pass access policies for org ${orgId}: ${policyCheck.error}`, `[handleOlmRegisterMessage] Olm user ${olm.userId} does not pass access policies for org ${orgId}: ${policyCheck.error}`,
{ orgId: client.orgId, clientId: client.clientId } { orgId: client.orgId, clientId: client.clientId }

View File

@@ -76,6 +76,15 @@ export async function setResourcePolicyHeaderAuth(
const { resourcePolicyId } = parsedParams.data; const { resourcePolicyId } = parsedParams.data;
const { headerAuth } = parsedBody.data; const { headerAuth } = parsedBody.data;
const headerAuthHash =
headerAuth !== null
? await hashPassword(
Buffer.from(
`${headerAuth.user}:${headerAuth.password}`
).toString("base64")
)
: null;
await db.transaction(async (trx) => { await db.transaction(async (trx) => {
await trx await trx
.delete(resourcePolicyHeaderAuth) .delete(resourcePolicyHeaderAuth)
@@ -86,13 +95,7 @@ export async function setResourcePolicyHeaderAuth(
) )
); );
if (headerAuth !== null) { if (headerAuth !== null && headerAuthHash !== null) {
const headerAuthHash = await hashPassword(
Buffer.from(
`${headerAuth.user}:${headerAuth.password}`
).toString("base64")
);
await trx.insert(resourcePolicyHeaderAuth).values({ await trx.insert(resourcePolicyHeaderAuth).values({
resourcePolicyId, resourcePolicyId,
headerAuthHash, headerAuthHash,

View File

@@ -1,7 +1,7 @@
import { Request, Response, NextFunction } from "express"; import { Request, Response, NextFunction } from "express";
import { z } from "zod"; import { z } from "zod";
import { db, resourcePolicyRules, resourcePolicies } from "@server/db"; import { db, resourcePolicyRules, resourcePolicies } from "@server/db";
import { eq } from "drizzle-orm"; import { and, eq, notInArray } from "drizzle-orm";
import response from "@server/lib/response"; import response from "@server/lib/response";
import HttpCode from "@server/types/HttpCode"; import HttpCode from "@server/types/HttpCode";
import createHttpError from "http-errors"; import createHttpError from "http-errors";
@@ -14,6 +14,7 @@ import {
import { OpenAPITags, registry } from "@server/openApi"; import { OpenAPITags, registry } from "@server/openApi";
const ruleSchema = z.strictObject({ const ruleSchema = z.strictObject({
ruleId: z.int().positive().optional(),
action: z.enum(["ACCEPT", "DROP", "PASS"]).openapi({ action: z.enum(["ACCEPT", "DROP", "PASS"]).openapi({
type: "string", type: "string",
enum: ["ACCEPT", "DROP", "PASS"], enum: ["ACCEPT", "DROP", "PASS"],
@@ -121,17 +122,74 @@ export async function setResourcePolicyRules(
.set({ applyRules }) .set({ applyRules })
.where(eq(resourcePolicies.resourcePolicyId, resourcePolicyId)); .where(eq(resourcePolicies.resourcePolicyId, resourcePolicyId));
const incomingRuleIds = rules
.map((r) => r.ruleId)
.filter((id): id is number => id !== undefined);
// Delete rules that are no longer in the incoming list
if (incomingRuleIds.length > 0) {
await trx await trx
.delete(resourcePolicyRules) .delete(resourcePolicyRules)
.where( .where(
eq(resourcePolicyRules.resourcePolicyId, resourcePolicyId) and(
eq(
resourcePolicyRules.resourcePolicyId,
resourcePolicyId
),
notInArray(
resourcePolicyRules.ruleId,
incomingRuleIds
)
)
); );
} else {
await trx
.delete(resourcePolicyRules)
.where(
eq(
resourcePolicyRules.resourcePolicyId,
resourcePolicyId
)
);
}
if (rules.length > 0) { // Update existing rules (those with a ruleId)
const existingRules = rules.filter(
(r): r is typeof r & { ruleId: number } =>
r.ruleId !== undefined
);
for (const rule of existingRules) {
await trx
.update(resourcePolicyRules)
.set({
action: rule.action,
match: rule.match,
value: rule.value,
priority: rule.priority,
enabled: rule.enabled
})
.where(
and(
eq(resourcePolicyRules.ruleId, rule.ruleId),
eq(
resourcePolicyRules.resourcePolicyId,
resourcePolicyId
)
)
);
}
// Insert new rules (those without a ruleId)
const newRules = rules.filter((r) => r.ruleId === undefined);
if (newRules.length > 0) {
await trx.insert(resourcePolicyRules).values( await trx.insert(resourcePolicyRules).values(
rules.map((rule) => ({ newRules.map((rule) => ({
resourcePolicyId, resourcePolicyId,
...rule action: rule.action,
match: rule.match,
value: rule.value,
priority: rule.priority,
enabled: rule.enabled
})) }))
); );
} }

View File

@@ -107,6 +107,13 @@ export async function setResourceHeaderAuth(
resource.resourcePolicyId === null && resource.resourcePolicyId === null &&
resource.defaultResourcePolicyId !== null; resource.defaultResourcePolicyId !== null;
const headerAuthHash =
user && password && extendedCompatibility !== null
? await hashPassword(
Buffer.from(`${user}:${password}`).toString("base64")
)
: null;
await db.transaction(async (trx) => { await db.transaction(async (trx) => {
if (isInlinePolicy) { if (isInlinePolicy) {
const policyId = resource.defaultResourcePolicyId!; const policyId = resource.defaultResourcePolicyId!;
@@ -116,11 +123,7 @@ export async function setResourceHeaderAuth(
eq(resourcePolicyHeaderAuth.resourcePolicyId, policyId) eq(resourcePolicyHeaderAuth.resourcePolicyId, policyId)
); );
if (user && password && extendedCompatibility !== null) { if (headerAuthHash !== null && extendedCompatibility !== null) {
const headerAuthHash = await hashPassword(
Buffer.from(`${user}:${password}`).toString("base64")
);
await trx.insert(resourcePolicyHeaderAuth).values({ await trx.insert(resourcePolicyHeaderAuth).values({
resourcePolicyId: policyId, resourcePolicyId: policyId,
headerAuthHash, headerAuthHash,
@@ -140,11 +143,7 @@ export async function setResourceHeaderAuth(
) )
); );
if (user && password && extendedCompatibility !== null) { if (headerAuthHash !== null && extendedCompatibility !== null) {
const headerAuthHash = await hashPassword(
Buffer.from(`${user}:${password}`).toString("base64")
);
await Promise.all([ await Promise.all([
trx trx
.insert(resourceHeaderAuth) .insert(resourceHeaderAuth)

View File

@@ -340,7 +340,8 @@ function PolicyAccessRulesSectionEdit({
? rules.filter((rule) => !rule.fromPolicy) ? rules.filter((rule) => !rule.fromPolicy)
: rules; : rules;
const rulesPayload = rulesToValidate.map( const rulesPayload = rulesToValidate.map(
({ action, match, value, priority, enabled }) => ({ ({ ruleId, action, match, value, priority, enabled, new: isNew }) => ({
...(isNew ? {} : { ruleId }),
action, action,
match, match,
value, value,