Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(core): add PATCH/GET /saml-applications/:id APIs #6827

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
import {
ApplicationType,
type SamlApplicationResponse,
type PatchSamlApplication,
} from '@logto/schemas';
import { generateStandardId } from '@logto/shared';
import { removeUndefinedKeys } from '@silverhand/essentials';

import RequestError from '#src/errors/RequestError/index.js';
import type Queries from '#src/tenants/Queries.js';
import assertThat from '#src/utils/assert-that.js';

import { ensembleSamlApplication, generateKeyPairAndCertificate } from './utils.js';

export const createSamlApplicationsLibrary = (queries: Queries) => {
const {
applications: { findApplicationById, updateApplicationById },
samlApplicationSecrets: { insertSamlApplicationSecret },
samlApplicationConfigs: {
findSamlApplicationConfigByApplicationId,
updateSamlApplicationConfig,
},
} = queries;

const createSamlApplicationSecret = async (
applicationId: string,
// Set certificate life span to 1 year by default.
lifeSpanInDays = 365
) => {
const { privateKey, certificate, notAfter } = await generateKeyPairAndCertificate(
lifeSpanInDays
);

return insertSamlApplicationSecret({
id: generateStandardId(),
applicationId,
privateKey,
certificate,
expiresAt: Math.floor(notAfter.getTime() / 1000),
active: false,
});

Check warning on line 41 in packages/core/src/saml-applications/libraries/saml-applications.ts

View check run for this annotation

Codecov / codecov/patch

packages/core/src/saml-applications/libraries/saml-applications.ts#L26-L41

Added lines #L26 - L41 were not covered by tests
};

const findSamlApplicationById = async (id: string): Promise<SamlApplicationResponse> => {
const application = await findApplicationById(id);
assertThat(
application.type === ApplicationType.SAML,
new RequestError({
code: 'application.saml.saml_application_only',
status: 422,
})
);

const samlConfig = await findSamlApplicationConfigByApplicationId(application.id);

return ensembleSamlApplication({ application, samlConfig });

Check warning on line 56 in packages/core/src/saml-applications/libraries/saml-applications.ts

View check run for this annotation

Codecov / codecov/patch

packages/core/src/saml-applications/libraries/saml-applications.ts#L45-L56

Added lines #L45 - L56 were not covered by tests
};

const updateSamlApplicationById = async (
id: string,
patchApplicationObject: PatchSamlApplication
): Promise<SamlApplicationResponse> => {
const { config, ...applicationData } = patchApplicationObject;
const originalApplication = await findApplicationById(id);

assertThat(
originalApplication.type === ApplicationType.SAML,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto: status: 422

new RequestError({
code: 'application.saml.saml_application_only',
status: 422,
})
);

const [updatedApplication, upToDateSamlConfig] = await Promise.all([
Object.keys(applicationData).length > 0
? updateApplicationById(id, removeUndefinedKeys(applicationData))
: originalApplication,
config
? updateSamlApplicationConfig({
set: config,
where: { applicationId: id },
jsonbMode: 'replace',
})
: findSamlApplicationConfigByApplicationId(id),
]);

return ensembleSamlApplication({
application: updatedApplication,
samlConfig: upToDateSamlConfig,
});

Check warning on line 90 in packages/core/src/saml-applications/libraries/saml-applications.ts

View check run for this annotation

Codecov / codecov/patch

packages/core/src/saml-applications/libraries/saml-applications.ts#L60-L90

Added lines #L60 - L90 were not covered by tests
};

return {
createSamlApplicationSecret,
findSamlApplicationById,
updateSamlApplicationById,
};
};
34 changes: 0 additions & 34 deletions packages/core/src/saml-applications/libraries/secrets.ts

This file was deleted.

43 changes: 43 additions & 0 deletions packages/core/src/saml-applications/libraries/utils.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,18 @@
import crypto from 'node:crypto';

import {
type SamlApplicationResponse,
type Application,
type SamlApplicationConfig,
type SamlAcsUrl,
BindingType,
} from '@logto/schemas';
import { addDays } from 'date-fns';
import forge from 'node-forge';

import RequestError from '#src/errors/RequestError/index.js';
import assertThat from '#src/utils/assert-that.js';

