Skip to content
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,93 @@ describe('OAuthController', () => {
expect(result.authorizationUrl).toContain('code_challenge=');
expect(result.authorizationUrl).toContain('code_challenge_method=S256');
});

it('should return a GitHub App install URL (state only) for appInstallFlow providers', async () => {
const manifest = {
id: 'github-app',
name: 'GitHub App',
auth: {
type: 'oauth2',
config: {
authorizeUrl:
'https://github.com/apps/{APP_SLUG}/installations/new',
tokenUrl: 'https://github.com/login/oauth/access_token',
pkce: false,
appInstallFlow: true,
additionalOAuthSettings: [{ id: 'appSlug', token: '{APP_SLUG}' }],
},
},
category: 'Development',
capabilities: [],
isActive: true,
};
mockedGetManifest.mockReturnValue(manifest as never);
mockOAuthCredentialsService.getCredentials.mockResolvedValue({
clientId: 'client_123',
clientSecret: 'secret_456',
scopes: [],
source: 'platform',
customSettings: { appSlug: 'comp-ai' },
});
mockProviderRepository.upsert.mockResolvedValue(undefined);
mockOAuthStateRepository.create.mockResolvedValue({
state: 'state_install',
});

const result = await controller.startOAuth('org_1', 'user_1', {
providerSlug: 'github-app',
});

// Install URL with the slug substituted and only `state` appended.
expect(result.authorizationUrl).toContain(
'https://github.com/apps/comp-ai/installations/new',
);
expect(result.authorizationUrl).toContain('state=state_install');
// GitHub ignores redirect_uri on the install URL and routes to the App's
// first registered callback, so we deliberately do NOT set it here.
expect(result.authorizationUrl).not.toContain('redirect_uri=');
// OAuth authorize params must NOT be on an install URL.
expect(result.authorizationUrl).not.toContain('client_id=');
expect(result.authorizationUrl).not.toContain('response_type=');
expect(result.authorizationUrl).not.toContain('scope=');
});

it('should throw PRECONDITION_FAILED for an install-flow provider when the app slug is not configured', async () => {
const manifest = {
id: 'github-app',
name: 'GitHub App',
auth: {
type: 'oauth2',
config: {
authorizeUrl:
'https://github.com/apps/{APP_SLUG}/installations/new',
tokenUrl: 'https://github.com/login/oauth/access_token',
pkce: false,
appInstallFlow: true,
additionalOAuthSettings: [{ id: 'appSlug', token: '{APP_SLUG}' }],
},
},
category: 'Development',
capabilities: [],
isActive: true,
};
mockedGetManifest.mockReturnValue(manifest as never);
// No customSettings.appSlug → {APP_SLUG} cannot be resolved.
mockOAuthCredentialsService.getCredentials.mockResolvedValue({
clientId: 'client_123',
clientSecret: 'secret_456',
scopes: [],
source: 'organization',
});
mockProviderRepository.upsert.mockResolvedValue(undefined);
mockOAuthStateRepository.create.mockResolvedValue({ state: 's' });

await expect(
controller.startOAuth('org_1', 'user_1', {
providerSlug: 'github-app',
}),
).rejects.toThrow(HttpException);
});
});

