From 6febe18daa5da7aeb4bad4031662c681dbbd5ad3 Mon Sep 17 00:00:00 2001 From: Evert Prants Date: Fri, 7 Jun 2024 17:24:27 +0300 Subject: [PATCH] Cleanups --- src/lib/constants.ts | 2 + src/lib/i18n/en/admin.json | 1 + src/lib/server/api-utils.ts | 6 +- .../email/templates/forgot-password.email.ts | 6 +- .../email/templates/invitation.email.ts | 6 +- .../templates/oauth2-invitation.email.ts | 6 +- .../email/templates/registration.email.ts | 6 +- .../server/oauth2/controller/authorization.ts | 18 +- src/lib/server/oauth2/model/client.ts | 9 +- src/lib/server/oauth2/response.ts | 161 +++++++++--------- src/lib/server/users/admin.ts | 2 + src/lib/server/users/index.ts | 10 +- .../accept-invite}/+server.ts | 5 +- src/routes/login/password/+page.server.ts | 4 +- .../ssoadmin/oauth2/[uuid]/+page.server.ts | 16 +- .../ssoadmin/oauth2/[uuid]/+page.svelte | 11 +- 16 files changed, 139 insertions(+), 130 deletions(-) rename src/routes/{ssoadmin/oauth2/invite => account/accept-invite}/+server.ts (88%) diff --git a/src/lib/constants.ts b/src/lib/constants.ts index 6846fac..d290e75 100644 --- a/src/lib/constants.ts +++ b/src/lib/constants.ts @@ -1 +1,3 @@ export const allowedImages = ['image/png', 'image/jpg', 'image/jpeg']; +export const OAUTH2_MAX_REDIRECTS = 5; +export const OAUTH2_MAX_URLS = 1; diff --git a/src/lib/i18n/en/admin.json b/src/lib/i18n/en/admin.json index 135b036..c3d5823 100644 --- a/src/lib/i18n/en/admin.json +++ b/src/lib/i18n/en/admin.json @@ -118,6 +118,7 @@ "invalidTitle": "Name must be between 3 and 32 characters long.", "invalidDescription": "Description must be at most 1000 characters long.", "deleteActivated": "Cannot delete an active application. Please deactivate it first.", + "illegalUrl": "You cannot add another URL of this type.", "invalidUrlId": "Invalid URL ID for deletion.", "invalidUrlType": "Invalid URL type provided.", "invalidUrl": "Invalid URL provided.", diff --git a/src/lib/server/api-utils.ts b/src/lib/server/api-utils.ts index 82f836d..063ab73 100644 --- a/src/lib/server/api-utils.ts +++ b/src/lib/server/api-utils.ts @@ -14,7 +14,7 @@ export class ApiUtils { const jsonBody = await request.json(); return jsonBody; } catch { - // Try next... + return {}; } } @@ -22,9 +22,7 @@ export class ApiUtils { const formBody = await request.formData(); return Object.fromEntries(formBody); } catch (err) { - // Skip... + return {}; } - - return {}; } } diff --git a/src/lib/server/email/templates/forgot-password.email.ts b/src/lib/server/email/templates/forgot-password.email.ts index 2d30ebc..f64280a 100644 --- a/src/lib/server/email/templates/forgot-password.email.ts +++ b/src/lib/server/email/templates/forgot-password.email.ts @@ -11,8 +11,7 @@ In order to change your password, please click on the following link. Change your password: ${url} -If you did not request a password change on ${PUBLIC_SITE_NAME}, you can safely ignore this email. - `, +If you did not request a password change on ${PUBLIC_SITE_NAME}, you can safely ignore this email.`, html: /* html */ `

${PUBLIC_SITE_NAME}

@@ -22,6 +21,5 @@ If you did not request a password change on ${PUBLIC_SITE_NAME}, you can safely

Change your password: ${url}

-

If you did not request a password change on ${PUBLIC_SITE_NAME}, you can safely ignore this email.

- ` +

If you did not request a password change on ${PUBLIC_SITE_NAME}, you can safely ignore this email.

` }); diff --git a/src/lib/server/email/templates/invitation.email.ts b/src/lib/server/email/templates/invitation.email.ts index a654e3c..61ec75c 100644 --- a/src/lib/server/email/templates/invitation.email.ts +++ b/src/lib/server/email/templates/invitation.email.ts @@ -9,8 +9,7 @@ Please click on the following link to create an account on ${PUBLIC_SITE_NAME}. Create your account here: ${url} -This email was sent to you because you have requested an account on ${PUBLIC_SITE_NAME}. If you did not request this, you may safely ignore this email. - `, +This email was sent to you because you have requested an account on ${PUBLIC_SITE_NAME}. If you did not request this, you may safely ignore this email.`, html: /* html */ `

${PUBLIC_SITE_NAME}

@@ -18,6 +17,5 @@ This email was sent to you because you have requested an account on ${PUBLIC_SIT

Create your account here: ${url}

-

This email was sent to you because you have requested an account on ${PUBLIC_SITE_NAME}. If you did not request this, you may safely ignore this email.

- ` +

