From 6819261e9bd145c591b00872ddbdcc846e6deb17 Mon Sep 17 00:00:00 2001 From: Neilk1021 Date: Tue, 28 Jul 2026 13:22:01 -0700 Subject: [PATCH 01/54] migration sql script --- sql/updates/29.sql | 168 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 168 insertions(+) create mode 100644 sql/updates/29.sql diff --git a/sql/updates/29.sql b/sql/updates/29.sql new file mode 100644 index 00000000000..f9641a51dcd --- /dev/null +++ b/sql/updates/29.sql @@ -0,0 +1,168 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +\c texera_db + +SET search_path TO texera_db; + +BEGIN; + +DO $$ +BEGIN + IF NOT EXISTS (SELECT 1 FROM pg_type WHERE typname = 'provider_type_enum') THEN +CREATE TYPE provider_type_enum AS ENUM ('LOCAL', 'GOOGLE', 'FACEBOOK'); +END IF; +END +$$; + +-- 2. The auth_provider table. +-- provider_id is the subject id at the provider: the login username for LOCAL, the +-- external id (Google sub, Facebook id) otherwise. It is created nullable here and +-- made NOT NULL at step 6, once existing rows have been backfilled. +CREATE TABLE IF NOT EXISTS auth_provider ( + uid INT NOT NULL, + provider_type provider_type_enum NOT NULL, + provider_id VARCHAR(256), + password VARCHAR(256), -- hashed credential; only for LOCAL + created_at TIMESTAMPTZ NOT NULL DEFAULT now(), + + PRIMARY KEY (uid, provider_type), + FOREIGN KEY (uid) REFERENCES "user"(uid) ON DELETE CASCADE, + + -- one identity per provider; for LOCAL this is what makes the login username unique + CONSTRAINT uq_provider_identity UNIQUE (provider_type, provider_id) + ); + +-- 3. Drop the credential check before touching provider_id. On a database that already +-- ran an earlier version of this migration the old check asserts provider_id IS NULL +-- for LOCAL, which would reject the backfill below. Re-added in its new shape at step 6. +ALTER TABLE auth_provider DROP CONSTRAINT IF EXISTS ck_provider_credential; + +-- 4. Pre-flight. "user".name has never been unique, but it is about to become a unique +-- authentication key, so abort naming the offenders rather than let the backfill die +-- with a bare unique_violation. Handles that are blank or whitespace-padded are +-- rejected too: they are reachable today (register stores the name un-trimmed) and +-- make for handles nobody can type. +DO $$ +DECLARE +offenders TEXT; + orphans TEXT; +BEGIN + IF EXISTS ( + SELECT 1 FROM information_schema.columns + WHERE table_schema = 'texera_db' AND table_name = 'user' AND column_name = 'password' + ) THEN + -- upgrading from the pre-auth_provider schema: handles come straight from "user" +SELECT string_agg(DISTINCT quote_literal(name), ', ') +INTO offenders +FROM "user" +WHERE password IS NOT NULL + AND (btrim(name) = '' OR name <> btrim(name) OR name IN ( + SELECT name FROM "user" WHERE password IS NOT NULL + GROUP BY name HAVING count(*) > 1)); + +SELECT string_agg(uid::TEXT, ', ') +INTO orphans +FROM "user" +WHERE password IS NULL AND google_id IS NULL; +ELSE + -- re-running: an earlier version of this migration created LOCAL rows with no handle +SELECT string_agg(DISTINCT quote_literal(u.name), ', ') +INTO offenders +FROM "user" u + JOIN auth_provider a ON a.uid = u.uid AND a.provider_type = 'LOCAL' +WHERE a.provider_id IS NULL + AND (btrim(u.name) = '' OR u.name <> btrim(u.name) OR u.name IN ( + SELECT u2.name + FROM "user" u2 + JOIN auth_provider a2 ON a2.uid = u2.uid AND a2.provider_type = 'LOCAL' + WHERE a2.provider_id IS NULL + GROUP BY u2.name HAVING count(*) > 1)); +END IF; + + IF offenders IS NOT NULL THEN + RAISE EXCEPTION 'migration 29: cannot promote "user".name to a login handle - ' + 'the following names are duplicated, blank, or whitespace-padded: %. ' + 'Resolve them and re-run.', offenders; +END IF; + + IF orphans IS NOT NULL THEN + RAISE NOTICE 'migration 29: uid(s) % have neither a password nor a google_id, so they ' + 'get no auth_provider row and cannot log in.', orphans; +END IF; +END +$$; + +-- 5. Backfill. +DO $$ +BEGIN + IF EXISTS ( + SELECT 1 FROM information_schema.columns + WHERE table_schema = 'texera_db' AND table_name = 'user' AND column_name = 'password' + ) THEN + INSERT INTO auth_provider (uid, provider_type, provider_id, password) +SELECT uid, 'LOCAL'::provider_type_enum, name, password +FROM "user" +WHERE password IS NOT NULL + ON CONFLICT (uid, provider_type) DO NOTHING; + +INSERT INTO auth_provider (uid, provider_type, provider_id) +SELECT uid, 'GOOGLE'::provider_type_enum, google_id +FROM "user" +WHERE google_id IS NOT NULL + ON CONFLICT (uid, provider_type) DO NOTHING; +END IF; +END +$$; + +-- Fill handles left NULL by an earlier version of this migration; a no-op otherwise. +UPDATE auth_provider a +SET provider_id = u.name + FROM "user" u +WHERE u.uid = a.uid + AND a.provider_type = 'LOCAL' + AND a.provider_id IS NULL; + +-- 6. Every row now has a handle, so make it mandatory and restore the credential check +-- in its new shape: a password exists for LOCAL and only for LOCAL. +ALTER TABLE auth_provider ALTER COLUMN provider_id SET NOT NULL; +ALTER TABLE auth_provider + ADD CONSTRAINT ck_provider_credential CHECK ((provider_type = 'LOCAL') = (password IS NOT NULL)); + +-- Keep the avatar as a provider-neutral profile column on "user" (rename in place). +-- Guarded so it is a no-op on a fresh DB where "user" already has "avatar". +DO $$ +BEGIN + IF EXISTS ( + SELECT 1 FROM information_schema.columns + WHERE table_schema = 'texera_db' AND table_name = 'user' AND column_name = 'google_avatar' + ) AND NOT EXISTS ( + SELECT 1 FROM information_schema.columns + WHERE table_schema = 'texera_db' AND table_name = 'user' AND column_name = 'avatar' + ) THEN +ALTER TABLE "user" RENAME COLUMN google_avatar TO avatar; +END IF; +END +$$; + +ALTER TABLE "user" DROP CONSTRAINT IF EXISTS ck_nulltest; +ALTER TABLE "user" DROP COLUMN IF EXISTS password; +ALTER TABLE "user" DROP COLUMN IF EXISTS google_id; + +COMMIT; \ No newline at end of file From ae02630ceb555fbbe9f24fdf6abd7552e1798523 Mon Sep 17 00:00:00 2001 From: Neilk1021 Date: Tue, 28 Jul 2026 13:29:41 -0700 Subject: [PATCH 02/54] refactor(sql): swap sql tables to add auth_provider table --- sql/changelog.xml | 4 ++ sql/texera_ddl.sql | 23 ++++++++--- sql/updates/29.sql | 100 +++++++++++++++++++-------------------------- 3 files changed, 64 insertions(+), 63 deletions(-) diff --git a/sql/changelog.xml b/sql/changelog.xml index 469868f8958..fb7712fd326 100644 --- a/sql/changelog.xml +++ b/sql/changelog.xml @@ -53,6 +53,10 @@ + + + + - + @@ -74,4 +74,4 @@ --> - \ No newline at end of file + diff --git a/sql/updates/31.sql b/sql/updates/31.sql index 998446701a1..6e6bcf0b688 100644 --- a/sql/updates/31.sql +++ b/sql/updates/31.sql @@ -26,29 +26,30 @@ BEGIN; DO $$ BEGIN IF NOT EXISTS (SELECT 1 FROM pg_type WHERE typname = 'provider_type_enum') THEN -CREATE TYPE provider_type_enum AS ENUM ('LOCAL', 'GOOGLE'); -END IF; + CREATE TYPE provider_type_enum AS ENUM ('LOCAL', 'GOOGLE'); + END IF; END $$; -CREATE TABLE IF NOT EXISTS auth_provider ( - uid INT NOT NULL, - provider_type provider_type_enum NOT NULL, - provider_id VARCHAR(256), - password VARCHAR(256), -- hashed credential; only for LOCAL +-- provider_id is nullable here and tightened to NOT NULL in step 6, once the backfill below +-- has given every row a handle. +CREATE TABLE IF NOT EXISTS auth_provider +( + uid INT NOT NULL, + provider_type provider_type_enum NOT NULL, + provider_id VARCHAR(256), + password VARCHAR(256), -- hashed credential; only for LOCAL created_at TIMESTAMPTZ NOT NULL DEFAULT now(), - PRIMARY KEY (uid, provider_type), FOREIGN KEY (uid) REFERENCES "user"(uid) ON DELETE CASCADE, - CONSTRAINT uq_provider_identity UNIQUE (provider_type, provider_id) - ); +); ALTER TABLE auth_provider DROP CONSTRAINT IF EXISTS ck_provider_credential; DO $$ DECLARE -offenders TEXT; + offenders TEXT; orphans TEXT; BEGIN IF EXISTS ( @@ -56,42 +57,42 @@ BEGIN WHERE table_schema = 'texera_db' AND table_name = 'user' AND column_name = 'password' ) THEN -- upgrading from the pre-auth_provider schema: handles come straight from "user" -SELECT string_agg(DISTINCT quote_literal(name), ', ') -INTO offenders -FROM "user" -WHERE password IS NOT NULL - AND (btrim(name) = '' OR name <> btrim(name) OR name IN ( - SELECT name FROM "user" WHERE password IS NOT NULL - GROUP BY name HAVING count(*) > 1)); - -SELECT string_agg(uid::TEXT, ', ') -INTO orphans -FROM "user" -WHERE password IS NULL AND google_id IS NULL; -ELSE -SELECT string_agg(DISTINCT quote_literal(u.name), ', ') -INTO offenders -FROM "user" u - JOIN auth_provider a ON a.uid = u.uid AND a.provider_type = 'LOCAL' -WHERE a.provider_id IS NULL - AND (btrim(u.name) = '' OR u.name <> btrim(u.name) OR u.name IN ( - SELECT u2.name - FROM "user" u2 - JOIN auth_provider a2 ON a2.uid = u2.uid AND a2.provider_type = 'LOCAL' - WHERE a2.provider_id IS NULL - GROUP BY u2.name HAVING count(*) > 1)); -END IF; + SELECT string_agg(DISTINCT quote_literal(name), ', ') + INTO offenders + FROM "user" + WHERE password IS NOT NULL + AND (btrim(name) = '' OR name <> btrim(name) OR name IN ( + SELECT name FROM "user" WHERE password IS NOT NULL + GROUP BY name HAVING count(*) > 1)); + + SELECT string_agg(uid::TEXT, ', ') + INTO orphans + FROM "user" + WHERE password IS NULL AND google_id IS NULL; + ELSE + SELECT string_agg(DISTINCT quote_literal(u.name), ', ') + INTO offenders + FROM "user" u + JOIN auth_provider a ON a.uid = u.uid AND a.provider_type = 'LOCAL' + WHERE a.provider_id IS NULL + AND (btrim(u.name) = '' OR u.name <> btrim(u.name) OR u.name IN ( + SELECT u2.name + FROM "user" u2 + JOIN auth_provider a2 ON a2.uid = u2.uid AND a2.provider_type = 'LOCAL' + WHERE a2.provider_id IS NULL + GROUP BY u2.name HAVING count(*) > 1)); + END IF; IF offenders IS NOT NULL THEN RAISE EXCEPTION 'migration 31: cannot promote "user".name to a login handle - ' 'the following names are duplicated, blank, or whitespace-padded: %. ' 'Resolve them and re-run.', offenders; -END IF; + END IF; IF orphans IS NOT NULL THEN RAISE NOTICE 'migration 31: uid(s) % have neither a password nor a google_id, so they ' 'get no auth_provider row and cannot log in.', orphans; -END IF; + END IF; END $$; @@ -103,24 +104,24 @@ BEGIN WHERE table_schema = 'texera_db' AND table_name = 'user' AND column_name = 'password' ) THEN INSERT INTO auth_provider (uid, provider_type, provider_id, password) -SELECT uid, 'LOCAL'::provider_type_enum, name, password -FROM "user" -WHERE password IS NOT NULL - ON CONFLICT (uid, provider_type) DO NOTHING; - -INSERT INTO auth_provider (uid, provider_type, provider_id) -SELECT uid, 'GOOGLE'::provider_type_enum, google_id -FROM "user" -WHERE google_id IS NOT NULL - ON CONFLICT (uid, provider_type) DO NOTHING; -END IF; + SELECT uid, 'LOCAL'::provider_type_enum, name, password + FROM "user" + WHERE password IS NOT NULL + ON CONFLICT (uid, provider_type) DO NOTHING; + + INSERT INTO auth_provider (uid, provider_type, provider_id) + SELECT uid, 'GOOGLE'::provider_type_enum, google_id + FROM "user" + WHERE google_id IS NOT NULL + ON CONFLICT (uid, provider_type) DO NOTHING; + END IF; END $$; -- Fill handles left NULL by an earlier version of this migration; a no-op otherwise. UPDATE auth_provider a SET provider_id = u.name - FROM "user" u +FROM "user" u WHERE u.uid = a.uid AND a.provider_type = 'LOCAL' AND a.provider_id IS NULL; @@ -142,8 +143,8 @@ BEGIN SELECT 1 FROM information_schema.columns WHERE table_schema = 'texera_db' AND table_name = 'user' AND column_name = 'avatar' ) THEN -ALTER TABLE "user" RENAME COLUMN google_avatar TO avatar; -END IF; + ALTER TABLE "user" RENAME COLUMN google_avatar TO avatar; + END IF; END $$; @@ -151,4 +152,4 @@ ALTER TABLE "user" DROP CONSTRAINT IF EXISTS ck_nulltest; ALTER TABLE "user" DROP COLUMN IF EXISTS password; ALTER TABLE "user" DROP COLUMN IF EXISTS google_id; -COMMIT; \ No newline at end of file +COMMIT; From ae68cb2eb1ddb11b8ee663ad0628d68e31c4017b Mon Sep 17 00:00:00 2001 From: Neilk1021 Date: Mon, 3 Aug 2026 12:14:39 -0700 Subject: [PATCH 17/54] refactor(auth): move creating a local account to its own helper object. --- .../resource/auth/LocalAuthProvisioner.scala | 114 ++++++++++++++++++ sql/texera_ddl.sql | 7 +- 2 files changed, 118 insertions(+), 3 deletions(-) create mode 100644 amber/src/main/scala/org/apache/texera/web/resource/auth/LocalAuthProvisioner.scala diff --git a/amber/src/main/scala/org/apache/texera/web/resource/auth/LocalAuthProvisioner.scala b/amber/src/main/scala/org/apache/texera/web/resource/auth/LocalAuthProvisioner.scala new file mode 100644 index 00000000000..7ae060031df --- /dev/null +++ b/amber/src/main/scala/org/apache/texera/web/resource/auth/LocalAuthProvisioner.scala @@ -0,0 +1,114 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.web.resource.auth + +import org.apache.texera.dao.SqlServer +import org.apache.texera.dao.jooq.generated.Tables.AUTH_PROVIDER +import org.apache.texera.dao.jooq.generated.enums.{ProviderTypeEnum, UserRoleEnum} +import org.apache.texera.dao.jooq.generated.tables.daos.{AuthProviderDao, UserDao} +import org.apache.texera.dao.jooq.generated.tables.pojos.{AuthProvider, User} +import org.jasypt.util.password.StrongPasswordEncryptor +import org.jooq.exception.DataAccessException + +import javax.ws.rs.WebApplicationException +import javax.ws.rs.core.Response + +/** + * The LOCAL half of authentication: password hashing and the "insert a user together with the + * credential it logs in with" transaction. The counterpart to [[ExternalAuthProvisioner]]. + * + * This exists because self-registration ([[AuthResource]]), admin-created accounts + * (`AdminUserResource`) and the admin bootstrap all need the same two-row insert, and each + * previously carried its own copy plus its own `StrongPasswordEncryptor`. A change to how a + * local credential is stored now lands in one place. + */ +object LocalAuthProvisioner { + + /** Postgres unique-violation SQLSTATE. */ + private val UNIQUE_VIOLATION = "23505" + + private val passwordEncryptor = new StrongPasswordEncryptor + + private def context = SqlServer.getInstance().context + + def hashPassword(rawPassword: String): String = + passwordEncryptor.encryptPassword(rawPassword) + + def checkPassword(rawPassword: String, hashedPassword: String): Boolean = + passwordEncryptor.checkPassword(rawPassword, hashedPassword) + + /** + * Whether `handle` is already taken as a LOCAL login handle. Note this asks + * `auth_provider.provider_id`, not `"user".name` — the display name is mutable and is not + * identity, so it cannot answer this question. + */ + def handleExists(handle: String): Boolean = + context.fetchExists( + context + .selectFrom(AUTH_PROVIDER) + .where(AUTH_PROVIDER.PROVIDER_TYPE.eq(ProviderTypeEnum.LOCAL)) + .and(AUTH_PROVIDER.PROVIDER_ID.eq(handle)) + ) + + /** + * Insert `user` and its LOCAL credential in one transaction, so a user row can never be left + * behind without the credential that makes it usable. `user` is mutated in place with the + * generated uid. + * + * The handle is passed explicitly rather than read off `user.getName`, so that identity is + * never re-derived from the mutable display name. Callers should pre-check with + * [[handleExists]] to report a friendly error; the unique-violation mapping here is the + * race fallback for two registrations of the same handle interleaving. + */ + def createLocalAccount(user: User, handle: String, rawPassword: String): Unit = { + val hashedPassword = hashPassword(rawPassword) + + try { + SqlServer.withTransaction(SqlServer.getInstance().createDSLContext()) { ctx => + val txUserDao = new UserDao(ctx.configuration()) + val txAuthDao = new AuthProviderDao(ctx.configuration()) + + txUserDao.insert(user) + + val auth = new AuthProvider + auth.setUid(user.getUid) + auth.setProviderType(ProviderTypeEnum.LOCAL) + auth.setProviderId(handle) + auth.setPassword(hashedPassword) + txAuthDao.insert(auth) + } + } catch { + case e: DataAccessException if e.sqlState() == UNIQUE_VIOLATION => + throw new WebApplicationException( + s"Login handle $handle is already taken", + e, + Response.Status.CONFLICT + ) + } + } + + /** Create an INACTIVE account whose display name is its login handle. */ + def createLocalAccount(handle: String, rawPassword: String): Unit = { + val user = new User + user.setName(handle) + user.setRole(UserRoleEnum.INACTIVE) + createLocalAccount(user, handle, rawPassword) + } +} diff --git a/sql/texera_ddl.sql b/sql/texera_ddl.sql index 5903cfbf2f5..62993d2beab 100644 --- a/sql/texera_ddl.sql +++ b/sql/texera_ddl.sql @@ -52,6 +52,7 @@ DROP TABLE IF EXISTS operator_port_cache CASCADE; DROP TABLE IF EXISTS workflow_user_access CASCADE; DROP TABLE IF EXISTS workflow_of_user CASCADE; DROP TABLE IF EXISTS user_config CASCADE; +DROP TABLE IF EXISTS auth_provider CASCADE; DROP TABLE IF EXISTS "user" CASCADE; DROP TABLE IF EXISTS user_last_active_time CASCADE; DROP TABLE IF EXISTS workflow CASCADE; @@ -79,7 +80,6 @@ DROP TABLE IF EXISTS computing_unit_user_access CASCADE; DROP TABLE IF EXISTS notebook CASCADE; DROP TABLE IF EXISTS workflow_notebook_mapping CASCADE; DROP TABLE IF EXISTS virtual_environments CASCADE; -DROP TYPE IF EXISTS provider_type_enum CASCADE; -- ============================================ -- 4. Create PostgreSQL enum types @@ -88,6 +88,7 @@ DROP TYPE IF EXISTS provider_type_enum CASCADE; DROP TYPE IF EXISTS user_role_enum CASCADE; DROP TYPE IF EXISTS privilege_enum CASCADE; DROP TYPE IF EXISTS action_enum CASCADE; +DROP TYPE IF EXISTS provider_type_enum CASCADE; CREATE TYPE user_role_enum AS ENUM ('INACTIVE', 'RESTRICTED', 'REGULAR', 'ADMIN'); CREATE TYPE action_enum AS ENUM ('like', 'unlike', 'view', 'clone'); @@ -105,7 +106,7 @@ CREATE TABLE IF NOT EXISTS "user" uid SERIAL PRIMARY KEY, name VARCHAR(256) NOT NULL, email VARCHAR(256) UNIQUE, - avatar VARCHAR(100), + avatar VARCHAR(100), role user_role_enum NOT NULL DEFAULT 'INACTIVE', comment TEXT, account_creation_time TIMESTAMPTZ NOT NULL DEFAULT now(), @@ -118,7 +119,7 @@ CREATE TABLE IF NOT EXISTS auth_provider uid INT NOT NULL, provider_type provider_type_enum NOT NULL, provider_id VARCHAR(256) NOT NULL, - password VARCHAR(256), + password VARCHAR(256), -- hashed credential; only for LOCAL created_at TIMESTAMPTZ NOT NULL DEFAULT now(), PRIMARY KEY (uid, provider_type), FOREIGN KEY (uid) REFERENCES "user"(uid) ON DELETE CASCADE, From f4e2c71ba05dbc652098359be7cff02d64f3044e Mon Sep 17 00:00:00 2001 From: Neilk1021 Date: Mon, 3 Aug 2026 12:22:24 -0700 Subject: [PATCH 18/54] refactor(auth): move creating a local account to its own helper object. --- .../web/resource/auth/AuthResource.scala | 65 +++-------- .../admin/user/AdminUserResource.scala | 58 +++------- .../web/resource/auth/AuthResourceSpec.scala | 26 +++++ .../auth/ExternalAuthProvisionerSpec.scala | 104 ++++++++++++++++-- .../admin/user/AdminUserResourceSpec.scala | 46 ++++++++ .../org/apache/texera/auth/JwtAuth.scala | 19 +++- .../org/apache/texera/auth/JwtParser.scala | 5 +- .../org/apache/texera/auth/JwtAuthSpec.scala | 21 +++- .../apache/texera/auth/JwtParserSpec.scala | 4 +- 9 files changed, 234 insertions(+), 114 deletions(-) rename amber/src/test/scala/org/apache/texera/web/{ => resource}/auth/ExternalAuthProvisionerSpec.scala (72%) diff --git a/amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala b/amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala index dd5c4dce6f7..88dd77310a3 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala @@ -25,12 +25,11 @@ import org.apache.texera.common.config.UserSystemConfig import org.apache.texera.dao.SqlServer import org.apache.texera.dao.jooq.generated.Tables.{AUTH_PROVIDER, USER} import org.apache.texera.dao.jooq.generated.enums.{ProviderTypeEnum, UserRoleEnum} -import org.apache.texera.dao.jooq.generated.tables.daos.{AuthProviderDao, UserDao} -import org.apache.texera.dao.jooq.generated.tables.pojos.{AuthProvider, User} +import org.apache.texera.dao.jooq.generated.tables.daos.UserDao +import org.apache.texera.dao.jooq.generated.tables.pojos.User import org.apache.texera.web.model.http.request.auth.{UserLoginRequest, UserRegistrationRequest} import org.apache.texera.web.model.http.response.TokenIssueResponse import org.apache.texera.web.resource.auth.AuthResource._ -import org.jasypt.util.password.StrongPasswordEncryptor import javax.ws.rs._ import javax.ws.rs.core.MediaType @@ -41,19 +40,6 @@ object AuthResource { private def context = SqlServer.getInstance().context private def userDao = new UserDao(context.configuration) - private val passwordEncryptor = new StrongPasswordEncryptor - - private def localHandleExists(handle: String): Boolean = { - context.fetchExists( - context - .selectFrom(AUTH_PROVIDER) - .where(AUTH_PROVIDER.PROVIDER_TYPE.eq(ProviderTypeEnum.LOCAL)) - .and(AUTH_PROVIDER.PROVIDER_ID.eq(handle)) - ) - } - - //TODO ASSERT THAT ALL USERS WERE MIGRATED CORRECTLY AND CHECK - /** * Retrieve exactly one User from databases with the given username and password. * The password is used to validate against the hashed password stored in the db. @@ -76,7 +62,7 @@ object AuthResource { Option(record).flatMap(r => { val encryptedPassword = r.get(AUTH_PROVIDER.PASSWORD) - if (passwordEncryptor.checkPassword(password, encryptedPassword)) { + if (LocalAuthProvisioner.checkPassword(password, encryptedPassword)) { Some(r.into(USER).into(classOf[User])) } else { None @@ -84,27 +70,6 @@ object AuthResource { }) } - /** - * Create a user together with the LOCAL credential it logs in with. The handle is passed - * explicitly rather than read off `user.getName`, so that identity is never re-derived - * from the mutable display name. - */ - private def insertLocalUser(user: User, handle: String, hashedPassword: String): Unit = { - SqlServer.withTransaction(SqlServer.getInstance().createDSLContext()) { ctx => - val txUserDao = new UserDao(ctx.configuration()) - val txAuthDao = new AuthProviderDao(ctx.configuration()) - - txUserDao.insert(user) - - val auth = new AuthProvider - auth.setUid(user.getUid) - auth.setProviderType(ProviderTypeEnum.LOCAL) - auth.setProviderId(handle) - auth.setPassword(hashedPassword) - txAuthDao.insert(auth) - } - } - def createAdminUser(): Unit = createAdminUser(UserSystemConfig.adminUsername.trim, UserSystemConfig.adminPassword.trim) @@ -116,7 +81,7 @@ object AuthResource { private[auth] def createAdminUser(adminUsername: String, adminPassword: String): Unit = { if (adminUsername.isEmpty || adminPassword.isEmpty) return - if (localHandleExists(adminUsername)) return + if (LocalAuthProvisioner.handleExists(adminUsername)) return if (userDao.fetchOneByEmail(adminUsername) != null) { logger.warn( @@ -131,7 +96,7 @@ object AuthResource { user.setEmail(adminUsername) user.setRole(UserRoleEnum.ADMIN) - insertLocalUser(user, adminUsername, passwordEncryptor.encryptPassword(adminPassword)) + LocalAuthProvisioner.createLocalAccount(user, adminUsername, adminPassword) } } @@ -145,7 +110,11 @@ class AuthResource { def login(request: UserLoginRequest): TokenIssueResponse = { retrieveUserByUsernameAndPassword(request.username, request.password) match { case Some(user) => - TokenIssueResponse(jwtToken(jwtClaims(user, TOKEN_EXPIRE_TIME_IN_MINUTES))) + // An account can hold both a LOCAL and a GOOGLE credential, and the frontend expects + // `googleId` in the token regardless of which one was used to sign in. + val googleId = + ExternalAuthProvisioner.providerIdOf(user.getUid, ProviderTypeEnum.GOOGLE) + TokenIssueResponse(jwtToken(jwtClaims(user, TOKEN_EXPIRE_TIME_IN_MINUTES, googleId))) case None => throw new NotAuthorizedException("Login credentials are incorrect.") } } @@ -165,8 +134,11 @@ class AuthResource { if (userpassword == null || userpassword.isEmpty) throw new NotAcceptableException("Password cannot be empty") - // Check if email already exists - val usernameExists = !userDao.fetchByName(username).isEmpty + // The username being registered becomes a LOCAL login handle, so the handle is what has to + // be free, not the display name. Asking `"user".name` instead both missed genuinely taken + // handles (letting the insert die on uq_provider_identity as a 500) and rejected free ones, + // because an external login rewrites the display name but never the handle. + val usernameExists = LocalAuthProvisioner.handleExists(username) val emailExists = userDao.fetchOneByEmail(useremail) != null (usernameExists, emailExists) match { @@ -179,11 +151,8 @@ class AuthResource { user.setName(username) user.setEmail(useremail) user.setRole(UserRoleEnum.RESTRICTED) - insertLocalUser( - user, - username, - AuthResource.passwordEncryptor.encryptPassword(userpassword) - ) + // Loses the race to a concurrent registration of the same handle as a 409. + LocalAuthProvisioner.createLocalAccount(user, username, userpassword) TokenIssueResponse(jwtToken(jwtClaims(user, TOKEN_EXPIRE_TIME_IN_MINUTES))) } } diff --git a/amber/src/main/scala/org/apache/texera/web/resource/dashboard/admin/user/AdminUserResource.scala b/amber/src/main/scala/org/apache/texera/web/resource/dashboard/admin/user/AdminUserResource.scala index de4f9dd891f..f3e0ba78c54 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/dashboard/admin/user/AdminUserResource.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/dashboard/admin/user/AdminUserResource.scala @@ -24,17 +24,13 @@ import org.apache.texera.dao.jooq.generated.enums.{ProviderTypeEnum, UserRoleEnu import org.apache.texera.dao.jooq.generated.tables.User.USER import org.apache.texera.dao.jooq.generated.tables.AuthProvider.AUTH_PROVIDER import org.apache.texera.dao.jooq.generated.tables.UserLastActiveTime.USER_LAST_ACTIVE_TIME -import org.apache.texera.dao.jooq.generated.tables.daos.{AuthProviderDao, UserDao} -import org.apache.texera.dao.jooq.generated.tables.pojos.{AuthProvider, User} +import org.apache.texera.dao.jooq.generated.tables.daos.UserDao +import org.apache.texera.dao.jooq.generated.tables.pojos.User import org.apache.texera.web.resource.EmailTemplate.createRoleChangeTemplate import org.apache.texera.web.resource.GmailResource.sendEmail -import org.apache.texera.web.resource.dashboard.admin.user.AdminUserResource.{ - passwordEncryptor, - userDao -} +import org.apache.texera.web.resource.auth.LocalAuthProvisioner +import org.apache.texera.web.resource.dashboard.admin.user.AdminUserResource.userDao import org.apache.texera.web.resource.dashboard.user.quota.UserQuotaResource._ -import org.jasypt.util.password.StrongPasswordEncryptor -import org.jooq.exception.DataAccessException import java.util import java.util.UUID @@ -49,7 +45,9 @@ case class UserInfo( googleId: String, localHandle: String, role: UserRoleEnum, - avatar: String, + // `"user".avatar` is no longer Google-specific, but this is the JSON key + // `admin-user.component.html` binds, so the wire name stays until the frontend migrates. + googleAvatar: String, comment: String, lastLogin: java.time.OffsetDateTime, // will be null if never logged in accountCreation: java.time.OffsetDateTime, @@ -63,8 +61,6 @@ object AdminUserResource { .getInstance() .createDSLContext() private def userDao = new UserDao(context.configuration) - private val passwordEncryptor = new StrongPasswordEncryptor - private val UNIQUE_VIOLATION = "23505" } @Path("/admin/user") @@ -89,13 +85,15 @@ class AdminUserResource { USER.UID, USER.NAME, USER.EMAIL, - // Both joins project a column called `provider_id`. fetchInto matches a case class by - // field NAME, so without these aliases the two collide and googleId silently receives - // whichever provider_id the mapper reaches first (the LOCAL handle). + // fetchInto maps onto a Scala case class POSITIONALLY, not by name: a case class has no + // no-arg constructor, so jOOQ falls through to ImmutablePOJOMapper. So the column order + // below must track the UserInfo field order. `last_active_time` landing on `lastLogin` two + // entries down only works because of that. The aliases are documentation (both joins + // project a column called `provider_id`); they do not drive the mapping. googleProvider.PROVIDER_ID.as("googleId"), localProvider.PROVIDER_ID.as("localHandle"), USER.ROLE, - USER.AVATAR, + USER.AVATAR.as("googleAvatar"), USER.COMMENT, USER_LAST_ACTIVE_TIME.LAST_ACTIVE_TIME, USER.ACCOUNT_CREATION_TIME, @@ -148,34 +146,8 @@ class AdminUserResource { * so the collision path is reachable: `addUser` derives its handle from a fresh UUID and * so cannot produce the unique violation this maps to a 409. */ - private[user] def createLocalAccount(handle: String, rawPassword: String): Unit = { - val password = passwordEncryptor.encryptPassword(rawPassword) - - try { - SqlServer.withTransaction(AdminUserResource.context) { ctx => - val txUserDao = new UserDao(ctx.configuration()) - val txAuthDao = new AuthProviderDao(ctx.configuration()) - - val newUser = new User - newUser.setName(handle) - newUser.setRole(UserRoleEnum.INACTIVE) - txUserDao.insert(newUser) - - val newAuth = new AuthProvider() - newAuth.setUid(newUser.getUid) - newAuth.setPassword(password) - newAuth.setProviderType(ProviderTypeEnum.LOCAL) - newAuth.setProviderId(handle) - txAuthDao.insert(newAuth) - } - } catch { - case e: DataAccessException if e.sqlState() == AdminUserResource.UNIQUE_VIOLATION => - throw new WebApplicationException( - new RuntimeException(s"Login handle $handle is already taken", e), - Response.Status.CONFLICT - ) - } - } + private[user] def createLocalAccount(handle: String, rawPassword: String): Unit = + LocalAuthProvisioner.createLocalAccount(handle, rawPassword) @GET @Path("/created_workflows") diff --git a/amber/src/test/scala/org/apache/texera/web/resource/auth/AuthResourceSpec.scala b/amber/src/test/scala/org/apache/texera/web/resource/auth/AuthResourceSpec.scala index c2c99c7fa5e..466033462e8 100644 --- a/amber/src/test/scala/org/apache/texera/web/resource/auth/AuthResourceSpec.scala +++ b/amber/src/test/scala/org/apache/texera/web/resource/auth/AuthResourceSpec.scala @@ -211,6 +211,32 @@ class AuthResourceSpec ex.getMessage should include("Email exists") } + // The taken-handle check has to ask auth_provider, not "user".name: an external login rewrites + // the display name but never the handle, so a name-based guard let the handle through and the + // insert then died on uq_provider_identity as a 500 instead of this 406. + it should "reject a taken handle even when the owner's display name has since drifted" in { + val owner = seedUser(uname("drift"), "pw") + // keep the cleanup prefix so the renamed row is still collected + owner.setName(uname("drift_renamed_by_google")) + userDao.update(owner) + + val ex = intercept[NotAcceptableException]( + resource.register(UserRegistrationRequest(uname("drift"), uemail("drift_other"), "pw2")) + ) + ex.getMessage should include("Username exists") + } + + // The mirror case: a display name that is not a handle must not block registration. + it should "allow a handle that only collides with some other account's display name" in { + val squatter = seedUser(uname("squatter"), "pw") + squatter.setName(uname("wanted")) + userDao.update(squatter) + + val response = + resource.register(UserRegistrationRequest(uname("wanted"), uemail("wanted"), "pw2")) + subjectOf(response.accessToken) shouldBe uname("wanted") + } + // ─── createAdminUser ──────────────────────────────────────────────────────── "createAdminUser" should "insert the configured admin with the ADMIN role and a hashed password" in { diff --git a/amber/src/test/scala/org/apache/texera/web/auth/ExternalAuthProvisionerSpec.scala b/amber/src/test/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisionerSpec.scala similarity index 72% rename from amber/src/test/scala/org/apache/texera/web/auth/ExternalAuthProvisionerSpec.scala rename to amber/src/test/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisionerSpec.scala index 8603069996e..8b83c979072 100644 --- a/amber/src/test/scala/org/apache/texera/web/auth/ExternalAuthProvisionerSpec.scala +++ b/amber/src/test/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisionerSpec.scala @@ -28,6 +28,8 @@ import org.scalatest.flatspec.AnyFlatSpec import org.scalatest.matchers.should.Matchers import org.scalatest.{BeforeAndAfterAll, BeforeAndAfterEach} +import javax.ws.rs.NotAuthorizedException + /** * Integration spec for [[ExternalAuthProvisioner]] against embedded Postgres * ([[MockTexeraDB]] loads the real `texera_ddl.sql`, so the `auth_provider` table and @@ -107,7 +109,8 @@ class ExternalAuthProvisionerSpec "google-sub-1", "New User", "new" + emailDomain, - Some("avatar1") + emailVerified = true, + avatar = Some("avatar1") ) ) @@ -130,7 +133,8 @@ class ExternalAuthProvisionerSpec "google-sub-return", "Ret", "ret" + emailDomain, - Some("a") + emailVerified = true, + avatar = Some("a") ) val first = ExternalAuthProvisioner.loginOrProvision(profile) @@ -141,14 +145,18 @@ class ExternalAuthProvisionerSpec userCountByEmail("ret") shouldBe 1 } - it should "refresh drifted profile fields for a known identity" in { + // The display name is the user's to edit and is not identity — the login handle lives in + // auth_provider.provider_id. Re-deriving it from the provider on every login silently reverted + // any rename made in Texera, so refresh leaves it alone and only the avatar follows the drift. + it should "refresh the avatar but keep the local display name for a known identity" in { ExternalAuthProvisioner.loginOrProvision( ExternalProfile( ProviderTypeEnum.GOOGLE, "sub-drift", "Old Name", "drift" + emailDomain, - Some("oldpic") + emailVerified = true, + avatar = Some("oldpic") ) ) val updated = ExternalAuthProvisioner.loginOrProvision( @@ -157,17 +165,84 @@ class ExternalAuthProvisionerSpec "sub-drift", "New Name", "drift" + emailDomain, - Some("newpic") + emailVerified = true, + avatar = Some("newpic") ) ) - updated.getName shouldBe "New Name" + updated.getName shouldBe "Old Name" updated.getAvatar shouldBe "newpic" // confirm it persisted, not just mutated in memory - userDao.fetchOneByUid(updated.getUid).getName shouldBe "New Name" + userDao.fetchOneByUid(updated.getUid).getName shouldBe "Old Name" userDao.fetchOneByUid(updated.getUid).getAvatar shouldBe "newpic" } + // ---- unverified email ------------------------------------------------------ + + // The email address is the only thing tying a first-time external identity to an account, so + // an unverified one must not be able to link onto — or squat on — someone else's row. The + // rejection reuses the generic credential error so it cannot be used to enumerate addresses. + it should "refuse to link a first-time identity onto an existing account on an unverified email" in { + val existing = seedUser("Victim", "victim") + + assertThrows[NotAuthorizedException] { + ExternalAuthProvisioner.loginOrProvision( + ExternalProfile( + ProviderTypeEnum.GOOGLE, + "sub-attacker", + "Attacker", + "victim" + emailDomain, + emailVerified = false + ) + ) + } + + providerRowCount(existing.getUid) shouldBe 0 + userDao.fetchOneByUid(existing.getUid).getName shouldBe "Victim" + } + + it should "refuse to provision a brand-new account on an unverified email" in { + assertThrows[NotAuthorizedException] { + ExternalAuthProvisioner.loginOrProvision( + ExternalProfile( + ProviderTypeEnum.GOOGLE, + "sub-unverified", + "Unverified", + "unverified" + emailDomain, + emailVerified = false + ) + ) + } + + userCountByEmail("unverified") shouldBe 0 + } + + // A provider that stops vouching for the address must not be able to move it either. + it should "not adopt an unverified email for a known identity" in { + val created = ExternalAuthProvisioner.loginOrProvision( + ExternalProfile( + ProviderTypeEnum.GOOGLE, + "sub-unverified-drift", + "Drifter", + "verified" + emailDomain, + emailVerified = true + ) + ) + + ExternalAuthProvisioner.loginOrProvision( + ExternalProfile( + ProviderTypeEnum.GOOGLE, + "sub-unverified-drift", + "Drifter", + "hijack" + emailDomain, + emailVerified = false + ) + ) + + userDao.fetchOneByUid(created.getUid).getEmail shouldBe "verified" + emailDomain + userCountByEmail("hijack") shouldBe 0 + } + it should "adopt the provider's new email address for a known identity" in { val created = ExternalAuthProvisioner.loginOrProvision( ExternalProfile( @@ -175,7 +250,8 @@ class ExternalAuthProvisionerSpec "sub-rename", "Renamer", "before" + emailDomain, - Some("pic") + emailVerified = true, + avatar = Some("pic") ) ) @@ -185,7 +261,8 @@ class ExternalAuthProvisionerSpec "sub-rename", "Renamer", "after" + emailDomain, - Some("pic") + emailVerified = true, + avatar = Some("pic") ) ) @@ -205,7 +282,8 @@ class ExternalAuthProvisionerSpec "sub-keeper", "Keeper", "keeper" + emailDomain, - None + emailVerified = true, + avatar = None ) ) @@ -225,7 +303,8 @@ class ExternalAuthProvisionerSpec "sub-link", "Local User", "linkme" + emailDomain, - Some("pic") + emailVerified = true, + avatar = Some("pic") ) ) @@ -246,7 +325,8 @@ class ExternalAuthProvisionerSpec "new-sub", "Rotating", "rotate" + emailDomain, - Some("p") + emailVerified = true, + avatar = Some("p") ) ) diff --git a/amber/src/test/scala/org/apache/texera/web/resource/dashboard/admin/user/AdminUserResourceSpec.scala b/amber/src/test/scala/org/apache/texera/web/resource/dashboard/admin/user/AdminUserResourceSpec.scala index cc2206cd26c..e1c72341af5 100644 --- a/amber/src/test/scala/org/apache/texera/web/resource/dashboard/admin/user/AdminUserResourceSpec.scala +++ b/amber/src/test/scala/org/apache/texera/web/resource/dashboard/admin/user/AdminUserResourceSpec.scala @@ -111,6 +111,25 @@ class AdminUserResourceSpec user } + /** + * Seed a credential row. `password` is left null for external providers because + * ck_provider_credential requires a password for LOCAL and only for LOCAL. The + * auth_provider FK is ON DELETE CASCADE, so `cleanup`'s user delete clears these. + */ + private def seedProvider( + uid: Int, + providerType: ProviderTypeEnum, + providerId: String, + password: String = null + ): Unit = + getDSLContext + .insertInto(AUTH_PROVIDER) + .set(AUTH_PROVIDER.UID, Integer.valueOf(uid)) + .set(AUTH_PROVIDER.PROVIDER_TYPE, providerType) + .set(AUTH_PROVIDER.PROVIDER_ID, providerId) + .set(AUTH_PROVIDER.PASSWORD, password) + .execute() + private def seedWorkflow(): Workflow = { val workflow = new Workflow workflow.setWid(testWid) @@ -169,6 +188,33 @@ class AdminUserResourceSpec resource.list().asScala.exists(_.uid == primaryUid) shouldBe false } + // The projection maps onto UserInfo positionally, and it left-joins auth_provider twice for + // the two credential kinds. Nothing else observes that the columns land on the fields they + // are meant to, which is the whole risk of a positional mapping — so pin it here for a user + // holding both credentials at once. + it should "report both credential handles and the avatar for a user with LOCAL and GOOGLE rows" in { + val user = makeUser(primaryUid, "dual") + user.setAvatar("avatar-blob") + userDao.insert(user) + seedProvider(primaryUid, ProviderTypeEnum.LOCAL, "dual-handle", password = "hashed") + seedProvider(primaryUid, ProviderTypeEnum.GOOGLE, "google-sub-dual") + + val listed = resource.list().asScala.find(_.uid == primaryUid) + + listed.map(u => (u.name, u.localHandle, u.googleId, u.googleAvatar)) shouldBe Some( + ("dual", "dual-handle", "google-sub-dual", "avatar-blob") + ) + } + + it should "leave the credential handles null for a user with no auth_provider rows" in { + userDao.insert(makeUser(primaryUid, "credential-less")) + + val listed = resource.list().asScala.find(_.uid == primaryUid) + + listed.map(_.localHandle) shouldBe Some(null) + listed.map(_.googleId) shouldBe Some(null) + } + // ─── addUser ──────────────────────────────────────────────────────────── "addUser" should "persist a new INACTIVE user" in { diff --git a/common/auth/src/main/scala/org/apache/texera/auth/JwtAuth.scala b/common/auth/src/main/scala/org/apache/texera/auth/JwtAuth.scala index 5b3364b69ac..df6b77c5288 100644 --- a/common/auth/src/main/scala/org/apache/texera/auth/JwtAuth.scala +++ b/common/auth/src/main/scala/org/apache/texera/auth/JwtAuth.scala @@ -51,13 +51,28 @@ object JwtAuth { jws.getCompactSerialization } - def jwtClaims(user: User, expireInDays: Int): JwtClaims = { + /** + * Build the claim set for `user`. The claim names are a contract with the hand-written + * TypeScript reader in `frontend/src/app/common/service/user/auth.service.ts`, which is not + * compiled against this file — so renaming one here silently breaks the frontend. `avatar` + * now lives on `"user"` rather than a Google-specific column, but the claim keeps its + * `googleAvatar` name until the frontend is migrated in lockstep. + * + * `googleId` is passed in rather than read off `user`, because the GOOGLE provider id lives + * in `auth_provider` and this module must stay DB-free: the four specs in + * `access-control-service` / `config-service` and the token re-issue paths in + * `ResultExportService` / `ComputingUnitManagingResource` all call this with no + * `auth_provider` context. Those re-issued tokens are service-to-service and never reach + * the browser, so omitting the claim there is harmless. + */ + def jwtClaims(user: User, expireInDays: Int, googleId: Option[String] = None): JwtClaims = { val claims = new JwtClaims claims.setSubject(user.getName) claims.setClaim("userId", user.getUid) claims.setClaim("email", user.getEmail) claims.setClaim("role", user.getRole) - claims.setClaim("avatar", user.getAvatar) + claims.setClaim("googleAvatar", user.getAvatar) + googleId.foreach(claims.setClaim("googleId", _)) claims.setExpirationTimeMinutesInTheFuture(TOKEN_EXPIRE_TIME_IN_MINUTES.toFloat) claims } diff --git a/common/auth/src/main/scala/org/apache/texera/auth/JwtParser.scala b/common/auth/src/main/scala/org/apache/texera/auth/JwtParser.scala index bdda679ae87..6cd540c2851 100644 --- a/common/auth/src/main/scala/org/apache/texera/auth/JwtParser.scala +++ b/common/auth/src/main/scala/org/apache/texera/auth/JwtParser.scala @@ -62,7 +62,10 @@ object JwtParser extends LazyLogging { // call writes Integer; widen via Number to handle both cases. val userId = claims.getClaimValue("userId", classOf[Number]).intValue() val role = UserRoleEnum.valueOf(claims.getClaimValue("role").asInstanceOf[String]) - val googleAvatar = claims.getClaimValue("avatar", classOf[String]) + val googleAvatar = claims.getClaimValue("googleAvatar", classOf[String]) + // The `googleId` claim is deliberately written but not read back: nothing server-side + // needs it (credentials live in auth_provider), and the only consumer is the frontend, + // which reads it straight off the raw token. new SessionUser( new User().tap { user => diff --git a/common/auth/src/test/scala/org/apache/texera/auth/JwtAuthSpec.scala b/common/auth/src/test/scala/org/apache/texera/auth/JwtAuthSpec.scala index 27a25abd0fa..d4d44db345f 100644 --- a/common/auth/src/test/scala/org/apache/texera/auth/JwtAuthSpec.scala +++ b/common/auth/src/test/scala/org/apache/texera/auth/JwtAuthSpec.scala @@ -43,20 +43,29 @@ class JwtAuthSpec extends AnyFlatSpec with Matchers { claims.getSubject shouldBe "alice" claims.getClaimValueAsString("userId") shouldBe "42" claims.getClaimValueAsString("email") shouldBe "alice@example.com" - claims.getClaimValueAsString("avatar") shouldBe "avatar-blob" + claims.getClaimValueAsString("googleAvatar") shouldBe "avatar-blob" claims.getClaimValueAsString("role") shouldBe UserRoleEnum.ADMIN.name } - // Credentials now live in auth_provider, and the token deliberately carries none of them: - // it identifies the user, it does not re-present the identity that authenticated them. - it should "not carry any provider credential in the claims" in { + // Passwords live in auth_provider and never leave it. `googleId` is a different matter: it is + // an identifier, not a credential, and the frontend still reads it off the token. + it should "not carry any password in the claims" in { val claims = JwtAuth.jwtClaims(buildUser(), 7) - claims.hasClaim("googleId") shouldBe false - claims.hasClaim("googleAvatar") shouldBe false claims.hasClaim("password") shouldBe false claims.hasClaim("providerId") shouldBe false } + // The GOOGLE provider id is not on the User pojo any more, so callers with no auth_provider + // context (service-to-service token re-issue) simply omit it rather than writing null. + it should "omit the googleId claim when no provider id is supplied" in { + JwtAuth.jwtClaims(buildUser(), 7).hasClaim("googleId") shouldBe false + } + + it should "carry the googleId claim when a provider id is supplied" in { + val claims = JwtAuth.jwtClaims(buildUser(), 7, Some("google-sub-123")) + claims.getClaimValueAsString("googleId") shouldBe "google-sub-123" + } + it should "derive the expiration from config, ignoring the expireInDays argument" in { // two very different expireInDays values must yield the same config-derived expiry window def expiryWindowMinutes(expireInDays: Int): Double = { diff --git a/common/auth/src/test/scala/org/apache/texera/auth/JwtParserSpec.scala b/common/auth/src/test/scala/org/apache/texera/auth/JwtParserSpec.scala index ca979147c62..f18185aad8e 100644 --- a/common/auth/src/test/scala/org/apache/texera/auth/JwtParserSpec.scala +++ b/common/auth/src/test/scala/org/apache/texera/auth/JwtParserSpec.scala @@ -40,7 +40,7 @@ class JwtParserSpec extends AnyFlatSpec with Matchers { claims.setClaim("userId", 42) claims.setClaim("email", "alice@example.com") claims.setClaim("role", UserRoleEnum.ADMIN.name) - claims.setClaim("avatar", "avatar-blob") + claims.setClaim("googleAvatar", "avatar-blob") claims.setExpirationTimeMinutesInTheFuture(10f) claims } @@ -158,7 +158,7 @@ class JwtParserSpec extends AnyFlatSpec with Matchers { bob.setClaim("userId", 7) bob.setClaim("email", "bob@example.com") bob.setClaim("role", UserRoleEnum.REGULAR.name) - bob.setClaim("avatar", "bob-avatar") + bob.setClaim("googleAvatar", "bob-avatar") bob.setExpirationTimeMinutesInTheFuture(10f) val aliceUser = JwtParser.parseToken(JwtAuth.jwtToken(alice)).get().getUser From 6102a002f174962bb2af1acaee71e9ecea6d3ab5 Mon Sep 17 00:00:00 2001 From: Neilk1021 Date: Mon, 3 Aug 2026 12:23:31 -0700 Subject: [PATCH 19/54] fix(auth): verify email for external users and readd google id --- .../auth/ExternalAuthProvisioner.scala | 54 ++++++++++++++++--- .../resource/auth/GoogleAuthResource.scala | 21 +++++--- .../web/auth/UserAuthenticatorSpec.scala | 2 +- .../auth/GoogleAuthResourceSpec.scala | 33 ++++++++++-- .../WorkflowExecutionsResourceSpec.scala | 16 ++++++ 5 files changed, 105 insertions(+), 21 deletions(-) diff --git a/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala b/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala index 71235669859..daaa2bc527c 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala @@ -1,5 +1,3 @@ -package org.apache.texera.web.resource.auth - /* * Licensed to the Apache Software Foundation (ASF) under one * or more contributor license agreements. See the NOTICE file @@ -19,6 +17,9 @@ package org.apache.texera.web.resource.auth * under the License. */ +package org.apache.texera.web.resource.auth + +import com.typesafe.scalalogging.LazyLogging import org.apache.texera.dao.SqlServer import org.apache.texera.dao.jooq.generated.Tables.{AUTH_PROVIDER, USER} import org.apache.texera.dao.jooq.generated.enums.{ProviderTypeEnum, UserRoleEnum} @@ -27,22 +28,29 @@ import org.apache.texera.dao.jooq.generated.tables.pojos.{AuthProvider, User} import org.jooq.DSLContext import java.time.OffsetDateTime +import javax.ws.rs.NotAuthorizedException import scala.util.chaining.scalaUtilChainingOps /** * A verified external identity (Google, Facebook, ...) reduced to the fields we * persist. `avatar` is optional: `None` means the provider supplies no avatar, so * the user's existing avatar column is left untouched rather than overwritten. + * + * `emailVerified` reports whether the provider itself vouches for `email`. It has no + * default on purpose: an email address is what links an external identity to an + * existing account, so treating an unverified one as trusted is an account-takeover + * path, and a defaulted flag is how that mistake comes back. */ final case class ExternalProfile( providerType: ProviderTypeEnum, providerId: String, name: String, email: String, + emailVerified: Boolean, avatar: Option[String] = None ) -object ExternalAuthProvisioner { +object ExternalAuthProvisioner extends LazyLogging { /** * Resolve the user behind an external identity, creating one if necessary, and @@ -71,6 +79,20 @@ object ExternalAuthProvisioner { } case None => + // First time we have seen this identity, so the email address is the only thing + // tying it to an account. It is either an existing one to link onto, or a new row that + // claims the address. Trusting an unverified address for that lets anyone who can + // mint an `email` claim take over, or squat on, someone else's account. The error + // is deliberately the same one a bad credential yields, so this does not become an + // oracle for which addresses are registered. + if (!profile.emailVerified) { + logger.warn( + s"Refusing to provision ${profile.providerType} identity ${profile.providerId}: " + + "the provider did not verify its email address." + ) + throw new NotAuthorizedException("Login credentials are incorrect.") + } + val user = Option(txUserDao.fetchOneByEmail(profile.email)) match { case Some(existing) => existing.tap { user => @@ -91,17 +113,33 @@ object ExternalAuthProvisioner { } } + /** The external id `uid` authenticates with at `providerType`, if it has one. */ + def providerIdOf(uid: Integer, providerType: ProviderTypeEnum): Option[String] = + Option( + SqlServer + .getInstance() + .context + .select(AUTH_PROVIDER.PROVIDER_ID) + .from(AUTH_PROVIDER) + .where(AUTH_PROVIDER.UID.eq(uid)) + .and(AUTH_PROVIDER.PROVIDER_TYPE.eq(providerType)) + .fetchOne(AUTH_PROVIDER.PROVIDER_ID) + ) + /** * Mutate `user` in place to match `profile`, returning true iff anything changed * (so the caller only issues an UPDATE when needed). + * + * `name` is deliberately not refreshed. It is the display name the user owns and may + * have edited in Texera, and it is not identity — the login handle lives in + * `auth_provider.provider_id`. Re-deriving it from the provider on every login silently + * reverted such edits. */ private def refresh(user: User, profile: ExternalProfile): Boolean = { var changed = false - if (user.getName != profile.name) { - user.setName(profile.name) - changed = true - } - if (user.getEmail != profile.email) { + // Same reasoning as the link path: only a provider-verified address may move the + // column that identifies the account. + if (profile.emailVerified && user.getEmail != profile.email) { user.setEmail(profile.email) changed = true } diff --git a/amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleAuthResource.scala b/amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleAuthResource.scala index 1b6f085da95..fa6d418fd04 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleAuthResource.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleAuthResource.scala @@ -37,7 +37,7 @@ object GoogleAuthResource { * Reduce a verified Google id-token payload to the fields we persist. Google omits `name` * for accounts with no profile name, and the provisioner writes `name` straight to a NOT * NULL column, so the address stands in for it. Only the last path segment of `picture` is - * kept — the frontend rebuilds the full `lh3.googleusercontent.com` URL around it. + * kept. The frontend rebuilds the full `lh3.googleusercontent.com` URL around it. */ private[auth] def profileOf(payload: GoogleIdToken.Payload): ExternalProfile = { val googleEmail = payload.getEmail @@ -46,11 +46,11 @@ object GoogleAuthResource { payload.getSubject, Option(payload.get("name").asInstanceOf[String]).filter(_.nonEmpty).getOrElse(googleEmail), googleEmail, - Some( - Option(payload.get("picture").asInstanceOf[String]) - .flatMap(_.split("/").lastOption) - .getOrElse("") - ) + // getEmailVerified boxes to null when the claim is absent; absent means unverified. + emailVerified = Option(payload.getEmailVerified).exists(_.booleanValue()), + avatar = Option(payload.get("picture").asInstanceOf[String]) + .filter(_.nonEmpty) + .map(_.split("/").last) ) } } @@ -86,8 +86,13 @@ class GoogleAuthResource { def login(credential: String): TokenIssueResponse = verifiedPayload(credential) match { case Some(payload) => - val user = ExternalAuthProvisioner.loginOrProvision(GoogleAuthResource.profileOf(payload)) - TokenIssueResponse(jwtToken(jwtClaims(user, TOKEN_EXPIRE_TIME_IN_MINUTES))) + val profile = GoogleAuthResource.profileOf(payload) + val user = ExternalAuthProvisioner.loginOrProvision(profile) + // The frontend reads `googleId` off the raw token; the provider id is already in hand + // here, so no lookup is needed. + TokenIssueResponse( + jwtToken(jwtClaims(user, TOKEN_EXPIRE_TIME_IN_MINUTES, Some(profile.providerId))) + ) case None => throw new NotAuthorizedException("Login credentials are incorrect.") } } diff --git a/amber/src/test/scala/org/apache/texera/web/auth/UserAuthenticatorSpec.scala b/amber/src/test/scala/org/apache/texera/web/auth/UserAuthenticatorSpec.scala index a435999aa1c..6c62f9fa2df 100644 --- a/amber/src/test/scala/org/apache/texera/web/auth/UserAuthenticatorSpec.scala +++ b/amber/src/test/scala/org/apache/texera/web/auth/UserAuthenticatorSpec.scala @@ -37,7 +37,7 @@ class UserAuthenticatorSpec extends AnyFlatSpec with Matchers { claims.setClaim("userId", 42) claims.setClaim("email", "alice@example.com") claims.setClaim("role", UserRoleEnum.ADMIN.name) - claims.setClaim("avatar", "avatar-blob") + claims.setClaim("googleAvatar", "avatar-blob") claims.setExpirationTimeMinutesInTheFuture(10f) claims } diff --git a/amber/src/test/scala/org/apache/texera/web/resource/auth/GoogleAuthResourceSpec.scala b/amber/src/test/scala/org/apache/texera/web/resource/auth/GoogleAuthResourceSpec.scala index 8572224343b..08925868d79 100644 --- a/amber/src/test/scala/org/apache/texera/web/resource/auth/GoogleAuthResourceSpec.scala +++ b/amber/src/test/scala/org/apache/texera/web/resource/auth/GoogleAuthResourceSpec.scala @@ -77,17 +77,20 @@ class GoogleAuthResourceSpec /** * A payload shaped like Google's. `name` and `picture` are ordinary JSON members rather than * typed fields, and passing null omits them — which is exactly the case the name fallback - * exists for. + * exists for. `emailVerified` defaults to true because that is the ordinary case; the + * provisioner refuses an unverified address outright. */ private def payload( subject: String, email: String, name: String = "Given Name", - picture: String = "https://lh3.googleusercontent.com/a/AVATAR-ID" + picture: String = "https://lh3.googleusercontent.com/a/AVATAR-ID", + emailVerified: java.lang.Boolean = true ): GoogleIdToken.Payload = { val p = new GoogleIdToken.Payload() p.setSubject(subject) p.setEmail(email) + p.setEmailVerified(emailVerified) if (name != null) p.set("name", name) if (picture != null) p.set("picture", picture) p @@ -153,10 +156,32 @@ class GoogleAuthResourceSpec userByEmail("avatar").getAvatar shouldBe "AVATAR-ID" } - it should "store an empty avatar when the payload carries no picture" in { + // A missing `picture` maps to None, not Some("") — so the provisioner's documented + // "leave the stored avatar alone" path is reachable instead of blanking the column. + it should "leave the avatar unset when the payload carries no picture" in { loginWith(payload("google-sub-nopic", "nopic" + emailDomain, picture = null)) - userByEmail("nopic").getAvatar shouldBe "" + userByEmail("nopic").getAvatar shouldBe null + } + + it should "keep an already-stored avatar when a later login carries no picture" in { + loginWith(payload("google-sub-keeppic", "keeppic" + emailDomain)) + userByEmail("keeppic").getAvatar shouldBe "AVATAR-ID" + + loginWith(payload("google-sub-keeppic", "keeppic" + emailDomain, picture = null)) + + userByEmail("keeppic").getAvatar shouldBe "AVATAR-ID" + } + + // ---- unverified email ---------------------------------------------------- + + it should "reject a payload whose email Google has not verified" in { + val resource = new StubbedGoogleAuthResource( + Some(payload("google-sub-unverified", "unverified" + emailDomain, emailVerified = false)) + ) + + assertThrows[NotAuthorizedException](resource.login("stubbed-credential")) + userByEmail("unverified") shouldBe null } // ---- verification failure ------------------------------------------------ diff --git a/amber/src/test/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowExecutionsResourceSpec.scala b/amber/src/test/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowExecutionsResourceSpec.scala index 1dfb8b4cf1f..b1137812d55 100644 --- a/amber/src/test/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowExecutionsResourceSpec.scala +++ b/amber/src/test/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowExecutionsResourceSpec.scala @@ -944,6 +944,22 @@ class WorkflowExecutionsResourceSpec assert(result.size == 2) } + // fetchInto maps onto WorkflowExecutionEntry POSITIONALLY (a case class has no no-arg + // constructor, and jOOQ's mapConstructorParameterNames defaults to false), so `USER.AVATAR` + // at projection position 5 lands on `googleAvatar` despite the names differing — exactly as + // `last_update_time` at position 9 lands on `completionTime`. Nothing observed either before, + // which is what makes an accidental column reorder silent. Pin both here. + it should "map the owner's avatar and completion time onto the entry despite the name mismatch" in { + grantReadAccess() + insertExecution(lastUpdateOffsetMillis = Some(0L)) + val entry = resource.retrieveExecutionsOfWorkflow(testWorkflowWid, session(testUser), null).head + assert(entry.userName == testUser.getName) + assert(entry.googleAvatar == "avatar_url") + // `last_update_time` is populated, so a null here would mean position 9 never reached + // `completionTime` — i.e. the mapping had silently become name-based. + assert(entry.completionTime != null) + } + it should "reject an invalid status filter with a BadRequestException" in { grantReadAccess() assertThrows[BadRequestException]( From 2ae76c6f07629872c77d1dbc2e1ab4bc045a7332 Mon Sep 17 00:00:00 2001 From: Neilk1021 Date: Tue, 4 Aug 2026 10:59:07 -0700 Subject: [PATCH 20/54] fix(auth): verify email for external users and readd google id --- .../auth/ExternalAuthProvisioner.scala | 41 +++++++++++++++- .../resource/auth/GoogleAuthResource.scala | 15 ++++-- sql/changelog.xml | 5 ++ sql/texera_ddl.sql | 3 +- sql/updates/32.sql | 49 +++++++++++++++++++ 5 files changed, 105 insertions(+), 8 deletions(-) create mode 100644 sql/updates/32.sql diff --git a/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala b/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala index daaa2bc527c..47e0fca9e70 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala @@ -27,14 +27,17 @@ import org.apache.texera.dao.jooq.generated.tables.daos.{AuthProviderDao, UserDa import org.apache.texera.dao.jooq.generated.tables.pojos.{AuthProvider, User} import org.jooq.DSLContext +import java.net.URI import java.time.OffsetDateTime import javax.ws.rs.NotAuthorizedException import scala.util.chaining.scalaUtilChainingOps +import scala.util.Try /** * A verified external identity (Google, Facebook, ...) reduced to the fields we - * persist. `avatar` is optional: `None` means the provider supplies no avatar, so - * the user's existing avatar column is left untouched rather than overwritten. + * persist. `avatar` is the complete URL the provider supplied, and is optional: + * `None` means the provider supplies no avatar, so the user's existing avatar column + * is left untouched rather than overwritten. * * `emailVerified` reports whether the provider itself vouches for `email`. It has no * default on purpose: an email address is what links an external identity to an @@ -51,6 +54,40 @@ final case class ExternalProfile( ) object ExternalAuthProvisioner extends LazyLogging { + // ── avatar host allowlist ── + private val ALLOWED_AVATAR_HOST_SUFFIXES: Set[String] = Set( + "googleusercontent.com" + ) + + /** Allow an exact host or any subdomain of an allowlisted suffix. */ + private[auth] def isAllowedAvatarHost(host: String): Boolean = { + if (host == null || host.isEmpty) return false + val lower = host.toLowerCase + ALLOWED_AVATAR_HOST_SUFFIXES.exists(suffix => lower == suffix || lower.endsWith("." + suffix)) + } + + /** + * The avatar URL to persist, or `None` to leave the stored value alone. Anything that is not + * an http(s) URL on an allowlisted host is dropped rather than rejected: a surprising avatar + * is not a reason to deny someone a login, and treating it as "provider supplied no avatar" + * falls back to the initials avatar. + */ + private[auth] def sanitizedAvatar(profile: ExternalProfile): Option[String] = + profile.avatar.filter { url => + val host = Try(URI.create(url)).toOption.filter { uri => + val scheme = Option(uri.getScheme).map(_.toLowerCase) + scheme.contains("http") || scheme.contains("https") + }.flatMap(uri => Option(uri.getHost)) + + val allowed = host.exists(isAllowedAvatarHost) + if (!allowed) { + logger.warn( + s"Ignoring avatar from ${profile.providerType} identity ${profile.providerId}: " + + s"'$url' is not an http(s) URL on an allowlisted host." + ) + } + allowed + } /** * Resolve the user behind an external identity, creating one if necessary, and diff --git a/amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleAuthResource.scala b/amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleAuthResource.scala index fa6d418fd04..ef89e1de8df 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleAuthResource.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleAuthResource.scala @@ -36,8 +36,15 @@ object GoogleAuthResource { /** * Reduce a verified Google id-token payload to the fields we persist. Google omits `name` * for accounts with no profile name, and the provisioner writes `name` straight to a NOT - * NULL column, so the address stands in for it. Only the last path segment of `picture` is - * kept. The frontend rebuilds the full `lh3.googleusercontent.com` URL around it. + * NULL column, so the address stands in for it. + * + * `picture` is kept as the complete URL Google supplied. It used to be reduced to its last + * path segment with the frontend rebuilding `lh3.googleusercontent.com` around it, which + * made the stored value meaningless for any other provider. [[ExternalAuthProvisioner]] + * allowlists the host before storing it. + * + * An absent `picture` maps to `None` rather than `Some("")`, so the provisioner's documented + * "leave the stored avatar alone" path is reachable instead of blanking the column. */ private[auth] def profileOf(payload: GoogleIdToken.Payload): ExternalProfile = { val googleEmail = payload.getEmail @@ -48,9 +55,7 @@ object GoogleAuthResource { googleEmail, // getEmailVerified boxes to null when the claim is absent; absent means unverified. emailVerified = Option(payload.getEmailVerified).exists(_.booleanValue()), - avatar = Option(payload.get("picture").asInstanceOf[String]) - .filter(_.nonEmpty) - .map(_.split("/").last) + avatar = Option(payload.get("picture").asInstanceOf[String]).map(_.trim).filter(_.nonEmpty) ) } } diff --git a/sql/changelog.xml b/sql/changelog.xml index 09591156618..959a7f4d649 100644 --- a/sql/changelog.xml +++ b/sql/changelog.xml @@ -68,6 +68,11 @@ + + + + + - - - - - + diff --git a/sql/texera_ddl.sql b/sql/texera_ddl.sql index 450b185d668..f5e58450674 100644 --- a/sql/texera_ddl.sql +++ b/sql/texera_ddl.sql @@ -106,7 +106,6 @@ CREATE TABLE IF NOT EXISTS "user" uid SERIAL PRIMARY KEY, name VARCHAR(256) NOT NULL, email VARCHAR(256) UNIQUE, - -- full avatar URL as supplied by the identity provider; hosts are allowlisted on write avatar VARCHAR(512), role user_role_enum NOT NULL DEFAULT 'INACTIVE', comment TEXT, diff --git a/sql/updates/31.sql b/sql/updates/31.sql deleted file mode 100644 index 6e6bcf0b688..00000000000 --- a/sql/updates/31.sql +++ /dev/null @@ -1,155 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -\c texera_db - -SET search_path TO texera_db; - -BEGIN; - -DO $$ -BEGIN - IF NOT EXISTS (SELECT 1 FROM pg_type WHERE typname = 'provider_type_enum') THEN - CREATE TYPE provider_type_enum AS ENUM ('LOCAL', 'GOOGLE'); - END IF; -END -$$; - --- provider_id is nullable here and tightened to NOT NULL in step 6, once the backfill below --- has given every row a handle. -CREATE TABLE IF NOT EXISTS auth_provider -( - uid INT NOT NULL, - provider_type provider_type_enum NOT NULL, - provider_id VARCHAR(256), - password VARCHAR(256), -- hashed credential; only for LOCAL - created_at TIMESTAMPTZ NOT NULL DEFAULT now(), - PRIMARY KEY (uid, provider_type), - FOREIGN KEY (uid) REFERENCES "user"(uid) ON DELETE CASCADE, - CONSTRAINT uq_provider_identity UNIQUE (provider_type, provider_id) -); - -ALTER TABLE auth_provider DROP CONSTRAINT IF EXISTS ck_provider_credential; - -DO $$ -DECLARE - offenders TEXT; - orphans TEXT; -BEGIN - IF EXISTS ( - SELECT 1 FROM information_schema.columns - WHERE table_schema = 'texera_db' AND table_name = 'user' AND column_name = 'password' - ) THEN - -- upgrading from the pre-auth_provider schema: handles come straight from "user" - SELECT string_agg(DISTINCT quote_literal(name), ', ') - INTO offenders - FROM "user" - WHERE password IS NOT NULL - AND (btrim(name) = '' OR name <> btrim(name) OR name IN ( - SELECT name FROM "user" WHERE password IS NOT NULL - GROUP BY name HAVING count(*) > 1)); - - SELECT string_agg(uid::TEXT, ', ') - INTO orphans - FROM "user" - WHERE password IS NULL AND google_id IS NULL; - ELSE - SELECT string_agg(DISTINCT quote_literal(u.name), ', ') - INTO offenders - FROM "user" u - JOIN auth_provider a ON a.uid = u.uid AND a.provider_type = 'LOCAL' - WHERE a.provider_id IS NULL - AND (btrim(u.name) = '' OR u.name <> btrim(u.name) OR u.name IN ( - SELECT u2.name - FROM "user" u2 - JOIN auth_provider a2 ON a2.uid = u2.uid AND a2.provider_type = 'LOCAL' - WHERE a2.provider_id IS NULL - GROUP BY u2.name HAVING count(*) > 1)); - END IF; - - IF offenders IS NOT NULL THEN - RAISE EXCEPTION 'migration 31: cannot promote "user".name to a login handle - ' - 'the following names are duplicated, blank, or whitespace-padded: %. ' - 'Resolve them and re-run.', offenders; - END IF; - - IF orphans IS NOT NULL THEN - RAISE NOTICE 'migration 31: uid(s) % have neither a password nor a google_id, so they ' - 'get no auth_provider row and cannot log in.', orphans; - END IF; -END -$$; - --- 5. Backfill. -DO $$ -BEGIN - IF EXISTS ( - SELECT 1 FROM information_schema.columns - WHERE table_schema = 'texera_db' AND table_name = 'user' AND column_name = 'password' - ) THEN - INSERT INTO auth_provider (uid, provider_type, provider_id, password) - SELECT uid, 'LOCAL'::provider_type_enum, name, password - FROM "user" - WHERE password IS NOT NULL - ON CONFLICT (uid, provider_type) DO NOTHING; - - INSERT INTO auth_provider (uid, provider_type, provider_id) - SELECT uid, 'GOOGLE'::provider_type_enum, google_id - FROM "user" - WHERE google_id IS NOT NULL - ON CONFLICT (uid, provider_type) DO NOTHING; - END IF; -END -$$; - --- Fill handles left NULL by an earlier version of this migration; a no-op otherwise. -UPDATE auth_provider a -SET provider_id = u.name -FROM "user" u -WHERE u.uid = a.uid - AND a.provider_type = 'LOCAL' - AND a.provider_id IS NULL; - --- 6. Every row now has a handle, so make it mandatory and restore the credential check --- in its new shape: a password exists for LOCAL and only for LOCAL. -ALTER TABLE auth_provider ALTER COLUMN provider_id SET NOT NULL; -ALTER TABLE auth_provider - ADD CONSTRAINT ck_provider_credential CHECK ((provider_type = 'LOCAL') = (password IS NOT NULL)); - --- Keep the avatar as a provider-neutral profile column on "user" (rename in place). --- Guarded so it is a no-op on a fresh DB where "user" already has "avatar". -DO $$ -BEGIN - IF EXISTS ( - SELECT 1 FROM information_schema.columns - WHERE table_schema = 'texera_db' AND table_name = 'user' AND column_name = 'google_avatar' - ) AND NOT EXISTS ( - SELECT 1 FROM information_schema.columns - WHERE table_schema = 'texera_db' AND table_name = 'user' AND column_name = 'avatar' - ) THEN - ALTER TABLE "user" RENAME COLUMN google_avatar TO avatar; - END IF; -END -$$; - -ALTER TABLE "user" DROP CONSTRAINT IF EXISTS ck_nulltest; -ALTER TABLE "user" DROP COLUMN IF EXISTS password; -ALTER TABLE "user" DROP COLUMN IF EXISTS google_id; - -COMMIT; diff --git a/sql/updates/32.sql b/sql/updates/32.sql index ec5c8a7c643..fad02c00633 100644 --- a/sql/updates/32.sql +++ b/sql/updates/32.sql @@ -17,12 +17,19 @@ * under the License. */ --- Store the identity provider's full avatar URL instead of a Google-specific URL fragment. +-- Relocate login credentials out of "user" into auth_provider, and make the avatar column +-- provider-neutral. -- --- "user".avatar used to hold only the last path segment of Google's `picture` claim, and the --- frontend rebuilt `https://lh3.googleusercontent.com/a/` around it. That made the --- column meaningless for any other provider. This promotes the stored fragments to complete --- URLs so the value is self-describing and provider-agnostic. +-- Part 1 moves `password` / `google_id` into an auth_provider row per (user, provider), so a +-- user can hold several external identities instead of exactly one Google account. +-- +-- Part 2 promotes the avatar from a Google-specific URL fragment to the provider's complete +-- URL. It used to hold only the last path segment of Google's `picture` claim, with the +-- frontend rebuilding `https://lh3.googleusercontent.com/a/` around it, which made +-- the column meaningless for any other provider. +-- +-- Both parts run in one transaction: part 2 depends on the google_avatar -> avatar rename in +-- part 1, so they cannot be applied independently. \c texera_db @@ -30,20 +37,178 @@ SET search_path TO texera_db; BEGIN; --- 1. A full URL does not fit in the old width. +-- ============================================================================ +-- Part 1: credentials move to auth_provider +-- ============================================================================ + +DO $$ +BEGIN + IF NOT EXISTS (SELECT 1 FROM pg_type WHERE typname = 'provider_type_enum') THEN + CREATE TYPE provider_type_enum AS ENUM ('LOCAL', 'GOOGLE'); + END IF; +END +$$; + +-- provider_id is nullable here and tightened to NOT NULL below, once the backfill has given +-- every row a handle. +CREATE TABLE IF NOT EXISTS auth_provider +( + uid INT NOT NULL, + provider_type provider_type_enum NOT NULL, + provider_id VARCHAR(256), + password VARCHAR(256), -- hashed credential; only for LOCAL + created_at TIMESTAMPTZ NOT NULL DEFAULT now(), + PRIMARY KEY (uid, provider_type), + FOREIGN KEY (uid) REFERENCES "user"(uid) ON DELETE CASCADE, + CONSTRAINT uq_provider_identity UNIQUE (provider_type, provider_id) +); + +ALTER TABLE auth_provider DROP CONSTRAINT IF EXISTS ck_provider_credential; + +DO $$ +DECLARE + offenders TEXT; + orphans TEXT; + has_placeholder BOOLEAN; +BEGIN + -- `is_placeholder` accounts (migration 31) deliberately have no credential, so they are + -- not orphans and must not be reported as such. Checked dynamically because this migration + -- also has to run against databases predating that column. + SELECT EXISTS ( + SELECT 1 FROM information_schema.columns + WHERE table_schema = 'texera_db' AND table_name = 'user' AND column_name = 'is_placeholder' + ) INTO has_placeholder; + + IF EXISTS ( + SELECT 1 FROM information_schema.columns + WHERE table_schema = 'texera_db' AND table_name = 'user' AND column_name = 'password' + ) THEN + -- upgrading from the pre-auth_provider schema: handles come straight from "user" + SELECT string_agg(DISTINCT quote_literal(name), ', ') + INTO offenders + FROM "user" + WHERE password IS NOT NULL + AND (btrim(name) = '' OR name <> btrim(name) OR name IN ( + SELECT name FROM "user" WHERE password IS NOT NULL + GROUP BY name HAVING count(*) > 1)); + + IF has_placeholder THEN + EXECUTE $q$ + SELECT string_agg(uid::TEXT, ', ') + FROM "user" + WHERE password IS NULL AND google_id IS NULL AND NOT is_placeholder + $q$ INTO orphans; + ELSE + SELECT string_agg(uid::TEXT, ', ') + INTO orphans + FROM "user" + WHERE password IS NULL AND google_id IS NULL; + END IF; + ELSE + SELECT string_agg(DISTINCT quote_literal(u.name), ', ') + INTO offenders + FROM "user" u + JOIN auth_provider a ON a.uid = u.uid AND a.provider_type = 'LOCAL' + WHERE a.provider_id IS NULL + AND (btrim(u.name) = '' OR u.name <> btrim(u.name) OR u.name IN ( + SELECT u2.name + FROM "user" u2 + JOIN auth_provider a2 ON a2.uid = u2.uid AND a2.provider_type = 'LOCAL' + WHERE a2.provider_id IS NULL + GROUP BY u2.name HAVING count(*) > 1)); + END IF; + + IF offenders IS NOT NULL THEN + RAISE EXCEPTION 'migration 32: cannot promote "user".name to a login handle - ' + 'the following names are duplicated, blank, or whitespace-padded: %. ' + 'Resolve them and re-run.', offenders; + END IF; + + IF orphans IS NOT NULL THEN + RAISE NOTICE 'migration 32: uid(s) % have neither a password nor a google_id, so they ' + 'get no auth_provider row and cannot log in.', orphans; + END IF; +END +$$; + +-- Backfill one auth_provider row per credential the user already had. +DO $$ +BEGIN + IF EXISTS ( + SELECT 1 FROM information_schema.columns + WHERE table_schema = 'texera_db' AND table_name = 'user' AND column_name = 'password' + ) THEN + INSERT INTO auth_provider (uid, provider_type, provider_id, password) + SELECT uid, 'LOCAL'::provider_type_enum, name, password + FROM "user" + WHERE password IS NOT NULL + ON CONFLICT (uid, provider_type) DO NOTHING; + + INSERT INTO auth_provider (uid, provider_type, provider_id) + SELECT uid, 'GOOGLE'::provider_type_enum, google_id + FROM "user" + WHERE google_id IS NOT NULL + ON CONFLICT (uid, provider_type) DO NOTHING; + END IF; +END +$$; + +-- Fill handles left NULL by an earlier version of this migration; a no-op otherwise. +UPDATE auth_provider a +SET provider_id = u.name +FROM "user" u +WHERE u.uid = a.uid + AND a.provider_type = 'LOCAL' + AND a.provider_id IS NULL; + +-- Every row now has a handle, so make it mandatory and restore the credential check in its +-- new shape: a password exists for LOCAL and only for LOCAL. +ALTER TABLE auth_provider ALTER COLUMN provider_id SET NOT NULL; +ALTER TABLE auth_provider + ADD CONSTRAINT ck_provider_credential CHECK ((provider_type = 'LOCAL') = (password IS NOT NULL)); + +-- Keep the avatar as a provider-neutral profile column on "user" (rename in place). +-- Guarded so it is a no-op on a fresh DB where "user" already has "avatar". +DO $$ +BEGIN + IF EXISTS ( + SELECT 1 FROM information_schema.columns + WHERE table_schema = 'texera_db' AND table_name = 'user' AND column_name = 'google_avatar' + ) AND NOT EXISTS ( + SELECT 1 FROM information_schema.columns + WHERE table_schema = 'texera_db' AND table_name = 'user' AND column_name = 'avatar' + ) THEN + ALTER TABLE "user" RENAME COLUMN google_avatar TO avatar; + END IF; +END +$$; + +-- ck_nulltest constrained password/google_id, which are about to disappear. Its "every user +-- has a credential" rule cannot be a row-level check once credentials live in a child table, +-- so it is dropped rather than reshaped: a user with no auth_provider row is now legal and +-- simply cannot log in. +ALTER TABLE "user" DROP CONSTRAINT IF EXISTS ck_nulltest; +ALTER TABLE "user" DROP COLUMN IF EXISTS password; +ALTER TABLE "user" DROP COLUMN IF EXISTS google_id; + +-- ============================================================================ +-- Part 2: the avatar becomes the provider's full URL +-- ============================================================================ + +-- A full URL does not fit in the old width. ALTER TABLE "user" ALTER COLUMN avatar TYPE VARCHAR(512); --- 2. Pictureless Google logins used to record an empty string; NULL is now the single --- representation of "this user has no avatar", so the frontend has one case to handle. +-- Pictureless Google logins used to record an empty string; NULL is now the single +-- representation of "this user has no avatar", so the frontend has one case to handle. UPDATE "user" SET avatar = NULL WHERE avatar = ''; --- 3. Promote the remaining bare fragments to absolute URLs. The `NOT LIKE` guard makes this --- idempotent and leaves already-absolute values (from a re-run, or from a provider added --- after this migration) untouched. +-- Promote the remaining bare fragments to absolute URLs. The `NOT LIKE` guard makes this +-- idempotent and leaves already-absolute values (from a re-run, or from a provider added +-- after this migration) untouched. UPDATE "user" SET avatar = 'https://lh3.googleusercontent.com/a/' || avatar WHERE avatar IS NOT NULL AND avatar NOT LIKE 'http://%' AND avatar NOT LIKE 'https://%'; -COMMIT; \ No newline at end of file +COMMIT; From f50aabf1cde43fde2b5db3bf6ee44b66d396d399 Mon Sep 17 00:00:00 2001 From: Neilk1021 Date: Wed, 5 Aug 2026 11:28:03 -0700 Subject: [PATCH 25/54] refactor(auth): revert behavior changes and fix email casing Keeps this PR a behavior-preserving relocation of login credentials, per review. Moved out to follow-up PRs (preserved on task/auth-behavior-snapshot): - the avatar representation change, from a URL fragment to the provider's full URL, spanning profileOf, the host allowlist, the frontend getAvatar and the migration's Part 2 - the unverified-email rejection - the malformed-credential 500 -> 401 verifiedPayload keeps its extracted shape, since the whole GoogleAuthResourceSpec depends on that seam; only the exception-swallowing behavior is reverted. Restored the display-name refresh dropped earlier, so refresh() again matches what the pre-refactor Google path did. Also fixes a regression this PR introduced: the first-time-identity email link had become case-sensitive, where the code it replaced used fetchUserByEmailIgnoreCase. "user".email is a case-sensitive UNIQUE and idx_user_email_lower is not unique, so a casing mismatch did not raise 23505 -- it silently created a duplicate account and stranded the original user's workflows, datasets and contributor links. Both lookups now match on lower(email) inside the transaction. Two tests cover it; they fail against the previous lookup. --- .../auth/ExternalAuthProvisioner.scala | 119 +++---- .../resource/auth/GoogleAuthResource.scala | 29 +- .../auth/ExternalAuthProvisionerSpec.scala | 318 +++++------------- .../auth/GoogleAuthResourceSpec.scala | 44 +-- .../common/service/user/stub-user.service.ts | 2 +- .../common/service/user/user.service.spec.ts | 27 +- .../app/common/service/user/user.service.ts | 22 +- .../user/user-avatar/user-avatar.component.ts | 5 +- sql/texera_ddl.sql | 6 +- sql/updates/32.sql | 42 +-- 10 files changed, 152 insertions(+), 462 deletions(-) diff --git a/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala b/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala index cf0b18e73b6..e2133a48174 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala @@ -25,71 +25,47 @@ import org.apache.texera.dao.jooq.generated.Tables.{AUTH_PROVIDER, USER} import org.apache.texera.dao.jooq.generated.enums.{ProviderTypeEnum, UserRoleEnum} import org.apache.texera.dao.jooq.generated.tables.daos.{AuthProviderDao, UserDao} import org.apache.texera.dao.jooq.generated.tables.pojos.{AuthProvider, User} +import org.apache.texera.common.util.EmailUtil import org.jooq.DSLContext +import org.jooq.impl.DSL -import java.net.URI import java.time.OffsetDateTime -import javax.ws.rs.NotAuthorizedException import scala.util.chaining.scalaUtilChainingOps -import scala.util.Try /** - * A verified external identity (Google, Facebook, ...) reduced to the fields we - * persist. `avatar` is the complete URL the provider supplied, and is optional: - * `None` means the provider supplies no avatar, so the user's existing avatar column - * is left untouched rather than overwritten. - * - * `emailVerified` reports whether the provider itself vouches for `email`. It has no - * default on purpose: an email address is what links an external identity to an - * existing account, so treating an unverified one as trusted is an account-takeover - * path, and a defaulted flag is how that mistake comes back. + * A verified external identity (Google, Facebook, ...) reduced to the fields we persist. */ final case class ExternalProfile( providerType: ProviderTypeEnum, providerId: String, name: String, email: String, - emailVerified: Boolean, - avatar: Option[String] = None + avatar: String ) object ExternalAuthProvisioner extends LazyLogging { - // ── avatar host allowlist ── - private val ALLOWED_AVATAR_HOST_SUFFIXES: Set[String] = Set( - "googleusercontent.com" - ) - - /** Allow an exact host or any subdomain of an allowlisted suffix. */ - private[auth] def isAllowedAvatarHost(host: String): Boolean = { - if (host == null || host.isEmpty) return false - val lower = host.toLowerCase - ALLOWED_AVATAR_HOST_SUFFIXES.exists(suffix => lower == suffix || lower.endsWith("." + suffix)) - } /** - * The avatar URL to persist, or `None` to leave the stored value alone. Anything that is not - * an http(s) URL on an allowlisted host is dropped rather than rejected: a surprising avatar - * is not a reason to deny someone a login, and treating it as "provider supplied no avatar" - * falls back to the initials avatar. + * The account owning `email`, matched case-insensitively and within the caller's transaction + * so it reads that transaction's own writes. + * + * Case-insensitivity is required, not a nicety: `"user".email` is a plain case-sensitive + * UNIQUE and `idx_user_email_lower` is not unique, so `Alice@x.com` and `alice@x.com` can + * coexist. Registration stores the address as the user typed it while contributor + * placeholders are stored lower-cased, so the casings provably differ in practice. An + * exact-match lookup here would miss, insert a second account without violating any + * constraint, and silently strand the original account's data. + * + * Mirrors `AuthResource.fetchUserByEmailIgnoreCase`, which cannot be reused directly because + * it opens its own DSLContext. */ - private[auth] def sanitizedAvatar(profile: ExternalProfile): Option[String] = - profile.avatar.filter { url => - val host = Try(URI.create(url)).toOption - .filter { uri => - val scheme = Option(uri.getScheme).map(_.toLowerCase) - scheme.contains("http") || scheme.contains("https") - } - .flatMap(uri => Option(uri.getHost)) - - val allowed = host.exists(isAllowedAvatarHost) - if (!allowed) { - logger.warn( - s"Ignoring avatar from ${profile.providerType} identity ${profile.providerId}: " + - s"'$url' is not an http(s) URL on an allowlisted host." - ) - } - allowed - } + private def userByEmailIgnoreCase(ctx: DSLContext, email: String): Option[User] = + Option( + ctx + .selectFrom(USER) + .where(DSL.lower(USER.EMAIL).eq(EmailUtil.normalize(email))) + .fetchOneInto(classOf[User]) + ) /** * Resolve the user behind an external identity, creating one if necessary, and @@ -118,27 +94,14 @@ object ExternalAuthProvisioner extends LazyLogging { } case None => - // First time we have seen this identity, so the email address is the only thing - // tying it to an account. It is either an existing one to link onto, or a new row that - // claims the address. Trusting an unverified address for that lets anyone who can - // mint an `email` claim take over, or squat on, someone else's account. The error - // is deliberately the same one a bad credential yields, so this does not become an - // oracle for which addresses are registered. - if (!profile.emailVerified) { - logger.warn( - s"Refusing to provision ${profile.providerType} identity ${profile.providerId}: " + - "the provider did not verify its email address." - ) - throw new NotAuthorizedException("Login credentials are incorrect.") - } - - val user = Option(txUserDao.fetchOneByEmail(profile.email)) match { + // First time we have seen this identity, so the email address is the only thing tying + // it to an account: either an existing one to link onto, or a new row that claims it. + val user = userByEmailIgnoreCase(ctx, profile.email) match { case Some(existing) => existing.tap { user => // A placeholder account (auto-created for a dataset contributor, never had a - // credential) is claimed by the first external identity that proves ownership - // of its email. It keeps its uid, so existing contributor links stay valid. - // Reaching here already required profile.emailVerified. + // credential) is claimed by the first external identity that presents its email. + // It keeps its uid, so existing contributor links stay valid. val claimed = user.getIsPlaceholder if (claimed) AuthResource.claimPlaceholder(user) // refresh() must run regardless so its mutations are applied @@ -149,14 +112,15 @@ object ExternalAuthProvisioner extends LazyLogging { val created = new User() created.setName(profile.name) created.setEmail(profile.email) - sanitizedAvatar(profile).foreach(created.setAvatar) + created.setAvatar(profile.avatar) created.setRole(UserRoleEnum.INACTIVE) try { txUserDao.insert(created) created } catch { + // A concurrent registration of the same address won the race; adopt its row. case e: org.jooq.exception.DataAccessException if e.sqlState() == "23505" => - Option(txUserDao.fetchOneByEmail(profile.email)).getOrElse(throw e) + userByEmailIgnoreCase(ctx, profile.email).getOrElse(throw e) } } @@ -181,25 +145,20 @@ object ExternalAuthProvisioner extends LazyLogging { /** * Mutate `user` in place to match `profile`, returning true iff anything changed * (so the caller only issues an UPDATE when needed). - * - * `name` is deliberately not refreshed. It is the display name the user owns and may - * have edited in Texera, and it is not identity — the login handle lives in - * `auth_provider.provider_id`. Re-deriving it from the provider on every login silently - * reverted such edits. */ private def refresh(user: User, profile: ExternalProfile): Boolean = { var changed = false - // Same reasoning as the link path: only a provider-verified address may move the - // column that identifies the account. - if (profile.emailVerified && user.getEmail != profile.email) { + if (user.getName != profile.name) { + user.setName(profile.name) + changed = true + } + if (user.getEmail != profile.email) { user.setEmail(profile.email) changed = true } - sanitizedAvatar(profile).foreach { avatar => - if (user.getAvatar != avatar) { - user.setAvatar(avatar) - changed = true - } + if (user.getAvatar != profile.avatar) { + user.setAvatar(profile.avatar) + changed = true } changed } diff --git a/amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleAuthResource.scala b/amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleAuthResource.scala index ae2d86dd08a..98d37430317 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleAuthResource.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleAuthResource.scala @@ -27,7 +27,6 @@ import org.apache.texera.common.config.UserSystemConfig import org.apache.texera.dao.jooq.generated.enums.ProviderTypeEnum import org.apache.texera.web.model.http.response.TokenIssueResponse -import java.io.IOException import java.util.Collections import javax.ws.rs._ import javax.ws.rs.core.MediaType @@ -37,15 +36,8 @@ object GoogleAuthResource { /** * Reduce a verified Google id-token payload to the fields we persist. Google omits `name` * for accounts with no profile name, and the provisioner writes `name` straight to a NOT - * NULL column, so the address stands in for it. - * - * `picture` is kept as the complete URL Google supplied. It used to be reduced to its last - * path segment with the frontend rebuilding `lh3.googleusercontent.com` around it, which - * made the stored value meaningless for any other provider. [[ExternalAuthProvisioner]] - * allowlists the host before storing it. - * - * An absent `picture` maps to `None` rather than `Some("")`, so the provisioner's documented - * "leave the stored avatar alone" path is reachable instead of blanking the column. + * NULL column, so the address stands in for it. Only the last path segment of `picture` is + * kept — the frontend rebuilds the full `lh3.googleusercontent.com` URL around it. */ private[auth] def profileOf(payload: GoogleIdToken.Payload): ExternalProfile = { val googleEmail = payload.getEmail @@ -54,9 +46,9 @@ object GoogleAuthResource { payload.getSubject, Option(payload.get("name").asInstanceOf[String]).filter(_.nonEmpty).getOrElse(googleEmail), googleEmail, - // getEmailVerified boxes to null when the claim is absent; absent means unverified. - emailVerified = Option(payload.getEmailVerified).exists(_.booleanValue()), - avatar = Option(payload.get("picture").asInstanceOf[String]).map(_.trim).filter(_.nonEmpty) + avatar = Option(payload.get("picture").asInstanceOf[String]) + .flatMap(_.split("/").lastOption) + .getOrElse("") ) } } @@ -80,15 +72,8 @@ class GoogleAuthResource { * instead of signing a token; kept a method rather than a constructor parameter because * Jersey instantiates this resource from `classOf[GoogleAuthResource]`. */ - protected def verifiedPayload(credential: String): Option[GoogleIdToken.Payload] = { - val idToken = - try Option(GoogleIdToken.parse(GsonFactory.getDefaultInstance, credential)) - catch { - case _: IllegalArgumentException | _: IOException => None - } - - idToken.filter(verifier.verify).map(_.getPayload) - } + protected def verifiedPayload(credential: String): Option[GoogleIdToken.Payload] = + Option(verifier.verify(credential)).map(_.getPayload) @POST @Consumes(Array(MediaType.TEXT_PLAIN)) diff --git a/amber/src/test/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisionerSpec.scala b/amber/src/test/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisionerSpec.scala index b11d4ad9d35..44b2efe683b 100644 --- a/amber/src/test/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisionerSpec.scala +++ b/amber/src/test/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisionerSpec.scala @@ -24,12 +24,11 @@ import org.apache.texera.dao.jooq.generated.Tables.{AUTH_PROVIDER, USER} import org.apache.texera.dao.jooq.generated.enums.{ProviderTypeEnum, UserRoleEnum} import org.apache.texera.dao.jooq.generated.tables.daos.{AuthProviderDao, UserDao} import org.apache.texera.dao.jooq.generated.tables.pojos.{AuthProvider, User} +import org.jooq.impl.DSL import org.scalatest.flatspec.AnyFlatSpec import org.scalatest.matchers.should.Matchers import org.scalatest.{BeforeAndAfterAll, BeforeAndAfterEach} -import javax.ws.rs.NotAuthorizedException - /** * Integration spec for [[ExternalAuthProvisioner]] against embedded Postgres * ([[MockTexeraDB]] loads the real `texera_ddl.sql`, so the `auth_provider` table and @@ -61,22 +60,29 @@ class ExternalAuthProvisionerSpec override protected def beforeEach(): Unit = cleanup() override protected def afterEach(): Unit = cleanup() + // Case-insensitive so it also collects rows seeded with a differing casing. private def cleanup(): Unit = - getDSLContext.deleteFrom(USER).where(USER.EMAIL.like("%" + emailDomain)).execute() + getDSLContext.deleteFrom(USER).where(DSL.lower(USER.EMAIL).like("%" + emailDomain)).execute() // ---- helpers ------------------------------------------------------------- - /** - * An avatar URL on an allowlisted host. The provisioner drops anything else, so tests that - * expect an avatar to be stored have to use a host the allowlist actually accepts. - */ - private def avatarUrl(id: String): String = s"https://lh3.googleusercontent.com/a/$id" + private def profile( + providerId: String, + name: String, + email: String, + avatar: String = "pic" + ): ExternalProfile = + ExternalProfile(ProviderTypeEnum.GOOGLE, providerId, name, email, avatar) /** Seed a user row directly; uid is DB-assigned and read back into the pojo. */ - private def seedUser(name: String, localPart: String, avatar: String = null): User = { + private def seedUser(name: String, localPart: String, avatar: String = null): User = + seedUserWithEmail(name, localPart + emailDomain, avatar) + + /** Seed a user row at a verbatim address, for the casing tests. */ + private def seedUserWithEmail(name: String, email: String, avatar: String = null): User = { val user = new User user.setName(name) - user.setEmail(localPart + emailDomain) + user.setEmail(email) user.setRole(UserRoleEnum.REGULAR) if (avatar != null) user.setAvatar(avatar) userDao.insert(user) @@ -103,27 +109,21 @@ class ExternalAuthProvisionerSpec .and(AUTH_PROVIDER.PROVIDER_TYPE.eq(pt)) .fetchOne(AUTH_PROVIDER.PROVIDER_ID) + /** Counts case-insensitively, so a duplicate differing only in case is still counted. */ private def userCountByEmail(localPart: String): Int = - getDSLContext.fetchCount(USER, USER.EMAIL.eq(localPart + emailDomain)) + getDSLContext.fetchCount(USER, DSL.lower(USER.EMAIL).eq(localPart + emailDomain)) // ---- new-identity provisioning ------------------------------------------- "ExternalAuthProvisioner.loginOrProvision" should "create an INACTIVE user and provider row for a brand-new Google identity" in { val user = ExternalAuthProvisioner.loginOrProvision( - ExternalProfile( - ProviderTypeEnum.GOOGLE, - "google-sub-1", - "New User", - "new" + emailDomain, - emailVerified = true, - avatar = Some(avatarUrl("avatar1")) - ) + profile("google-sub-1", "New User", "new" + emailDomain, avatar = "avatar1") ) user.getUid should not be null user.getName shouldBe "New User" user.getEmail shouldBe "new" + emailDomain - user.getAvatar shouldBe avatarUrl("avatar1") + user.getAvatar shouldBe "avatar1" user.getRole shouldBe UserRoleEnum.INACTIVE providerRowCount(user.getUid) shouldBe 1 @@ -133,143 +133,38 @@ class ExternalAuthProvisionerSpec // ---- returning known identity -------------------------------------------- it should "be idempotent for a returning identity (same uid, no duplicate provider row or user)" in { - val profile = - ExternalProfile( - ProviderTypeEnum.GOOGLE, - "google-sub-return", - "Ret", - "ret" + emailDomain, - emailVerified = true, - avatar = Some(avatarUrl("a")) - ) - - val first = ExternalAuthProvisioner.loginOrProvision(profile) - val second = ExternalAuthProvisioner.loginOrProvision(profile) + val p = profile("google-sub-return", "Ret", "ret" + emailDomain, avatar = "a") + + val first = ExternalAuthProvisioner.loginOrProvision(p) + val second = ExternalAuthProvisioner.loginOrProvision(p) second.getUid shouldBe first.getUid providerRowCount(first.getUid) shouldBe 1 userCountByEmail("ret") shouldBe 1 } - // The display name is the user's to edit and is not identity — the login handle lives in - // auth_provider.provider_id. Re-deriving it from the provider on every login silently reverted - // any rename made in Texera, so refresh leaves it alone and only the avatar follows the drift. - it should "refresh the avatar but keep the local display name for a known identity" in { + it should "refresh drifted profile fields for a known identity" in { ExternalAuthProvisioner.loginOrProvision( - ExternalProfile( - ProviderTypeEnum.GOOGLE, - "sub-drift", - "Old Name", - "drift" + emailDomain, - emailVerified = true, - avatar = Some(avatarUrl("oldpic")) - ) + profile("sub-drift", "Old Name", "drift" + emailDomain, avatar = "oldpic") ) val updated = ExternalAuthProvisioner.loginOrProvision( - ExternalProfile( - ProviderTypeEnum.GOOGLE, - "sub-drift", - "New Name", - "drift" + emailDomain, - emailVerified = true, - avatar = Some(avatarUrl("newpic")) - ) + profile("sub-drift", "New Name", "drift" + emailDomain, avatar = "newpic") ) - updated.getName shouldBe "Old Name" - updated.getAvatar shouldBe avatarUrl("newpic") + updated.getName shouldBe "New Name" + updated.getAvatar shouldBe "newpic" // confirm it persisted, not just mutated in memory - userDao.fetchOneByUid(updated.getUid).getName shouldBe "Old Name" - userDao.fetchOneByUid(updated.getUid).getAvatar shouldBe avatarUrl("newpic") - } - - // ---- unverified email ------------------------------------------------------ - - // The email address is the only thing tying a first-time external identity to an account, so - // an unverified one must not be able to link onto — or squat on — someone else's row. The - // rejection reuses the generic credential error so it cannot be used to enumerate addresses. - it should "refuse to link a first-time identity onto an existing account on an unverified email" in { - val existing = seedUser("Victim", "victim") - - assertThrows[NotAuthorizedException] { - ExternalAuthProvisioner.loginOrProvision( - ExternalProfile( - ProviderTypeEnum.GOOGLE, - "sub-attacker", - "Attacker", - "victim" + emailDomain, - emailVerified = false - ) - ) - } - - providerRowCount(existing.getUid) shouldBe 0 - userDao.fetchOneByUid(existing.getUid).getName shouldBe "Victim" - } - - it should "refuse to provision a brand-new account on an unverified email" in { - assertThrows[NotAuthorizedException] { - ExternalAuthProvisioner.loginOrProvision( - ExternalProfile( - ProviderTypeEnum.GOOGLE, - "sub-unverified", - "Unverified", - "unverified" + emailDomain, - emailVerified = false - ) - ) - } - - userCountByEmail("unverified") shouldBe 0 - } - - // A provider that stops vouching for the address must not be able to move it either. - it should "not adopt an unverified email for a known identity" in { - val created = ExternalAuthProvisioner.loginOrProvision( - ExternalProfile( - ProviderTypeEnum.GOOGLE, - "sub-unverified-drift", - "Drifter", - "verified" + emailDomain, - emailVerified = true - ) - ) - - ExternalAuthProvisioner.loginOrProvision( - ExternalProfile( - ProviderTypeEnum.GOOGLE, - "sub-unverified-drift", - "Drifter", - "hijack" + emailDomain, - emailVerified = false - ) - ) - - userDao.fetchOneByUid(created.getUid).getEmail shouldBe "verified" + emailDomain - userCountByEmail("hijack") shouldBe 0 + userDao.fetchOneByUid(updated.getUid).getName shouldBe "New Name" + userDao.fetchOneByUid(updated.getUid).getAvatar shouldBe "newpic" } it should "adopt the provider's new email address for a known identity" in { val created = ExternalAuthProvisioner.loginOrProvision( - ExternalProfile( - ProviderTypeEnum.GOOGLE, - "sub-rename", - "Renamer", - "before" + emailDomain, - emailVerified = true, - avatar = Some(avatarUrl("pic")) - ) + profile("sub-rename", "Renamer", "before" + emailDomain) ) val updated = ExternalAuthProvisioner.loginOrProvision( - ExternalProfile( - ProviderTypeEnum.GOOGLE, - "sub-rename", - "Renamer", - "after" + emailDomain, - emailVerified = true, - avatar = Some(avatarUrl("pic")) - ) + profile("sub-rename", "Renamer", "after" + emailDomain) ) updated.getUid shouldBe created.getUid @@ -277,138 +172,79 @@ class ExternalAuthProvisionerSpec userCountByEmail("before") shouldBe 0 } - // `avatar = None` is the documented contract for providers that supply no picture: the - // column keeps whatever it held rather than being blanked on every login. - it should "leave the stored avatar untouched when the provider supplies none" in { - val existing = seedUser("Keeper", "keeper", avatar = avatarUrl("keep-me")) + // ---- email match, no provider yet ---------------------------------------- + + it should "link a new provider to an existing email-matched user instead of creating a duplicate" in { + val existing = seedUser("Local User", "linkme") val result = ExternalAuthProvisioner.loginOrProvision( - ExternalProfile( - ProviderTypeEnum.GOOGLE, - "sub-keeper", - "Keeper", - "keeper" + emailDomain, - emailVerified = true, - avatar = None - ) + profile("sub-link", "Local User", "linkme" + emailDomain) ) result.getUid shouldBe existing.getUid - result.getAvatar shouldBe avatarUrl("keep-me") - userDao.fetchOneByUid(existing.getUid).getAvatar shouldBe avatarUrl("keep-me") + userCountByEmail("linkme") shouldBe 1 + providerIdOf(existing.getUid, ProviderTypeEnum.GOOGLE) shouldBe "sub-link" } - // ---- avatar host allowlist ------------------------------------------------- - - // The stored avatar is a provider-supplied URL the frontend renders directly, so an - // unexpected host is dropped rather than persisted. Dropping beats rejecting: a surprising - // avatar is not a reason to deny a login, and the user falls back to the initials avatar. - it should "drop an avatar served from a host outside the allowlist" in { - val user = ExternalAuthProvisioner.loginOrProvision( - ExternalProfile( - ProviderTypeEnum.GOOGLE, - "sub-badhost", - "Bad Host", - "badhost" + emailDomain, - emailVerified = true, - avatar = Some("https://evil.example.com/tracker.gif") - ) - ) - - user.getAvatar shouldBe null - userDao.fetchOneByUid(user.getUid).getAvatar shouldBe null - } + // `"user".email` is a plain case-sensitive UNIQUE and `idx_user_email_lower` is not unique, so + // an exact-match lookup would not merely miss — the follow-up insert would succeed and fork the + // account silently. Registration stores the address as typed while contributor placeholders are + // stored lower-cased, so the casings really do differ in practice. Mirrors the register-path + // guard asserted in AuthResourceSpec. + it should "link to an existing account whose stored email differs only in case" in { + val existing = seedUserWithEmail("Mixed Case", "MixedCase" + emailDomain) - it should "drop a non-http(s) avatar such as a javascript: or data: URL" in { - val user = ExternalAuthProvisioner.loginOrProvision( - ExternalProfile( - ProviderTypeEnum.GOOGLE, - "sub-badscheme", - "Bad Scheme", - "badscheme" + emailDomain, - emailVerified = true, - avatar = Some("javascript:alert(1)") - ) + val result = ExternalAuthProvisioner.loginOrProvision( + profile("sub-casing", "Mixed Case", "mixedcase" + emailDomain) ) - user.getAvatar shouldBe null + result.getUid shouldBe existing.getUid + userCountByEmail("mixedcase") shouldBe 1 + providerIdOf(existing.getUid, ProviderTypeEnum.GOOGLE) shouldBe "sub-casing" } - // A provider that starts serving avatars from an unexpected host must not be able to - // overwrite one that is already stored. - it should "keep the stored avatar when a later login supplies a disallowed host" in { - val existing = seedUser("Holder", "holder", avatar = avatarUrl("original")) - seedExternalProvider(existing.getUid, ProviderTypeEnum.GOOGLE, "sub-holder") + // A contributor placeholder is stored lower-cased by DatasetResource, so a provider reporting + // the address with different casing must still claim it rather than orphan the contributor link. + it should "claim a placeholder account whose stored email differs only in case" in { + val placeholder = seedUserWithEmail("Ghost", "ghost" + emailDomain) + placeholder.setIsPlaceholder(true) + userDao.update(placeholder) - ExternalAuthProvisioner.loginOrProvision( - ExternalProfile( - ProviderTypeEnum.GOOGLE, - "sub-holder", - "Holder", - "holder" + emailDomain, - emailVerified = true, - avatar = Some("https://evil.example.com/tracker.gif") - ) - ) - - userDao.fetchOneByUid(existing.getUid).getAvatar shouldBe avatarUrl("original") - } - - it should "accept an avatar on a subdomain of an allowlisted host" in { - val user = ExternalAuthProvisioner.loginOrProvision( - ExternalProfile( - ProviderTypeEnum.GOOGLE, - "sub-subdomain", - "Subdomain", - "subdomain" + emailDomain, - emailVerified = true, - avatar = Some("https://lh6.googleusercontent.com/a/OTHER-CDN") - ) + val result = ExternalAuthProvisioner.loginOrProvision( + profile("sub-ghost", "Ghost", "GHOST" + emailDomain) ) - user.getAvatar shouldBe "https://lh6.googleusercontent.com/a/OTHER-CDN" + result.getUid shouldBe placeholder.getUid + userDao.fetchOneByUid(placeholder.getUid).getIsPlaceholder shouldBe false + userCountByEmail("ghost") shouldBe 1 } - // ---- email match, no provider yet ---------------------------------------- - - it should "link a new provider to an existing email-matched user instead of creating a duplicate" in { - val existing = seedUser("Local User", "linkme") + it should "claim a placeholder account when an external identity presents its email" in { + val placeholder = seedUser("Placeholder", "claimme") + placeholder.setIsPlaceholder(true) + userDao.update(placeholder) val result = ExternalAuthProvisioner.loginOrProvision( - ExternalProfile( - ProviderTypeEnum.GOOGLE, - "sub-link", - "Local User", - "linkme" + emailDomain, - emailVerified = true, - avatar = Some(avatarUrl("pic")) - ) + profile("sub-claim", "Claimer", "claimme" + emailDomain) ) - result.getUid shouldBe existing.getUid - userCountByEmail("linkme") shouldBe 1 - providerIdOf(existing.getUid, ProviderTypeEnum.GOOGLE) shouldBe "sub-link" + result.getUid shouldBe placeholder.getUid + val claimed = userDao.fetchOneByUid(placeholder.getUid) + claimed.getIsPlaceholder shouldBe false + claimed.getComment should include("Claimed contributor placeholder at ") } - // ---- email match, provider row exists with a different id (upsert) -------- + // ---- provider id rotation ------------------------------------------------- - it should "update the existing provider id in place rather than inserting a colliding row" in { + it should "update the stored provider id when the same user returns with a new one" in { val existing = seedUser("Rotating", "rotate") seedExternalProvider(existing.getUid, ProviderTypeEnum.GOOGLE, "old-sub") - val result = ExternalAuthProvisioner.loginOrProvision( - ExternalProfile( - ProviderTypeEnum.GOOGLE, - "new-sub", - "Rotating", - "rotate" + emailDomain, - emailVerified = true, - avatar = Some(avatarUrl("p")) - ) + ExternalAuthProvisioner.loginOrProvision( + profile("new-sub", "Rotating", "rotate" + emailDomain) ) - result.getUid shouldBe existing.getUid - providerRowCount(existing.getUid) shouldBe 1 // upserted, not a second row + providerRowCount(existing.getUid) shouldBe 1 providerIdOf(existing.getUid, ProviderTypeEnum.GOOGLE) shouldBe "new-sub" } } diff --git a/amber/src/test/scala/org/apache/texera/web/resource/auth/GoogleAuthResourceSpec.scala b/amber/src/test/scala/org/apache/texera/web/resource/auth/GoogleAuthResourceSpec.scala index 8ce86f32904..6324558d392 100644 --- a/amber/src/test/scala/org/apache/texera/web/resource/auth/GoogleAuthResourceSpec.scala +++ b/amber/src/test/scala/org/apache/texera/web/resource/auth/GoogleAuthResourceSpec.scala @@ -77,20 +77,17 @@ class GoogleAuthResourceSpec /** * A payload shaped like Google's. `name` and `picture` are ordinary JSON members rather than * typed fields, and passing null omits them — which is exactly the case the name fallback - * exists for. `emailVerified` defaults to true because that is the ordinary case; the - * provisioner refuses an unverified address outright. + * exists for. */ private def payload( subject: String, email: String, name: String = "Given Name", - picture: String = "https://lh3.googleusercontent.com/a/AVATAR-ID", - emailVerified: java.lang.Boolean = true + picture: String = "https://lh3.googleusercontent.com/a/AVATAR-ID" ): GoogleIdToken.Payload = { val p = new GoogleIdToken.Payload() p.setSubject(subject) p.setEmail(email) - p.setEmailVerified(emailVerified) if (name != null) p.set("name", name) if (picture != null) p.set("picture", picture) p @@ -150,37 +147,16 @@ class GoogleAuthResourceSpec userByEmail("blank").getName shouldBe "blank" + emailDomain } - it should "store the picture URL in full" in { + it should "store only the last path segment of the picture URL" in { loginWith(payload("google-sub-avatar", "avatar" + emailDomain)) - userByEmail("avatar").getAvatar shouldBe "https://lh3.googleusercontent.com/a/AVATAR-ID" + userByEmail("avatar").getAvatar shouldBe "AVATAR-ID" } - it should "leave the avatar unset when the payload carries no picture" in { + it should "store an empty avatar when the payload carries no picture" in { loginWith(payload("google-sub-nopic", "nopic" + emailDomain, picture = null)) - userByEmail("nopic").getAvatar shouldBe null - } - - it should "keep an already-stored avatar when a later login carries no picture" in { - val stored = "https://lh3.googleusercontent.com/a/AVATAR-ID" - loginWith(payload("google-sub-keeppic", "keeppic" + emailDomain)) - userByEmail("keeppic").getAvatar shouldBe stored - - loginWith(payload("google-sub-keeppic", "keeppic" + emailDomain, picture = null)) - - userByEmail("keeppic").getAvatar shouldBe stored - } - - // ---- unverified email ---------------------------------------------------- - - it should "reject a payload whose email Google has not verified" in { - val resource = new StubbedGoogleAuthResource( - Some(payload("google-sub-unverified", "unverified" + emailDomain, emailVerified = false)) - ) - - assertThrows[NotAuthorizedException](resource.login("stubbed-credential")) - userByEmail("unverified") shouldBe null + userByEmail("nopic").getAvatar shouldBe "" } // ---- verification failure ------------------------------------------------ @@ -191,14 +167,6 @@ class GoogleAuthResourceSpec a[NotAuthorizedException] should be thrownBy resource.login("not-a-real-credential") } - // The one case that runs the real `verifiedPayload` rather than the stub. A string that is not - // three dot-separated parts fails the local JWT parse, so no request to Google is made and this - // stays a unit test. The parse failure is swallowed into None, so a malformed credential yields - // the same 401 as any other bad credential instead of escaping as a 500. - it should "reject a malformed credential with a 401 before reaching Google" in { - a[NotAuthorizedException] should be thrownBy new GoogleAuthResource().login("not-a-jwt") - } - // ---- client id ----------------------------------------------------------- behavior of "getClientId" diff --git a/frontend/src/app/common/service/user/stub-user.service.ts b/frontend/src/app/common/service/user/stub-user.service.ts index af9be1c0c64..3f2fc82a23f 100644 --- a/frontend/src/app/common/service/user/stub-user.service.ts +++ b/frontend/src/app/common/service/user/stub-user.service.ts @@ -83,7 +83,7 @@ export class StubUserService implements PublicInterfaceOf { return this.user; } - getAvatar(avatarUrl: string): Observable { + getAvatar(googleAvatar: string): Observable { return of(undefined); } diff --git a/frontend/src/app/common/service/user/user.service.spec.ts b/frontend/src/app/common/service/user/user.service.spec.ts index bee3facd6d2..f4792d4e65c 100644 --- a/frontend/src/app/common/service/user/user.service.spec.ts +++ b/frontend/src/app/common/service/user/user.service.spec.ts @@ -207,17 +207,13 @@ describe("UserService", () => { // ─── avatar fetching ────────────────────────────────────────────────────── - // The stored value is the provider's complete URL, not a Google-specific fragment, so it is - // fetched as-is and is also the cache key. - const AVATAR_URL = "https://lh3.googleusercontent.com/a/AVATAR-ID"; - - it("getAvatar returns undefined for an empty avatar url", async () => { + it("getAvatar returns undefined for an empty avatar id", async () => { expect(await firstValueFrom(service.getAvatar(""))).toBeUndefined(); }); it("getAvatar returns the cached object URL while the entry is still fresh", async () => { - (service as any).cache.set(AVATAR_URL, { url: "blob:cached", expiry: Date.now() + 60_000 }); - expect(await firstValueFrom(service.getAvatar(AVATAR_URL))).toBe("blob:cached"); + (service as any).cache.set("cached-id", { url: "blob:cached", expiry: Date.now() + 60_000 }); + expect(await firstValueFrom(service.getAvatar("cached-id"))).toBe("blob:cached"); }); describe("getAvatar network path", () => { @@ -241,29 +237,18 @@ describe("UserService", () => { globalThis.fetch = vi.fn().mockResolvedValue({ ok: true, blob: () => Promise.resolve(blob) }) as any; URL.createObjectURL = vi.fn().mockReturnValue("blob:fetched"); - const result = await firstValueFrom(service.getAvatar(AVATAR_URL)); + const result = await firstValueFrom(service.getAvatar("remote-id")); expect(result).toBe("blob:fetched"); - // fetched verbatim — no CDN prefix is reconstructed here any more - expect(globalThis.fetch).toHaveBeenCalledWith(AVATAR_URL, { + expect(globalThis.fetch).toHaveBeenCalledWith("https://lh3.googleusercontent.com/a/remote-id", { referrerPolicy: "no-referrer", }); expect(URL.createObjectURL).toHaveBeenCalledWith(blob); }); - it("fetches an avatar hosted anywhere the backend allowed, not just Google's CDN", async () => { - const blob = new Blob(["img"]); - globalThis.fetch = vi.fn().mockResolvedValue({ ok: true, blob: () => Promise.resolve(blob) }) as any; - URL.createObjectURL = vi.fn().mockReturnValue("blob:other"); - - const otherHost = "https://avatars.example-provider.com/u/12345"; - expect(await firstValueFrom(service.getAvatar(otherHost))).toBe("blob:other"); - expect(globalThis.fetch).toHaveBeenCalledWith(otherHost, { referrerPolicy: "no-referrer" }); - }); - it("returns undefined when the avatar fetch fails", async () => { globalThis.fetch = vi.fn().mockResolvedValue({ ok: false, status: 500 }) as any; - expect(await firstValueFrom(service.getAvatar("https://lh3.googleusercontent.com/a/BAD"))).toBeUndefined(); + expect(await firstValueFrom(service.getAvatar("bad-id"))).toBeUndefined(); }); }); }); diff --git a/frontend/src/app/common/service/user/user.service.ts b/frontend/src/app/common/service/user/user.service.ts index c02a80effb6..2905c2e8925 100644 --- a/frontend/src/app/common/service/user/user.service.ts +++ b/frontend/src/app/common/service/user/user.service.ts @@ -152,32 +152,24 @@ export class UserService { return { result: true, message: "Email frontend validation success." }; } - /** - * Fetch the avatar at `avatarUrl` and expose it as an object URL, cached for `cacheDuration`. - * - * `avatarUrl` is the complete URL the identity provider supplied, stored as-is on the user - * record. It used to be only the last path segment of Google's `picture` claim, with this - * method rebuilding `https://lh3.googleusercontent.com/a/` around it — which made - * the stored value unusable for any other provider. The backend allowlists the host before - * storing it, so the value that arrives here has already been validated. - */ - getAvatar(avatarUrl: string): Observable { - if (!avatarUrl) return of(undefined); + getAvatar(googleAvatar: string): Observable { + if (!googleAvatar) return of(undefined); - const cached = this.cache.get(avatarUrl); + const cached = this.cache.get(googleAvatar); if (cached) { if (Date.now() <= cached.expiry) { return of(cached.url); } else { URL.revokeObjectURL(cached.url); - this.cache.delete(avatarUrl); + this.cache.delete(googleAvatar); } } - return this.fetchBlob(avatarUrl).pipe( + const url = `https://lh3.googleusercontent.com/a/${googleAvatar}`; + return this.fetchBlob(url).pipe( map(blob => { const blobUrl = URL.createObjectURL(blob); - this.cache.set(avatarUrl, { + this.cache.set(googleAvatar, { url: blobUrl, expiry: Date.now() + this.cacheDuration, }); diff --git a/frontend/src/app/dashboard/component/user/user-avatar/user-avatar.component.ts b/frontend/src/app/dashboard/component/user/user-avatar/user-avatar.component.ts index 4f2b38a9be0..9ffdf900dd4 100644 --- a/frontend/src/app/dashboard/component/user/user-avatar/user-avatar.component.ts +++ b/frontend/src/app/dashboard/component/user/user-avatar/user-avatar.component.ts @@ -34,9 +34,8 @@ import { NzAvatarComponent } from "ng-zorro-antd/avatar"; /** * UserAvatarComponent is used to show the avatar of a user - * A user provisioned through an identity provider shows that provider's profile picture, - * fetched from the complete URL stored on the user record - * A user without one shows a default avatar with their initials + * The avatar of a Google user will be its Google profile picture + * The avatar of a normal user will be a default one with the initial */ export class UserAvatarComponent implements OnChanges { @Input() googleAvatar?: string; diff --git a/sql/texera_ddl.sql b/sql/texera_ddl.sql index 71c76055a01..c4891f07cc7 100644 --- a/sql/texera_ddl.sql +++ b/sql/texera_ddl.sql @@ -106,7 +106,7 @@ CREATE TABLE IF NOT EXISTS "user" uid SERIAL PRIMARY KEY, name VARCHAR(256) NOT NULL, email VARCHAR(256) UNIQUE, - avatar VARCHAR(512), + avatar VARCHAR(100), role user_role_enum NOT NULL DEFAULT 'INACTIVE', comment TEXT, account_creation_time TIMESTAMPTZ NOT NULL DEFAULT now(), @@ -114,10 +114,6 @@ CREATE TABLE IF NOT EXISTS "user" joining_reason VARCHAR(500), -- placeholder accounts are auto-created for dataset contributors and carry no credentials until claimed is_placeholder BOOLEAN NOT NULL DEFAULT FALSE - -- ck_nulltest ("every non-placeholder account has a credential") is deliberately not - -- carried over: credentials now live in auth_provider, so the rule spans two tables and - -- cannot be a row-level CHECK. A user with no auth_provider row is legal and simply - -- cannot log in; migration 32 reports any it finds. ); CREATE TABLE IF NOT EXISTS auth_provider diff --git a/sql/updates/32.sql b/sql/updates/32.sql index fad02c00633..999ca8e13de 100644 --- a/sql/updates/32.sql +++ b/sql/updates/32.sql @@ -17,19 +17,13 @@ * under the License. */ --- Relocate login credentials out of "user" into auth_provider, and make the avatar column --- provider-neutral. +-- Relocate login credentials out of "user" into auth_provider. -- --- Part 1 moves `password` / `google_id` into an auth_provider row per (user, provider), so a --- user can hold several external identities instead of exactly one Google account. --- --- Part 2 promotes the avatar from a Google-specific URL fragment to the provider's complete --- URL. It used to hold only the last path segment of Google's `picture` claim, with the --- frontend rebuilding `https://lh3.googleusercontent.com/a/` around it, which made --- the column meaningless for any other provider. --- --- Both parts run in one transaction: part 2 depends on the google_avatar -> avatar rename in --- part 1, so they cannot be applied independently. +-- Moves `password` / `google_id` into an auth_provider row per (user, provider), so a user can +-- hold several external identities instead of exactly one Google account, and renames +-- `google_avatar` to the provider-neutral `avatar`. The rename is in place: the column keeps +-- its width and every stored value, so this migration does not change what any user's avatar +-- resolves to. \c texera_db @@ -37,10 +31,6 @@ SET search_path TO texera_db; BEGIN; --- ============================================================================ --- Part 1: credentials move to auth_provider --- ============================================================================ - DO $$ BEGIN IF NOT EXISTS (SELECT 1 FROM pg_type WHERE typname = 'provider_type_enum') THEN @@ -191,24 +181,4 @@ ALTER TABLE "user" DROP CONSTRAINT IF EXISTS ck_nulltest; ALTER TABLE "user" DROP COLUMN IF EXISTS password; ALTER TABLE "user" DROP COLUMN IF EXISTS google_id; --- ============================================================================ --- Part 2: the avatar becomes the provider's full URL --- ============================================================================ - --- A full URL does not fit in the old width. -ALTER TABLE "user" ALTER COLUMN avatar TYPE VARCHAR(512); - --- Pictureless Google logins used to record an empty string; NULL is now the single --- representation of "this user has no avatar", so the frontend has one case to handle. -UPDATE "user" SET avatar = NULL WHERE avatar = ''; - --- Promote the remaining bare fragments to absolute URLs. The `NOT LIKE` guard makes this --- idempotent and leaves already-absolute values (from a re-run, or from a provider added --- after this migration) untouched. -UPDATE "user" -SET avatar = 'https://lh3.googleusercontent.com/a/' || avatar -WHERE avatar IS NOT NULL - AND avatar NOT LIKE 'http://%' - AND avatar NOT LIKE 'https://%'; - COMMIT; From ac8b90b5a449e38fc45d26b4af7b0bd54d889f06 Mon Sep 17 00:00:00 2001 From: Neilk1021 Date: Wed, 5 Aug 2026 11:28:17 -0700 Subject: [PATCH 26/54] refactor(auth): drop unread localHandle and fix stale docs localHandle and its local_provider LEFT JOIN had no reader: the admin UI types /admin/user/list as the frontend User, which has no such field, so the value was dropped at the TS boundary. Removed both rather than ship unread API surface. The admin still cannot see or change a local user's login handle -- that gap is a follow-up, not something to fix by leaving an unused column in the payload. Kept and strengthened the positional-mapping comment on the projection, since removing a projected column is exactly the hazard it warns about. Doc corrections: - AuthResourceSpec pointed at insertLocalUser, which this PR renamed away; the seam it mirrors is LocalAuthProvisioner.createLocalAccount - JwtAuth said "the four specs", which is three files and four call sites; the count is dropped so it cannot rot again - AuthResource's race comment had no subject and read as though the caller loses the race --- .../web/resource/auth/AuthResource.scala | 2 +- .../admin/user/AdminUserResource.scala | 13 ++++-------- .../web/resource/auth/AuthResourceSpec.scala | 5 ++++- .../admin/user/AdminUserResourceSpec.scala | 20 ++++++++----------- .../org/apache/texera/auth/JwtAuth.scala | 2 +- 5 files changed, 18 insertions(+), 24 deletions(-) diff --git a/amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala b/amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala index 5f0eeaf78f3..84f9788e0cb 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala @@ -196,7 +196,7 @@ class AuthResource { user.setName(username) user.setEmail(useremail) user.setRole(UserRoleEnum.RESTRICTED) - // Loses the race to a concurrent registration of the same handle as a 409. + // Reports losing the race to a concurrent registration of the same handle as a 409. LocalAuthProvisioner.createLocalAccount(user, username, userpassword) TokenIssueResponse(jwtToken(jwtClaims(user, TOKEN_EXPIRE_TIME_IN_MINUTES))) } diff --git a/amber/src/main/scala/org/apache/texera/web/resource/dashboard/admin/user/AdminUserResource.scala b/amber/src/main/scala/org/apache/texera/web/resource/dashboard/admin/user/AdminUserResource.scala index 0f5ef2c9c76..b68e73bfe41 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/dashboard/admin/user/AdminUserResource.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/dashboard/admin/user/AdminUserResource.scala @@ -43,7 +43,6 @@ case class UserInfo( name: String, email: String, googleId: String, - localHandle: String, role: UserRoleEnum, // `"user".avatar` is no longer Google-specific, but this is the JSON key // `admin-user.component.html` binds, so the wire name stays until the frontend migrates. @@ -79,7 +78,6 @@ class AdminUserResource { def list(): util.List[UserInfo] = { val googleProvider = AUTH_PROVIDER.as("google_provider") - val localProvider = AUTH_PROVIDER.as("local_provider") AdminUserResource.context .select( @@ -88,11 +86,11 @@ class AdminUserResource { USER.EMAIL, // fetchInto maps onto a Scala case class POSITIONALLY, not by name: a case class has no // no-arg constructor, so jOOQ falls through to ImmutablePOJOMapper. So the column order - // below must track the UserInfo field order. `last_active_time` landing on `lastLogin` two - // entries down only works because of that. The aliases are documentation (both joins - // project a column called `provider_id`); they do not drive the mapping. + // below must track the UserInfo field order — adding, removing or reordering a projected + // column here without doing the same to UserInfo silently shifts every later field. + // `last_active_time` landing on `lastLogin` only works because of that. The aliases are + // documentation; they do not drive the mapping. googleProvider.PROVIDER_ID.as("googleId"), - localProvider.PROVIDER_ID.as("localHandle"), USER.ROLE, USER.AVATAR.as("googleAvatar"), USER.COMMENT, @@ -108,9 +106,6 @@ class AdminUserResource { .leftJoin(googleProvider) .on(googleProvider.PROVIDER_TYPE.eq(ProviderTypeEnum.GOOGLE)) .and(googleProvider.UID.eq(USER.UID)) - .leftJoin(localProvider) - .on(localProvider.PROVIDER_TYPE.eq(ProviderTypeEnum.LOCAL)) - .and(localProvider.UID.eq(USER.UID)) .fetchInto(classOf[UserInfo]) } diff --git a/amber/src/test/scala/org/apache/texera/web/resource/auth/AuthResourceSpec.scala b/amber/src/test/scala/org/apache/texera/web/resource/auth/AuthResourceSpec.scala index 8bdd62a5cff..4ecf657c4e3 100644 --- a/amber/src/test/scala/org/apache/texera/web/resource/auth/AuthResourceSpec.scala +++ b/amber/src/test/scala/org/apache/texera/web/resource/auth/AuthResourceSpec.scala @@ -77,7 +77,10 @@ class AuthResourceSpec getDSLContext.deleteFrom(USER).where(USER.NAME.eq(UserSystemConfig.adminUsername)).execute() } - /** Seed a user plus the LOCAL auth_provider row it logs in with, mirroring `insertLocalUser`. */ + /** + * Seed a user plus the LOCAL auth_provider row it logs in with, mirroring + * `LocalAuthProvisioner.createLocalAccount`. + */ private def seedUser( name: String, password: String, diff --git a/amber/src/test/scala/org/apache/texera/web/resource/dashboard/admin/user/AdminUserResourceSpec.scala b/amber/src/test/scala/org/apache/texera/web/resource/dashboard/admin/user/AdminUserResourceSpec.scala index e1c72341af5..644954916d7 100644 --- a/amber/src/test/scala/org/apache/texera/web/resource/dashboard/admin/user/AdminUserResourceSpec.scala +++ b/amber/src/test/scala/org/apache/texera/web/resource/dashboard/admin/user/AdminUserResourceSpec.scala @@ -188,11 +188,10 @@ class AdminUserResourceSpec resource.list().asScala.exists(_.uid == primaryUid) shouldBe false } - // The projection maps onto UserInfo positionally, and it left-joins auth_provider twice for - // the two credential kinds. Nothing else observes that the columns land on the fields they - // are meant to, which is the whole risk of a positional mapping — so pin it here for a user - // holding both credentials at once. - it should "report both credential handles and the avatar for a user with LOCAL and GOOGLE rows" in { + // The projection maps onto UserInfo positionally, so a column landing on the wrong field is + // silent. Nothing else observes it — pin it here for a user holding both credential kinds, + // which also proves the LOCAL row does not leak into the GOOGLE-joined column. + it should "report the google id and the avatar for a user with LOCAL and GOOGLE rows" in { val user = makeUser(primaryUid, "dual") user.setAvatar("avatar-blob") userDao.insert(user) @@ -201,18 +200,15 @@ class AdminUserResourceSpec val listed = resource.list().asScala.find(_.uid == primaryUid) - listed.map(u => (u.name, u.localHandle, u.googleId, u.googleAvatar)) shouldBe Some( - ("dual", "dual-handle", "google-sub-dual", "avatar-blob") + listed.map(u => (u.name, u.googleId, u.googleAvatar)) shouldBe Some( + ("dual", "google-sub-dual", "avatar-blob") ) } - it should "leave the credential handles null for a user with no auth_provider rows" in { + it should "leave the google id null for a user with no auth_provider rows" in { userDao.insert(makeUser(primaryUid, "credential-less")) - val listed = resource.list().asScala.find(_.uid == primaryUid) - - listed.map(_.localHandle) shouldBe Some(null) - listed.map(_.googleId) shouldBe Some(null) + resource.list().asScala.find(_.uid == primaryUid).map(_.googleId) shouldBe Some(null) } // ─── addUser ──────────────────────────────────────────────────────────── diff --git a/common/auth/src/main/scala/org/apache/texera/auth/JwtAuth.scala b/common/auth/src/main/scala/org/apache/texera/auth/JwtAuth.scala index df6b77c5288..f4d99bcac29 100644 --- a/common/auth/src/main/scala/org/apache/texera/auth/JwtAuth.scala +++ b/common/auth/src/main/scala/org/apache/texera/auth/JwtAuth.scala @@ -59,7 +59,7 @@ object JwtAuth { * `googleAvatar` name until the frontend is migrated in lockstep. * * `googleId` is passed in rather than read off `user`, because the GOOGLE provider id lives - * in `auth_provider` and this module must stay DB-free: the four specs in + * in `auth_provider` and this module must stay DB-free: the specs in * `access-control-service` / `config-service` and the token re-issue paths in * `ResultExportService` / `ComputingUnitManagingResource` all call this with no * `auth_provider` context. Those re-issued tokens are service-to-service and never reach From b0b411f4900b1b2f73c85684531cb76c89c457af Mon Sep 17 00:00:00 2001 From: Neilk1021 Date: Wed, 5 Aug 2026 12:22:23 -0700 Subject: [PATCH 27/54] refactor(auth): port the new admin computing-unit spec to auth_provider AdminComputingUnitResourceSpec arrived with #6854 while this branch was open and seeds its fixture with setPassword and setGoogleAvatar, neither of which the User pojo still has. Nothing conflicts textually -- the file is new and this branch never touched it -- so the merge is clean and only the compile catches it. Dropped the password (credentials live in auth_provider, and this spec exercises the listing rather than login) and moved the avatar to the renamed column. The value is preserved because the suite asserts ownerGoogleAvatar downstream. --- .../service/resource/AdminComputingUnitResourceSpec.scala | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/computing-unit-managing-service/src/test/scala/org/apache/texera/service/resource/AdminComputingUnitResourceSpec.scala b/computing-unit-managing-service/src/test/scala/org/apache/texera/service/resource/AdminComputingUnitResourceSpec.scala index a721986b695..f3d2f9e498e 100644 --- a/computing-unit-managing-service/src/test/scala/org/apache/texera/service/resource/AdminComputingUnitResourceSpec.scala +++ b/computing-unit-managing-service/src/test/scala/org/apache/texera/service/resource/AdminComputingUnitResourceSpec.scala @@ -67,8 +67,10 @@ class AdminComputingUnitResourceSpec u.setName(name) u.setEmail(s"user$uid@example.com") u.setRole(UserRoleEnum.ADMIN) - u.setPassword("password") - u.setGoogleAvatar(s"avatar-$uid") + // Credentials live in auth_provider now, and this spec exercises the listing rather than + // login, so the user needs none. The avatar column is provider-neutral; the DTO field it + // feeds is still named ownerGoogleAvatar. + u.setAvatar(s"avatar-$uid") u } From 6456fdb8a9dd86f6c5d045c7f496ef5a0e939e1c Mon Sep 17 00:00:00 2001 From: Neilk1021 Date: Sun, 9 Aug 2026 16:39:43 -0700 Subject: [PATCH 28/54] fix(auth): fix jwtClaims signature --- .../apache/texera/AccessControlResourceSpec.scala | 2 +- .../service/resource/LiteLLMProxyAuthSpec.scala | 2 +- .../texera/web/resource/auth/AuthResource.scala | 8 ++++---- .../resource/auth/ExternalAuthProvisioner.scala | 8 -------- .../web/resource/auth/GoogleAuthResource.scala | 2 +- .../texera/web/service/ResultExportService.scala | 2 +- .../scala/org/apache/texera/auth/JwtAuth.scala | 2 +- .../scala/org/apache/texera/auth/JwtAuthSpec.scala | 14 +++++++------- .../resource/ComputingUnitManagingResource.scala | 2 +- .../service/resource/ConfigResourceSpec.scala | 4 ++-- 10 files changed, 19 insertions(+), 27 deletions(-) diff --git a/access-control-service/src/test/scala/org/apache/texera/AccessControlResourceSpec.scala b/access-control-service/src/test/scala/org/apache/texera/AccessControlResourceSpec.scala index b2214eb2b97..b4f1b89617f 100644 --- a/access-control-service/src/test/scala/org/apache/texera/AccessControlResourceSpec.scala +++ b/access-control-service/src/test/scala/org/apache/texera/AccessControlResourceSpec.scala @@ -137,7 +137,7 @@ class AccessControlResourceSpec computingUnitOfUserDao.insert(cuAccess) } - val claims = JwtAuth.jwtClaims(testUser1, 1) + val claims = JwtAuth.jwtClaims(testUser1) token = JwtAuth.jwtToken(claims) } diff --git a/access-control-service/src/test/scala/org/apache/texera/service/resource/LiteLLMProxyAuthSpec.scala b/access-control-service/src/test/scala/org/apache/texera/service/resource/LiteLLMProxyAuthSpec.scala index 77c31c62860..2fc5c10062a 100644 --- a/access-control-service/src/test/scala/org/apache/texera/service/resource/LiteLLMProxyAuthSpec.scala +++ b/access-control-service/src/test/scala/org/apache/texera/service/resource/LiteLLMProxyAuthSpec.scala @@ -165,7 +165,7 @@ class LiteLLMProxyAuthSpec extends AnyFlatSpec with Matchers with BeforeAndAfter u.setName("test") u.setEmail("test@example.com") u.setRole(role) - JwtAuth.jwtToken(JwtAuth.jwtClaims(u, expireInDays = 1)) + JwtAuth.jwtToken(JwtAuth.jwtClaims(u)) } private val chatBody = """{"model":"gpt-4o-mini","messages":[]}""" diff --git a/amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala b/amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala index 0f26ec0075e..4b5eed473a4 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala @@ -112,7 +112,7 @@ object AuthResource { if (LocalAuthProvisioner.handleExists(adminUsername)) return - if (userDao.fetchOneByEmail(adminUsername) != null) { + if (fetchUserByEmailIgnoreCase(adminUsername) != null) { logger.warn( s"Not creating the admin account: '$adminUsername' is already used as an email address " + "by an account with no local credential. Grant that account the ADMIN role instead." @@ -143,7 +143,7 @@ class AuthResource { // `googleId` in the token regardless of which one was used to sign in. val googleId = ExternalAuthProvisioner.providerIdOf(user.getUid, ProviderTypeEnum.GOOGLE) - TokenIssueResponse(jwtToken(jwtClaims(user, TOKEN_EXPIRE_TIME_IN_MINUTES, googleId))) + TokenIssueResponse(jwtToken(jwtClaims(user, googleId))) case None => throw new NotAuthorizedException("Login credentials are incorrect.") } } @@ -183,7 +183,7 @@ class AuthResource { existingByEmail.setName(username) claimPlaceholder(existingByEmail) LocalAuthProvisioner.claimWithLocalCredential(existingByEmail, username, userpassword) - return TokenIssueResponse(jwtToken(jwtClaims(existingByEmail, TOKEN_EXPIRE_TIME_IN_MINUTES))) + return TokenIssueResponse(jwtToken(jwtClaims(existingByEmail))) } (usernameExists, emailExists) match { @@ -198,7 +198,7 @@ class AuthResource { user.setRole(UserRoleEnum.INACTIVE) // Reports losing the race to a concurrent registration of the same handle as a 409. LocalAuthProvisioner.createLocalAccount(user, username, userpassword) - TokenIssueResponse(jwtToken(jwtClaims(user, TOKEN_EXPIRE_TIME_IN_MINUTES))) + TokenIssueResponse(jwtToken(jwtClaims(user))) } } diff --git a/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala b/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala index e2133a48174..b5daa837f15 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala @@ -88,23 +88,16 @@ object ExternalAuthProvisioner extends LazyLogging { .fetchOne() ) match { case Some(record) => - // known identity: refresh the profile fields if they drifted txUserDao.fetchOneByUid(record.get(USER.UID)).tap { user => if (refresh(user, profile)) txUserDao.update(user) } case None => - // First time we have seen this identity, so the email address is the only thing tying - // it to an account: either an existing one to link onto, or a new row that claims it. val user = userByEmailIgnoreCase(ctx, profile.email) match { case Some(existing) => existing.tap { user => - // A placeholder account (auto-created for a dataset contributor, never had a - // credential) is claimed by the first external identity that presents its email. - // It keeps its uid, so existing contributor links stay valid. val claimed = user.getIsPlaceholder if (claimed) AuthResource.claimPlaceholder(user) - // refresh() must run regardless so its mutations are applied val drifted = refresh(user, profile) if (drifted || claimed) txUserDao.update(user) } @@ -118,7 +111,6 @@ object ExternalAuthProvisioner extends LazyLogging { txUserDao.insert(created) created } catch { - // A concurrent registration of the same address won the race; adopt its row. case e: org.jooq.exception.DataAccessException if e.sqlState() == "23505" => userByEmailIgnoreCase(ctx, profile.email).getOrElse(throw e) } diff --git a/amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleAuthResource.scala b/amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleAuthResource.scala index 98d37430317..6f961a4c7f0 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleAuthResource.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleAuthResource.scala @@ -87,7 +87,7 @@ class GoogleAuthResource { // The frontend reads `googleId` off the raw token; the provider id is already in hand // here, so no lookup is needed. TokenIssueResponse( - jwtToken(jwtClaims(user, TOKEN_EXPIRE_TIME_IN_MINUTES, Some(profile.providerId))) + jwtToken(jwtClaims(user, Some(profile.providerId))) ) case None => throw new NotAuthorizedException("Login credentials are incorrect.") } diff --git a/amber/src/main/scala/org/apache/texera/web/service/ResultExportService.scala b/amber/src/main/scala/org/apache/texera/web/service/ResultExportService.scala index 605e6a9e582..2ce825c0418 100644 --- a/amber/src/main/scala/org/apache/texera/web/service/ResultExportService.scala +++ b/amber/src/main/scala/org/apache/texera/web/service/ResultExportService.scala @@ -550,7 +550,7 @@ class ResultExportService(workflowIdentity: WorkflowIdentity, computingUnitId: I connection.setRequestProperty("Content-Type", "application/octet-stream") connection.setRequestProperty( "Authorization", - s"Bearer ${JwtAuth.jwtToken(jwtClaims(user, TOKEN_EXPIRE_TIME_IN_MINUTES))}" + s"Bearer ${JwtAuth.jwtToken(jwtClaims(user))}" ) connection.setChunkedStreamingMode(0) diff --git a/common/auth/src/main/scala/org/apache/texera/auth/JwtAuth.scala b/common/auth/src/main/scala/org/apache/texera/auth/JwtAuth.scala index f4d99bcac29..4067fe8e889 100644 --- a/common/auth/src/main/scala/org/apache/texera/auth/JwtAuth.scala +++ b/common/auth/src/main/scala/org/apache/texera/auth/JwtAuth.scala @@ -65,7 +65,7 @@ object JwtAuth { * `auth_provider` context. Those re-issued tokens are service-to-service and never reach * the browser, so omitting the claim there is harmless. */ - def jwtClaims(user: User, expireInDays: Int, googleId: Option[String] = None): JwtClaims = { + def jwtClaims(user: User, googleId: Option[String] = None): JwtClaims = { val claims = new JwtClaims claims.setSubject(user.getName) claims.setClaim("userId", user.getUid) diff --git a/common/auth/src/test/scala/org/apache/texera/auth/JwtAuthSpec.scala b/common/auth/src/test/scala/org/apache/texera/auth/JwtAuthSpec.scala index d4d44db345f..48a2e24ce96 100644 --- a/common/auth/src/test/scala/org/apache/texera/auth/JwtAuthSpec.scala +++ b/common/auth/src/test/scala/org/apache/texera/auth/JwtAuthSpec.scala @@ -39,7 +39,7 @@ class JwtAuthSpec extends AnyFlatSpec with Matchers { } "JwtAuth.jwtClaims" should "map every User field onto the matching claim" in { - val claims = JwtAuth.jwtClaims(buildUser(), 7) + val claims = JwtAuth.jwtClaims(buildUser()) claims.getSubject shouldBe "alice" claims.getClaimValueAsString("userId") shouldBe "42" claims.getClaimValueAsString("email") shouldBe "alice@example.com" @@ -50,7 +50,7 @@ class JwtAuthSpec extends AnyFlatSpec with Matchers { // Passwords live in auth_provider and never leave it. `googleId` is a different matter: it is // an identifier, not a credential, and the frontend still reads it off the token. it should "not carry any password in the claims" in { - val claims = JwtAuth.jwtClaims(buildUser(), 7) + val claims = JwtAuth.jwtClaims(buildUser()) claims.hasClaim("password") shouldBe false claims.hasClaim("providerId") shouldBe false } @@ -58,18 +58,18 @@ class JwtAuthSpec extends AnyFlatSpec with Matchers { // The GOOGLE provider id is not on the User pojo any more, so callers with no auth_provider // context (service-to-service token re-issue) simply omit it rather than writing null. it should "omit the googleId claim when no provider id is supplied" in { - JwtAuth.jwtClaims(buildUser(), 7).hasClaim("googleId") shouldBe false + JwtAuth.jwtClaims(buildUser()).hasClaim("googleId") shouldBe false } it should "carry the googleId claim when a provider id is supplied" in { - val claims = JwtAuth.jwtClaims(buildUser(), 7, Some("google-sub-123")) + val claims = JwtAuth.jwtClaims(buildUser(), Some("google-sub-123")) claims.getClaimValueAsString("googleId") shouldBe "google-sub-123" } it should "derive the expiration from config, ignoring the expireInDays argument" in { // two very different expireInDays values must yield the same config-derived expiry window def expiryWindowMinutes(expireInDays: Int): Double = { - val claims = JwtAuth.jwtClaims(buildUser(), expireInDays) + val claims = JwtAuth.jwtClaims(buildUser()) claims.getExpirationTime should not be null claims.getExpirationTime.getValue / 60.0 - NumericDate.now().getValue / 60.0 } @@ -78,7 +78,7 @@ class JwtAuthSpec extends AnyFlatSpec with Matchers { } it should "produce a token that round-trips back to the same user via JwtParser" in { - val token = JwtAuth.jwtToken(JwtAuth.jwtClaims(buildUser(), 1)) + val token = JwtAuth.jwtToken(JwtAuth.jwtClaims(buildUser())) val parsed = JwtParser.parseToken(token) parsed.isPresent shouldBe true val user = parsed.get().getUser @@ -94,7 +94,7 @@ class JwtAuthSpec extends AnyFlatSpec with Matchers { user.setUid(7) user.setName("bob") user.setRole(UserRoleEnum.ADMIN) - val claims = JwtAuth.jwtClaims(user, 1) + val claims = JwtAuth.jwtClaims(user) claims.getSubject shouldBe "bob" claims.getClaimValueAsString("email") shouldBe null } diff --git a/computing-unit-managing-service/src/main/scala/org/apache/texera/service/resource/ComputingUnitManagingResource.scala b/computing-unit-managing-service/src/main/scala/org/apache/texera/service/resource/ComputingUnitManagingResource.scala index 4c50e1508b5..8a97aedae01 100644 --- a/computing-unit-managing-service/src/main/scala/org/apache/texera/service/resource/ComputingUnitManagingResource.scala +++ b/computing-unit-managing-service/src/main/scala/org/apache/texera/service/resource/ComputingUnitManagingResource.scala @@ -380,7 +380,7 @@ class ComputingUnitManagingResource { } val computingUnit = new WorkflowComputingUnit() - val userToken = JwtAuth.jwtToken(jwtClaims(user.user, TOKEN_EXPIRE_TIME_IN_MINUTES)) + val userToken = JwtAuth.jwtToken(jwtClaims(user.user)) computingUnit.setUid(user.getUid) computingUnit.setName(param.name) computingUnit.setCreationTime(new Timestamp(System.currentTimeMillis())) diff --git a/config-service/src/test/scala/org/apache/texera/service/resource/ConfigResourceSpec.scala b/config-service/src/test/scala/org/apache/texera/service/resource/ConfigResourceSpec.scala index b9d78fb48c9..295de41c563 100644 --- a/config-service/src/test/scala/org/apache/texera/service/resource/ConfigResourceSpec.scala +++ b/config-service/src/test/scala/org/apache/texera/service/resource/ConfigResourceSpec.scala @@ -104,7 +104,7 @@ class ConfigResourceSpec u.setName("test-regular") u.setEmail("test-regular@example.com") u.setRole(UserRoleEnum.REGULAR) - JwtAuth.jwtToken(JwtAuth.jwtClaims(u, expireInDays = 1)) + JwtAuth.jwtToken(JwtAuth.jwtClaims(u)) } private def adminToken(): String = { @@ -113,7 +113,7 @@ class ConfigResourceSpec u.setName("test-admin") u.setEmail("test-admin@example.com") u.setRole(UserRoleEnum.ADMIN) - JwtAuth.jwtToken(JwtAuth.jwtClaims(u, expireInDays = 1)) + JwtAuth.jwtToken(JwtAuth.jwtClaims(u)) } "GET /config/pre-login" should "return 200 without an Authorization header" in { From 32faa38a5f74f322f566c746f9ff331442256cf0 Mon Sep 17 00:00:00 2001 From: Neilk1021 Date: Sun, 9 Aug 2026 16:51:55 -0700 Subject: [PATCH 29/54] fix(auth): fix jwtClaims signature --- .../org/apache/texera/web/resource/auth/AuthResource.scala | 2 +- .../texera/web/resource/auth/GoogleAuthResourceSpec.scala | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala b/amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala index 4b5eed473a4..9b23ec9f778 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala @@ -48,7 +48,7 @@ object AuthResource { * Retrieve exactly one User from databases with the given username and password. * The password is used to validate against the hashed password stored in the db. * - * @param username String + * @param username the LOCAL login handle to authenticate * @param password String, plain text password * @return */ diff --git a/amber/src/test/scala/org/apache/texera/web/resource/auth/GoogleAuthResourceSpec.scala b/amber/src/test/scala/org/apache/texera/web/resource/auth/GoogleAuthResourceSpec.scala index 6324558d392..109d2d3de2d 100644 --- a/amber/src/test/scala/org/apache/texera/web/resource/auth/GoogleAuthResourceSpec.scala +++ b/amber/src/test/scala/org/apache/texera/web/resource/auth/GoogleAuthResourceSpec.scala @@ -39,7 +39,7 @@ import javax.ws.rs.NotAuthorizedException * network round trip — so the suite overrides `verifiedPayload` and drives the resource with * payloads built by hand. What it pins down is everything downstream of verification: how a * Google payload becomes an [[ExternalProfile]] (the name fallback and the avatar reduced to - * its last path segment) and that an unverifiable credential is a 401 rather than a crash. + * its last path segment) and that a credential Google does not verify is a 401. */ class GoogleAuthResourceSpec extends AnyFlatSpec From e30adc9892ad04069273419801f7782a4ed0c25f Mon Sep 17 00:00:00 2001 From: Neilk1021 Date: Sun, 9 Aug 2026 16:52:53 -0700 Subject: [PATCH 30/54] fix(auth): update changelog desc --- sql/changelog.xml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sql/changelog.xml b/sql/changelog.xml index 541ed4c4a57..586618f39b7 100644 --- a/sql/changelog.xml +++ b/sql/changelog.xml @@ -73,8 +73,8 @@ - + From 8273c1d396c46fe6d660fed040052be9976d0dfe Mon Sep 17 00:00:00 2001 From: Neilk1021 Date: Sun, 9 Aug 2026 17:00:07 -0700 Subject: [PATCH 31/54] fix(auth): ran scalafixAll --- .../org/apache/texera/web/resource/auth/AuthResource.scala | 2 +- .../apache/texera/web/resource/auth/GoogleAuthResource.scala | 2 +- .../org/apache/texera/web/service/ResultExportService.scala | 2 +- .../texera/service/resource/ComputingUnitManagingResource.scala | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala b/amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala index 9b23ec9f778..914d1082947 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala @@ -20,7 +20,7 @@ package org.apache.texera.web.resource.auth import com.typesafe.scalalogging.Logger -import org.apache.texera.auth.JwtAuth.{TOKEN_EXPIRE_TIME_IN_MINUTES, jwtClaims, jwtToken} +import org.apache.texera.auth.JwtAuth.{jwtClaims, jwtToken} import org.apache.texera.common.config.UserSystemConfig import org.apache.texera.common.util.EmailUtil import org.apache.texera.dao.SqlServer diff --git a/amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleAuthResource.scala b/amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleAuthResource.scala index 6f961a4c7f0..d64738cfb60 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleAuthResource.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleAuthResource.scala @@ -22,7 +22,7 @@ package org.apache.texera.web.resource.auth import com.google.api.client.googleapis.auth.oauth2.{GoogleIdToken, GoogleIdTokenVerifier} import com.google.api.client.http.javanet.NetHttpTransport import com.google.api.client.json.gson.GsonFactory -import org.apache.texera.auth.JwtAuth.{TOKEN_EXPIRE_TIME_IN_MINUTES, jwtClaims, jwtToken} +import org.apache.texera.auth.JwtAuth.{jwtClaims, jwtToken} import org.apache.texera.common.config.UserSystemConfig import org.apache.texera.dao.jooq.generated.enums.ProviderTypeEnum import org.apache.texera.web.model.http.response.TokenIssueResponse diff --git a/amber/src/main/scala/org/apache/texera/web/service/ResultExportService.scala b/amber/src/main/scala/org/apache/texera/web/service/ResultExportService.scala index 2ce825c0418..4e73a0e655f 100644 --- a/amber/src/main/scala/org/apache/texera/web/service/ResultExportService.scala +++ b/amber/src/main/scala/org/apache/texera/web/service/ResultExportService.scala @@ -36,7 +36,7 @@ import org.apache.arrow.vector.ipc.ArrowFileWriter import org.apache.commons.io.IOUtils import org.apache.commons.lang3.StringUtils import org.apache.texera.auth.JwtAuth -import org.apache.texera.auth.JwtAuth.{TOKEN_EXPIRE_TIME_IN_MINUTES, jwtClaims} +import org.apache.texera.auth.JwtAuth.jwtClaims import org.apache.texera.dao.jooq.generated.tables.pojos.User import org.apache.texera.web.model.http.request.result.{OperatorExportInfo, ResultExportRequest} import org.apache.texera.web.model.http.response.result.ResultExportResponse diff --git a/computing-unit-managing-service/src/main/scala/org/apache/texera/service/resource/ComputingUnitManagingResource.scala b/computing-unit-managing-service/src/main/scala/org/apache/texera/service/resource/ComputingUnitManagingResource.scala index 8a97aedae01..6e8a165e3ba 100644 --- a/computing-unit-managing-service/src/main/scala/org/apache/texera/service/resource/ComputingUnitManagingResource.scala +++ b/computing-unit-managing-service/src/main/scala/org/apache/texera/service/resource/ComputingUnitManagingResource.scala @@ -26,7 +26,7 @@ import jakarta.annotation.security.RolesAllowed import jakarta.ws.rs._ import jakarta.ws.rs.core.{MediaType, Response} import org.apache.commons.lang3.StringUtils -import org.apache.texera.auth.JwtAuth.{TOKEN_EXPIRE_TIME_IN_MINUTES, jwtClaims} +import org.apache.texera.auth.JwtAuth.jwtClaims import org.apache.texera.auth.{JwtAuth, SessionUser} import org.apache.texera.common.config.KubernetesConfig.{ cpuLimitOptions, From 6020d0deef02c2e28737b2d7e42f1bcc813b5c39 Mon Sep 17 00:00:00 2001 From: Neilk1021 Date: Sun, 9 Aug 2026 21:00:00 -0700 Subject: [PATCH 32/54] fix(auth): hoist provisioning into its own method to allow proper try catch and retry on duplicate login --- .../auth/ExternalAuthProvisioner.scala | 23 +++++++++++-------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala b/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala index b5daa837f15..45e50a765f1 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala @@ -72,7 +72,17 @@ object ExternalAuthProvisioner extends LazyLogging { * ensure its auth-provider row is present and up to date. Runs in a single * transaction and returns the (possibly newly created) user. */ - def loginOrProvision(profile: ExternalProfile): User = + def loginOrProvision(profile: ExternalProfile): User = { + + try{ + provision(profile) + } catch { + case e: org.jooq.exception.DataAccessException if e.sqlState() == "23505" => + provision(profile) + } + } + + private def provision(profile: ExternalProfile) = { SqlServer.withTransaction(SqlServer.getInstance().createDSLContext()) { ctx => val txUserDao = new UserDao(ctx.configuration()) val txAuthDao = new AuthProviderDao(ctx.configuration()) @@ -107,19 +117,14 @@ object ExternalAuthProvisioner extends LazyLogging { created.setEmail(profile.email) created.setAvatar(profile.avatar) created.setRole(UserRoleEnum.INACTIVE) - try { - txUserDao.insert(created) - created - } catch { - case e: org.jooq.exception.DataAccessException if e.sqlState() == "23505" => - userByEmailIgnoreCase(ctx, profile.email).getOrElse(throw e) - } + txUserDao.insert(created) + created } - upsertProvider(ctx, txAuthDao, user, profile) user } } + } /** The external id `uid` authenticates with at `providerType`, if it has one. */ def providerIdOf(uid: Integer, providerType: ProviderTypeEnum): Option[String] = From 658d85cb94aa570a6e69d21e430cca598fa05bd3 Mon Sep 17 00:00:00 2001 From: Neilk1021 Date: Sun, 9 Aug 2026 21:00:12 -0700 Subject: [PATCH 33/54] fix(auth): fix naming mistake --- sql/updates/33.sql | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sql/updates/33.sql b/sql/updates/33.sql index 999ca8e13de..d0bf53cbf21 100644 --- a/sql/updates/33.sql +++ b/sql/updates/33.sql @@ -109,13 +109,13 @@ BEGIN END IF; IF offenders IS NOT NULL THEN - RAISE EXCEPTION 'migration 32: cannot promote "user".name to a login handle - ' + RAISE EXCEPTION 'migration 33: cannot promote "user".name to a login handle - ' 'the following names are duplicated, blank, or whitespace-padded: %. ' 'Resolve them and re-run.', offenders; END IF; IF orphans IS NOT NULL THEN - RAISE NOTICE 'migration 32: uid(s) % have neither a password nor a google_id, so they ' + RAISE NOTICE 'migration 33: uid(s) % have neither a password nor a google_id, so they ' 'get no auth_provider row and cannot log in.', orphans; END IF; END From a27ddeca4c2eeaebcc55e50dcbc96a12f3fc9934 Mon Sep 17 00:00:00 2001 From: Neilk1021 Date: Sun, 9 Aug 2026 21:00:21 -0700 Subject: [PATCH 34/54] fix(auth): drop unneeded comments --- .../resource/dashboard/admin/user/AdminUserResourceSpec.scala | 1 - 1 file changed, 1 deletion(-) diff --git a/amber/src/test/scala/org/apache/texera/web/resource/dashboard/admin/user/AdminUserResourceSpec.scala b/amber/src/test/scala/org/apache/texera/web/resource/dashboard/admin/user/AdminUserResourceSpec.scala index 319fec272cf..b88fe2a8704 100644 --- a/amber/src/test/scala/org/apache/texera/web/resource/dashboard/admin/user/AdminUserResourceSpec.scala +++ b/amber/src/test/scala/org/apache/texera/web/resource/dashboard/admin/user/AdminUserResourceSpec.scala @@ -101,7 +101,6 @@ class AdminUserResourceSpec .where(DATASET.OWNER_UID.in(primaryUid, secondaryUid)) .execute() getDSLContext.deleteFrom(USER).where(USER.UID.in(primaryUid, secondaryUid)).execute() - // addUser() inserts an INACTIVE user with an auto-generated uid and a "User" name. getDSLContext .deleteFrom(USER) .where(USER.ROLE.eq(UserRoleEnum.INACTIVE).and(USER.NAME.like("User%"))) From 2bb5ce1a90eb841386acdcd15f584430dceaaa5e Mon Sep 17 00:00:00 2001 From: Neilk1021 Date: Sun, 9 Aug 2026 21:06:37 -0700 Subject: [PATCH 35/54] fix(auth): run sbtfmtAll --- .../texera/web/resource/auth/ExternalAuthProvisioner.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala b/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala index 45e50a765f1..bfc8b4cb587 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala @@ -74,7 +74,7 @@ object ExternalAuthProvisioner extends LazyLogging { */ def loginOrProvision(profile: ExternalProfile): User = { - try{ + try { provision(profile) } catch { case e: org.jooq.exception.DataAccessException if e.sqlState() == "23505" => From 8099703cf038b9e6276f5af4fb8221c41da98cbf Mon Sep 17 00:00:00 2001 From: Neil Ketteringham <53205839+Neilk1021@users.noreply.github.com> Date: Mon, 10 Aug 2026 07:06:08 -0700 Subject: [PATCH 36/54] Update amber/src/main/scala/org/apache/texera/web/resource/auth/LocalAuthProvisioner.scala Co-authored-by: Yicong Huang <17627829+Yicong-Huang@users.noreply.github.com> Signed-off-by: Neil Ketteringham <53205839+Neilk1021@users.noreply.github.com> --- .../texera/web/resource/auth/LocalAuthProvisioner.scala | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/amber/src/main/scala/org/apache/texera/web/resource/auth/LocalAuthProvisioner.scala b/amber/src/main/scala/org/apache/texera/web/resource/auth/LocalAuthProvisioner.scala index a0367144e6b..a1aee81bb2d 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/auth/LocalAuthProvisioner.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/auth/LocalAuthProvisioner.scala @@ -96,11 +96,10 @@ object LocalAuthProvisioner { } } catch { case e: DataAccessException if e.sqlState() == UNIQUE_VIOLATION => - throw new WebApplicationException( - s"Login handle $handle is already taken", - e, - Response.Status.CONFLICT - ) + val message = + if (handleExists(handle)) s"Login handle $handle is already taken" + else s"Email ${user.getEmail} is already registered" + throw new WebApplicationException(message, e, Response.Status.CONFLICT) } } From ae142256637e33e94233272ebf171dc7094cffc7 Mon Sep 17 00:00:00 2001 From: Neil Ketteringham <53205839+Neilk1021@users.noreply.github.com> Date: Mon, 10 Aug 2026 07:06:23 -0700 Subject: [PATCH 37/54] Update common/auth/src/main/scala/org/apache/texera/auth/JwtAuth.scala Co-authored-by: Yicong Huang <17627829+Yicong-Huang@users.noreply.github.com> Signed-off-by: Neil Ketteringham <53205839+Neilk1021@users.noreply.github.com> --- .../auth/src/main/scala/org/apache/texera/auth/JwtAuth.scala | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/common/auth/src/main/scala/org/apache/texera/auth/JwtAuth.scala b/common/auth/src/main/scala/org/apache/texera/auth/JwtAuth.scala index 4067fe8e889..3abf1af0bea 100644 --- a/common/auth/src/main/scala/org/apache/texera/auth/JwtAuth.scala +++ b/common/auth/src/main/scala/org/apache/texera/auth/JwtAuth.scala @@ -62,8 +62,9 @@ object JwtAuth { * in `auth_provider` and this module must stay DB-free: the specs in * `access-control-service` / `config-service` and the token re-issue paths in * `ResultExportService` / `ComputingUnitManagingResource` all call this with no - * `auth_provider` context. Those re-issued tokens are service-to-service and never reach - * the browser, so omitting the claim there is harmless. + * `auth_provider` context, as does `AuthResource.register`. Omitting the claim is harmless + * on all of them: a service-to-service token never reaches the browser, and a freshly + * registered LOCAL account has no Google identity to name. */ def jwtClaims(user: User, googleId: Option[String] = None): JwtClaims = { val claims = new JwtClaims From 379215c07112c5b2c0b8f95b18e3f8baca678ccf Mon Sep 17 00:00:00 2001 From: Neil Ketteringham <53205839+Neilk1021@users.noreply.github.com> Date: Mon, 10 Aug 2026 07:06:44 -0700 Subject: [PATCH 38/54] Update amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala Co-authored-by: Yicong Huang <17627829+Yicong-Huang@users.noreply.github.com> Signed-off-by: Neil Ketteringham <53205839+Neilk1021@users.noreply.github.com> --- .../texera/web/resource/auth/ExternalAuthProvisioner.scala | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala b/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala index bfc8b4cb587..d6a53192e65 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala @@ -69,8 +69,9 @@ object ExternalAuthProvisioner extends LazyLogging { /** * Resolve the user behind an external identity, creating one if necessary, and - * ensure its auth-provider row is present and up to date. Runs in a single - * transaction and returns the (possibly newly created) user. + * ensure its auth-provider row is present and up to date. Each attempt runs in one + * transaction; a unique violation means a concurrent login won the race, so the whole + * attempt is re-run once and resolves against the row that login committed. */ def loginOrProvision(profile: ExternalProfile): User = { From f655a5e579f154c7735e265c292994bcd44b1989 Mon Sep 17 00:00:00 2001 From: Neil Ketteringham <53205839+Neilk1021@users.noreply.github.com> Date: Mon, 10 Aug 2026 10:03:57 -0700 Subject: [PATCH 39/54] Update amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala Co-authored-by: Yicong Huang <17627829+Yicong-Huang@users.noreply.github.com> Signed-off-by: Neil Ketteringham <53205839+Neilk1021@users.noreply.github.com> --- .../texera/web/resource/auth/ExternalAuthProvisioner.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala b/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala index d6a53192e65..e6105d8d059 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala @@ -70,8 +70,8 @@ object ExternalAuthProvisioner extends LazyLogging { /** * Resolve the user behind an external identity, creating one if necessary, and * ensure its auth-provider row is present and up to date. Each attempt runs in one - * transaction; a unique violation means a concurrent login won the race, so the whole - * attempt is re-run once and resolves against the row that login committed. + * transaction. A unique violation is taken to mean a concurrent login won the race, so the + * attempt is re-run once; a violation from any other constraint then fails the same way. */ def loginOrProvision(profile: ExternalProfile): User = { From 33c958b64e4aa96295df133482b2849c70d787b7 Mon Sep 17 00:00:00 2001 From: Neil Ketteringham <53205839+Neilk1021@users.noreply.github.com> Date: Mon, 10 Aug 2026 10:04:13 -0700 Subject: [PATCH 40/54] Update amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleAuthResource.scala Co-authored-by: Yicong Huang <17627829+Yicong-Huang@users.noreply.github.com> Signed-off-by: Neil Ketteringham <53205839+Neilk1021@users.noreply.github.com> --- .../apache/texera/web/resource/auth/GoogleAuthResource.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleAuthResource.scala b/amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleAuthResource.scala index d64738cfb60..2d42d872c22 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleAuthResource.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleAuthResource.scala @@ -68,8 +68,8 @@ class GoogleAuthResource { /** * Verify `credential` against Google, yielding its payload, or None if it is not a valid - * token for this client. The only seam that reaches the network, so tests override it - * instead of signing a token; kept a method rather than a constructor parameter because + * token for this client. This is the only seam that reaches the network, so tests override + * it instead of signing a token; it is kept as a method rather than a parameter because * Jersey instantiates this resource from `classOf[GoogleAuthResource]`. */ protected def verifiedPayload(credential: String): Option[GoogleIdToken.Payload] = From 0c3d0f082536b6f2a723e1e87841545a48de876d Mon Sep 17 00:00:00 2001 From: Neil Ketteringham <53205839+Neilk1021@users.noreply.github.com> Date: Mon, 10 Aug 2026 10:04:56 -0700 Subject: [PATCH 41/54] Update amber/src/main/scala/org/apache/texera/web/resource/auth/LocalAuthProvisioner.scala Co-authored-by: Yicong Huang <17627829+Yicong-Huang@users.noreply.github.com> Signed-off-by: Neil Ketteringham <53205839+Neilk1021@users.noreply.github.com> --- .../texera/web/resource/auth/LocalAuthProvisioner.scala | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/amber/src/main/scala/org/apache/texera/web/resource/auth/LocalAuthProvisioner.scala b/amber/src/main/scala/org/apache/texera/web/resource/auth/LocalAuthProvisioner.scala index a1aee81bb2d..22ef9a4118f 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/auth/LocalAuthProvisioner.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/auth/LocalAuthProvisioner.scala @@ -126,11 +126,10 @@ object LocalAuthProvisioner { } } catch { case e: DataAccessException if e.sqlState() == UNIQUE_VIOLATION => - throw new WebApplicationException( - s"Login handle $handle is already taken", - e, - Response.Status.CONFLICT - ) + val message = + if (handleExists(handle)) s"Login handle $handle is already taken" + else s"Account for ${user.getEmail} has already been claimed" + throw new WebApplicationException(message, e, Response.Status.CONFLICT) } } From ac1f4089868bb9a53b2c452372e42f06a0bf0132 Mon Sep 17 00:00:00 2001 From: Neilk1021 Date: Mon, 10 Aug 2026 10:14:03 -0700 Subject: [PATCH 42/54] refactor(auth): migrate UNIQUE_VIOLATION constraint to AuthResource to be referenced by other files in Auth. --- .../org/apache/texera/web/resource/auth/AuthResource.scala | 4 ++-- .../texera/web/resource/auth/LocalAuthProvisioner.scala | 5 ++--- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala b/amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala index 914d1082947..6666d65169b 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala @@ -26,7 +26,6 @@ import org.apache.texera.common.util.EmailUtil import org.apache.texera.dao.SqlServer import org.apache.texera.dao.jooq.generated.Tables.{AUTH_PROVIDER, USER} import org.apache.texera.dao.jooq.generated.enums.{ProviderTypeEnum, UserRoleEnum} -import org.apache.texera.dao.jooq.generated.tables.daos.UserDao import org.apache.texera.dao.jooq.generated.tables.pojos.User import org.apache.texera.web.model.http.request.auth.{UserLoginRequest, UserRegistrationRequest} import org.apache.texera.web.model.http.response.TokenIssueResponse @@ -42,7 +41,8 @@ object AuthResource { private val logger: Logger = Logger(classOf[AuthResource]) private def context = SqlServer.getInstance().context - private def userDao = new UserDao(context.configuration) + + private[auth] val UNIQUE_VIOLATION = "23505" /** * Retrieve exactly one User from databases with the given username and password. diff --git a/amber/src/main/scala/org/apache/texera/web/resource/auth/LocalAuthProvisioner.scala b/amber/src/main/scala/org/apache/texera/web/resource/auth/LocalAuthProvisioner.scala index 22ef9a4118f..439a7e66959 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/auth/LocalAuthProvisioner.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/auth/LocalAuthProvisioner.scala @@ -42,7 +42,6 @@ import javax.ws.rs.core.Response object LocalAuthProvisioner { /** Postgres unique-violation SQLSTATE. */ - private val UNIQUE_VIOLATION = "23505" private val passwordEncryptor = new StrongPasswordEncryptor @@ -95,7 +94,7 @@ object LocalAuthProvisioner { txAuthDao.insert(auth) } } catch { - case e: DataAccessException if e.sqlState() == UNIQUE_VIOLATION => + case e: DataAccessException if e.sqlState() == AuthResource.UNIQUE_VIOLATION => val message = if (handleExists(handle)) s"Login handle $handle is already taken" else s"Email ${user.getEmail} is already registered" @@ -125,7 +124,7 @@ object LocalAuthProvisioner { new AuthProviderDao(ctx.configuration()).insert(auth) } } catch { - case e: DataAccessException if e.sqlState() == UNIQUE_VIOLATION => + case e: DataAccessException if e.sqlState() == AuthResource.UNIQUE_VIOLATION => val message = if (handleExists(handle)) s"Login handle $handle is already taken" else s"Account for ${user.getEmail} has already been claimed" From 88ba36b5ec84b12ba7c1b5ffd8f29f527aedcebc Mon Sep 17 00:00:00 2001 From: Neilk1021 Date: Mon, 10 Aug 2026 10:15:00 -0700 Subject: [PATCH 43/54] fix(auth): cleanup dead JwtAuthSpec test to stop using dead parameter. --- .../scala/org/apache/texera/auth/JwtAuthSpec.scala | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/common/auth/src/test/scala/org/apache/texera/auth/JwtAuthSpec.scala b/common/auth/src/test/scala/org/apache/texera/auth/JwtAuthSpec.scala index 48a2e24ce96..6ae84a4200d 100644 --- a/common/auth/src/test/scala/org/apache/texera/auth/JwtAuthSpec.scala +++ b/common/auth/src/test/scala/org/apache/texera/auth/JwtAuthSpec.scala @@ -66,15 +66,11 @@ class JwtAuthSpec extends AnyFlatSpec with Matchers { claims.getClaimValueAsString("googleId") shouldBe "google-sub-123" } - it should "derive the expiration from config, ignoring the expireInDays argument" in { - // two very different expireInDays values must yield the same config-derived expiry window - def expiryWindowMinutes(expireInDays: Int): Double = { - val claims = JwtAuth.jwtClaims(buildUser()) - claims.getExpirationTime should not be null - claims.getExpirationTime.getValue / 60.0 - NumericDate.now().getValue / 60.0 - } - expiryWindowMinutes(1) shouldBe (AuthConfig.jwtExpirationMinutes.toDouble +- 2.0) - expiryWindowMinutes(100000) shouldBe (AuthConfig.jwtExpirationMinutes.toDouble +- 2.0) + it should "derive the expiration from AuthConfig.jwtExpirationMinutes" in { + val claims = JwtAuth.jwtClaims(buildUser()) + claims.getExpirationTime should not be null + val windowMinutes = claims.getExpirationTime.getValue / 60.0 - NumericDate.now().getValue / 60.0 + windowMinutes shouldBe (AuthConfig.jwtExpirationMinutes.toDouble +- 2.0) } it should "produce a token that round-trips back to the same user via JwtParser" in { From 7f057ea8cd79425af0a466f25b6528f7ef343cc2 Mon Sep 17 00:00:00 2001 From: Yicong Huang <17627829+Yicong-Huang@users.noreply.github.com> Date: Mon, 10 Aug 2026 10:59:23 -0700 Subject: [PATCH 44/54] refactor(amber): read the unique-violation SQLSTATE from a shared constant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The constant added last round sat in `AuthResource` as `private[auth]`, which left the one other file in that package — `ExternalAuthProvisioner` — still spelling `"23505"` inline, and put a property of the database inside an HTTP resource. It now lives beside `SqlServer` in `common/dao`, where every caller that catches the code can reach it regardless of package. Also drops the scaladoc the move orphaned in `LocalAuthProvisioner` (it had come to document the password encryptor), gives `ExternalAuthProvisioner`'s retry comment a referent for what happens on the second failure, and completes a sentence in `WorkflowExecutionsResourceSpec`. Co-Authored-By: Claude Opus 5 (1M context) --- .../web/resource/auth/AuthResource.scala | 2 -- .../auth/ExternalAuthProvisioner.scala | 7 ++-- .../resource/auth/LocalAuthProvisioner.scala | 8 ++--- .../WorkflowExecutionsResourceSpec.scala | 4 +-- .../org/apache/texera/dao/SqlStates.scala | 34 +++++++++++++++++++ 5 files changed, 43 insertions(+), 12 deletions(-) create mode 100644 common/dao/src/main/scala/org/apache/texera/dao/SqlStates.scala diff --git a/amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala b/amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala index 6666d65169b..e9861f7bf95 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala @@ -42,8 +42,6 @@ object AuthResource { private def context = SqlServer.getInstance().context - private[auth] val UNIQUE_VIOLATION = "23505" - /** * Retrieve exactly one User from databases with the given username and password. * The password is used to validate against the hashed password stored in the db. diff --git a/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala b/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala index e6105d8d059..789eebc6c52 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala @@ -20,13 +20,14 @@ package org.apache.texera.web.resource.auth import com.typesafe.scalalogging.LazyLogging -import org.apache.texera.dao.SqlServer +import org.apache.texera.dao.{SqlServer, SqlStates} import org.apache.texera.dao.jooq.generated.Tables.{AUTH_PROVIDER, USER} import org.apache.texera.dao.jooq.generated.enums.{ProviderTypeEnum, UserRoleEnum} import org.apache.texera.dao.jooq.generated.tables.daos.{AuthProviderDao, UserDao} import org.apache.texera.dao.jooq.generated.tables.pojos.{AuthProvider, User} import org.apache.texera.common.util.EmailUtil import org.jooq.DSLContext +import org.jooq.exception.DataAccessException import org.jooq.impl.DSL import java.time.OffsetDateTime @@ -71,14 +72,14 @@ object ExternalAuthProvisioner extends LazyLogging { * Resolve the user behind an external identity, creating one if necessary, and * ensure its auth-provider row is present and up to date. Each attempt runs in one * transaction. A unique violation is taken to mean a concurrent login won the race, so the - * attempt is re-run once; a violation from any other constraint then fails the same way. + * attempt is re-run once; if the retry violates a constraint too, that exception propagates. */ def loginOrProvision(profile: ExternalProfile): User = { try { provision(profile) } catch { - case e: org.jooq.exception.DataAccessException if e.sqlState() == "23505" => + case e: DataAccessException if e.sqlState() == SqlStates.UNIQUE_VIOLATION => provision(profile) } } diff --git a/amber/src/main/scala/org/apache/texera/web/resource/auth/LocalAuthProvisioner.scala b/amber/src/main/scala/org/apache/texera/web/resource/auth/LocalAuthProvisioner.scala index 439a7e66959..22e11dcba04 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/auth/LocalAuthProvisioner.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/auth/LocalAuthProvisioner.scala @@ -19,7 +19,7 @@ package org.apache.texera.web.resource.auth -import org.apache.texera.dao.SqlServer +import org.apache.texera.dao.{SqlServer, SqlStates} import org.apache.texera.dao.jooq.generated.Tables.AUTH_PROVIDER import org.apache.texera.dao.jooq.generated.enums.{ProviderTypeEnum, UserRoleEnum} import org.apache.texera.dao.jooq.generated.tables.daos.{AuthProviderDao, UserDao} @@ -41,8 +41,6 @@ import javax.ws.rs.core.Response */ object LocalAuthProvisioner { - /** Postgres unique-violation SQLSTATE. */ - private val passwordEncryptor = new StrongPasswordEncryptor private def context = SqlServer.getInstance().context @@ -94,7 +92,7 @@ object LocalAuthProvisioner { txAuthDao.insert(auth) } } catch { - case e: DataAccessException if e.sqlState() == AuthResource.UNIQUE_VIOLATION => + case e: DataAccessException if e.sqlState() == SqlStates.UNIQUE_VIOLATION => val message = if (handleExists(handle)) s"Login handle $handle is already taken" else s"Email ${user.getEmail} is already registered" @@ -124,7 +122,7 @@ object LocalAuthProvisioner { new AuthProviderDao(ctx.configuration()).insert(auth) } } catch { - case e: DataAccessException if e.sqlState() == AuthResource.UNIQUE_VIOLATION => + case e: DataAccessException if e.sqlState() == SqlStates.UNIQUE_VIOLATION => val message = if (handleExists(handle)) s"Login handle $handle is already taken" else s"Account for ${user.getEmail} has already been claimed" diff --git a/amber/src/test/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowExecutionsResourceSpec.scala b/amber/src/test/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowExecutionsResourceSpec.scala index b1137812d55..a0a3622e06f 100644 --- a/amber/src/test/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowExecutionsResourceSpec.scala +++ b/amber/src/test/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowExecutionsResourceSpec.scala @@ -947,8 +947,8 @@ class WorkflowExecutionsResourceSpec // fetchInto maps onto WorkflowExecutionEntry POSITIONALLY (a case class has no no-arg // constructor, and jOOQ's mapConstructorParameterNames defaults to false), so `USER.AVATAR` // at projection position 5 lands on `googleAvatar` despite the names differing — exactly as - // `last_update_time` at position 9 lands on `completionTime`. Nothing observed either before, - // which is what makes an accidental column reorder silent. Pin both here. + // `last_update_time` at position 9 lands on `completionTime`. Neither mapping was asserted + // anywhere before, which is what makes an accidental column reorder silent. Pin both here. it should "map the owner's avatar and completion time onto the entry despite the name mismatch" in { grantReadAccess() insertExecution(lastUpdateOffsetMillis = Some(0L)) diff --git a/common/dao/src/main/scala/org/apache/texera/dao/SqlStates.scala b/common/dao/src/main/scala/org/apache/texera/dao/SqlStates.scala new file mode 100644 index 00000000000..0f9d76bffb9 --- /dev/null +++ b/common/dao/src/main/scala/org/apache/texera/dao/SqlStates.scala @@ -0,0 +1,34 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.dao + +/** + * SQLSTATE codes callers match on when turning a `DataAccessException` into an HTTP response. + * + * These live beside [[SqlServer]] rather than in whichever resource happens to need one first: + * the code is a property of the database, not of any one endpoint, and the callers that catch it + * sit in unrelated packages. A constant owned by one of those packages is unreachable from the + * rest, so each would keep its own literal. + */ +object SqlStates { + + /** Postgres unique-violation: a unique constraint or primary key was already satisfied. */ + val UNIQUE_VIOLATION = "23505" +} From 8be10a15785e20be82055b3536b107ac1c9b31c2 Mon Sep 17 00:00:00 2001 From: Yicong Huang <17627829+Yicong-Huang@users.noreply.github.com> Date: Mon, 10 Aug 2026 10:59:23 -0700 Subject: [PATCH 45/54] test(amber): pin LocalAuthProvisioner's unique-violation messages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both `createLocalAccount` and `claimWithLocalCredential` map a unique violation onto a 409 whose text names which constraint fired, and neither branch had a test. Three review rounds each found a defect inside these two blocks — the handler running in an already-aborted transaction, the sibling method the fix skipped, and the wrong constraint being named — so the branches were being corrected by inspection with nothing pinning the result. Each case drives the real constraint against the real DDL under embedded Postgres: `uq_provider_identity` and `user_email_key` for the insert path, `PRIMARY KEY (uid, provider_type)` and `uq_provider_identity` for the claim path. The assertions cover the cause named as well as the causes *not* named, since telling a user their free handle is taken was the last defect here, and they also pin the transaction boundary: a lost race leaves behind neither a credential-less account nor a placeholder flipped to claimed. Co-Authored-By: Claude Opus 5 (1M context) --- .../auth/LocalAuthProvisionerSpec.scala | 197 ++++++++++++++++++ 1 file changed, 197 insertions(+) create mode 100644 amber/src/test/scala/org/apache/texera/web/resource/auth/LocalAuthProvisionerSpec.scala diff --git a/amber/src/test/scala/org/apache/texera/web/resource/auth/LocalAuthProvisionerSpec.scala b/amber/src/test/scala/org/apache/texera/web/resource/auth/LocalAuthProvisionerSpec.scala new file mode 100644 index 00000000000..9a6b2653605 --- /dev/null +++ b/amber/src/test/scala/org/apache/texera/web/resource/auth/LocalAuthProvisionerSpec.scala @@ -0,0 +1,197 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.web.resource.auth + +import org.apache.texera.dao.MockTexeraDB +import org.apache.texera.dao.jooq.generated.Tables.{AUTH_PROVIDER, USER} +import org.apache.texera.dao.jooq.generated.enums.{ProviderTypeEnum, UserRoleEnum} +import org.apache.texera.dao.jooq.generated.tables.daos.{AuthProviderDao, UserDao} +import org.apache.texera.dao.jooq.generated.tables.pojos.{AuthProvider, User} +import org.jooq.impl.DSL +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers +import org.scalatest.{BeforeAndAfterAll, BeforeAndAfterEach} + +import javax.ws.rs.WebApplicationException +import javax.ws.rs.core.Response + +/** + * Integration spec for [[LocalAuthProvisioner]]'s unique-violation handling against embedded + * Postgres ([[MockTexeraDB]] loads the real `texera_ddl.sql`, so `uq_provider_identity`, + * `PRIMARY KEY (uid, provider_type)` and `user_email_key` are the constraints that actually + * fire here). + * + * These paths are only reachable by losing a race, so callers pre-check and the handlers are + * the fallback — which is exactly why they need a test rather than a caller. Each case drives + * the constraint directly and asserts the message names the cause that actually fired: naming + * the wrong one tells a user their free handle is taken, and the two write methods reach + * different constraint sets, so a fix applied to one does not cover the other. + */ +class LocalAuthProvisionerSpec + extends AnyFlatSpec + with Matchers + with BeforeAndAfterAll + with BeforeAndAfterEach + with MockTexeraDB { + + // Shared suffix so cleanup can target this suite's rows precisely; the auth_provider FK is + // ON DELETE CASCADE, so deleting the user clears its credential rows. + private val emailDomain = "@local-provisioner-test.com" + + private var userDao: UserDao = _ + private var authDao: AuthProviderDao = _ + + override protected def beforeAll(): Unit = { + initializeDBAndReplaceDSLContext() + userDao = new UserDao(getDSLContext.configuration()) + authDao = new AuthProviderDao(getDSLContext.configuration()) + } + + override protected def afterAll(): Unit = shutdownDB() + + override protected def beforeEach(): Unit = cleanup() + override protected def afterEach(): Unit = cleanup() + + private def cleanup(): Unit = + getDSLContext.deleteFrom(USER).where(DSL.lower(USER.EMAIL).like("%" + emailDomain)).execute() + + // ---- helpers ------------------------------------------------------------- + + private def newUser(name: String, localPart: String, placeholder: Boolean = false): User = { + val user = new User + user.setName(name) + user.setEmail(localPart + emailDomain) + user.setRole(UserRoleEnum.INACTIVE) + user.setIsPlaceholder(placeholder) + user + } + + /** Seed a user that already holds a LOCAL credential under `handle`. */ + private def seedLocalAccount(name: String, localPart: String, handle: String): User = { + val user = newUser(name, localPart) + user.setRole(UserRoleEnum.REGULAR) + userDao.insert(user) + val auth = new AuthProvider + auth.setUid(user.getUid) + auth.setProviderType(ProviderTypeEnum.LOCAL) + auth.setProviderId(handle) + auth.setPassword(LocalAuthProvisioner.hashPassword("seeded-pw")) + authDao.insert(auth) + user + } + + /** Seed a credential-less placeholder, the row a dataset contributor gets. */ + private def seedPlaceholder(localPart: String): User = { + val user = newUser(localPart, localPart, placeholder = true) + userDao.insert(user) + user + } + + private def userCountByEmail(localPart: String): Int = + getDSLContext.fetchCount(USER, DSL.lower(USER.EMAIL).eq(localPart + emailDomain)) + + private def conflictFrom(body: => Unit): WebApplicationException = { + val thrown = intercept[WebApplicationException](body) + thrown.getResponse.getStatus shouldBe Response.Status.CONFLICT.getStatusCode + thrown + } + + // ---- createLocalAccount -------------------------------------------------- + + "LocalAuthProvisioner.createLocalAccount" should + "report a taken handle when uq_provider_identity fires" in { + seedLocalAccount("Existing", "existing", handle = "shared-handle") + + val thrown = conflictFrom( + LocalAuthProvisioner.createLocalAccount(newUser("New", "fresh"), "shared-handle", "pw") + ) + + thrown.getMessage should include("Login handle shared-handle is already taken") + // The user insert and the credential insert share one transaction, so losing the race must + // not leave a credential-less account behind under the email that was being registered. + userCountByEmail("fresh") shouldBe 0 + } + + it should "report a registered email when user_email_key fires" in { + seedLocalAccount("Existing", "taken-email", handle = "handle-a") + + val thrown = conflictFrom( + LocalAuthProvisioner + .createLocalAccount(newUser("New", "taken-email"), "handle-b", "pw") + ) + + thrown.getMessage should include(s"Email taken-email$emailDomain is already registered") + // The seeded row is the only one; the losing insert rolled back rather than adding a second. + userCountByEmail("taken-email") shouldBe 1 + getDSLContext.fetchCount(AUTH_PROVIDER, AUTH_PROVIDER.PROVIDER_ID.eq("handle-b")) shouldBe 0 + } + + // ---- claimWithLocalCredential -------------------------------------------- + + "LocalAuthProvisioner.claimWithLocalCredential" should + "report an already-claimed account when the (uid, provider_type) primary key fires" in { + // A placeholder someone else already claimed: it holds a LOCAL row under *their* handle, + // so the handle this caller asks for is free and only the primary key can fire. + val claimed = seedPlaceholder("contributor") + val auth = new AuthProvider + auth.setUid(claimed.getUid) + auth.setProviderType(ProviderTypeEnum.LOCAL) + auth.setProviderId("first-claimer") + auth.setPassword(LocalAuthProvisioner.hashPassword("pw")) + authDao.insert(auth) + + claimed.setIsPlaceholder(false) + val thrown = conflictFrom( + LocalAuthProvisioner.claimWithLocalCredential(claimed, "second-claimer", "pw") + ) + + thrown.getMessage should include( + s"Account for contributor$emailDomain has already been claimed" + ) + // The handle was free, so reporting it as taken would be the wrong cause. + thrown.getMessage should not include "already taken" + // The claim and the credential share one transaction, so the rollback must leave the + // placeholder flag as it was rather than marking an account claimed with no new credential. + userDao.fetchOneByUid(claimed.getUid).getIsPlaceholder shouldBe true + providerIdOf(claimed.getUid) shouldBe "first-claimer" + } + + it should "report a taken handle when uq_provider_identity fires" in { + seedLocalAccount("Other", "other", handle = "wanted-handle") + val placeholder = seedPlaceholder("claimant") + + placeholder.setIsPlaceholder(false) + val thrown = conflictFrom( + LocalAuthProvisioner.claimWithLocalCredential(placeholder, "wanted-handle", "pw") + ) + + thrown.getMessage should include("Login handle wanted-handle is already taken") + thrown.getMessage should not include "has already been claimed" + userDao.fetchOneByUid(placeholder.getUid).getIsPlaceholder shouldBe true + } + + private def providerIdOf(uid: Integer): String = + getDSLContext + .select(AUTH_PROVIDER.PROVIDER_ID) + .from(AUTH_PROVIDER) + .where(AUTH_PROVIDER.UID.eq(uid)) + .and(AUTH_PROVIDER.PROVIDER_TYPE.eq(ProviderTypeEnum.LOCAL)) + .fetchOne(AUTH_PROVIDER.PROVIDER_ID) +} From 75a8af14b2da34705ff8fe3ae6f18462581329ce Mon Sep 17 00:00:00 2001 From: Yicong Huang <17627829+Yicong-Huang@users.noreply.github.com> Date: Mon, 10 Aug 2026 11:31:40 -0700 Subject: [PATCH 46/54] fix(amber): normalize login handles in migration 33 instead of refusing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The migration raised `RAISE EXCEPTION` when any password-holding account's name was blank, whitespace-padded, or shared with another. Those are reachable states today — `AdminUserResource.updateUser` writes `user.getName` through without trimming, and nothing has ever constrained `"user".name` to be unique — so a single such row turned changeset 33 into a failed liquibase changeset and a deployment that cannot start until an operator hand-edits the database. Nothing about minting a handle for the first time requires the old name to be clean, so it is normalized: trimmed, replaced by `user-` when trimming leaves nothing, and suffixed with `-` when it still collides (the lowest uid keeps the unsuffixed form). The de-duplication runs inside the filling UPDATE rather than only in the following pass, because `uq_provider_identity` is already in force and two accounts named "john" otherwise collide with each other within that single statement. The bounded loop then resolves a suffixed handle that collides with a literal one, following 28.sql's dataset-name precedent. The LOCAL backfill now inserts the row without a handle and lets that UPDATE mint it, since NULLs do not collide under the unique constraint — inserting the raw name would abort before any normalization could run. Every minted handle that differs from the account name is reported via RAISE NOTICE, with a closing count: the handle is what the user types to log in, so an operator must be able to see which accounts got one they cannot guess. Verified against embedded Postgres on three paths — an old-schema database seeded with all of the previously-fatal shapes (padded, blank, three-way duplicate, and a trim that creates a fresh collision), a re-run over the already-migrated result, and a database built from this PR's own DDL. All three commit; afterwards no LOCAL row has a null or duplicated handle, every GOOGLE row still carries its original google_id verbatim, and ck_provider_credential holds. Co-Authored-By: Claude Opus 5 (1M context) --- sql/updates/33.sql | 144 +++++++++++++++++++++++++++++++++------------ 1 file changed, 107 insertions(+), 37 deletions(-) diff --git a/sql/updates/33.sql b/sql/updates/33.sql index d0bf53cbf21..d142622f1b9 100644 --- a/sql/updates/33.sql +++ b/sql/updates/33.sql @@ -55,9 +55,12 @@ CREATE TABLE IF NOT EXISTS auth_provider ALTER TABLE auth_provider DROP CONSTRAINT IF EXISTS ck_provider_credential; +-- Report the accounts that end up unable to log in. This only reports: a name that is blank, +-- padded or shared is normalized into a usable handle further down rather than rejected, because +-- refusing the migration over a cosmetic name turns one untrimmed row — which `AdminUserResource` +-- can write today — into a deployment that cannot start, and liquibase marks the changeset failed. DO $$ DECLARE - offenders TEXT; orphans TEXT; has_placeholder BOOLEAN; BEGIN @@ -73,15 +76,6 @@ BEGIN SELECT 1 FROM information_schema.columns WHERE table_schema = 'texera_db' AND table_name = 'user' AND column_name = 'password' ) THEN - -- upgrading from the pre-auth_provider schema: handles come straight from "user" - SELECT string_agg(DISTINCT quote_literal(name), ', ') - INTO offenders - FROM "user" - WHERE password IS NOT NULL - AND (btrim(name) = '' OR name <> btrim(name) OR name IN ( - SELECT name FROM "user" WHERE password IS NOT NULL - GROUP BY name HAVING count(*) > 1)); - IF has_placeholder THEN EXECUTE $q$ SELECT string_agg(uid::TEXT, ', ') @@ -94,24 +88,6 @@ BEGIN FROM "user" WHERE password IS NULL AND google_id IS NULL; END IF; - ELSE - SELECT string_agg(DISTINCT quote_literal(u.name), ', ') - INTO offenders - FROM "user" u - JOIN auth_provider a ON a.uid = u.uid AND a.provider_type = 'LOCAL' - WHERE a.provider_id IS NULL - AND (btrim(u.name) = '' OR u.name <> btrim(u.name) OR u.name IN ( - SELECT u2.name - FROM "user" u2 - JOIN auth_provider a2 ON a2.uid = u2.uid AND a2.provider_type = 'LOCAL' - WHERE a2.provider_id IS NULL - GROUP BY u2.name HAVING count(*) > 1)); - END IF; - - IF offenders IS NOT NULL THEN - RAISE EXCEPTION 'migration 33: cannot promote "user".name to a login handle - ' - 'the following names are duplicated, blank, or whitespace-padded: %. ' - 'Resolve them and re-run.', offenders; END IF; IF orphans IS NOT NULL THEN @@ -128,8 +104,12 @@ BEGIN SELECT 1 FROM information_schema.columns WHERE table_schema = 'texera_db' AND table_name = 'user' AND column_name = 'password' ) THEN - INSERT INTO auth_provider (uid, provider_type, provider_id, password) - SELECT uid, 'LOCAL'::provider_type_enum, name, password + -- The handle is deliberately left NULL here and minted below. `uq_provider_identity` + -- is already in force and treats NULLs as distinct, so inserting the raw name would + -- abort on the first pair of users sharing one — before the normalization that resolves + -- it could run. + INSERT INTO auth_provider (uid, provider_type, password) + SELECT uid, 'LOCAL'::provider_type_enum, password FROM "user" WHERE password IS NOT NULL ON CONFLICT (uid, provider_type) DO NOTHING; @@ -143,13 +123,103 @@ BEGIN END $$; --- Fill handles left NULL by an earlier version of this migration; a no-op otherwise. -UPDATE auth_provider a -SET provider_id = u.name -FROM "user" u -WHERE u.uid = a.uid - AND a.provider_type = 'LOCAL' - AND a.provider_id IS NULL; +-- Mint every LOCAL handle from "user".name, normalizing rather than rejecting: the name is +-- trimmed, a name with nothing left is replaced by a uid-derived handle, and handles that still +-- collide are deterministically suffixed with "-" (kept: the lowest uid), truncated to fit +-- VARCHAR(256). The loop is bounded because a suffixed handle can itself collide with a literal +-- one, the same way 28.sql deduplicates dataset names. +-- +-- Every change is reported via RAISE NOTICE: the handle is what the user types to log in, so an +-- operator has to be able to see which accounts got one they would not guess and tell them. +DO $$ +DECLARE + rec RECORD; + changed INT := 0; + iterations INT := 0; +BEGIN + -- Trim, give a name that is blank or whitespace-only a deterministic stand-in, and separate + -- names that are already shared. The de-duplication has to happen inside this statement, not + -- only in the loop below: `uq_provider_identity` is in force, so two accounts named "john" + -- would collide with each other *within this one UPDATE* before any later pass could fix it. + FOR rec IN + UPDATE auth_provider a + SET provider_id = CASE + WHEN src.rn = 1 THEN src.base + ELSE LEFT(src.base, 256 - LENGTH('-' || a.uid::TEXT)) + || '-' || a.uid::TEXT + END + FROM ( + SELECT u.uid, + u.name AS old_name, + CASE + WHEN btrim(u.name) = '' THEN 'user-' || u.uid::TEXT + ELSE btrim(u.name) + END AS base, + ROW_NUMBER() OVER ( + PARTITION BY CASE + WHEN btrim(u.name) = '' THEN 'user-' || u.uid::TEXT + ELSE btrim(u.name) + END + ORDER BY u.uid + ) AS rn + FROM "user" u + JOIN auth_provider ap + ON ap.uid = u.uid + AND ap.provider_type = 'LOCAL' + AND ap.provider_id IS NULL + ) src + WHERE a.uid = src.uid + AND a.provider_type = 'LOCAL' + AND a.provider_id IS NULL + RETURNING a.uid, src.old_name, a.provider_id AS new_handle + LOOP + IF rec.old_name IS DISTINCT FROM rec.new_handle THEN + changed := changed + 1; + RAISE NOTICE 'migration 33: minted LOCAL handle for uid=%: "%" -> "%"', + rec.uid, rec.old_name, rec.new_handle; + END IF; + END LOOP; + + -- Resolve handles shared by more than one account. + LOOP + FOR rec IN + UPDATE auth_provider a + SET provider_id = LEFT(a.provider_id, 256 - LENGTH('-' || a.uid::TEXT)) + || '-' || a.uid::TEXT + FROM ( + SELECT uid, provider_id AS old_handle, + ROW_NUMBER() OVER (PARTITION BY provider_id ORDER BY uid) AS rn + FROM auth_provider + WHERE provider_type = 'LOCAL' + ) dups + WHERE a.uid = dups.uid AND a.provider_type = 'LOCAL' AND dups.rn > 1 + RETURNING a.uid, dups.old_handle, a.provider_id AS new_handle + LOOP + changed := changed + 1; + RAISE NOTICE 'migration 33: LOCAL handle for uid=% was already taken: "%" -> "%"', + rec.uid, rec.old_handle, rec.new_handle; + END LOOP; + + EXIT WHEN NOT EXISTS ( + SELECT 1 FROM auth_provider + WHERE provider_type = 'LOCAL' + GROUP BY provider_id HAVING COUNT(*) > 1 + ); + + iterations := iterations + 1; + IF iterations > 10 THEN + RAISE EXCEPTION 'migration 33: could not make LOCAL login handles unique after 10 ' + 'passes; resolve the duplicates in "user".name manually and re-run.'; + END IF; + END LOOP; + + IF changed > 0 THEN + RAISE NOTICE 'migration 33: % LOCAL login handle(s) differ from the account name they ' + 'were minted from; those users cannot guess their handle and must be told ' + 'it or given a reset.', changed; + END IF; +END +$$; -- Every row now has a handle, so make it mandatory and restore the credential check in its -- new shape: a password exists for LOCAL and only for LOCAL. From a5a83d493edd5379b8a3c164a212fb52c8afad6f Mon Sep 17 00:00:00 2001 From: Neilk1021 Date: Mon, 10 Aug 2026 13:20:32 -0700 Subject: [PATCH 47/54] fix(sql): apply uq_provider constraint at the end of migration --- sql/updates/33.sql | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/sql/updates/33.sql b/sql/updates/33.sql index d142622f1b9..7f1a821de0e 100644 --- a/sql/updates/33.sql +++ b/sql/updates/33.sql @@ -49,11 +49,13 @@ CREATE TABLE IF NOT EXISTS auth_provider password VARCHAR(256), -- hashed credential; only for LOCAL created_at TIMESTAMPTZ NOT NULL DEFAULT now(), PRIMARY KEY (uid, provider_type), - FOREIGN KEY (uid) REFERENCES "user"(uid) ON DELETE CASCADE, - CONSTRAINT uq_provider_identity UNIQUE (provider_type, provider_id) + FOREIGN KEY (uid) REFERENCES "user"(uid) ON DELETE CASCADE ); +-- Both constraints are (re-)added at the end, so drop them first to keep this file re-runnable +-- on a database that already has them. ALTER TABLE auth_provider DROP CONSTRAINT IF EXISTS ck_provider_credential; +ALTER TABLE auth_provider DROP CONSTRAINT IF EXISTS uq_provider_identity; -- Report the accounts that end up unable to log in. This only reports: a name that is blank, -- padded or shared is normalized into a usable handle further down rather than rejected, because @@ -104,10 +106,9 @@ BEGIN SELECT 1 FROM information_schema.columns WHERE table_schema = 'texera_db' AND table_name = 'user' AND column_name = 'password' ) THEN - -- The handle is deliberately left NULL here and minted below. `uq_provider_identity` - -- is already in force and treats NULLs as distinct, so inserting the raw name would - -- abort on the first pair of users sharing one — before the normalization that resolves - -- it could run. + -- The handle is left NULL here and minted below, where trimming, blank names and + -- collisions are all resolved in one place; `provider_id IS NULL` is what marks a row + -- as not yet minted. INSERT INTO auth_provider (uid, provider_type, password) SELECT uid, 'LOCAL'::provider_type_enum, password FROM "user" @@ -138,9 +139,9 @@ DECLARE iterations INT := 0; BEGIN -- Trim, give a name that is blank or whitespace-only a deterministic stand-in, and separate - -- names that are already shared. The de-duplication has to happen inside this statement, not - -- only in the loop below: `uq_provider_identity` is in force, so two accounts named "john" - -- would collide with each other *within this one UPDATE* before any later pass could fix it. + -- names that are already shared. Separating them here rather than leaving all of it to the + -- loop below settles the common case in one pass, so the loop only has to resolve the + -- residue: a minted "-" handle that collides with a literal one. FOR rec IN UPDATE auth_provider a SET provider_id = CASE @@ -221,9 +222,16 @@ BEGIN END $$; --- Every row now has a handle, so make it mandatory and restore the credential check in its --- new shape: a password exists for LOCAL and only for LOCAL. +-- Every row now has a handle, so make it mandatory, enforce uniqueness, and restore the +-- credential check in its new shape: a password exists for LOCAL and only for LOCAL. +-- +-- uq_provider_identity belongs here rather than in CREATE TABLE: Postgres checks a +-- non-deferrable UNIQUE as each index tuple is inserted, so in force it aborts the minting +-- UPDATE above the moment that derives a colliding handle, before the loop can resolve it. +-- Same ordering as 28.sql, which deduplicates before adding dataset_owner_uid_name_key. ALTER TABLE auth_provider ALTER COLUMN provider_id SET NOT NULL; +ALTER TABLE auth_provider + ADD CONSTRAINT uq_provider_identity UNIQUE (provider_type, provider_id); ALTER TABLE auth_provider ADD CONSTRAINT ck_provider_credential CHECK ((provider_type = 'LOCAL') = (password IS NOT NULL)); From d1dbfbe8a3975a9ddb4c39fa2df031f426012542 Mon Sep 17 00:00:00 2001 From: Neilk1021 Date: Mon, 10 Aug 2026 13:21:28 -0700 Subject: [PATCH 48/54] fix(auth): verify google email and migrate verifier to shared companion object --- .../auth/ExternalAuthProvisioner.scala | 4 ++ .../resource/auth/GoogleAuthResource.scala | 25 ++++++++--- .../auth/GoogleAuthResourceSpec.scala | 44 ++++++++++++++++++- 3 files changed, 64 insertions(+), 9 deletions(-) diff --git a/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala b/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala index 789eebc6c52..cf086bc6a28 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala @@ -35,6 +35,10 @@ import scala.util.chaining.scalaUtilChainingOps /** * A verified external identity (Google, Facebook, ...) reduced to the fields we persist. + * + * `email` must be non-blank and provider-verified: `loginOrProvision` links the identity to the + * account owning that address and claims its placeholder, so an unverified address is a + * takeover. Each provider checks this in its own mapping function (Google: `email_verified`). */ final case class ExternalProfile( providerType: ProviderTypeEnum, diff --git a/amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleAuthResource.scala b/amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleAuthResource.scala index 2d42d872c22..9dfa08e1a7f 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleAuthResource.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleAuthResource.scala @@ -33,14 +33,25 @@ import javax.ws.rs.core.MediaType object GoogleAuthResource { + final private lazy val clientId = UserSystemConfig.googleClientId /** * Reduce a verified Google id-token payload to the fields we persist. Google omits `name` * for accounts with no profile name, and the provisioner writes `name` straight to a NOT * NULL column, so the address stands in for it. Only the last path segment of `picture` is * kept — the frontend rebuilds the full `lh3.googleusercontent.com` URL around it. + * + * A payload with no address, or whose `email_verified` is not true, is refused rather than + * mapped — see [[ExternalProfile]] for why. Absent is not true: Google may omit the claim, and + * Workspace and custom-domain accounts can report false. */ private[auth] def profileOf(payload: GoogleIdToken.Payload): ExternalProfile = { val googleEmail = payload.getEmail + if (googleEmail == null || googleEmail.isBlank) { + throw new NotAuthorizedException("Login credentials are incorrect.") + } + if (!Option(payload.getEmailVerified).exists(_.booleanValue)) { + throw new NotAuthorizedException("Login credentials are incorrect.") + } ExternalProfile( ProviderTypeEnum.GOOGLE, payload.getSubject, @@ -51,20 +62,20 @@ object GoogleAuthResource { .getOrElse("") ) } + + private lazy val verifier = + new GoogleIdTokenVerifier.Builder(new NetHttpTransport, GsonFactory.getDefaultInstance) + .setAudience(Collections.singletonList(clientId)) + .build() } @Path("/auth/google") class GoogleAuthResource { - final private lazy val clientId = UserSystemConfig.googleClientId @GET @Path("/clientid") - def getClientId: String = clientId + def getClientId: String = GoogleAuthResource.clientId - private lazy val verifier = - new GoogleIdTokenVerifier.Builder(new NetHttpTransport, GsonFactory.getDefaultInstance) - .setAudience(Collections.singletonList(clientId)) - .build() /** * Verify `credential` against Google, yielding its payload, or None if it is not a valid @@ -73,7 +84,7 @@ class GoogleAuthResource { * Jersey instantiates this resource from `classOf[GoogleAuthResource]`. */ protected def verifiedPayload(credential: String): Option[GoogleIdToken.Payload] = - Option(verifier.verify(credential)).map(_.getPayload) + Option(GoogleAuthResource.verifier.verify(credential)).map(_.getPayload) @POST @Consumes(Array(MediaType.TEXT_PLAIN)) diff --git a/amber/src/test/scala/org/apache/texera/web/resource/auth/GoogleAuthResourceSpec.scala b/amber/src/test/scala/org/apache/texera/web/resource/auth/GoogleAuthResourceSpec.scala index 109d2d3de2d..02aed679514 100644 --- a/amber/src/test/scala/org/apache/texera/web/resource/auth/GoogleAuthResourceSpec.scala +++ b/amber/src/test/scala/org/apache/texera/web/resource/auth/GoogleAuthResourceSpec.scala @@ -77,19 +77,22 @@ class GoogleAuthResourceSpec /** * A payload shaped like Google's. `name` and `picture` are ordinary JSON members rather than * typed fields, and passing null omits them — which is exactly the case the name fallback - * exists for. + * exists for. `emailVerified` defaults to true, the only kind of payload the resource accepts; + * null omits the claim. */ private def payload( subject: String, email: String, name: String = "Given Name", - picture: String = "https://lh3.googleusercontent.com/a/AVATAR-ID" + picture: String = "https://lh3.googleusercontent.com/a/AVATAR-ID", + emailVerified: java.lang.Boolean = true ): GoogleIdToken.Payload = { val p = new GoogleIdToken.Payload() p.setSubject(subject) p.setEmail(email) if (name != null) p.set("name", name) if (picture != null) p.set("picture", picture) + if (emailVerified != null) p.setEmailVerified(emailVerified) p } @@ -167,6 +170,43 @@ class GoogleAuthResourceSpec a[NotAuthorizedException] should be thrownBy resource.login("not-a-real-credential") } + // Not merely untidy input: matching on an unverified address is an account takeover. + it should "reject a token whose email_verified is false, leaving the matching account alone" in { + loginWith(payload("google-sub-owner", "victim" + emailDomain, name = "Real Owner")) + val owner = userByEmail("victim").getUid + + val resource = new StubbedGoogleAuthResource( + Some(payload("google-sub-attacker", "victim" + emailDomain, emailVerified = false)) + ) + a[NotAuthorizedException] should be thrownBy resource.login("stubbed-credential") + + // the victim's account still points at the original identity, and no second one was added + googleIdOf(owner) shouldBe "google-sub-owner" + getDSLContext.fetchCount(USER, USER.EMAIL.eq("victim" + emailDomain)) shouldBe 1 + } + + it should "reject a token that omits email_verified rather than assuming it" in { + val resource = new StubbedGoogleAuthResource( + Some(payload("google-sub-noflag", "noflag" + emailDomain, emailVerified = null)) + ) + + a[NotAuthorizedException] should be thrownBy resource.login("stubbed-credential") + userByEmail("noflag") shouldBe null + } + + // A null address NPEs in `EmailUtil.normalize`, and as the name fallback violates NOT NULL. + it should "reject a token with no email address" in { + val resource = new StubbedGoogleAuthResource(Some(payload("google-sub-noemail", null))) + + a[NotAuthorizedException] should be thrownBy resource.login("stubbed-credential") + } + + it should "reject a token whose email is blank" in { + val resource = new StubbedGoogleAuthResource(Some(payload("google-sub-blankemail", " "))) + + a[NotAuthorizedException] should be thrownBy resource.login("stubbed-credential") + } + // ---- client id ----------------------------------------------------------- behavior of "getClientId" From 027bfff78d0ca794b189b93534c62e6606ca34dc Mon Sep 17 00:00:00 2001 From: Neilk1021 Date: Mon, 10 Aug 2026 13:25:32 -0700 Subject: [PATCH 49/54] refactor(auth): collapse the createLocalAccount seam into its one caller AdminUserResource.createLocalAccount was a one-line delegate with a single caller and no test of its own; the collision path it claimed to expose is covered directly in LocalAuthProvisionerSpec. LocalAuthProvisioner's 2-arg createLocalAccount overload existed only to feed that wrapper, so both layers go and addUser calls the 3-arg form directly. Co-Authored-By: Claude Opus 5 (1M context) --- .../web/resource/auth/LocalAuthProvisioner.scala | 10 +--------- .../dashboard/admin/user/AdminUserResource.scala | 14 +++++--------- 2 files changed, 6 insertions(+), 18 deletions(-) diff --git a/amber/src/main/scala/org/apache/texera/web/resource/auth/LocalAuthProvisioner.scala b/amber/src/main/scala/org/apache/texera/web/resource/auth/LocalAuthProvisioner.scala index 22e11dcba04..c92f05658aa 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/auth/LocalAuthProvisioner.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/auth/LocalAuthProvisioner.scala @@ -21,7 +21,7 @@ package org.apache.texera.web.resource.auth import org.apache.texera.dao.{SqlServer, SqlStates} import org.apache.texera.dao.jooq.generated.Tables.AUTH_PROVIDER -import org.apache.texera.dao.jooq.generated.enums.{ProviderTypeEnum, UserRoleEnum} +import org.apache.texera.dao.jooq.generated.enums.ProviderTypeEnum import org.apache.texera.dao.jooq.generated.tables.daos.{AuthProviderDao, UserDao} import org.apache.texera.dao.jooq.generated.tables.pojos.{AuthProvider, User} import org.jasypt.util.password.StrongPasswordEncryptor @@ -129,12 +129,4 @@ object LocalAuthProvisioner { throw new WebApplicationException(message, e, Response.Status.CONFLICT) } } - - /** Create an INACTIVE account whose display name is its login handle. */ - def createLocalAccount(handle: String, rawPassword: String): Unit = { - val user = new User - user.setName(handle) - user.setRole(UserRoleEnum.INACTIVE) - createLocalAccount(user, handle, rawPassword) - } } diff --git a/amber/src/main/scala/org/apache/texera/web/resource/dashboard/admin/user/AdminUserResource.scala b/amber/src/main/scala/org/apache/texera/web/resource/dashboard/admin/user/AdminUserResource.scala index 18ea91cd300..807b75dbc32 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/dashboard/admin/user/AdminUserResource.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/dashboard/admin/user/AdminUserResource.scala @@ -136,17 +136,13 @@ class AdminUserResource { @Path("/add") def addUser(): Unit = { val random = UUID.randomUUID().toString - createLocalAccount("User" + random, random) + val handle = "User" + random + val user = new User + user.setName(handle) + user.setRole(UserRoleEnum.INACTIVE) + LocalAuthProvisioner.createLocalAccount(user, handle, random) } - /** - * Create a user together with the LOCAL credential it logs in with. Split out of `addUser` - * so the collision path is reachable: `addUser` derives its handle from a fresh UUID and - * so cannot produce the unique violation this maps to a 409. - */ - private[user] def createLocalAccount(handle: String, rawPassword: String): Unit = - LocalAuthProvisioner.createLocalAccount(handle, rawPassword) - @GET @Path("/created_datasets") @Produces(Array(MediaType.APPLICATION_JSON)) From 997e16d88cb0794fdd60c2fe203584c2b3afb0f6 Mon Sep 17 00:00:00 2001 From: Neilk1021 Date: Mon, 10 Aug 2026 13:26:16 -0700 Subject: [PATCH 50/54] fix(auth): stop deriving an admin-created account's password from its handle addUser used one UUID for both the username and the password, so anyone able to read /admin/user/list could authenticate as any account created this way. Two independent UUIDs cost nothing. Co-Authored-By: Claude Opus 5 (1M context) --- .../resource/dashboard/admin/user/AdminUserResource.scala | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/amber/src/main/scala/org/apache/texera/web/resource/dashboard/admin/user/AdminUserResource.scala b/amber/src/main/scala/org/apache/texera/web/resource/dashboard/admin/user/AdminUserResource.scala index 807b75dbc32..b550ac46e00 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/dashboard/admin/user/AdminUserResource.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/dashboard/admin/user/AdminUserResource.scala @@ -135,12 +135,13 @@ class AdminUserResource { @POST @Path("/add") def addUser(): Unit = { - val random = UUID.randomUUID().toString - val handle = "User" + random + // Two independent UUIDs: the handle is visible to anyone who can read /list, so deriving the + // password from it would let any such caller log in as the new account. + val handle = "User" + UUID.randomUUID().toString val user = new User user.setName(handle) user.setRole(UserRoleEnum.INACTIVE) - LocalAuthProvisioner.createLocalAccount(user, handle, random) + LocalAuthProvisioner.createLocalAccount(user, handle, UUID.randomUUID().toString) } @GET From 0fbeb5ede253f7d19580551f3d76cfcdbf6762bd Mon Sep 17 00:00:00 2001 From: Neilk1021 Date: Mon, 10 Aug 2026 13:28:02 -0700 Subject: [PATCH 51/54] refactor(auth): route both email lookups through one query ExternalAuthProvisioner copied AuthResource's case-insensitive email lookup, and both copies carried the same rule in prose that had to stay in sync by hand. The copy's stated reason was wrong: createDSLContext() returns the shared context rather than opening one. The real reason is transaction scope, which a DSLContext overload satisfies without duplicating the query. Co-Authored-By: Claude Opus 5 (1M context) --- .../web/resource/auth/AuthResource.scala | 19 +++++++++++++--- .../auth/ExternalAuthProvisioner.scala | 22 +++---------------- 2 files changed, 19 insertions(+), 22 deletions(-) diff --git a/amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala b/amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala index e9861f7bf95..b05ed180bd9 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala @@ -30,6 +30,7 @@ import org.apache.texera.dao.jooq.generated.tables.pojos.User import org.apache.texera.web.model.http.request.auth.{UserLoginRequest, UserRegistrationRequest} import org.apache.texera.web.model.http.response.TokenIssueResponse import org.apache.texera.web.resource.auth.AuthResource._ +import org.jooq.DSLContext import org.jooq.impl.DSL import java.time.Instant @@ -75,11 +76,23 @@ object AuthResource { /** * Email identity is matched case-insensitively (backed by idx_user_email_lower), * while stored emails keep their original casing. + * + * Case-insensitivity is required, not a nicety: `"user".email` is a plain case-sensitive + * UNIQUE and `idx_user_email_lower` is not unique, so `Alice@x.com` and `alice@x.com` can + * coexist. Registration stores the address as the user typed it while contributor + * placeholders are stored lower-cased, so the casings provably differ in practice. An + * exact-match lookup would miss, insert a second account without violating any constraint, + * and silently strand the original account's data. */ def fetchUserByEmailIgnoreCase(email: String): User = - SqlServer - .getInstance() - .createDSLContext() + fetchUserByEmailIgnoreCase(SqlServer.getInstance().createDSLContext(), email) + + /** + * As above, against a caller-supplied context. [[ExternalAuthProvisioner]] passes its + * transaction's context so the lookup reads that transaction's own writes. + */ + def fetchUserByEmailIgnoreCase(ctx: DSLContext, email: String): User = + ctx .selectFrom(USER) .where(DSL.lower(USER.EMAIL).eq(EmailUtil.normalize(email))) .fetchOneInto(classOf[User]) diff --git a/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala b/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala index cf086bc6a28..fd3a5a2c9d6 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala @@ -25,10 +25,8 @@ import org.apache.texera.dao.jooq.generated.Tables.{AUTH_PROVIDER, USER} import org.apache.texera.dao.jooq.generated.enums.{ProviderTypeEnum, UserRoleEnum} import org.apache.texera.dao.jooq.generated.tables.daos.{AuthProviderDao, UserDao} import org.apache.texera.dao.jooq.generated.tables.pojos.{AuthProvider, User} -import org.apache.texera.common.util.EmailUtil import org.jooq.DSLContext import org.jooq.exception.DataAccessException -import org.jooq.impl.DSL import java.time.OffsetDateTime import scala.util.chaining.scalaUtilChainingOps @@ -52,25 +50,11 @@ object ExternalAuthProvisioner extends LazyLogging { /** * The account owning `email`, matched case-insensitively and within the caller's transaction - * so it reads that transaction's own writes. - * - * Case-insensitivity is required, not a nicety: `"user".email` is a plain case-sensitive - * UNIQUE and `idx_user_email_lower` is not unique, so `Alice@x.com` and `alice@x.com` can - * coexist. Registration stores the address as the user typed it while contributor - * placeholders are stored lower-cased, so the casings provably differ in practice. An - * exact-match lookup here would miss, insert a second account without violating any - * constraint, and silently strand the original account's data. - * - * Mirrors `AuthResource.fetchUserByEmailIgnoreCase`, which cannot be reused directly because - * it opens its own DSLContext. + * so it reads that transaction's own writes. See + * [[AuthResource.fetchUserByEmailIgnoreCase]] for why the match cannot be exact. */ private def userByEmailIgnoreCase(ctx: DSLContext, email: String): Option[User] = - Option( - ctx - .selectFrom(USER) - .where(DSL.lower(USER.EMAIL).eq(EmailUtil.normalize(email))) - .fetchOneInto(classOf[User]) - ) + Option(AuthResource.fetchUserByEmailIgnoreCase(ctx, email)) /** * Resolve the user behind an external identity, creating one if necessary, and From 7d8b5beac50e146a933c5f94053b318b452139ba Mon Sep 17 00:00:00 2001 From: Neilk1021 Date: Mon, 10 Aug 2026 13:29:29 -0700 Subject: [PATCH 52/54] refactor(auth): tidy provision's returning-user path and naming - map the joined record in place instead of re-reading the same user row by uid, saving a query on the returning-identity path - rename `claimed` to `wasPlaceholder`; it holds "is an unclaimed placeholder", so `if (drifted || claimed)` read backwards - declare provision's `: User` return type rather than inferring it through a two-branch match - drop a stray blank line in loginOrProvision Co-Authored-By: Claude Opus 5 (1M context) --- .../web/resource/auth/ExternalAuthProvisioner.scala | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala b/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala index fd3a5a2c9d6..c89d5b72ba9 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala @@ -63,7 +63,6 @@ object ExternalAuthProvisioner extends LazyLogging { * attempt is re-run once; if the retry violates a constraint too, that exception propagates. */ def loginOrProvision(profile: ExternalProfile): User = { - try { provision(profile) } catch { @@ -72,7 +71,7 @@ object ExternalAuthProvisioner extends LazyLogging { } } - private def provision(profile: ExternalProfile) = { + private def provision(profile: ExternalProfile): User = { SqlServer.withTransaction(SqlServer.getInstance().createDSLContext()) { ctx => val txUserDao = new UserDao(ctx.configuration()) val txAuthDao = new AuthProviderDao(ctx.configuration()) @@ -88,7 +87,9 @@ object ExternalAuthProvisioner extends LazyLogging { .fetchOne() ) match { case Some(record) => - txUserDao.fetchOneByUid(record.get(USER.UID)).tap { user => + // The join above already selected every USER column, so map in place rather than + // re-reading the same row by uid. + record.into(USER).into(classOf[User]).tap { user => if (refresh(user, profile)) txUserDao.update(user) } @@ -96,10 +97,10 @@ object ExternalAuthProvisioner extends LazyLogging { val user = userByEmailIgnoreCase(ctx, profile.email) match { case Some(existing) => existing.tap { user => - val claimed = user.getIsPlaceholder - if (claimed) AuthResource.claimPlaceholder(user) + val wasPlaceholder = user.getIsPlaceholder + if (wasPlaceholder) AuthResource.claimPlaceholder(user) val drifted = refresh(user, profile) - if (drifted || claimed) txUserDao.update(user) + if (drifted || wasPlaceholder) txUserDao.update(user) } case None => val created = new User() From 0e770006f35df5392988872531433d3071e4d715 Mon Sep 17 00:00:00 2001 From: Neilk1021 Date: Mon, 10 Aug 2026 13:30:50 -0700 Subject: [PATCH 53/54] docs(auth): record that the googleId claim is now absent, not null googleId used to be set unconditionally, so a local-only user's token carried "googleId": null; it is now omitted entirely. Names the one reader that can tell the difference (flarum.service.ts, which passes it as a Flarum password) so the next person does not have to rediscover it. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/main/scala/org/apache/texera/auth/JwtAuth.scala | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/common/auth/src/main/scala/org/apache/texera/auth/JwtAuth.scala b/common/auth/src/main/scala/org/apache/texera/auth/JwtAuth.scala index 3abf1af0bea..d3270bfa0a2 100644 --- a/common/auth/src/main/scala/org/apache/texera/auth/JwtAuth.scala +++ b/common/auth/src/main/scala/org/apache/texera/auth/JwtAuth.scala @@ -65,6 +65,12 @@ object JwtAuth { * `auth_provider` context, as does `AuthResource.register`. Omitting the claim is harmless * on all of them: a service-to-service token never reaches the browser, and a freshly * registered LOCAL account has no Google identity to name. + * + * Note this changes the token's shape, not just where the value comes from: `googleId` was + * previously written unconditionally, so a local-only user's token carried `"googleId": null` + * and now omits the claim. The frontend declares it optional (`common/type/user.ts`), so the + * only reader that can tell is `flarum.service.ts`, which passes it as a Flarum account + * password — a path that was already broken for exactly the users who lack the claim. */ def jwtClaims(user: User, googleId: Option[String] = None): JwtClaims = { val claims = new JwtClaims From 0656b0222c5abbae408f73913a3b02d0abf399cc Mon Sep 17 00:00:00 2001 From: Neilk1021 Date: Mon, 10 Aug 2026 13:41:52 -0700 Subject: [PATCH 54/54] fix(auth): lint --- .../apache/texera/web/resource/auth/GoogleAuthResource.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleAuthResource.scala b/amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleAuthResource.scala index 9dfa08e1a7f..6dd81c8cb0b 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleAuthResource.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleAuthResource.scala @@ -34,6 +34,7 @@ import javax.ws.rs.core.MediaType object GoogleAuthResource { final private lazy val clientId = UserSystemConfig.googleClientId + /** * Reduce a verified Google id-token payload to the fields we persist. Google omits `name` * for accounts with no profile name, and the provisioner writes `name` straight to a NOT @@ -76,7 +77,6 @@ class GoogleAuthResource { @Path("/clientid") def getClientId: String = GoogleAuthResource.clientId - /** * Verify `credential` against Google, yielding its payload, or None if it is not a valid * token for this client. This is the only seam that reaches the network, so tests override