describe('oauthCallback', () => {
Expand Down
66 changes: 59 additions & 7 deletions apps/api/src/integration-platform/controllers/oauth.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,44 @@ export class OAuthController {
}
const authUrl = new URL(authorizeUrl);

// GitHub App installation flow: the connect step is an App *installation*
// (github.com/apps/{slug}/installations/new), not a standard OAuth authorize.
// GitHub redirects to the App's FIRST registered callback URL after install
// and IGNORES any redirect_uri passed to the install URL (confirmed GitHub
// behavior), so only `state` is set here — client_id, response_type and
// scope do not apply either. Per-environment routing is therefore handled by
// using a separate GitHub App per environment (each with its own single
// callback URL), NOT via redirect_uri. The OAuth `code` still comes back on
// that callback (with "Request user authorization during installation"
// enabled), so token exchange proceeds normally afterwards.
if (oauthConfig.appInstallFlow) {
// Every placeholder in the install URL (e.g. {APP_SLUG}) must have been
// resolved from credentials above. If a required setting like the GitHub
// App slug was never persisted (e.g. org-scoped credentials saved without
// customSettings), fail loudly with a clear error instead of redirecting
// the user to a broken GitHub URL.
const unresolvedTokens = (oauthConfig.additionalOAuthSettings ?? [])
.map((setting) => setting.token)
.filter(
(token): token is string => !!token && authorizeUrl.includes(token),
);
if (unresolvedTokens.length > 0) {
throw new HttpException(
{
message: `GitHub App is not fully configured for ${providerSlug} (missing: ${unresolvedTokens.join(', ')}). Set the App slug in the integration credentials.`,
setupInstructions: oauthConfig.setupInstructions,
createAppUrl: oauthConfig.createAppUrl,
},
HttpStatus.PRECONDITION_FAILED,
);
}
authUrl.searchParams.set('state', oauthState.state);
this.logger.log(
`Starting GitHub App install flow for ${providerSlug}, org: ${organizationId} (credentials from ${credentials.source})`,
);
return { authorizationUrl: authUrl.toString() };
}

// Standard OAuth2 params
authUrl.searchParams.set('client_id', credentials.clientId);
authUrl.searchParams.set('response_type', 'code');
Expand Down Expand Up @@ -287,7 +325,10 @@ export class OAuthController {
// is the one completing it. The `state` token alone provides CSRF
// protection, but a session match guards against state-token leakage and
// ensures the completing user still has an active session for the org.
const sessionMismatch = await this.checkSessionMatchesState(req, oauthState);
const sessionMismatch = await this.checkSessionMatchesState(
req,
oauthState,
);
if (sessionMismatch) {
await this.oauthStateRepository.delete(state);
this.logger.warn(
Expand Down Expand Up @@ -355,11 +396,15 @@ export class OAuthController {
}

// Store tokens and mark connection as active
await this.credentialVaultService.storeOAuthTokens(connection.id, tokens, {
preserveExistingRefreshToken:
oauthState.providerSlug === 'gcp' ||
oauthState.providerSlug === 'google-workspace',
});
await this.credentialVaultService.storeOAuthTokens(
connection.id,
tokens,
{
preserveExistingRefreshToken:
oauthState.providerSlug === 'gcp' ||
oauthState.providerSlug === 'google-workspace',
},
);

// Mark cloud OAuth reconnect completion so reconnect banners clear after successful OAuth.
if (manifest.category === 'Cloud') {
Expand All @@ -377,6 +422,14 @@ export class OAuthController {
});
}

// NOTE: GitHub's App install flow returns an `installation_id` on this
// callback, but that value is attacker-spoofable — GitHub's own docs say
// not to rely on it — so we intentionally do NOT persist it here, and
// nothing reads it today. If/when GitHub App installation-token auth is
// added, resolve the installation via `GET /user/installations` with the
// user token (which also proves the user owns it) rather than trusting the
// value from this callback.

await this.connectionService.activateConnection(connection.id);

// Provider-specific post-OAuth actions
Expand Down Expand Up @@ -643,5 +696,4 @@ export class OAuthController {
}
return url.toString();
}

}
27 changes: 15 additions & 12 deletions apps/api/src/isms/documents/generate.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,10 @@ function buildTx() {
ismsInterestedParty: registerTable(),
ismsInterestedPartyRequirement: registerTable(),
ismsObjective: registerTable(),
ismsDocument: { findFirst: jest.fn().mockResolvedValue(null) },
ismsDocumentVersion: {
ismsDocument: {
findFirst: jest.fn().mockResolvedValue(null),
create: jest.fn().mockResolvedValue({}),
// CS-701: the draft narrative lives on IsmsDocument, not a version row.
findUnique: jest.fn().mockResolvedValue(null),
update: jest.fn().mockResolvedValue({}),
},
};
Expand Down Expand Up @@ -132,24 +132,27 @@ describe('runDerivation', () => {
expect(tx.ismsObjective.createMany).toHaveBeenCalled();
});

it('creates a version with the derived narrative for isms_scope', async () => {
it('writes the derived narrative onto an empty document draft (isms_scope)', async () => {
const tx = buildTx();
await runDerivation({ tx: asTx(tx), type: 'isms_scope', ...baseArgs });
expect(tx.ismsDocumentVersion.create).toHaveBeenCalled();
const created = tx.ismsDocumentVersion.create.mock.calls[0][0].data;
expect(created.narrative.certificateScopeSentence).toBeDefined();
expect(tx.ismsDocument.update).toHaveBeenCalledWith(
expect.objectContaining({ where: { id: 'doc_1' } }),
);
const updated = tx.ismsDocument.update.mock.calls[0][0].data;
expect(updated.draftNarrative.certificateScopeSentence).toBeDefined();
});

it('updates the existing version narrative for leadership', async () => {
it('preserves a non-empty existing draft narrative (CS-437 override)', async () => {
const tx = buildTx();
tx.ismsDocumentVersion.findFirst.mockResolvedValue({ id: 'ver_1' });
tx.ismsDocument.findUnique.mockResolvedValue({
draftNarrative: { certificateScopeSentence: 'Manual edit' },
});
await runDerivation({
tx: asTx(tx),
type: 'leadership_commitment',
...baseArgs,
});
expect(tx.ismsDocumentVersion.update).toHaveBeenCalledWith(
expect.objectContaining({ where: { id: 'ver_1' } }),
);
// A regenerate must never clobber the customer's manual narrative.
expect(tx.ismsDocument.update).not.toHaveBeenCalled();
});
});
25 changes: 10 additions & 15 deletions apps/api/src/isms/documents/generate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -174,22 +174,17 @@ async function generateNarrative({
if (!derived) return;
const narrative: Prisma.InputJsonValue = JSON.parse(JSON.stringify(derived));

const latest = await tx.ismsDocumentVersion.findFirst({
where: { documentId, isLatest: true },
// The draft narrative lives on IsmsDocument (CS-701). Preserve a non-empty
// existing draft so a regenerate never clobbers the customer's manual edits
// (CS-437 override); seed an absent/empty draft with the derived narrative.
const document = await tx.ismsDocument.findUnique({
where: { id: documentId },
select: { draftNarrative: true },
});
if (latest) {
// Preserve a non-empty narrative so a regenerate never clobbers the
// customer's manual edits (CS-437 override). An absent or empty ({}) value —
// e.g. a snapshot-only version — is still seeded with the derived narrative.
if (hasNarrativeContent(latest.narrative)) return;
await tx.ismsDocumentVersion.update({
where: { id: latest.id },
data: { narrative },
});
return;
}
await tx.ismsDocumentVersion.create({
data: { documentId, version: 1, isLatest: true, narrative },
if (document && hasNarrativeContent(document.draftNarrative)) return;
await tx.ismsDocument.update({
where: { id: documentId },
data: { draftNarrative: narrative },
});
}

Expand Down
14 changes: 11 additions & 3 deletions apps/api/src/isms/documents/org-profile.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { db } from '@db';
import type { Prisma } from '@db';
import { DEFAULT_INTENDED_OUTCOMES } from '../wizard/wizard-defaults';
import { parseStoredAnswers } from '../wizard/wizard-schema';
import type { IsmsKeyValue } from '../utils/export-shared';
Expand Down Expand Up @@ -29,23 +30,30 @@ const QUESTIONS = {
export async function loadOrgProfile({
organizationId,
frameworkId,
client = db,
}: {
organizationId: string;
frameworkId: string;
/**
* DB client to read through. Defaults to the global `db`; the approve flow
* passes its transaction client so the snapshot's org profile reads share the
* same isolation context as the register rows written in the same transaction.
*/
client?: Prisma.TransactionClient;
}): Promise<IsmsOrgProfile> {
const [organization, contextEntries, profile] = await Promise.all([
db.organization.findUnique({
client.organization.findUnique({
where: { id: organizationId },
select: { name: true, website: true },
}),
db.context.findMany({
client.context.findMany({
where: { organizationId },
// Deterministic ordering so duplicate questions resolve to the same answer
// every export; the earliest-created entry wins (id breaks createdAt ties).
orderBy: [{ createdAt: 'asc' }, { id: 'asc' }],
select: { question: true, answer: true },
}),
db.ismsProfile.findUnique({
client.ismsProfile.findUnique({
where: { organizationId_frameworkId: { organizationId, frameworkId } },
select: { answers: true },
}),
Expand Down
14 changes: 12 additions & 2 deletions apps/api/src/isms/dto/export-isms-document.dto.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { ApiProperty } from '@nestjs/swagger';
import { IsIn } from 'class-validator';
import { ApiProperty, ApiPropertyOptional } from '@nestjs/swagger';
import { IsIn, IsOptional, IsString } from 'class-validator';

export class ExportIsmsDocumentDto {
@ApiProperty({
Expand All @@ -9,4 +9,14 @@ export class ExportIsmsDocumentDto {
})
@IsIn(['pdf', 'docx'])
format!: 'pdf' | 'docx';

@ApiPropertyOptional({
description:
"Published version to export. Omit to export the document's current " +
'published version (or the working draft if it has never been published); ' +
'provide a version id to download exactly what was approved at that version.',
})
@IsOptional()
@IsString()
versionId?: string;
}
6 changes: 3 additions & 3 deletions apps/api/src/isms/isms-context-issue.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { Injectable, NotFoundException } from '@nestjs/common';
import { db } from '@db';
import type { Prisma } from '@db';
import { invalidateApprovalIfNeeded } from './utils/approval';
import { lockDocumentForPositions } from './utils/document-lock';
import { lockDocument } from './utils/document-lock';
import type {
CreateContextIssueInput,
UpdateContextIssueInput,
Expand All @@ -28,7 +28,7 @@ export class IsmsContextIssueService {
await this.requireDocument({ documentId, organizationId });

return db.$transaction(async (tx) => {
await lockDocumentForPositions(tx, documentId);
await lockDocument(tx, documentId);
const position =
dto.position ?? (await this.nextPosition({ tx, documentId }));
await invalidateApprovalIfNeeded({ tx, documentId });
Expand Down Expand Up @@ -92,7 +92,7 @@ export class IsmsContextIssueService {
/**
* Next position uses max(position)+1 so it survives deletes. Runs on the
* transaction client; the create first takes a per-document advisory lock
* (lockDocumentForPositions) so concurrent creates can't read the same max.
* (lockDocument) so concurrent creates can't read the same max.
*/
private async nextPosition({
tx,
Expand Down
Loading
Loading