From 9c143b4853197f9a323d9df47718e8d769d617c1 Mon Sep 17 00:00:00 2001 From: Harald Schilly Date: Fri, 26 Jul 2024 15:33:46 +0200 Subject: [PATCH 01/10] auth/sso: make it possible to have one SSO for every account --- .../server/auth/sso/check-required-sso.ts | 11 ++++++++++- src/packages/server/auth/sso/passport-login.ts | 18 +++++++++--------- 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/src/packages/server/auth/sso/check-required-sso.ts b/src/packages/server/auth/sso/check-required-sso.ts index 1554a39961..6a92c5cac9 100644 --- a/src/packages/server/auth/sso/check-required-sso.ts +++ b/src/packages/server/auth/sso/check-required-sso.ts @@ -39,9 +39,18 @@ export function getEmailDomain(email: string): string { return email.trim().toLowerCase().split("@")[1]; } +/** + * This checks if the email's domain is either exactly the ssoDomain or a subdomain. + * E.g. for "foo.edu", an email "name@mail.foo.edu" is covered as well. + * + * Special case: an sso domain "*" covers all domains. This is kind of a complete "take over", + * because all accounts on that instance of CoCalc have to go through that SSO mechanism. + * Note: In that case, it makes no sense to have more than one SSO mechanism configured. + */ export function emailBelongsToDomain( emailDomain: string, - ssoDomain: string + ssoDomain: string, ): boolean { + if (ssoDomain === "*") return true; return emailDomain === ssoDomain || emailDomain.endsWith(`.${ssoDomain}`); } diff --git a/src/packages/server/auth/sso/passport-login.ts b/src/packages/server/auth/sso/passport-login.ts index 64f76b7f3c..3147dc35a5 100644 --- a/src/packages/server/auth/sso/passport-login.ts +++ b/src/packages/server/auth/sso/passport-login.ts @@ -22,27 +22,27 @@ import Cookies from "cookies"; import * as _ from "lodash"; import { isEmpty } from "lodash"; +import { REMEMBER_ME_COOKIE_NAME } from "@cocalc/backend/auth/cookie-names"; import base_path from "@cocalc/backend/base-path"; import getLogger from "@cocalc/backend/logger"; import { set_email_address_verified } from "@cocalc/database/postgres/account-queries"; import type { PostgreSQL } from "@cocalc/database/postgres/types"; -import { legacyManageApiKey } from "@cocalc/server/api/manage"; -import generateHash from "@cocalc/server/auth/hash"; -import { REMEMBER_ME_COOKIE_NAME } from "@cocalc/backend/auth/cookie-names"; -import { createRememberMeCookie } from "@cocalc/server/auth/remember-me"; -import { sanitizeID } from "@cocalc/server/auth/sso/sanitize-id"; -import { sanitizeProfile } from "@cocalc/server/auth/sso/sanitize-profile"; import { PassportLoginLocals, PassportLoginOpts, PassportStrategyDB, } from "@cocalc/database/settings/auth-sso-types"; +import getEmailAddress from "@cocalc/server/accounts/get-email-address"; +import isBanned from "@cocalc/server/accounts/is-banned"; +import { legacyManageApiKey } from "@cocalc/server/api/manage"; +import generateHash from "@cocalc/server/auth/hash"; +import { createRememberMeCookie } from "@cocalc/server/auth/remember-me"; +import { sanitizeID } from "@cocalc/server/auth/sso/sanitize-id"; +import { sanitizeProfile } from "@cocalc/server/auth/sso/sanitize-profile"; import { callback2 as cb2 } from "@cocalc/util/async-utils"; import { HELP_EMAIL } from "@cocalc/util/theme"; -import getEmailAddress from "../../accounts/get-email-address"; import { emailBelongsToDomain, getEmailDomain } from "./check-required-sso"; import { SSO_API_KEY_COOKIE_NAME } from "./consts"; -import isBanned from "@cocalc/server/accounts/is-banned"; const logger = getLogger("server:auth:sso:passport-login"); @@ -248,7 +248,7 @@ export class PassportLogin { } // similar to the above, for a specific email address - private checkEmailExclusiveSSO(email_address): boolean { + private checkEmailExclusiveSSO(email_address: string): boolean { const emailDomain = getEmailDomain(email_address.toLocaleLowerCase()); for (const strategyName in this.opts.passports) { const strategy = this.opts.passports[strategyName]; From 932007a83683ca23fd1c2d6e78e1e07a50847a8d Mon Sep 17 00:00:00 2001 From: Harald Schilly Date: Mon, 29 Jul 2024 10:54:17 +0200 Subject: [PATCH 02/10] SSO/update_on_login: change email address if no account with that new email address already exists --- src/packages/database/postgres/types.ts | 1 + .../account/config/account/name.tsx | 3 +- .../next/components/misc/save-button.tsx | 12 +++--- .../server/auth/sso/passport-login.ts | 39 ++++++++++++++----- .../server/auth/sso/unlink-strategy.ts | 6 +-- src/packages/server/auth/throttle.ts | 5 ++- src/packages/util/misc.ts | 4 +- 7 files changed, 46 insertions(+), 24 deletions(-) diff --git a/src/packages/database/postgres/types.ts b/src/packages/database/postgres/types.ts index 31d82ab561..12e9887b84 100644 --- a/src/packages/database/postgres/types.ts +++ b/src/packages/database/postgres/types.ts @@ -92,6 +92,7 @@ export interface UpdateAccountInfoAndPassportOpts { id: string; profile: any; passport_profile: any; + email_address?: string; } export interface PostgreSQL extends EventEmitter { diff --git a/src/packages/next/components/account/config/account/name.tsx b/src/packages/next/components/account/config/account/name.tsx index 19c6fd4fb2..1fd0d9e1e1 100644 --- a/src/packages/next/components/account/config/account/name.tsx +++ b/src/packages/next/components/account/config/account/name.tsx @@ -85,7 +85,7 @@ function ConfigureName() { type="error" showIcon /> - )}{" "} + )} {get.loading ? ( ) : ( @@ -131,7 +131,6 @@ function ConfigureName() { for content you share publicly. {original.name && ( <> - {" "} Setting a username provides optional nicer URL's for shared public documents. Your username can be between 1 and 39 characters, contain upper and lower case letters, numbers, and diff --git a/src/packages/next/components/misc/save-button.tsx b/src/packages/next/components/misc/save-button.tsx index 69d9f15b1d..10e3f782b3 100644 --- a/src/packages/next/components/misc/save-button.tsx +++ b/src/packages/next/components/misc/save-button.tsx @@ -1,12 +1,14 @@ -import { CSSProperties, useEffect, useMemo, useRef, useState } from "react"; -import { cloneDeep, debounce, isEqual } from "lodash"; import { Alert, Button, Space } from "antd"; -import useIsMounted from "lib/hooks/mounted"; +import { cloneDeep, debounce, isEqual } from "lodash"; +import { CSSProperties, useEffect, useMemo, useRef, useState } from "react"; + import Loading from "components/share/loading"; import api from "lib/api/post"; +import useIsMounted from "lib/hooks/mounted"; + import { Icon } from "@cocalc/frontend/components/icon"; -import { SCHEMA } from "@cocalc/util/schema"; import { keys } from "@cocalc/util/misc"; +import { SCHEMA } from "@cocalc/util/schema"; interface Props { edited: any; @@ -119,7 +121,7 @@ export default function SaveButton({ const doSaveDebounced = useMemo( () => debounce(doSave, debounce_ms), - [onSave] + [onSave], ); useEffect(() => { diff --git a/src/packages/server/auth/sso/passport-login.ts b/src/packages/server/auth/sso/passport-login.ts index 3147dc35a5..cde87e7fd2 100644 --- a/src/packages/server/auth/sso/passport-login.ts +++ b/src/packages/server/auth/sso/passport-login.ts @@ -26,7 +26,10 @@ import { REMEMBER_ME_COOKIE_NAME } from "@cocalc/backend/auth/cookie-names"; import base_path from "@cocalc/backend/base-path"; import getLogger from "@cocalc/backend/logger"; import { set_email_address_verified } from "@cocalc/database/postgres/account-queries"; -import type { PostgreSQL } from "@cocalc/database/postgres/types"; +import type { + PostgreSQL, + UpdateAccountInfoAndPassportOpts, +} from "@cocalc/database/postgres/types"; import { PassportLoginLocals, PassportLoginOpts, @@ -40,6 +43,7 @@ import { createRememberMeCookie } from "@cocalc/server/auth/remember-me"; import { sanitizeID } from "@cocalc/server/auth/sso/sanitize-id"; import { sanitizeProfile } from "@cocalc/server/auth/sso/sanitize-profile"; import { callback2 as cb2 } from "@cocalc/util/async-utils"; +import { is_valid_email_address } from "@cocalc/util/misc"; import { HELP_EMAIL } from "@cocalc/util/theme"; import { emailBelongsToDomain, getEmailDomain } from "./check-required-sso"; import { SSO_API_KEY_COOKIE_NAME } from "./consts"; @@ -487,22 +491,37 @@ export class PassportLogin { if (locals.new_account_created || locals.account_id == null) return; const L = logger.extend("maybe_update_account_profile").debug; - // if (opts.emails != null) { - // locals.email_address = opts.emails[0]; - // } - - L(`account exists and we update name of user based on SSO`); - await this.database.update_account_and_passport({ + const upd: UpdateAccountInfoAndPassportOpts = { account_id: locals.account_id, first_name: opts.first_name, last_name: opts.last_name, strategy: opts.strategyName, id: opts.id, profile: opts.profile, - // but not the email address, at least for now - // email_address: locals.email_address, passport_profile: opts.profile, - }); + }; + + if (Array.isArray(opts.emails) && opts.emails.length >= 1) { + locals.email_address = opts.emails[0]; + } + + // We update the email address, if it does not belong to another account. + // Most likely, this just returns the very same account (hence an account exists). + if (is_valid_email_address(locals.email_address)) { + const existing_account_id = await cb2(this.database.account_exists, { + email_address: locals.email_address, + }); + if (!existing_account_id) { + // There is no account with the new email address, hence we can update the email address as well + upd.email_address = locals.email_address; + L( + `No existing account with email address ${locals.email_address} provided by the SSO strategy. Hence we change the email address of account ${locals.account_id} as well.`, + ); + } + } + + L(`account exists and we update name of user based on SSO`); + await this.database.update_account_and_passport(upd); } // There is a special case, where an api_key was requested. diff --git a/src/packages/server/auth/sso/unlink-strategy.ts b/src/packages/server/auth/sso/unlink-strategy.ts index 923b07233e..1e75e791eb 100644 --- a/src/packages/server/auth/sso/unlink-strategy.ts +++ b/src/packages/server/auth/sso/unlink-strategy.ts @@ -10,9 +10,9 @@ upstream SSO provider. */ import getPool from "@cocalc/database/pool"; +import getStrategies from "@cocalc/database/settings/get-sso-strategies"; import { is_valid_uuid_string } from "@cocalc/util/misc"; import { checkRequiredSSO } from "./check-required-sso"; -import getStrategies from "@cocalc/database/settings/get-sso-strategies"; // The name should be something like "google-9999601658192", i.e., a key // of the passports field. @@ -42,7 +42,7 @@ export default async function unlinkStrategy(opts: Options): Promise { // if we can't find the strategy, we still let users unlink it – maybe no longer available? await pool.query( "UPDATE accounts SET passports = passports - $2 WHERE account_id=$1", - [account_id, name] + [account_id, name], ); } @@ -62,7 +62,7 @@ export async function isBlockedUnlinkStrategy(opts: Opts): Promise { const emailQuery = await pool.query( "SELECT email_address FROM accounts WHERE account_id=$1", - [account_id] + [account_id], ); const email = emailQuery.rows[0].email_address; if (email) { diff --git a/src/packages/server/auth/throttle.ts b/src/packages/server/auth/throttle.ts index 7c73830967..e8fab593ae 100644 --- a/src/packages/server/auth/throttle.ts +++ b/src/packages/server/auth/throttle.ts @@ -4,8 +4,9 @@ attacks. This is in memory per-backend server, and doesn't touch the database. */ -import getStrategies from "@cocalc/database/settings/get-sso-strategies"; import LRU from "lru-cache"; + +import getStrategies from "@cocalc/database/settings/get-sso-strategies"; import { checkRequiredSSO } from "./sso/check-required-sso"; const emailShortCache = new LRU({ @@ -30,7 +31,7 @@ async function isExclusiveEmail(email: string) { export async function signInCheck( email: string, ip?: string, - auth_token: boolean = false + auth_token: boolean = false, ): Promise { if ((emailShortCache.get(email) ?? 0) > 5) { // A given email address is allowed at most 5 failed login attempts per minute diff --git a/src/packages/util/misc.ts b/src/packages/util/misc.ts index 236a7ea120..9b0c5a473c 100644 --- a/src/packages/util/misc.ts +++ b/src/packages/util/misc.ts @@ -403,10 +403,10 @@ const reValidEmail = (function () { return new RegExp(sValidEmail); })(); -export function is_valid_email_address(email: string): boolean { +export function is_valid_email_address(email?: unknown): boolean { // From http://stackoverflow.com/questions/46155/validate-email-address-in-javascript // but converted to Javascript; it's near the middle but claims to be exactly RFC822. - if (reValidEmail.test(email)) { + if (typeof email === "string" && reValidEmail.test(email)) { return true; } else { return false; From 0cc199cad4d881cba597331b1f51978a58146a36 Mon Sep 17 00:00:00 2001 From: Harald Schilly Date: Mon, 29 Jul 2024 14:19:05 +0200 Subject: [PATCH 03/10] next/set-email-address: syntax error SQL --- src/packages/server/accounts/set-email-address.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/packages/server/accounts/set-email-address.ts b/src/packages/server/accounts/set-email-address.ts index f2bbee12d0..04ae58bfbc 100644 --- a/src/packages/server/accounts/set-email-address.ts +++ b/src/packages/server/accounts/set-email-address.ts @@ -72,7 +72,7 @@ export default async function setEmailAddress( // user has no password set, so we can set it – but not the email address if (!password_hash) { await pool.query( - "UPDATE accounts SET password_hash=$1, WHERE account_id=$2", + "UPDATE accounts SET password_hash=$1 WHERE account_id=$2", [passwordHash(password), account_id] ); } From 56e723089097ce50a5facdd7dddd8b4bd88c1801 Mon Sep 17 00:00:00 2001 From: Harald Schilly Date: Mon, 29 Jul 2024 15:40:23 +0200 Subject: [PATCH 04/10] sso/update email on login: more debugging and double checking --- .../database/postgres/account-queries.ts | 24 +++------ src/packages/database/postgres/passport.ts | 52 +++++++++++++------ src/packages/database/postgres/types.ts | 8 +++ src/packages/next/pages/sso/index.tsx | 3 +- .../server/auth/sso/passport-login.ts | 19 +++---- src/scripts/auth/gen-sso.py | 4 +- 6 files changed, 62 insertions(+), 48 deletions(-) diff --git a/src/packages/database/postgres/account-queries.ts b/src/packages/database/postgres/account-queries.ts index e1f4147a64..2a26615110 100644 --- a/src/packages/database/postgres/account-queries.ts +++ b/src/packages/database/postgres/account-queries.ts @@ -12,19 +12,19 @@ import { len, } from "@cocalc/util/misc"; import { is_a_site_license_manager } from "./site-license/search"; -import { PostgreSQL } from "./types"; +import { PostgreSQL, SetAccountFields } from "./types"; //import getLogger from "@cocalc/backend/logger"; //const L = getLogger("db:pg:account-queries"); /* For now we define "paying customer" to mean they have a subscription. It's OK if it expired. They at least bought one once. This is mainly used for anti-abuse purposes... - + TODO: modernize this or don't use this at all... */ export async function is_paying_customer( db: PostgreSQL, - account_id: string + account_id: string, ): Promise { let x; try { @@ -44,17 +44,9 @@ export async function is_paying_customer( return await is_a_site_license_manager(db, account_id); } -interface SetAccountFields { - db: PostgreSQL; - account_id: string; - email_address?: string | undefined; - first_name?: string | undefined; - last_name?: string | undefined; -} - // this is like set_account_info_if_different, but only sets the fields if they're not set export async function set_account_info_if_not_set( - opts: SetAccountFields + opts: SetAccountFields, ): Promise { return await set_account_info_if_different(opts, false); } @@ -62,7 +54,7 @@ export async function set_account_info_if_not_set( // This sets the given fields of an account, if it is different from the current value – except for the email address, which we only set but not change export async function set_account_info_if_different( opts: SetAccountFields, - overwrite = true + overwrite = true, ): Promise { const columns = ["email_address", "first_name", "last_name"]; @@ -106,7 +98,7 @@ export async function set_account_info_if_different( export async function set_account( db: PostgreSQL, account_id: string, - set: { [field: string]: any } + set: { [field: string]: any }, ): Promise { await db.async_query({ query: "UPDATE accounts", @@ -118,7 +110,7 @@ export async function set_account( export async function get_account( db: PostgreSQL, account_id: string, - columns: string[] + columns: string[], ): Promise { return await callback2(db.get_account.bind(db), { account_id, @@ -133,7 +125,7 @@ interface SetEmailAddressVerifiedOpts { } export async function set_email_address_verified( - opts: SetEmailAddressVerifiedOpts + opts: SetEmailAddressVerifiedOpts, ): Promise { const { db, account_id, email_address } = opts; assert_valid_account_id(account_id); diff --git a/src/packages/database/postgres/passport.ts b/src/packages/database/postgres/passport.ts index 3ae7b957c0..b80588d267 100644 --- a/src/packages/database/postgres/passport.ts +++ b/src/packages/database/postgres/passport.ts @@ -8,25 +8,27 @@ import { PassportStrategyDB } from "@cocalc/database/settings/auth-sso-types"; import { getPassportsCached, - setPassportsCached + setPassportsCached, } from "@cocalc/database/settings/server-settings"; +import { callback2 as cb2 } from "@cocalc/util/async-utils"; import { to_json } from "@cocalc/util/misc"; import { CB } from "@cocalc/util/types/database"; import { set_account_info_if_different, set_account_info_if_not_set, - set_email_address_verified + set_email_address_verified, } from "./account-queries"; import { CreatePassportOpts, PassportExistsOpts, PostgreSQL, - UpdateAccountInfoAndPassportOpts + SetAccountFields, + UpdateAccountInfoAndPassportOpts, } from "./types"; export async function set_passport_settings( db: PostgreSQL, - opts: PassportStrategyDB & { cb?: CB } + opts: PassportStrategyDB & { cb?: CB }, ): Promise { const { strategy, conf, info } = opts; let err = null; @@ -50,7 +52,7 @@ export async function set_passport_settings( export async function get_passport_settings( db: PostgreSQL, - opts: { strategy: string; cb?: (data: object) => void } + opts: { strategy: string; cb?: (data: object) => void }, ): Promise { const { rows } = await db.async_query({ query: "SELECT conf, info FROM passport_settings", @@ -63,7 +65,7 @@ export async function get_passport_settings( } export async function get_all_passport_settings( - db: PostgreSQL + db: PostgreSQL, ): Promise { return ( await db.async_query({ @@ -73,7 +75,7 @@ export async function get_all_passport_settings( } export async function get_all_passport_settings_cached( - db: PostgreSQL + db: PostgreSQL, ): Promise { const passports = getPassportsCached(); if (passports != null) { @@ -103,7 +105,7 @@ export function _passport_key(opts) { export async function create_passport( db: PostgreSQL, - opts: CreatePassportOpts + opts: CreatePassportOpts, ): Promise { const dbg = db._dbg("create_passport"); dbg({ id: opts.id, strategy: opts.strategy, profile: to_json(opts.profile) }); @@ -121,7 +123,7 @@ export async function create_passport( }); dbg( - `setting other account info ${opts.account_id}: ${opts.email_address}, ${opts.first_name}, ${opts.last_name}` + `setting other account info ${opts.account_id}: ${opts.email_address}, ${opts.first_name}, ${opts.last_name}`, ); await set_account_info_if_not_set({ db: db, @@ -150,7 +152,7 @@ export async function create_passport( export async function passport_exists( db: PostgreSQL, - opts: PassportExistsOpts + opts: PassportExistsOpts, ): Promise { try { const result = await db.async_query({ @@ -178,25 +180,41 @@ export async function passport_exists( export async function update_account_and_passport( db: PostgreSQL, - opts: UpdateAccountInfoAndPassportOpts + opts: UpdateAccountInfoAndPassportOpts, ) { - // we deliberately do not update the email address, because if the SSO - // strategy sends a different one, this would break the "link". - // rather, if the email (and hence most likely the email address) changes on the - // SSO side, this would equal to creating a new account. + // This also updates the email address, if it is set in opts and does not exist with another account yet. + // NOTE: this changed in July 2024. Prior to that, changing the email address of the same account (by ID) in SSO, + // would not change the email address. const dbg = db._dbg("update_account_and_passport"); dbg( `updating account info ${to_json({ first_name: opts.first_name, last_name: opts.last_name, - })}` + email_addres: opts.email_address, + })}`, ); - await set_account_info_if_different({ + + const upd: SetAccountFields = { db: db, account_id: opts.account_id, first_name: opts.first_name, last_name: opts.last_name, + }; + + // Most likely, this just returns the very same account (since the account already exists). + const existing_account_id = await cb2(db.account_exists, { + email_address: opts.email_address, }); + if (!existing_account_id) { + // There is no account with the new email address, hence we can update the email address as well + upd.email_address = opts.email_address; + dbg( + `No existing account with email address ${opts.email_address}. Therefore, we change the email address of account ${opts.account_id} as well.`, + ); + } + + // this set_account_info_if_different checks again if the email exists on another account, but it would throw an error. + await set_account_info_if_different(upd); const key = _passport_key(opts); dbg(`updating passport ${to_json({ key, profile: opts.profile })}`); await db.async_query({ diff --git a/src/packages/database/postgres/types.ts b/src/packages/database/postgres/types.ts index 12e9887b84..d8ba457476 100644 --- a/src/packages/database/postgres/types.ts +++ b/src/packages/database/postgres/types.ts @@ -318,3 +318,11 @@ export interface PostgreSQL extends EventEmitter { }>; }): Promise; } + +export interface SetAccountFields { + db: PostgreSQL; + account_id: string; + email_address?: string | undefined; + first_name?: string | undefined; + last_name?: string | undefined; +} diff --git a/src/packages/next/pages/sso/index.tsx b/src/packages/next/pages/sso/index.tsx index 28c7ca577c..dfe057156b 100644 --- a/src/packages/next/pages/sso/index.tsx +++ b/src/packages/next/pages/sso/index.tsx @@ -32,7 +32,8 @@ export default function SignupIndex(props: Props) { function renderDomains(domains) { if (domains == null || domains.length === 0) return; - return {to_human_list(domains ?? [])}; + const names = (domains ?? []).map((d) => (d === "*" ? "all domains" : d)); + return {to_human_list(names)}; } function extra(sso) { diff --git a/src/packages/server/auth/sso/passport-login.ts b/src/packages/server/auth/sso/passport-login.ts index cde87e7fd2..6074feaacc 100644 --- a/src/packages/server/auth/sso/passport-login.ts +++ b/src/packages/server/auth/sso/passport-login.ts @@ -277,6 +277,7 @@ export class PassportLogin { L( "check to see if the passport already exists indexed by the given id -- in that case we will log user in", ); + // L({ locals }); const passport_account_id = await this.database.passport_exists({ strategy: opts.strategyName, @@ -315,11 +316,12 @@ export class PassportLogin { // user authenticated, passport not known, adding to the user's account await this.createPassport(opts, locals); } else { + L(`passport_account_id=${passport_account_id}`); if ( locals.has_valid_remember_me && locals.account_id !== passport_account_id ) { - L("passport exists but is associated with another account already"); + L("passport exists, but is associated with another account already"); throw Error( `Your ${opts.strategyName} account is already attached to another CoCalc account. First sign into that account and unlink ${opts.strategyName} in account settings, if you want to instead associate it with this account.`, ); @@ -348,6 +350,7 @@ export class PassportLogin { locals: PassportLoginLocals, ): Promise { const L = logger.extend("check_existing_emails").debug; + // L({ locals }); // handle case where passport doesn't exist, but we know one or more email addresses → check for matching email if (locals.account_id != null || opts.emails == null) return; @@ -414,6 +417,7 @@ export class PassportLogin { ): Promise { if (locals.account_id) return; const L = logger.extend("maybe_create_account").debug; + // L({ locals }); L( "no existing account to link, so create new account that can be accessed using this passport", @@ -506,18 +510,9 @@ export class PassportLogin { } // We update the email address, if it does not belong to another account. - // Most likely, this just returns the very same account (hence an account exists). + if (is_valid_email_address(locals.email_address)) { - const existing_account_id = await cb2(this.database.account_exists, { - email_address: locals.email_address, - }); - if (!existing_account_id) { - // There is no account with the new email address, hence we can update the email address as well - upd.email_address = locals.email_address; - L( - `No existing account with email address ${locals.email_address} provided by the SSO strategy. Hence we change the email address of account ${locals.account_id} as well.`, - ); - } + upd.email_address = locals.email_address; } L(`account exists and we update name of user based on SSO`); diff --git a/src/scripts/auth/gen-sso.py b/src/scripts/auth/gen-sso.py index 535eaa3e8c..5d69be67b8 100755 --- a/src/scripts/auth/gen-sso.py +++ b/src/scripts/auth/gen-sso.py @@ -159,7 +159,7 @@ "last_name": "lastName", "full_name": "displayName", "emails": "email", - "id": "email", + "id": "id", }, "issuer": "https://cocalc.com", "cert": saml20cert @@ -170,7 +170,7 @@ "public": False, "display": "Saml20", "description": "Testing my SAML 2.0 IdP", - "exclusive_domains": ["example.com"], + "exclusive_domains": ["*"], "update_on_login": True, "cookie_ttl_s": 24 * 60 * 60, # 24 hours } From 32f69593ac883aa40d0efd89f5082f65f9dbb5cc Mon Sep 17 00:00:00 2001 From: Harald Schilly Date: Mon, 29 Jul 2024 16:45:41 +0200 Subject: [PATCH 05/10] sso/update_on_login: figure out how to use a check_hook --- src/packages/database/settings/auth-sso-types.ts | 2 +- src/packages/util/db-schema/accounts.ts | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/packages/database/settings/auth-sso-types.ts b/src/packages/database/settings/auth-sso-types.ts index c8611fd809..b13209749d 100644 --- a/src/packages/database/settings/auth-sso-types.ts +++ b/src/packages/database/settings/auth-sso-types.ts @@ -127,7 +127,7 @@ export interface PassportStrategyDBConfig { * - description: A longer description of the strategy, could be markdown, shown on the dedicated /sso/... pages. * - icon: A URL to an icon * - disabled: if true, this is ignored during the initialization - * - update_on_login: if true, the user's profile is updated on login (first and last name, not email) + * - update_on_login: if true, the user's profile is updated on login (first and last name, not email) and NOT by the user. * - cookie_ttl_s: how long the remember_me cookied lasts (default is a month or so). * This could be set to a much shorter period to force users more frequently to re-login. */ diff --git a/src/packages/util/db-schema/accounts.ts b/src/packages/util/db-schema/accounts.ts index 61fd6f972c..b21a53396a 100644 --- a/src/packages/util/db-schema/accounts.ts +++ b/src/packages/util/db-schema/accounts.ts @@ -424,6 +424,11 @@ Table({ } } } + // check, if account is exclusively controlled by SSO and its update_on_login config is true + const {email_address} = obj + if (email_address != null) { + // TODO + } cb(); }, }, From 9e5bf9703970317b616d23fe61c05d2bb6465381 Mon Sep 17 00:00:00 2001 From: Harald Schilly Date: Wed, 28 Aug 2024 15:52:54 +0200 Subject: [PATCH 06/10] db/account: refactor checkRequiredSSO and use it to check, if modifying the email_address is allowed or if update_on_login is set, additionally prohibit changing first or last name --- .../database/postgres-server-queries.coffee | 14 +++++++ .../database/postgres/account-queries.ts | 6 ++- src/packages/database/postgres/passport.ts | 14 ++++++- src/packages/database/postgres/types.ts | 3 ++ .../database/settings/get-sso-strategies.ts | 4 +- src/packages/next/components/auth/sso.tsx | 3 +- .../server/accounts/set-email-address.ts | 2 +- .../server/auth/check-email-exclusive-sso.ts | 2 +- .../server/auth/sso/passport-login.ts | 14 ++++--- .../server/auth/sso/unlink-strategy.ts | 2 +- src/packages/server/auth/throttle.ts | 2 +- .../auth-check-required-sso.ts} | 13 +++--- src/packages/util/db-schema/accounts.ts | 40 +++++++++++++++++-- src/packages/util/types/sso.ts | 1 + 14 files changed, 95 insertions(+), 25 deletions(-) rename src/packages/{server/auth/sso/check-required-sso.ts => util/auth-check-required-sso.ts} (96%) diff --git a/src/packages/database/postgres-server-queries.coffee b/src/packages/database/postgres-server-queries.coffee index de60cbdf46..b9886cb8da 100644 --- a/src/packages/database/postgres-server-queries.coffee +++ b/src/packages/database/postgres-server-queries.coffee @@ -61,6 +61,7 @@ read = require('read') {pii_expire} = require("./postgres/pii") passwordHash = require("@cocalc/backend/auth/password-hash").default; registrationTokens = require('./postgres/registration-tokens').default; +getStrategiesSSO = require("@cocalc/database/settings/get-sso-strategies").default; stripe_name = require('@cocalc/util/stripe/name').default; @@ -2647,6 +2648,19 @@ exports.extend_PostgreSQL = (ext) -> class PostgreSQL extends ext return result.rows[0].organization_id return undefined + getStrategiesSSO: () => + return await getStrategiesSSO() + + get_email_address_for_account_id: (account_id) => + result = await @async_query + query : 'SELECT email_address FROM accounts' + where : [ 'account_id :: UUID = $1' ] + params : [ account_id ] + if result.rows.length > 0 + result.rows[0].email_address + else + return undefined + # async registrationTokens: (options, query) => return await registrationTokens(@, options, query) diff --git a/src/packages/database/postgres/account-queries.ts b/src/packages/database/postgres/account-queries.ts index 2a26615110..e4ccd1e49b 100644 --- a/src/packages/database/postgres/account-queries.ts +++ b/src/packages/database/postgres/account-queries.ts @@ -47,7 +47,7 @@ export async function is_paying_customer( // this is like set_account_info_if_different, but only sets the fields if they're not set export async function set_account_info_if_not_set( opts: SetAccountFields, -): Promise { +): Promise<{ email_changed: boolean }> { return await set_account_info_if_different(opts, false); } @@ -55,7 +55,7 @@ export async function set_account_info_if_not_set( export async function set_account_info_if_different( opts: SetAccountFields, overwrite = true, -): Promise { +): Promise<{ email_changed: boolean }> { const columns = ["email_address", "first_name", "last_name"]; // this could throw an error for "no such account" @@ -93,6 +93,8 @@ export async function set_account_info_if_different( account_id: opts.account_id, }); } + + return { email_changed: !!do_email }; } export async function set_account( diff --git a/src/packages/database/postgres/passport.ts b/src/packages/database/postgres/passport.ts index b80588d267..8bc2718068 100644 --- a/src/packages/database/postgres/passport.ts +++ b/src/packages/database/postgres/passport.ts @@ -178,6 +178,7 @@ export async function passport_exists( } } +// this is only used in passport-login/maybeUpdateAccountAndPassport! export async function update_account_and_passport( db: PostgreSQL, opts: UpdateAccountInfoAndPassportOpts, @@ -205,6 +206,7 @@ export async function update_account_and_passport( const existing_account_id = await cb2(db.account_exists, { email_address: opts.email_address, }); + if (!existing_account_id) { // There is no account with the new email address, hence we can update the email address as well upd.email_address = opts.email_address; @@ -214,7 +216,7 @@ export async function update_account_and_passport( } // this set_account_info_if_different checks again if the email exists on another account, but it would throw an error. - await set_account_info_if_different(upd); + const { email_changed } = await set_account_info_if_different(upd); const key = _passport_key(opts); dbg(`updating passport ${to_json({ key, profile: opts.profile })}`); await db.async_query({ @@ -226,4 +228,14 @@ export async function update_account_and_passport( "account_id = $::UUID": opts.account_id, }, }); + + // since we update the email address of an account based on a change from the SSO mechanism + // we can assume the new email address is also "verified" + if (email_changed && typeof upd.email_address === "string") { + await set_email_address_verified({ + db, + account_id: opts.account_id, + email_address: upd.email_address, + }); + } } diff --git a/src/packages/database/postgres/types.ts b/src/packages/database/postgres/types.ts index d8ba457476..faae50ac7b 100644 --- a/src/packages/database/postgres/types.ts +++ b/src/packages/database/postgres/types.ts @@ -14,6 +14,7 @@ import { QueryRows, UntypedQueryResult, } from "@cocalc/util/types/database"; +import type { Strategy } from "@cocalc/util/types/sso"; import { Changes } from "./changefeed"; export type { QueryResult }; @@ -317,6 +318,8 @@ export interface PostgreSQL extends EventEmitter { email_address: string; }>; }): Promise; + + getStrategiesSSO(): Promise; } export interface SetAccountFields { diff --git a/src/packages/database/settings/get-sso-strategies.ts b/src/packages/database/settings/get-sso-strategies.ts index 045c533198..5447b3fa7d 100644 --- a/src/packages/database/settings/get-sso-strategies.ts +++ b/src/packages/database/settings/get-sso-strategies.ts @@ -19,7 +19,8 @@ export default async function getStrategies(): Promise { COALESCE(info -> 'display', conf -> 'display') as display, COALESCE(info -> 'public', conf -> 'public') as public, COALESCE(info -> 'exclusive_domains', conf -> 'exclusive_domains') as exclusive_domains, - COALESCE(info -> 'do_not_hide', 'false'::JSONB) as do_not_hide + COALESCE(info -> 'do_not_hide', 'false'::JSONB) as do_not_hide, + COALESCE(info -> 'update_on_login', 'false'::JSONB) as update_on_login FROM passport_settings WHERE strategy != 'site_conf' @@ -39,6 +40,7 @@ export default async function getStrategies(): Promise { public: row.public ?? true, exclusiveDomains: row.exclusive_domains ?? [], doNotHide: row.do_not_hide ?? false, + updateOnLogin: row.update_on_login ?? false, }; }); } diff --git a/src/packages/next/components/auth/sso.tsx b/src/packages/next/components/auth/sso.tsx index c83b6e5829..7ba333f4e2 100644 --- a/src/packages/next/components/auth/sso.tsx +++ b/src/packages/next/components/auth/sso.tsx @@ -9,7 +9,7 @@ import { join } from "path"; import { CSSProperties, ReactNode, useMemo } from "react"; import { Icon } from "@cocalc/frontend/components/icon"; -import { checkRequiredSSO } from "@cocalc/server/auth/sso/check-required-sso"; +import { checkRequiredSSO } from "@cocalc/util/auth-check-required-sso"; import { PRIMARY_SSO } from "@cocalc/util/types/passport-types"; import { Strategy } from "@cocalc/util/types/sso"; import Loading from "components/share/loading"; @@ -67,6 +67,7 @@ export default function SSO(props: SSOProps) { public: true, exclusiveDomains: [], doNotHide: true, + updateOnLogin: false, }; return ( diff --git a/src/packages/server/accounts/set-email-address.ts b/src/packages/server/accounts/set-email-address.ts index 04ae58bfbc..a2f09a4430 100644 --- a/src/packages/server/accounts/set-email-address.ts +++ b/src/packages/server/accounts/set-email-address.ts @@ -21,7 +21,7 @@ import passwordHash, { verifyPassword, } from "@cocalc/backend/auth/password-hash"; import getPool from "@cocalc/database/pool"; -import { checkRequiredSSO } from "@cocalc/server/auth/sso/check-required-sso"; +import { checkRequiredSSO } from "@cocalc/util/auth-check-required-sso"; import getStrategies from "@cocalc/database/settings/get-sso-strategies"; import { isValidUUID, diff --git a/src/packages/server/auth/check-email-exclusive-sso.ts b/src/packages/server/auth/check-email-exclusive-sso.ts index c8f5654e5c..9bd2fac393 100644 --- a/src/packages/server/auth/check-email-exclusive-sso.ts +++ b/src/packages/server/auth/check-email-exclusive-sso.ts @@ -5,7 +5,7 @@ import { PostgreSQL } from "@cocalc/database/postgres/types"; import getStrategies from "@cocalc/database/settings/get-sso-strategies"; -import { checkRequiredSSO } from "@cocalc/server/auth/sso/check-required-sso"; +import { checkRequiredSSO } from "@cocalc/util/auth-check-required-sso"; export async function checkEmailExclusiveSSO( db: PostgreSQL, diff --git a/src/packages/server/auth/sso/passport-login.ts b/src/packages/server/auth/sso/passport-login.ts index 6074feaacc..214256cdbe 100644 --- a/src/packages/server/auth/sso/passport-login.ts +++ b/src/packages/server/auth/sso/passport-login.ts @@ -13,8 +13,9 @@ * via an SSO strategy, we link this passport to your exsiting account. There is just one exception, * which are SSO strategies which "exclusively" manage a domain. * 2. If you're not signed in and try to sign in, this checks if there is already an account – and creates it if not. - * 3. If you sign in and the SSO strategy is set to "update_on_login", it will reset the name of the user to the - * data from the SSO provider. However, the user can still modify the name. + * 3. If you sign in and the SSO strategy is set to "update_on_login", + * it will reset the name of the user to the data from the SSO provider. + * Users can only modify their first and last name, if that SSO mechanism isn't exclusive! * 4. If you already have an email address belonging to a newly introduced exclusive domain, it will start to be controlled by it. */ @@ -45,8 +46,9 @@ import { sanitizeProfile } from "@cocalc/server/auth/sso/sanitize-profile"; import { callback2 as cb2 } from "@cocalc/util/async-utils"; import { is_valid_email_address } from "@cocalc/util/misc"; import { HELP_EMAIL } from "@cocalc/util/theme"; -import { emailBelongsToDomain, getEmailDomain } from "./check-required-sso"; +import { emailBelongsToDomain } from "@cocalc/util/auth-check-required-sso"; import { SSO_API_KEY_COOKIE_NAME } from "./consts"; +import { getEmailDomain } from "@cocalc/util/auth-check-required-sso"; const logger = getLogger("server:auth:sso:passport-login"); @@ -240,7 +242,7 @@ export class PassportLogin { const exclusiveDomains = strategy.info?.exclusive_domains ?? []; if (!isEmpty(exclusiveDomains)) { for (const email of opts.emails ?? []) { - const emailDomain = getEmailDomain(email.toLocaleLowerCase()); + const emailDomain = getEmailDomain(email.toLowerCase()); for (const ssoDomain of exclusiveDomains) { if (emailBelongsToDomain(emailDomain, ssoDomain)) { return true; @@ -253,7 +255,7 @@ export class PassportLogin { // similar to the above, for a specific email address private checkEmailExclusiveSSO(email_address: string): boolean { - const emailDomain = getEmailDomain(email_address.toLocaleLowerCase()); + const emailDomain = getEmailDomain(email_address.toLowerCase()); for (const strategyName in this.opts.passports) { const strategy = this.opts.passports[strategyName]; for (const ssoDomain of strategy.info?.exclusive_domains ?? []) { @@ -510,7 +512,7 @@ export class PassportLogin { } // We update the email address, if it does not belong to another account. - + if (is_valid_email_address(locals.email_address)) { upd.email_address = locals.email_address; } diff --git a/src/packages/server/auth/sso/unlink-strategy.ts b/src/packages/server/auth/sso/unlink-strategy.ts index 1e75e791eb..af843d8909 100644 --- a/src/packages/server/auth/sso/unlink-strategy.ts +++ b/src/packages/server/auth/sso/unlink-strategy.ts @@ -12,7 +12,7 @@ upstream SSO provider. import getPool from "@cocalc/database/pool"; import getStrategies from "@cocalc/database/settings/get-sso-strategies"; import { is_valid_uuid_string } from "@cocalc/util/misc"; -import { checkRequiredSSO } from "./check-required-sso"; +import { checkRequiredSSO } from "@cocalc/util/auth-check-required-sso"; // The name should be something like "google-9999601658192", i.e., a key // of the passports field. diff --git a/src/packages/server/auth/throttle.ts b/src/packages/server/auth/throttle.ts index e8fab593ae..02bf63b6b7 100644 --- a/src/packages/server/auth/throttle.ts +++ b/src/packages/server/auth/throttle.ts @@ -7,7 +7,7 @@ the database. import LRU from "lru-cache"; import getStrategies from "@cocalc/database/settings/get-sso-strategies"; -import { checkRequiredSSO } from "./sso/check-required-sso"; +import { checkRequiredSSO } from "@cocalc/util/auth-check-required-sso"; const emailShortCache = new LRU({ max: 10000, // avoid memory issues diff --git a/src/packages/server/auth/sso/check-required-sso.ts b/src/packages/util/auth-check-required-sso.ts similarity index 96% rename from src/packages/server/auth/sso/check-required-sso.ts rename to src/packages/util/auth-check-required-sso.ts index 6a92c5cac9..62ed0a76f7 100644 --- a/src/packages/server/auth/sso/check-required-sso.ts +++ b/src/packages/util/auth-check-required-sso.ts @@ -5,18 +5,19 @@ import { Strategy } from "@cocalc/util/types/sso"; -/** - * If the domain of a given email address belongs to an SSO strategy, - * which is configured to be an "exclusive" domain, then return the Strategy. - * This also matches subdomains, i.e. "foo@bar.baz.edu" is goverend by "baz.edu". - */ - interface Opts { email: string | undefined; strategies: Strategy[] | undefined; specificStrategy?: string; } +/** + * If the domain of a given email address belongs to an SSO strategy, + * which is configured to be an "exclusive" domain, then return the Strategy. + * This also matches subdomains, i.e. "foo@bar.baz.edu" is goverend by "baz.edu". + * + * Optionally, if @specificStrategy is set, only that strategy is checked! + */ export function checkRequiredSSO(opts: Opts): Strategy | undefined { const { email, strategies, specificStrategy } = opts; // if the domain of email is contained in any of the strategie's exclusiveDomain array, return that strategy's name diff --git a/src/packages/util/db-schema/accounts.ts b/src/packages/util/db-schema/accounts.ts index eae815df93..31bcb733b7 100644 --- a/src/packages/util/db-schema/accounts.ts +++ b/src/packages/util/db-schema/accounts.ts @@ -15,7 +15,9 @@ import { OTHER_SETTINGS_USERDEFINED_LLM, } from "./defaults"; +import { checkRequiredSSO } from "@cocalc/util/auth-check-required-sso"; import { DEFAULT_LOCALE } from "@cocalc/util/consts/locale"; +import { Strategy } from "@cocalc/util/types/sso"; Table({ name: "accounts", @@ -399,6 +401,7 @@ Table({ // obviously min_balance can't be set! }, async check_hook(db, obj, account_id, _project_id, cb) { + // db is of type PostgreSQL defined in @cocalc/database/postgres/types if (obj["name"] != null) { // NOTE: there is no way to unset/remove a username after one is set... try { @@ -415,6 +418,7 @@ Table({ return; } } + // Hook to truncate some text fields to at most 254 characters, to avoid // further trouble down the line. for (const field of ["first_name", "last_name", "email_address"]) { @@ -427,10 +431,38 @@ Table({ } } } - // check, if account is exclusively controlled by SSO and its update_on_login config is true - const {email_address} = obj - if (email_address != null) { - // TODO + + // if account is exclusively controlled by SSO, you're maybe prohibited from changing account details + const current_email_address = + await db.get_email_address_for_account_id(account_id); + console.log({ current_email_address }); + if (typeof current_email_address === "string") { + const strategies: Strategy[] = await db.getStrategiesSSO(); + const strategy = checkRequiredSSO({ + strategies, + email: current_email_address, + }); + console.log({ strategy }); + console.log(obj); + // we got a required exclusive SSO for the given account_id + if (strategy != null) { + // if user tries to change email_address + if (typeof obj.email_address === "string") { + cb(`You are not allowed to change your email address.`); + return; + } + // ... or tries to change first or last name, but strategy has update_on_login set + if ( + strategy.updateOnLogin && + (typeof obj.first_name === "string" || + obj.last_name === "string") + ) { + cb( + `You are not allowed to change your first or last name. You have to change it at your single-sign-on provider: ${strategy.display}.`, + ); + return; + } + } } cb(); }, diff --git a/src/packages/util/types/sso.ts b/src/packages/util/types/sso.ts index 0cd7a6d016..a3d47bc916 100644 --- a/src/packages/util/types/sso.ts +++ b/src/packages/util/types/sso.ts @@ -12,4 +12,5 @@ export interface Strategy { public: boolean; // true for general broad audiences, like google, default true exclusiveDomains: string[]; // list of domains, e.g. ["foo.com"], which must go through that SSO mechanism (and block regular email signup) doNotHide: boolean; // if true and a public=false, show it directly on the login/signup page + updateOnLogin: boolean; // if true and account is goverend by an exclusiveDomain, user's are not allowed to change their first and last name } From d931e6e59c65d91ade6b7aead4db165d4f9ee3df Mon Sep 17 00:00:00 2001 From: Harald Schilly Date: Thu, 24 Oct 2024 16:39:17 +0200 Subject: [PATCH 07/10] =?UTF-8?q?sso:=20tweak=20the=20checkRequiredSSO=20l?= =?UTF-8?q?ogic=20(match=20"*"=20at=20the=20end,=20if=20no=20others=20matc?= =?UTF-8?q?h)=20=E2=80=93=20and=20write=20tests=20to=20nail=20this=20down?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../server/auth/sso/passport-login.ts | 2 + .../util/auth-check-required-sso.test.ts | 80 +++++++++++++++++++ src/packages/util/auth-check-required-sso.ts | 18 +++-- src/packages/util/types/sso.ts | 2 +- 4 files changed, 95 insertions(+), 7 deletions(-) create mode 100644 src/packages/util/auth-check-required-sso.test.ts diff --git a/src/packages/server/auth/sso/passport-login.ts b/src/packages/server/auth/sso/passport-login.ts index 9f97082498..21fc4cb89f 100644 --- a/src/packages/server/auth/sso/passport-login.ts +++ b/src/packages/server/auth/sso/passport-login.ts @@ -253,6 +253,7 @@ export class PassportLogin { const exclusiveDomains = strategy.info?.exclusive_domains ?? []; if (!isEmpty(exclusiveDomains)) { for (const email of opts.emails ?? []) { + FIX ME const emailDomain = getEmailDomain(email.toLowerCase()); for (const ssoDomain of exclusiveDomains) { if (emailBelongsToDomain(emailDomain, ssoDomain)) { @@ -268,6 +269,7 @@ export class PassportLogin { private checkEmailExclusiveSSO(email_address: string): boolean { const emailDomain = getEmailDomain(email_address.toLowerCase()); for (const strategyName in this.opts.passports) { + FIX ME const strategy = this.opts.passports[strategyName]; for (const ssoDomain of strategy.info?.exclusive_domains ?? []) { if (emailBelongsToDomain(emailDomain, ssoDomain)) { diff --git a/src/packages/util/auth-check-required-sso.test.ts b/src/packages/util/auth-check-required-sso.test.ts new file mode 100644 index 0000000000..bb7db235a3 --- /dev/null +++ b/src/packages/util/auth-check-required-sso.test.ts @@ -0,0 +1,80 @@ +import { + emailBelongsToDomain, + getEmailDomain, + checkRequiredSSO, +} from "./auth-check-required-sso"; +import { Strategy } from "./types/sso"; + +const SSO = { + display: "", + backgroundColor: "", + public: false, + doNotHide: true, + updateOnLogin: true, +} as const; + +describe("Check Required SSO", () => { + test("getEmailDomain", () => { + expect(getEmailDomain("foo@bar.com")).toBe("bar.com"); + expect(getEmailDomain("foo@bar.co.uk")).toBe("bar.co.uk"); + }); + + test("emailBelongsToDomain", () => { + expect(emailBelongsToDomain("foo.com", "foo.com")).toBe(true); + expect(emailBelongsToDomain("bar.foo.com", "foo.com")).toBe(true); + expect(emailBelongsToDomain("foo.com", "bar.com")).toBe(false); + expect(emailBelongsToDomain("foo.com", "foo.co.uk")).toBe(false); + expect(emailBelongsToDomain("foo.com", "foo.com.uk")).toBe(false); + expect(emailBelongsToDomain("foobar.com", "bar.com")).toBe(false); + expect(emailBelongsToDomain("foobar.com", "bar.com")).toBe(false); + expect(emailBelongsToDomain("foobar.com", "*")).toBe(false); + }); + + const foo = { name: "foo", exclusiveDomains: ["foo.co.uk"], ...SSO }; + const bar = { name: "bar", exclusiveDomains: ["*"], ...SSO }; + const baz = { + name: "baz", + exclusiveDomains: ["baz.com", "abc.com"], + ...SSO, + }; + + test("checkRequiredSSO", () => { + const strategies: Strategy[] = [foo, baz] as const; + + expect(checkRequiredSSO({ email: "x@baz.com", strategies })?.name).toEqual( + "baz", + ); + expect( + checkRequiredSSO({ email: "x@foo.abc.com", strategies })?.name, + ).toEqual("baz"); + expect( + checkRequiredSSO({ email: "x@students.foo.co.uk", strategies })?.name, + ).toEqual("foo"); + // no match on naive substring from the right + expect( + checkRequiredSSO({ email: "abc@foobaz.com", strategies }), + ).toBeUndefined(); + // no catch-all for an unrelated domain, returns no strategy + expect( + checkRequiredSSO({ email: "x@gmail.com", strategies }), + ).toBeUndefined(); + }); + + test("checkRequiredSSO/catchall", () => { + const strategies: Strategy[] = [foo, bar, baz] as const; + + expect(checkRequiredSSO({ email: "x@baz.com", strategies })?.name).toEqual( + "baz", + ); + expect( + checkRequiredSSO({ email: "x@foo.abc.com", strategies })?.name, + ).toEqual("baz"); + expect( + checkRequiredSSO({ email: "x@students.foo.co.uk", strategies })?.name, + ).toEqual("foo"); + // this is the essential difference to above + expect( + checkRequiredSSO({ email: "x@gmail.com", strategies })?.name, + ).toEqual("bar"); + }); +}); diff --git a/src/packages/util/auth-check-required-sso.ts b/src/packages/util/auth-check-required-sso.ts index 62ed0a76f7..c186cfc8a7 100644 --- a/src/packages/util/auth-check-required-sso.ts +++ b/src/packages/util/auth-check-required-sso.ts @@ -16,7 +16,11 @@ interface Opts { * which is configured to be an "exclusive" domain, then return the Strategy. * This also matches subdomains, i.e. "foo@bar.baz.edu" is goverend by "baz.edu". * - * Optionally, if @specificStrategy is set, only that strategy is checked! + * Special case: an sso domain "*" covers all domains, not covered by any other + * exclusive SSO strategy. If there is just one such "*"-SSO strategy, it will deal with all + * accounts. + * + * Optionally, if @specificStrategy is set, only that strategy or "*" is checked! */ export function checkRequiredSSO(opts: Opts): Strategy | undefined { const { email, strategies, specificStrategy } = opts; @@ -29,11 +33,18 @@ export function checkRequiredSSO(opts: Opts): Strategy | undefined { for (const strategy of strategies) { if (specificStrategy && specificStrategy !== strategy.name) continue; for (const ssoDomain of strategy.exclusiveDomains) { + if (ssoDomain === "*") continue; // dealt with below if (emailBelongsToDomain(emailDomain, ssoDomain)) { return strategy; } } } + // At this point, we either matched an existing strategy (above) or there is a "*" strategy + for (const strategy of strategies) { + if (strategy.exclusiveDomains.includes("*")) { + return strategy; + } + } } export function getEmailDomain(email: string): string { @@ -43,15 +54,10 @@ export function getEmailDomain(email: string): string { /** * This checks if the email's domain is either exactly the ssoDomain or a subdomain. * E.g. for "foo.edu", an email "name@mail.foo.edu" is covered as well. - * - * Special case: an sso domain "*" covers all domains. This is kind of a complete "take over", - * because all accounts on that instance of CoCalc have to go through that SSO mechanism. - * Note: In that case, it makes no sense to have more than one SSO mechanism configured. */ export function emailBelongsToDomain( emailDomain: string, ssoDomain: string, ): boolean { - if (ssoDomain === "*") return true; return emailDomain === ssoDomain || emailDomain.endsWith(`.${ssoDomain}`); } diff --git a/src/packages/util/types/sso.ts b/src/packages/util/types/sso.ts index a3d47bc916..d927ba43f2 100644 --- a/src/packages/util/types/sso.ts +++ b/src/packages/util/types/sso.ts @@ -10,7 +10,7 @@ export interface Strategy { icon?: string; // name of or URL to icon to display for SSO backgroundColor: string; // background color for icon, if not a link public: boolean; // true for general broad audiences, like google, default true - exclusiveDomains: string[]; // list of domains, e.g. ["foo.com"], which must go through that SSO mechanism (and block regular email signup) + exclusiveDomains: string[]; // list of domains, e.g. ["foo.com"], which must go through that SSO mechanism (and block regular email signup). The domain "*" implies all domains are "taken over" by that startegy – only use that once for one strategy. doNotHide: boolean; // if true and a public=false, show it directly on the login/signup page updateOnLogin: boolean; // if true and account is goverend by an exclusiveDomain, user's are not allowed to change their first and last name } From 66fed2fa25eea12bfe3555b005a15c0b034ef758 Mon Sep 17 00:00:00 2001 From: Harald Schilly Date: Wed, 30 Oct 2024 10:12:07 +0100 Subject: [PATCH 08/10] auth/sso: refactor/ehancing the logic for passport-login checks and removing the auth/is-domain-exclusive-sso.ts logic --- .../next/pages/api/v2/auth/sign-up.ts | 30 ++++++------ src/packages/next/pages/sso/[id].tsx | 4 +- .../server/auth/is-domain-exclusive-sso.ts | 48 ------------------- .../server/auth/sso/passport-login.ts | 39 ++++++++------- src/packages/server/auth/throttle.ts | 4 +- .../util/auth-check-required-sso.test.ts | 2 +- src/packages/util/auth-check-required-sso.ts | 2 +- 7 files changed, 43 insertions(+), 86 deletions(-) delete mode 100644 src/packages/server/auth/is-domain-exclusive-sso.ts diff --git a/src/packages/next/pages/api/v2/auth/sign-up.ts b/src/packages/next/pages/api/v2/auth/sign-up.ts index 362d0d9bb3..f458786e86 100644 --- a/src/packages/next/pages/api/v2/auth/sign-up.ts +++ b/src/packages/next/pages/api/v2/auth/sign-up.ts @@ -15,23 +15,24 @@ Sign up for a new account: 5. Sign user in */ -import { v4 } from "uuid"; import { Request, Response } from "express"; +import { v4 } from "uuid"; + +import { getServerSettings } from "@cocalc/database/settings/server-settings"; +import createAccount from "@cocalc/server/accounts/create-account"; +import isAccountAvailable from "@cocalc/server/auth/is-account-available"; +import passwordStrength from "@cocalc/server/auth/password-strength"; +import reCaptcha from "@cocalc/server/auth/recaptcha"; +import { isExclusiveSSOEmail } from "@cocalc/server/auth/throttle"; +import redeemRegistrationToken from "@cocalc/server/auth/tokens/redeem"; +import sendWelcomeEmail from "@cocalc/server/email/welcome-email"; +import getSiteLicenseId from "@cocalc/server/public-paths/site-license-id"; import { - len, is_valid_email_address as isValidEmailAddress, + len, } from "@cocalc/util/misc"; -import isAccountAvailable from "@cocalc/server/auth/is-account-available"; -import isDomainExclusiveSSO from "@cocalc/server/auth/is-domain-exclusive-sso"; -import createAccount from "@cocalc/server/accounts/create-account"; -import { getAccount, signUserIn } from "./sign-in"; -import sendWelcomeEmail from "@cocalc/server/email/welcome-email"; -import redeemRegistrationToken from "@cocalc/server/auth/tokens/redeem"; -import { getServerSettings } from "@cocalc/database/settings/server-settings"; import getParams from "lib/api/get-params"; -import reCaptcha from "@cocalc/server/auth/recaptcha"; -import getSiteLicenseId from "@cocalc/server/public-paths/site-license-id"; -import passwordStrength from "@cocalc/server/auth/password-strength"; +import { getAccount, signUserIn } from "./sign-in"; interface Issues { terms?: string; @@ -128,11 +129,12 @@ export default async function signUp(req: Request, res: Response) { }); return; } - const exclusive = await isDomainExclusiveSSO(email); + const exclusive = await isExclusiveSSOEmail(email); if (exclusive) { + const name = exclusive.display ?? exclusive.name; res.json({ issues: { - email: `To sign up with "@${exclusive}", you have to use the corresponding single sign on mechanism. Delete your email address above, then click the SSO icon.`, + email: `To sign up with "@${name}", you have to use the corresponding single sign on mechanism. Delete your email address above, then click the SSO icon.`, }, }); return; diff --git a/src/packages/next/pages/sso/[id].tsx b/src/packages/next/pages/sso/[id].tsx index 835fe8ec9a..64889848f4 100644 --- a/src/packages/next/pages/sso/[id].tsx +++ b/src/packages/next/pages/sso/[id].tsx @@ -57,9 +57,9 @@ export default function Signup(props: Props) { {r_human_list( (domains ?? []).map((d) => ( - {d} + {d === "*" ? "all domains" : d} - )) + )), )} ); diff --git a/src/packages/server/auth/is-domain-exclusive-sso.ts b/src/packages/server/auth/is-domain-exclusive-sso.ts deleted file mode 100644 index c1537e4153..0000000000 --- a/src/packages/server/auth/is-domain-exclusive-sso.ts +++ /dev/null @@ -1,48 +0,0 @@ -import getPool from "@cocalc/database/pool"; -import { parseDomain, ParseResultType } from "parse-domain"; - -export default async function isDomainExclusiveSSO( - email_address: string -): Promise { - if (!email_address) { - return; - } - - const raw_domain = email_address.split("@")[1]?.trim().toLowerCase(); - if (!raw_domain) { - return; - } - - const exclusiveDomains = await getExclusiveDomains(); - if (exclusiveDomains.length == 0) { - // For most servers, this is the case. - return; - } - - const parsed = parseDomain(raw_domain); - if (parsed.type != ParseResultType.Listed) { - // Domain not in the public suffix list - return; - } - - const { domain, topLevelDomains } = parsed; - const canonical = [domain ?? "", ...topLevelDomains].join("."); - if (exclusiveDomains.includes(canonical)) { - return canonical; - } -} - -async function getExclusiveDomains(): Promise { - const pool = getPool("minutes"); // exclusive sso is meant for a on prem settings where config RARELY changes. - const { rows } = await pool.query( - "SELECT conf#>'{exclusive_domains}' as exclusive_domains FROM passport_settings" - ); - const v: string[] = []; - for (const row of rows) { - const { exclusive_domains } = row; - if (exclusive_domains) { - v.push(...exclusive_domains); - } - } - return v; -} diff --git a/src/packages/server/auth/sso/passport-login.ts b/src/packages/server/auth/sso/passport-login.ts index 21fc4cb89f..cbc7b0c8fd 100644 --- a/src/packages/server/auth/sso/passport-login.ts +++ b/src/packages/server/auth/sso/passport-login.ts @@ -21,7 +21,6 @@ import Cookies from "cookies"; import * as _ from "lodash"; -import { isEmpty } from "lodash"; import { REMEMBER_ME_COOKIE_NAME } from "@cocalc/backend/auth/cookie-names"; import base_path from "@cocalc/backend/base-path"; @@ -247,34 +246,38 @@ export class PassportLogin { }); } + private emailBelongsToStrategy( + email_address: string, + strategy: PassportStrategyDB, + ): boolean { + const exclusiveDomains = strategy.info?.exclusive_domains ?? []; + const emailDomain = getEmailDomain(email_address.toLowerCase()); + for (const ssoDomain of exclusiveDomains) { + if (ssoDomain === "*" || emailBelongsToDomain(emailDomain, ssoDomain)) { + return true; + } + } + return false; + } + // this checks if the login info contains an email address, which belongs to an exclusive SSO strategy + // this only checks for a single (given) strategy, in the checkPassportExists method. private checkExclusiveSSO(opts: PassportLoginOpts): boolean { const strategy = opts.passports[opts.strategyName]; - const exclusiveDomains = strategy.info?.exclusive_domains ?? []; - if (!isEmpty(exclusiveDomains)) { - for (const email of opts.emails ?? []) { - FIX ME - const emailDomain = getEmailDomain(email.toLowerCase()); - for (const ssoDomain of exclusiveDomains) { - if (emailBelongsToDomain(emailDomain, ssoDomain)) { - return true; - } - } + for (const email_address of opts.emails ?? []) { + if (this.emailBelongsToStrategy(email_address, strategy)) { + return true; } } return false; } - // similar to the above, for a specific email address + // similar to the above, for a specific email address. The difference is, this covers all known strategies. private checkEmailExclusiveSSO(email_address: string): boolean { - const emailDomain = getEmailDomain(email_address.toLowerCase()); for (const strategyName in this.opts.passports) { - FIX ME const strategy = this.opts.passports[strategyName]; - for (const ssoDomain of strategy.info?.exclusive_domains ?? []) { - if (emailBelongsToDomain(emailDomain, ssoDomain)) { - return true; - } + if (this.emailBelongsToStrategy(email_address, strategy)) { + return true; } } return false; diff --git a/src/packages/server/auth/throttle.ts b/src/packages/server/auth/throttle.ts index 02bf63b6b7..0c67a02900 100644 --- a/src/packages/server/auth/throttle.ts +++ b/src/packages/server/auth/throttle.ts @@ -23,7 +23,7 @@ const ipLongCache = new LRU({ ttl: 1000 * 60 * 60, }); -async function isExclusiveEmail(email: string) { +export async function isExclusiveSSOEmail(email: string) { const strategies = await getStrategies(); return checkRequiredSSO({ email, strategies }); } @@ -52,7 +52,7 @@ export async function signInCheck( } // unless user has an auth token, we check if the email address is part of an exclusive SSO mechanism (and block password sign ins) if (!auth_token) { - const exclusiveSSO = await isExclusiveEmail(email); + const exclusiveSSO = await isExclusiveSSOEmail(email); if (exclusiveSSO != null) { const name = exclusiveSSO.display ?? exclusiveSSO.name; return `You have to sign in using the Single-Sign-On mechanism "${name}" of your institution.`; diff --git a/src/packages/util/auth-check-required-sso.test.ts b/src/packages/util/auth-check-required-sso.test.ts index bb7db235a3..27c17f32af 100644 --- a/src/packages/util/auth-check-required-sso.test.ts +++ b/src/packages/util/auth-check-required-sso.test.ts @@ -5,7 +5,7 @@ import { } from "./auth-check-required-sso"; import { Strategy } from "./types/sso"; -const SSO = { +const SSO: Readonly> = { display: "", backgroundColor: "", public: false, diff --git a/src/packages/util/auth-check-required-sso.ts b/src/packages/util/auth-check-required-sso.ts index c186cfc8a7..929c3abe9e 100644 --- a/src/packages/util/auth-check-required-sso.ts +++ b/src/packages/util/auth-check-required-sso.ts @@ -25,7 +25,7 @@ interface Opts { export function checkRequiredSSO(opts: Opts): Strategy | undefined { const { email, strategies, specificStrategy } = opts; // if the domain of email is contained in any of the strategie's exclusiveDomain array, return that strategy's name - if (email == null) return; + if (!email) return; if (strategies == null || strategies.length === 0) return; if (email.indexOf("@") === -1) return; const emailDomain = getEmailDomain(email); From c4fc5936d9c366b21fb573cfeb65bb443ec8922d Mon Sep 17 00:00:00 2001 From: Harald Schilly Date: Wed, 6 Nov 2024 18:31:26 +0100 Subject: [PATCH 09/10] server/db: fix merge issue --- src/packages/database/postgres/passport.ts | 2 +- src/packages/database/postgres/types.ts | 16 ++++++++-------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/packages/database/postgres/passport.ts b/src/packages/database/postgres/passport.ts index 8bc2718068..7d571a3961 100644 --- a/src/packages/database/postgres/passport.ts +++ b/src/packages/database/postgres/passport.ts @@ -126,7 +126,7 @@ export async function create_passport( `setting other account info ${opts.account_id}: ${opts.email_address}, ${opts.first_name}, ${opts.last_name}`, ); await set_account_info_if_not_set({ - db: db, + db, account_id: opts.account_id, email_address: opts.email_address, first_name: opts.first_name, diff --git a/src/packages/database/postgres/types.ts b/src/packages/database/postgres/types.ts index bebb11f5a8..00a9fdb757 100644 --- a/src/packages/database/postgres/types.ts +++ b/src/packages/database/postgres/types.ts @@ -326,14 +326,6 @@ export interface PostgreSQL extends EventEmitter { }): Promise; getStrategiesSSO(): Promise; -} - -export interface SetAccountFields { - db: PostgreSQL; - account_id: string; - email_address?: string | undefined; - first_name?: string | undefined; - last_name?: string | undefined; user_query_cancel_changefeed(opts: { id: any; cb?: CB }): void; @@ -367,5 +359,13 @@ export interface SetAccountFields { ensure_connection_to_project?: (project_id: string, cb?: CB) => Promise; } +export interface SetAccountFields { + db: PostgreSQL; + account_id: string; + email_address?: string | undefined; + first_name?: string | undefined; + last_name?: string | undefined; +} + // This is an extension of BaseProject in projects/control/base.ts type Project = EventEmitter & {}; From 8c68168babe6d51f7a0ee5bb75549179b3144b34 Mon Sep 17 00:00:00 2001 From: Harald Schilly Date: Thu, 14 Nov 2024 15:16:42 +0100 Subject: [PATCH 10/10] check required SSO: ensure we really deal with an email address and test is_valid_email_address thouroughly --- .../util/auth-check-required-sso.test.ts | 5 ++- src/packages/util/auth-check-required-sso.ts | 5 ++- src/packages/util/misc.test.ts | 35 +++++++++++++++++++ 3 files changed, 43 insertions(+), 2 deletions(-) diff --git a/src/packages/util/auth-check-required-sso.test.ts b/src/packages/util/auth-check-required-sso.test.ts index 27c17f32af..ed18506c10 100644 --- a/src/packages/util/auth-check-required-sso.test.ts +++ b/src/packages/util/auth-check-required-sso.test.ts @@ -26,7 +26,7 @@ describe("Check Required SSO", () => { expect(emailBelongsToDomain("foo.com", "foo.co.uk")).toBe(false); expect(emailBelongsToDomain("foo.com", "foo.com.uk")).toBe(false); expect(emailBelongsToDomain("foobar.com", "bar.com")).toBe(false); - expect(emailBelongsToDomain("foobar.com", "bar.com")).toBe(false); + expect(emailBelongsToDomain("foobar.com", "bazfoobar.com")).toBe(false); expect(emailBelongsToDomain("foobar.com", "*")).toBe(false); }); @@ -47,6 +47,9 @@ describe("Check Required SSO", () => { expect( checkRequiredSSO({ email: "x@foo.abc.com", strategies })?.name, ).toEqual("baz"); + expect( + checkRequiredSSO({ email: "instructor+123@foo.co.uk", strategies })?.name, + ).toEqual("foo"); expect( checkRequiredSSO({ email: "x@students.foo.co.uk", strategies })?.name, ).toEqual("foo"); diff --git a/src/packages/util/auth-check-required-sso.ts b/src/packages/util/auth-check-required-sso.ts index 929c3abe9e..b966a856df 100644 --- a/src/packages/util/auth-check-required-sso.ts +++ b/src/packages/util/auth-check-required-sso.ts @@ -3,6 +3,7 @@ * License: MS-RSL – see LICENSE.md for details */ +import { is_valid_email_address } from "@cocalc/util/misc"; import { Strategy } from "@cocalc/util/types/sso"; interface Opts { @@ -14,7 +15,8 @@ interface Opts { /** * If the domain of a given email address belongs to an SSO strategy, * which is configured to be an "exclusive" domain, then return the Strategy. - * This also matches subdomains, i.e. "foo@bar.baz.edu" is goverend by "baz.edu". + * This also matches subdomains, i.e. "foo@bar.baz.edu" is goverend by "baz.edu", + * while "foo@barbaz.edu" is NOT goverend by "baz.edu". * * Special case: an sso domain "*" covers all domains, not covered by any other * exclusive SSO strategy. If there is just one such "*"-SSO strategy, it will deal with all @@ -28,6 +30,7 @@ export function checkRequiredSSO(opts: Opts): Strategy | undefined { if (!email) return; if (strategies == null || strategies.length === 0) return; if (email.indexOf("@") === -1) return; + if (!is_valid_email_address(email)) return; const emailDomain = getEmailDomain(email); if (!emailDomain) return; for (const strategy of strategies) { diff --git a/src/packages/util/misc.test.ts b/src/packages/util/misc.test.ts index f162a5ece5..e3ae929d70 100644 --- a/src/packages/util/misc.test.ts +++ b/src/packages/util/misc.test.ts @@ -343,3 +343,38 @@ describe("suggest_duplicate_filename", () => { expect(dup("asdf-")).toBe("asdf--1"); }); }); + +describe("is_valid email_address", () => { + const ivea = misc.is_valid_email_address; + test("valid", () => { + expect(ivea("foo@bar.com")).toBe(true); + expect(ivea("foo+bar@bar.com")).toBe(true); + expect(ivea("foo.bar@bar.com")).toBe(true); + expect(ivea("foo-bar@bar.com")).toBe(true); + expect(ivea("foo_bar@bar.com")).toBe(true); + expect(ivea("123@bar.com")).toBe(true); + expect(ivea("foo@123.com")).toBe(true); + expect(ivea("foo@bar.co.uk")).toBe(true); + expect(ivea("foobar@bar.com")).toBe(true); + expect(ivea("foo.bar@bar.com")).toBe(true); + expect(ivea("foo+bar@bar.com")).toBe(true); + expect(ivea("FOO@BAR.BAZ")).toBe(true); + }); + test("invalid", () => { + expect(ivea(123)).toBe(false); + expect(ivea({})).toBe(false); + expect(ivea([])).toBe(false); + expect(ivea(null)).toBe(false); + expect(ivea(undefined)).toBe(false); + expect(ivea("abc")).toBe(false); + expect(ivea("abc@foo@bar.com")).toBe(false); + expect(ivea("foo@bar.")).toBe(false); + expect(ivea("foo@.bar.com")).toBe(false); + expect(ivea("foo@bar..com")).toBe(false); + expect(ivea("@bar.com")).toBe(false); + expect(ivea("foo@")).toBe(false); + expect(ivea("foo")).toBe(false); + expect(ivea("foo bar@bar.com")).toBe(false); + expect(ivea("foo@bar@bar.com")).toBe(false); + }); +});