This email was sent to you because you have requested an account on ${PUBLIC_SITE_NAME}. If you did not request this, you may safely ignore this email.

` }); diff --git a/src/lib/server/email/templates/oauth2-invitation.email.ts b/src/lib/server/email/templates/oauth2-invitation.email.ts index 59f15e5..4514054 100644 --- a/src/lib/server/email/templates/oauth2-invitation.email.ts +++ b/src/lib/server/email/templates/oauth2-invitation.email.ts @@ -15,8 +15,7 @@ Please use the following link to accept the invitation. Accept invitation: ${url} -This email was sent to you because someone invited you to contribute to an application on ${PUBLIC_SITE_NAME}. If you believe that this was sent in error, you may safely ignore this email. - `, +This email was sent to you because someone invited you to contribute to an application on ${PUBLIC_SITE_NAME}. If you believe that this was sent in error, you may safely ignore this email.`, html: /* html */ `

${PUBLIC_SITE_NAME}

@@ -26,6 +25,5 @@ This email was sent to you because someone invited you to contribute to an appli

Accept invitation: ${url}

-

This email was sent to you because someone invited you to contribute to an application on ${PUBLIC_SITE_NAME}. If you believe that this was sent in error, you may safely ignore this email.

- ` +

This email was sent to you because someone invited you to contribute to an application on ${PUBLIC_SITE_NAME}. If you believe that this was sent in error, you may safely ignore this email.

` }); diff --git a/src/lib/server/email/templates/registration.email.ts b/src/lib/server/email/templates/registration.email.ts index 82f6f97..23f5b56 100644 --- a/src/lib/server/email/templates/registration.email.ts +++ b/src/lib/server/email/templates/registration.email.ts @@ -11,8 +11,7 @@ In order to proceed with logging in, please click on the following link to activ Activate your account: ${url} -This email was sent to you because you have created an account on ${PUBLIC_SITE_NAME}. If you did not create an account, you may contact us or just let the account expire. - `, +This email was sent to you because you have created an account on ${PUBLIC_SITE_NAME}. If you did not create an account, you may contact us or just let the account expire.`, html: /* html */ `

${PUBLIC_SITE_NAME}

@@ -22,6 +21,5 @@ This email was sent to you because you have created an account on ${PUBLIC_SITE_

Activate your account: ${url}

-

This email was sent to you because you have created an account on ${PUBLIC_SITE_NAME}. If you did not create an account, you may contact us or just let the account expire.

- ` +

This email was sent to you because you have created an account on ${PUBLIC_SITE_NAME}. If you did not create an account, you may contact us or just let the account expire.

