sso/update email on login: more debugging and double checking

Author: haraldschilly

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<boolean> {
   let x;
   try {
@@ -44,25 +44,17 @@ 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<void> {
   return await set_account_info_if_different(opts, false);
 }
 
 // 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<void> {
   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<void> {
   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<void> {
   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<void> {
   const { db, account_id, email_address } = opts;
   assert_valid_account_id(account_id);

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<void> {
   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<any> {
   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<PassportStrategyDB[]> {
   return (
     await db.async_query<PassportStrategyDB>({
@@ -73,7 +75,7 @@ export async function get_all_passport_settings(
 }
 
 export async function get_all_passport_settings_cached(
-  db: PostgreSQL
+  db: PostgreSQL,
 ): Promise<PassportStrategyDB[]> {
   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<void> {
   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<string | undefined> {
   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({

src/packages/database/postgres/types.ts

@@ -318,3 +318,11 @@ export interface PostgreSQL extends EventEmitter {
     }>;
   }): Promise<void>;
 }
+
+export interface SetAccountFields {
+  db: PostgreSQL;
+  account_id: string;
+  email_address?: string | undefined;
+  first_name?: string | undefined;
+  last_name?: string | undefined;
+}

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 <Text type="secondary">{to_human_list(domains ?? [])}</Text>;
+    const names = (domains ?? []).map((d) => (d === "*" ? "all domains" : d));
+    return <Text type="secondary">{to_human_list(names)}</Text>;
   }
 
   function extra(sso) {

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<void> {
     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<void> {
     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`);

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
         }