From 3b0b98a123b1dbbb3c5056153264b508b0975baf Mon Sep 17 00:00:00 2001 From: Qing Fu <16662647+zz5840@users.noreply.github.com> Date: Sun, 26 Nov 2023 14:35:20 +0800 Subject: [PATCH 1/4] feat(oauth): limited discord server sign-in --- backend/prisma/seed/config.seed.ts | 8 ++- .../oauth/exceptions/errorPage.exception.ts | 2 +- .../oauth/filter/errorPageException.filter.ts | 17 ++++++- .../src/oauth/provider/discord.provider.ts | 50 ++++++++++++++++--- .../oauth/provider/genericOidc.provider.ts | 11 +++- backend/src/oauth/provider/github.provider.ts | 13 ++--- frontend/src/i18n/translations/en-US.ts | 5 ++ 7 files changed, 86 insertions(+), 20 deletions(-) diff --git a/backend/prisma/seed/config.seed.ts b/backend/prisma/seed/config.seed.ts index bc74a5b68..187306373 100644 --- a/backend/prisma/seed/config.seed.ts +++ b/backend/prisma/seed/config.seed.ts @@ -180,6 +180,10 @@ const configVariables: ConfigVariables = { type: "boolean", defaultValue: "false", }, + "discord-limitedGuild": { + type: "string", + defaultValue: "", + }, "discord-clientId": { type: "string", defaultValue: "", @@ -262,8 +266,8 @@ async function migrateConfigVariables() { for (const existingConfigVariable of existingConfigVariables) { const configVariable = configVariables[existingConfigVariable.category]?.[ - existingConfigVariable.name - ]; + existingConfigVariable.name + ]; if (!configVariable) { await prisma.config.delete({ where: { diff --git a/backend/src/oauth/exceptions/errorPage.exception.ts b/backend/src/oauth/exceptions/errorPage.exception.ts index 6ebb10a6e..e901cad46 100644 --- a/backend/src/oauth/exceptions/errorPage.exception.ts +++ b/backend/src/oauth/exceptions/errorPage.exception.ts @@ -7,7 +7,7 @@ export class ErrorPageException extends Error { */ constructor( public readonly key: string = "default", - public readonly redirect: string = "/", + public readonly redirect: string = undefined, public readonly params?: string[], ) { super("error"); diff --git a/backend/src/oauth/filter/errorPageException.filter.ts b/backend/src/oauth/filter/errorPageException.filter.ts index bc4b6858c..055efa3dc 100644 --- a/backend/src/oauth/filter/errorPageException.filter.ts +++ b/backend/src/oauth/filter/errorPageException.filter.ts @@ -9,14 +9,27 @@ export class ErrorPageExceptionFilter implements ExceptionFilter { constructor(private config: ConfigService) {} catch(exception: ErrorPageException, host: ArgumentsHost) { - this.logger.error(exception); + this.logger.error( + JSON.stringify({ + error: exception.key, + params: exception.params, + redirect: exception.redirect, + }), + ); const ctx = host.switchToHttp(); const response = ctx.getResponse(); const url = new URL(`${this.config.get("general.appUrl")}/error`); - url.searchParams.set("redirect", exception.redirect); url.searchParams.set("error", exception.key); + if (exception.redirect) { + url.searchParams.set("redirect", exception.redirect); + } else { + const redirect = ctx.getRequest().cookies.access_token + ? "/account" + : "/auth/signIn"; + url.searchParams.set("redirect", redirect); + } if (exception.params) { url.searchParams.set("params", exception.params.join(",")); } diff --git a/backend/src/oauth/provider/discord.provider.ts b/backend/src/oauth/provider/discord.provider.ts index 97a8b5d16..ea45ba1a8 100644 --- a/backend/src/oauth/provider/discord.provider.ts +++ b/backend/src/oauth/provider/discord.provider.ts @@ -1,10 +1,10 @@ -import { OAuthProvider, OAuthToken } from "./oauthProvider.interface"; +import { Injectable } from "@nestjs/common"; +import fetch from "node-fetch"; +import { ConfigService } from "../../config/config.service"; import { OAuthCallbackDto } from "../dto/oauthCallback.dto"; import { OAuthSignInDto } from "../dto/oauthSignIn.dto"; -import { ConfigService } from "../../config/config.service"; -import { BadRequestException, Injectable } from "@nestjs/common"; -import fetch from "node-fetch"; - +import { ErrorPageException } from "../exceptions/errorPage.exception"; +import { OAuthProvider, OAuthToken } from "./oauthProvider.interface"; @Injectable() export class DiscordProvider implements OAuthProvider { constructor(private config: ConfigService) {} @@ -18,7 +18,9 @@ export class DiscordProvider implements OAuthProvider { this.config.get("general.appUrl") + "/api/oauth/callback/discord", response_type: "code", state: state, - scope: "identify email", + scope: + "identify email" + + (this.config.get("oauth.discord-limitedGuild") ? " guilds" : ""), }).toString(), ); } @@ -69,7 +71,14 @@ export class DiscordProvider implements OAuthProvider { }); const user = (await res.json()) as DiscordUser; if (user.verified === false) { - throw new BadRequestException("Unverified account."); + throw new ErrorPageException("unverified_account", undefined, [ + "provider_discord", + ]); + } + + const guild = this.config.get("oauth.discord-limitedGuild"); + if (guild) { + await this.checkLimitedGuild(token, guild); } return { @@ -79,6 +88,24 @@ export class DiscordProvider implements OAuthProvider { email: user.email, }; } + + async checkLimitedGuild(token: OAuthToken, guildId: string) { + try { + const res = await fetch("https://discord.com/api/v10/users/@me/guilds", { + method: "get", + headers: { + Accept: "application/json", + Authorization: `${token.tokenType || "Bearer"} ${token.accessToken}`, + }, + }); + const guilds = (await res.json()) as DiscordPartialGuild[]; + if (!guilds.some((guild) => guild.id === guildId)) { + throw new ErrorPageException("discord_guild_permission_denied"); + } + } catch { + throw new ErrorPageException("discord_guild_permission_denied"); + } + } } export interface DiscordToken { @@ -96,3 +123,12 @@ export interface DiscordUser { email: string; verified: boolean; } + +export interface DiscordPartialGuild { + id: string; + name: string; + icon: string; + owner: boolean; + permissions: string; + features: string[]; +} diff --git a/backend/src/oauth/provider/genericOidc.provider.ts b/backend/src/oauth/provider/genericOidc.provider.ts index bec617d4f..8bcd4c3ef 100644 --- a/backend/src/oauth/provider/genericOidc.provider.ts +++ b/backend/src/oauth/provider/genericOidc.provider.ts @@ -1,4 +1,4 @@ -import { BadRequestException } from "@nestjs/common"; +import { Logger } from "@nestjs/common"; import fetch from "node-fetch"; import { ConfigService } from "../../config/config.service"; import { JwtService } from "@nestjs/jwt"; @@ -7,11 +7,15 @@ import { nanoid } from "nanoid"; import { OAuthCallbackDto } from "../dto/oauthCallback.dto"; import { OAuthProvider, OAuthToken } from "./oauthProvider.interface"; import { OAuthSignInDto } from "../dto/oauthSignIn.dto"; +import { ErrorPageException } from "../exceptions/errorPage.exception"; export abstract class GenericOidcProvider implements OAuthProvider { protected discoveryUri: string; private configuration: OidcConfigurationCache; private jwk: OidcJwkCache; + private logger: Logger = new Logger( + Object.getPrototypeOf(this).constructor.name, + ); protected constructor( protected name: string, @@ -112,7 +116,10 @@ export abstract class GenericOidcProvider implements OAuthProvider { const nonce = await this.cache.get(key); await this.cache.del(key); if (nonce !== idTokenData.nonce) { - throw new BadRequestException("Invalid token"); + this.logger.error( + `Invalid nonce. Expected ${nonce}, but got ${idTokenData.nonce}`, + ); + throw new ErrorPageException("invalid_token"); } return { diff --git a/backend/src/oauth/provider/github.provider.ts b/backend/src/oauth/provider/github.provider.ts index 45258f9d8..9c3c131cc 100644 --- a/backend/src/oauth/provider/github.provider.ts +++ b/backend/src/oauth/provider/github.provider.ts @@ -1,9 +1,10 @@ -import { OAuthProvider, OAuthToken } from "./oauthProvider.interface"; +import { Injectable } from "@nestjs/common"; +import fetch from "node-fetch"; +import { ConfigService } from "../../config/config.service"; import { OAuthCallbackDto } from "../dto/oauthCallback.dto"; import { OAuthSignInDto } from "../dto/oauthSignIn.dto"; -import { ConfigService } from "../../config/config.service"; -import fetch from "node-fetch"; -import { BadRequestException, Injectable } from "@nestjs/common"; +import { ErrorPageException } from "../exceptions/errorPage.exception"; +import { OAuthProvider, OAuthToken } from "./oauthProvider.interface"; @Injectable() export class GitHubProvider implements OAuthProvider { @@ -48,12 +49,12 @@ export class GitHubProvider implements OAuthProvider { async getUserInfo(token: OAuthToken): Promise { if (!token.scope.includes("user:email")) { - throw new BadRequestException("No email permission granted"); + throw new ErrorPageException("no_email", undefined, ["provider_github"]); } const user = await this.getGitHubUser(token); const email = await this.getGitHubEmail(token); if (!email) { - throw new BadRequestException("No email found"); + throw new ErrorPageException("no_email", undefined, ["provider_github"]); } return { diff --git a/frontend/src/i18n/translations/en-US.ts b/frontend/src/i18n/translations/en-US.ts index 36eefcc4c..4a357346d 100644 --- a/frontend/src/i18n/translations/en-US.ts +++ b/frontend/src/i18n/translations/en-US.ts @@ -472,6 +472,8 @@ export default { "admin.config.oauth.microsoft-client-secret.description": "Client secret of the Microsoft OAuth app", "admin.config.oauth.discord-enabled": "Discord", "admin.config.oauth.discord-enabled.description": "Whether Discord login is enabled", + "admin.config.oauth.discord-limited-guild": "Discord limited server ID", + "admin.config.oauth.discord-limited-guild.description": "Limit signing in to users in a specific server. Leave ituser. blank to disable.", "admin.config.oauth.discord-client-id": "Discord Client ID", "admin.config.oauth.discord-client-id.description": "Client ID of the Discord OAuth app", "admin.config.oauth.discord-client-secret": "Discord Client secret", @@ -496,10 +498,13 @@ export default { "error.msg.default": "Something went wrong.", "error.msg.access_denied": "You canceled the authentication process, please try again.", "error.msg.expired_token": "The authentication process took too long, please try again.", + "error.msg.invalid_token": "Internal Error", "error.msg.no_user": "User linked to this {0} account doesn't exist.", "error.msg.no_email": "Can't get email address from this {0} account.", "error.msg.already_linked": "This {0} account is already linked to another account.", "error.msg.not_linked": "This {0} account haven't linked to any account yet.", + "error.msg.unverified_account": "This {0} account is unverified, please try again after verification.", + "error.msg.discord_guild_permission_denied": "You are not allowed to sign in.", "error.param.provider_github": "GitHub", "error.param.provider_google": "Google", "error.param.provider_microsoft": "Microsoft", From ce2fb6d19b56ceb454011be39697aa52cb9d2550 Mon Sep 17 00:00:00 2001 From: Qing Fu <16662647+zz5840@users.noreply.github.com> Date: Sun, 26 Nov 2023 17:39:21 +0800 Subject: [PATCH 2/4] fix: typo --- frontend/src/i18n/translations/en-US.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/src/i18n/translations/en-US.ts b/frontend/src/i18n/translations/en-US.ts index 4a357346d..c56cd2baa 100644 --- a/frontend/src/i18n/translations/en-US.ts +++ b/frontend/src/i18n/translations/en-US.ts @@ -473,7 +473,7 @@ export default { "admin.config.oauth.discord-enabled": "Discord", "admin.config.oauth.discord-enabled.description": "Whether Discord login is enabled", "admin.config.oauth.discord-limited-guild": "Discord limited server ID", - "admin.config.oauth.discord-limited-guild.description": "Limit signing in to users in a specific server. Leave ituser. blank to disable.", + "admin.config.oauth.discord-limited-guild.description": "Limit signing in to users in a specific server. Leave it blank to disable.", "admin.config.oauth.discord-client-id": "Discord Client ID", "admin.config.oauth.discord-client-id.description": "Client ID of the Discord OAuth app", "admin.config.oauth.discord-client-secret": "Discord Client secret", From e74b445fdbdba94ab28130ef9ab583292e5b2a77 Mon Sep 17 00:00:00 2001 From: Qing Fu <16662647+zz5840@users.noreply.github.com> Date: Sun, 26 Nov 2023 20:03:34 +0800 Subject: [PATCH 3/4] style: change undefined to optional --- backend/src/oauth/exceptions/errorPage.exception.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/src/oauth/exceptions/errorPage.exception.ts b/backend/src/oauth/exceptions/errorPage.exception.ts index e901cad46..0bfd62a29 100644 --- a/backend/src/oauth/exceptions/errorPage.exception.ts +++ b/backend/src/oauth/exceptions/errorPage.exception.ts @@ -7,7 +7,7 @@ export class ErrorPageException extends Error { */ constructor( public readonly key: string = "default", - public readonly redirect: string = undefined, + public readonly redirect?: string, public readonly params?: string[], ) { super("error"); From 721eda08f0dfb7f37b04b628089b57dabdfdf4f9 Mon Sep 17 00:00:00 2001 From: Qing Fu <16662647+zz5840@users.noreply.github.com> Date: Wed, 29 Nov 2023 17:17:31 +0800 Subject: [PATCH 4/4] style: remove conditional operator --- backend/src/oauth/provider/discord.provider.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/backend/src/oauth/provider/discord.provider.ts b/backend/src/oauth/provider/discord.provider.ts index ea45ba1a8..30bd2398a 100644 --- a/backend/src/oauth/provider/discord.provider.ts +++ b/backend/src/oauth/provider/discord.provider.ts @@ -10,6 +10,10 @@ export class DiscordProvider implements OAuthProvider { constructor(private config: ConfigService) {} getAuthEndpoint(state: string): Promise { + let scope = "identify email"; + if (this.config.get("oauth.discord-limitedGuild")) { + scope += " guilds"; + } return Promise.resolve( "https://discord.com/api/oauth2/authorize?" + new URLSearchParams({ @@ -17,10 +21,8 @@ export class DiscordProvider implements OAuthProvider { redirect_uri: this.config.get("general.appUrl") + "/api/oauth/callback/discord", response_type: "code", - state: state, - scope: - "identify email" + - (this.config.get("oauth.discord-limitedGuild") ? " guilds" : ""), + state, + scope, }).toString(), ); }