export const generateKeyPairAndCertificate = async (lifeSpanInDays = 365) => {
const keypair = forge.pki.rsa.generateKeyPair({ bits: 4096 });
return createCertificate(keypair, lifeSpanInDays);
Expand All @@ -23,7 +33,7 @@
cert.validity.notAfter = notAfter;
/* eslint-enable @silverhand/fp/no-mutation */

// TODO: read from tenant config or let user customize before downloading

Check warning on line 36 in packages/core/src/saml-applications/libraries/utils.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/core/src/saml-applications/libraries/utils.ts#L36

[no-warning-comments] Unexpected 'todo' comment: 'TODO: read from tenant config or let...'.
const subjectAttributes: forge.pki.CertificateField[] = [
{
name: 'commonName',
Expand Down Expand Up @@ -56,3 +66,36 @@
notAfter,
};
};

/**
* According to the design, a SAML app will be associated with multiple records from various tables.
* Therefore, when complete SAML app data is required, it is necessary to retrieve multiple related records and assemble them into a comprehensive SAML app dataset. This dataset includes:
* - A record from the `applications` table with a `type` of `SAML`
* - A record from the `saml_application_configs` table
*/
export const ensembleSamlApplication = ({
application,
samlConfig,
}: {
application: Application;
samlConfig: Pick<SamlApplicationConfig, 'attributeMapping' | 'entityId' | 'acsUrl'>;
}): SamlApplicationResponse => {
return {
...application,
...samlConfig,
};
};

Check warning on line 87 in packages/core/src/saml-applications/libraries/utils.ts

View check run for this annotation

Codecov / codecov/patch

packages/core/src/saml-applications/libraries/utils.ts#L77-L87

Added lines #L77 - L87 were not covered by tests

/**
* Only HTTP-POST binding is supported for receiving SAML assertions at the moment.
*/
export const validateAcsUrl = (acsUrl: SamlAcsUrl) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to validate the URL format here?

Copy link
Contributor Author

@darcyYe darcyYe Nov 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, updated samlAcsUrlGuard to do the check in another place.

const { binding } = acsUrl;
assertThat(
binding === BindingType.POST,
new RequestError({
code: 'application.saml.acs_url_binding_not_supported',
status: 422,
})
);
};

Check warning on line 101 in packages/core/src/saml-applications/libraries/utils.ts

View check run for this annotation

Codecov / codecov/patch

packages/core/src/saml-applications/libraries/utils.ts#L93-L101

Added lines #L93 - L101 were not covered by tests
2 changes: 1 addition & 1 deletion packages/core/src/saml-applications/queries/configs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
const updateSamlApplicationConfig = buildUpdateWhereWithPool(pool)(SamlApplicationConfigs, true);