` }); diff --git a/src/lib/server/oauth2/controller/authorization.ts b/src/lib/server/oauth2/controller/authorization.ts index e544577..7820359 100644 --- a/src/lib/server/oauth2/controller/authorization.ts +++ b/src/lib/server/oauth2/controller/authorization.ts @@ -20,7 +20,7 @@ import { OAuth2Users } from '../model/user'; import { OAuth2Response } from '../response'; export class OAuth2AuthorizationController { - static prehandle = async (url: URL, locals: App.Locals) => { + private static prehandle = async (url: URL, locals: App.Locals) => { if (!url.searchParams.has('redirect_uri')) { throw new InvalidRequest('redirect_uri field is mandatory for authorization endpoint'); } @@ -52,8 +52,8 @@ export class OAuth2AuthorizationController { // Support multiple types const responseTypes = responseType.split(' '); let grantTypes: string[] = []; - for (const i in responseTypes) { - switch (responseTypes[i]) { + for (const responseType of responseTypes) { + switch (responseType) { case 'code': grantTypes.push('authorization_code'); break; @@ -61,10 +61,8 @@ export class OAuth2AuthorizationController { grantTypes.push('implicit'); break; case 'id_token': - grantTypes.push('id_token'); - break; case 'none': - grantTypes.push(responseTypes[i]); + grantTypes.push(responseType); break; default: throw new UnsupportedResponseType('Unknown response_type parameter passed'); @@ -95,7 +93,7 @@ export class OAuth2AuthorizationController { // The client needs to support all grant types for (const grantType of grantTypes) { - if (!OAuth2Clients.checkGrantType(client, grantType) && grantType !== 'none') { + if (grantType !== 'none' && !OAuth2Clients.checkGrantType(client, grantType)) { throw new UnauthorizedClient('This client does not support grant type ' + grantType); } } @@ -127,7 +125,7 @@ export class OAuth2AuthorizationController { }; }; - static posthandle = async ( + private static posthandle = async ( url: URL, { client, @@ -141,9 +139,9 @@ export class OAuth2AuthorizationController { }: Awaited> ) => { let resObj: Record = {}; - for (const i in grantTypes) { + for (const grantType of grantTypes) { let data = null; - switch (grantTypes[i]) { + switch (grantType) { case 'authorization_code': data = await OAuth2Codes.create( user.id, diff --git a/src/lib/server/oauth2/model/client.ts b/src/lib/server/oauth2/model/client.ts index 36a2a2d..43cba96 100644 --- a/src/lib/server/oauth2/model/client.ts +++ b/src/lib/server/oauth2/model/client.ts @@ -215,6 +215,11 @@ export class OAuth2Clients { ) { const filterText = `%${filters?.filter?.toLowerCase()}%`; const limit = filters?.limit || 20; + + // LEFT JOINs below will include more rows than we allow with LIMIT, + // so we need to do a subquery with the limiting first. + // The LEFT JOIN in the subquery only contributes to the WHERE clause + // and will not affect the returned row count. const allowedClients = DB.drizzle .select({ id: oauth2Client.id }) .from(oauth2Client) @@ -404,7 +409,7 @@ export class OAuth2Clients { static async sendManagerInvitationEmail(client: OAuth2Client, actor: User, email: string) { const token = await UserTokens.create( 'invite', - new Date(Date.now() + 3600 * 1000), + new Date(Date.now() + 7 * 24 * 60 * 60 * 1000), actor.id, undefined, `clientmanager=${client.client_id}` @@ -413,7 +418,7 @@ export class OAuth2Clients { const content = OAuth2InvitationEmail( actor.display_name, client.title, - `${PUBLIC_URL}/ssoadmin/oauth2/invite?${params.toString()}` + `${PUBLIC_URL}/account/accept-invite?${params.toString()}` ); // TODO: logging diff --git a/src/lib/server/oauth2/response.ts b/src/lib/server/oauth2/response.ts index 2756cca..73b2c34 100644 --- a/src/lib/server/oauth2/response.ts +++ b/src/lib/server/oauth2/response.ts @@ -15,14 +15,65 @@ export interface OAuth2TokenResponse { expires_in?: number; token_type?: string; state?: string; - active?: boolean; +} + +export interface OAuth2IntrospectResponse { + active: boolean; scope?: string; + username?: string; client_id?: string; exp?: number; } +export type OAuth2ResponseType = OAuth2TokenResponse | OAuth2IntrospectResponse; + export class OAuth2Response { - static createResponse(code: number, data: unknown) { + static error(url: URL, err: OAuth2Error, redirectUri?: string) { + if (!(err instanceof OAuth2Error)) { + throw err; + } + + OAuth2Response.doErrorRedirect(url, err, redirectUri); + + return OAuth2Response.createErrorResponse(err); + } + + static errorPlain(url: URL, err: OAuth2Error, redirectUri?: string) { + if (!(err instanceof OAuth2Error)) { + throw err; + } + + OAuth2Response.doErrorRedirect(url, err, redirectUri); + + return { + error: err.code, + error_description: err.message + }; + } + + static response( + url: URL, + obj: OAuth2ResponseType, + redirectUri?: string, + fragment: boolean = false + ) { + OAuth2Response.doResponseRedirect(url, obj, redirectUri, fragment); + + return OAuth2Response.createResponse(200, obj); + } + + static responsePlain( + url: URL, + obj: OAuth2ResponseType, + redirectUri?: string, + fragment: boolean = false + ) { + OAuth2Response.doResponseRedirect(url, obj, redirectUri, fragment); + + return obj; + } + + private static createResponse(code: number, data: unknown) { const isJson = typeof data === 'object'; const body = isJson ? JSON.stringify(data) : (data as string); return new Response(body, { @@ -33,104 +84,44 @@ export class OAuth2Response { }); } - static createErrorResponse(err: OAuth2Error) { + private static createErrorResponse(err: OAuth2Error) { return OAuth2Response.createResponse(err.status, { error: err.code, error_description: err.message }); } - static redirect(redirectUri: string) { + private static doResponseRedirect( + url: URL, + obj: OAuth2ResponseType, + redirectUri?: string, + fragment: boolean = false + ) { + if (!redirectUri) return; + const searchJoinChar = redirectUri.includes('?') ? '&' : '?'; + redirectUri += fragment ? '#' : searchJoinChar; + + if (url.searchParams.has('state')) { + (obj as OAuth2TokenResponse).state = url.searchParams.get('state') as string; + } + + redirectUri += new URLSearchParams(obj as Record).toString(); return redirect(302, redirectUri); } - static error(url: URL, err: OAuth2Error, redirectUri?: string) { - if (!(err instanceof OAuth2Error)) { - throw err; - } - - if (redirectUri) { - const obj: ErrorResponseData = { - error: err.code, - error_description: err.message - }; - - if (url.searchParams.has('state')) { - obj.state = url.searchParams.get('state') as string; - } - - redirectUri += '?' + new URLSearchParams(obj as Record).toString(); - - return redirect(302, redirectUri); - } - - return OAuth2Response.createErrorResponse(err); - } - - static errorPlain(url: URL, err: OAuth2Error, redirectUri?: string) { - if (!(err instanceof OAuth2Error)) { - throw err; - } - - if (redirectUri) { - const obj: ErrorResponseData = { - error: err.code, - error_description: err.message - }; - - if (url.searchParams.has('state')) { - obj.state = url.searchParams.get('state') as string; - } - - redirectUri += '?' + new URLSearchParams(obj as Record).toString(); - - return redirect(302, redirectUri); - } - - return { + private static doErrorRedirect(url: URL, err: OAuth2Error, redirectUri?: string) { + if (!redirectUri) return; + const obj: ErrorResponseData = { error: err.code, error_description: err.message }; - } - static response( - url: URL, - obj: OAuth2TokenResponse, - redirectUri?: string, - fragment: boolean = false - ) { - if (redirectUri) { - redirectUri += fragment ? '#' : redirectUri.indexOf('?') === -1 ? '?' : '&'; - - if (url.searchParams.has('state')) { - obj.state = url.searchParams.get('state') as string; - } - - redirectUri += new URLSearchParams(obj as Record).toString(); - return redirect(302, redirectUri); + if (url.searchParams.has('state')) { + obj.state = url.searchParams.get('state') as string; } - return OAuth2Response.createResponse(200, obj); - } + redirectUri += '?' + new URLSearchParams(obj as Record).toString(); - static responsePlain( - url: URL, - obj: OAuth2TokenResponse, - redirectUri?: string, - fragment: boolean = false - ) { - if (redirectUri) { - const searchJoinChar = redirectUri.includes('?') ? '&' : '?'; - redirectUri += fragment ? '#' : searchJoinChar; - - if (url.searchParams.has('state')) { - obj.state = url.searchParams.get('state') as string; - } - - redirectUri += new URLSearchParams(obj as Record).toString(); - return redirect(302, redirectUri); - } - - return obj; + return redirect(302, redirectUri); } } diff --git a/src/lib/server/users/admin.ts b/src/lib/server/users/admin.ts index 3e3a9d6..21824b8 100644 --- a/src/lib/server/users/admin.ts +++ b/src/lib/server/users/admin.ts @@ -75,6 +75,8 @@ export class UsersAdmin { .from(user) .where(searchExpression); + // LEFT JOINs below will include more rows than we allow with LIMIT, + // so we need to do a subquery with the limiting first. const baseQuery = DB.drizzle .select({ id: user.id }) .from(user) diff --git a/src/lib/server/users/index.ts b/src/lib/server/users/index.ts index 71ac720..3f182d2 100644 --- a/src/lib/server/users/index.ts +++ b/src/lib/server/users/index.ts @@ -229,7 +229,7 @@ export class Users { static async sendRegistrationEmail(user: User) { const token = await UserTokens.create( 'activation', - new Date(Date.now() + 3600 * 1000), + new Date(Date.now() + 7 * 24 * 60 * 60 * 1000), user.id ); @@ -254,7 +254,11 @@ export class Users { * @param user User */ static async sendPasswordEmail(user: User) { - const token = await UserTokens.create('password', new Date(Date.now() + 3600 * 1000), user.id); + const token = await UserTokens.create( + 'password', + new Date(Date.now() + 60 * 60 * 1000), + user.id + ); const params = new URLSearchParams({ token: token.token }); const content = ForgotPasswordEmail( user.username, @@ -280,7 +284,7 @@ export class Users { static async sendInvitationEmail(email: string) { const token = await UserTokens.create( 'invite', - new Date(Date.now() + 3600 * 1000), + new Date(Date.now() + 7 * 24 * 60 * 60 * 1000), undefined, undefined, `register=${email}` diff --git a/src/routes/ssoadmin/oauth2/invite/+server.ts b/src/routes/account/accept-invite/+server.ts similarity index 88% rename from src/routes/ssoadmin/oauth2/invite/+server.ts rename to src/routes/account/accept-invite/+server.ts index 5508f95..e8ed1af 100644 --- a/src/routes/ssoadmin/oauth2/invite/+server.ts +++ b/src/routes/account/accept-invite/+server.ts @@ -3,9 +3,8 @@ import { OAuth2Clients } from '$lib/server/oauth2/index.js'; import { UserTokens, Users } from '$lib/server/users'; export const GET = async ({ locals, url }) => { - const userInfo = locals.session.data?.user; - const currentUser = await Users.getBySession(userInfo); - if (!userInfo || !currentUser) { + const currentUser = await Users.getBySession(locals.session.data?.user); + if (!currentUser) { await locals.session.destroy(); return ApiUtils.redirect(`/login?redirectTo=${encodeURIComponent(url.pathname)}`); } diff --git a/src/routes/login/password/+page.server.ts b/src/routes/login/password/+page.server.ts index 6dc3bdf..733f7db 100644 --- a/src/routes/login/password/+page.server.ts +++ b/src/routes/login/password/+page.server.ts @@ -10,6 +10,8 @@ interface PasswordRequest { repeatPassword: string; } +// Sending an email asynchronously has a similar amount of delay, +// so lets fake it. TODO: offload email sending somewhere else. const failDelay = () => new Promise((resolve) => setTimeout(resolve, 2000 + (Math.random() * 2000 - 1000))); @@ -35,7 +37,7 @@ export const actions = { const user = await Users.getByLogin(email); if (!user || user.activated === 0) { await failDelay(); - return { success: true }; + return { success: 'sent' }; } try { diff --git a/src/routes/ssoadmin/oauth2/[uuid]/+page.server.ts b/src/routes/ssoadmin/oauth2/[uuid]/+page.server.ts index 6e84262..b366ed3 100644 --- a/src/routes/ssoadmin/oauth2/[uuid]/+page.server.ts +++ b/src/routes/ssoadmin/oauth2/[uuid]/+page.server.ts @@ -1,8 +1,13 @@ +import { OAUTH2_MAX_REDIRECTS, OAUTH2_MAX_URLS } from '$lib/constants.js'; import { AdminUtils } from '$lib/server/admin-utils'; import { Changesets } from '$lib/server/changesets.js'; import { CryptoUtils } from '$lib/server/crypto-utils.js'; import type { OAuth2Client, User } from '$lib/server/drizzle'; -import { OAuth2ClientURLType, OAuth2Clients } from '$lib/server/oauth2'; +import { + OAuth2ClientURLType, + OAuth2Clients, + type OAuth2ClientAdminListItem +} from '$lib/server/oauth2'; import { Uploads } from '$lib/server/upload.js'; import { Users } from '$lib/server/users'; import { hasPrivileges } from '$lib/utils'; @@ -182,6 +187,15 @@ export const actions = { return fail(400, { errors: ['invalidUrl'] }); } + // Do not allow adding multiple URLs of the same type, except for redirects. + const existingURLs = (details as OAuth2ClientAdminListItem).urls; + const existingOfType = existingURLs.filter((entry) => entry.type === type).length; + const maxOfType = + type === OAuth2ClientURLType.REDIRECT_URI ? OAUTH2_MAX_REDIRECTS : OAUTH2_MAX_URLS; + if (maxOfType < existingOfType) { + return fail(400, { errors: ['illegalUrl'] }); + } + await OAuth2Clients.addUrl(details, type, url); return { errors: [] }; diff --git a/src/routes/ssoadmin/oauth2/[uuid]/+page.svelte b/src/routes/ssoadmin/oauth2/[uuid]/+page.svelte index 24f37c7..6e2977c 100644 --- a/src/routes/ssoadmin/oauth2/[uuid]/+page.svelte +++ b/src/routes/ssoadmin/oauth2/[uuid]/+page.svelte @@ -9,13 +9,14 @@ import FormControl from '$lib/components/form/FormControl.svelte'; import FormSection from '$lib/components/form/FormSection.svelte'; import FormWrapper from '$lib/components/form/FormWrapper.svelte'; + import FormErrors from '$lib/components/form/FormErrors.svelte'; + import ActionButton from '$lib/components/ActionButton.svelte'; + import type { ActionData, PageData } from './$types'; import { t } from '$lib/i18n'; import { page } from '$app/stores'; - import type { ActionData, PageData } from './$types'; import { writable } from 'svelte/store'; - import FormErrors from '$lib/components/form/FormErrors.svelte'; import { PUBLIC_SITE_NAME, PUBLIC_URL } from '$env/static/public'; - import ActionButton from '$lib/components/ActionButton.svelte'; + import { OAUTH2_MAX_REDIRECTS, OAUTH2_MAX_URLS } from '$lib/constants'; export let data: PageData; export let form: ActionData; @@ -31,9 +32,9 @@ // Can have up to five redirect URIs, only one of other types const countOfType = data.details.urls.filter(({ type: subType }) => type === subType).length; if (type === 'redirect_uri') { - return countOfType < 5; + return countOfType < OAUTH2_MAX_REDIRECTS; } - return !countOfType; + return countOfType < OAUTH2_MAX_URLS; }); $: splitScopes = data.details.scope?.split(' ') || []; $: splitGrants = data.details.grants?.split(' ') || [];