const findSamlApplicationConfigByApplicationId = async (applicationId: string) =>
pool.maybeOne<SamlApplicationConfig>(sql`
pool.one<SamlApplicationConfig>(sql`

Check warning on line 19 in packages/core/src/saml-applications/queries/configs.ts

View check run for this annotation

Codecov / codecov/patch

packages/core/src/saml-applications/queries/configs.ts#L19

Added line #L19 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This config should be nullable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When creating a SAML app, a record in the saml_application_configs table is inserted simultaneously. Subsequent operations only involve DB "update" actions on this record.
If this record is not inserted during the creation of the SAML app, additional checks will be required when updates to the config are needed later. This means that we can always expect this query to retrieve exact ONE record.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the design of the console, when creating a SAML app, you only need to provide name/description.

select ${sql.join(Object.values(fields), sql`, `)}
from ${table}
where ${fields.applicationId}=${applicationId}
Expand Down
64 changes: 59 additions & 5 deletions packages/core/src/saml-applications/routes/index.ts
Original file line number Diff line number Diff line change
@@ -1,19 +1,22 @@
import {
ApplicationType,
samlApplicationCreateGuard,
samlApplicationPatchGuard,
samlApplicationResponseGuard,
} from '@logto/schemas';
import { generateStandardId } from '@logto/shared';
import { removeUndefinedKeys } from '@silverhand/essentials';
import { z } from 'zod';

import RequestError from '#src/errors/RequestError/index.js';
import koaGuard from '#src/middleware/koa-guard.js';
import { buildOidcClientMetadata } from '#src/oidc/utils.js';
import { generateInternalSecret } from '#src/routes/applications/application-secret.js';
import type { ManagementApiRouter, RouterInitArgs } from '#src/routes/types.js';
import { ensembleSamlApplication, validateAcsUrl } from '#src/saml-applications/routes/utils.js';
import assertThat from '#src/utils/assert-that.js';

import { ensembleSamlApplication, validateAcsUrl } from '../libraries/utils.js';

export default function samlApplicationRoutes<T extends ManagementApiRouter>(
...[router, { queries, libraries }]: RouterInitArgs<T>
) {
Expand All @@ -22,15 +25,19 @@
samlApplicationConfigs: { insertSamlApplicationConfig },
} = queries;
const {
samlApplicationSecrets: { createSamlApplicationSecret },
samlApplications: {
createSamlApplicationSecret,
findSamlApplicationById,
updateSamlApplicationById,
},

Check warning on line 32 in packages/core/src/saml-applications/routes/index.ts

View check run for this annotation

Codecov / codecov/patch

packages/core/src/saml-applications/routes/index.ts#L28-L32

Added lines #L28 - L32 were not covered by tests
} = libraries;

router.post(
'/saml-applications',
koaGuard({
body: samlApplicationCreateGuard,
response: samlApplicationResponseGuard,
status: [201, 400],
status: [201, 400, 422],

Check warning on line 40 in packages/core/src/saml-applications/routes/index.ts

View check run for this annotation

Codecov / codecov/patch

packages/core/src/saml-applications/routes/index.ts#L40

Added line #L40 was not covered by tests
}),
async (ctx, next) => {
const { name, description, customData, config } = ctx.guard.body;
Expand Down Expand Up @@ -72,17 +79,64 @@
}
);

router.get(
'/saml-applications/:id',
koaGuard({
params: z.object({
id: z.string(),
}),
response: samlApplicationResponseGuard,
status: [200, 404, 422],
}),
async (ctx, next) => {
const { id } = ctx.guard.params;

const samlApplication = await findSamlApplicationById(id);

ctx.status = 200;
ctx.body = samlApplication;

return next();
}
);

router.patch(
'/saml-applications/:id',
koaGuard({
params: z.object({ id: z.string() }),
body: samlApplicationPatchGuard,
response: samlApplicationResponseGuard,
status: [200, 404, 422],
}),
async (ctx, next) => {
const { id } = ctx.guard.params;

const updatedSamlApplication = await updateSamlApplicationById(id, ctx.guard.body);

ctx.status = 200;
ctx.body = updatedSamlApplication;

return next();
}
);

Check warning on line 122 in packages/core/src/saml-applications/routes/index.ts

View check run for this annotation

Codecov / codecov/patch

packages/core/src/saml-applications/routes/index.ts#L82-L122

Added lines #L82 - L122 were not covered by tests
router.delete(
'/saml-applications/:id',
koaGuard({
params: z.object({ id: z.string() }),
status: [204, 400, 404],
status: [204, 422, 404],

Check warning on line 127 in packages/core/src/saml-applications/routes/index.ts

View check run for this annotation

Codecov / codecov/patch

packages/core/src/saml-applications/routes/index.ts#L127

Added line #L127 was not covered by tests
}),
async (ctx, next) => {
const { id } = ctx.guard.params;

const { type } = await findApplicationById(id);
assertThat(type === ApplicationType.SAML, 'application.saml.saml_application_only');
assertThat(
type === ApplicationType.SAML,
new RequestError({
code: 'application.saml.saml_application_only',
status: 422,
})
);

Check warning on line 139 in packages/core/src/saml-applications/routes/index.ts

View check run for this annotation

Codecov / codecov/patch

packages/core/src/saml-applications/routes/index.ts#L133-L139

Added lines #L133 - L139 were not covered by tests

await deleteApplicationById(id);

Expand Down
42 changes: 0 additions & 42 deletions packages/core/src/saml-applications/routes/utils.ts

This file was deleted.

4 changes: 2 additions & 2 deletions packages/core/src/tenants/Libraries.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
import { createSsoConnectorLibrary } from '#src/libraries/sso-connector.js';
import { createUserLibrary } from '#src/libraries/user.js';
import { createVerificationStatusLibrary } from '#src/libraries/verification-status.js';
import { createSamlApplicationSecretsLibrary } from '#src/saml-applications/libraries/secrets.js';
import { createSamlApplicationsLibrary } from '#src/saml-applications/libraries/saml-applications.js';

import type Queries from './Queries.js';

Expand All @@ -38,7 +38,7 @@
passcodes = createPasscodeLibrary(this.queries, this.connectors);
applications = createApplicationLibrary(this.queries);
verificationStatuses = createVerificationStatusLibrary(this.queries);
samlApplicationSecrets = createSamlApplicationSecretsLibrary(this.queries);
samlApplications = createSamlApplicationsLibrary(this.queries);
roleScopes = createRoleScopeLibrary(this.queries);
domains = createDomainLibrary(this.queries);
protectedApps = createProtectedAppLibrary(this.queries);
Expand All @@ -58,7 +58,7 @@
this.connectors
);

constructor(

Check warning on line 61 in packages/core/src/tenants/Libraries.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/core/src/tenants/Libraries.ts#L61

[max-params] Constructor has too many parameters (5). Maximum allowed is 4.
public readonly tenantId: string,
private readonly queries: Queries,
// Explicitly passing connector library to eliminate dependency issue
Expand Down
17 changes: 16 additions & 1 deletion packages/integration-tests/src/api/saml-application.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
import { type SamlApplicationResponse, type CreateSamlApplication } from '@logto/schemas';
import {
type SamlApplicationResponse,
type CreateSamlApplication,
type PatchSamlApplication,
} from '@logto/schemas';

import { authedAdminApi } from './api.js';

Expand All @@ -11,3 +15,14 @@ export const createSamlApplication = async (createSamlApplication: CreateSamlApp

export const deleteSamlApplication = async (id: string) =>
authedAdminApi.delete(`saml-applications/${id}`);

export const updateSamlApplication = async (
id: string,
patchSamlApplication: PatchSamlApplication
) =>
authedAdminApi
.patch(`saml-applications/${id}`, { json: patchSamlApplication })
.json<SamlApplicationResponse>();

export const getSamlApplication = async (id: string) =>
authedAdminApi.get(`saml-applications/${id}`).json<SamlApplicationResponse>();
Loading
Loading