Yicong-Huang commented on code in PR #7055: URL: https://github.com/apache/texera/pull/7055#discussion_r3746301407
########## amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala: ########## @@ -0,0 +1,188 @@ +/* + * 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 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} +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.time.OffsetDateTime +import scala.util.chaining.scalaUtilChainingOps + +/** + * A verified external identity (Google, Facebook, ...) reduced to the fields we persist. + */ +final case class ExternalProfile( + providerType: ProviderTypeEnum, + providerId: String, + name: String, + email: String, + avatar: String +) + +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 `[email protected]` and `[email protected]` 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 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 + * 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 = + SqlServer.withTransaction(SqlServer.getInstance().createDSLContext()) { ctx => + val txUserDao = new UserDao(ctx.configuration()) + val txAuthDao = new AuthProviderDao(ctx.configuration()) + + Option( + ctx + .select() + .from(USER) + .join(AUTH_PROVIDER) + .on(USER.UID.eq(AUTH_PROVIDER.UID)) + .where(AUTH_PROVIDER.PROVIDER_TYPE.eq(profile.providerType)) + .and(AUTH_PROVIDER.PROVIDER_ID.eq(profile.providerId)) + .fetchOne() + ) match { + case Some(record) => + txUserDao.fetchOneByUid(record.get(USER.UID)).tap { user => + if (refresh(user, profile)) txUserDao.update(user) + } + + case None => + val user = userByEmailIgnoreCase(ctx, profile.email) match { + case Some(existing) => + existing.tap { user => + val claimed = user.getIsPlaceholder + if (claimed) AuthResource.claimPlaceholder(user) + val drifted = refresh(user, profile) + if (drifted || claimed) txUserDao.update(user) + } + case None => + val created = new User() + created.setName(profile.name) + 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) Review Comment: This recovery cannot run. When `txUserDao.insert` raises 23505, Postgres aborts the transaction and refuses every later command with 25P02 — so this `userByEmailIgnoreCase` fails too, and the concurrent login still 500s, pointing at the recovery query rather than the collision. `LocalAuthProvisioner` has the shape that works: its `try` wraps the whole `withTransaction` call (`:84-105`), so the handler runs after rollback. Hoisting the catch out of the transaction here, and re-running once, lands the second request on the account the first created. ########## amber/src/main/scala/org/apache/texera/web/resource/auth/LocalAuthProvisioner.scala: ########## @@ -0,0 +1,145 @@ +/* + * 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 + ) + } + } + + /** + * Persist `user` and give it a LOCAL credential in one transaction, for an account row that + * already exists — claiming a dataset-contributor placeholder. The counterpart to + * [[createLocalAccount]], which inserts the user instead of updating it; both write the + * credential in the same transaction as the user row so an account can never be left in a + * state where it looks claimed but has nothing to log in with. + */ + def claimWithLocalCredential(user: User, handle: String, rawPassword: String): Unit = { Review Comment: This is `createLocalAccount` again with one line changed — `new UserDao(ctx.configuration()).update(user)` in place of `txUserDao.insert(user)`. The hashing, the transaction, the `AuthProvider` construction and the 23505 to 409 mapping are all copied. Worth folding into one private method that takes the user-row operation as a parameter, so the next change to how a LOCAL credential is written lands once. That is the duplication this object was extracted to end. ########## amber/src/main/scala/org/apache/texera/web/resource/dashboard/admin/user/AdminUserResource.scala: ########## @@ -119,14 +135,18 @@ class AdminUserResource { @POST @Path("/add") def addUser(): Unit = { - val random = System.currentTimeMillis().toString - val newUser = new User - newUser.setName("User" + random) - newUser.setPassword(new StrongPasswordEncryptor().encryptPassword(random)) - newUser.setRole(UserRoleEnum.INACTIVE) - userDao.insert(newUser) + val random = UUID.randomUUID().toString Review Comment: `AdminUserResourceSpec.scala:104` still describes the old handle format — "an INACTIVE user with ... a `User<millis>` name" — which this line replaced with a UUID. The `LIKE "User%"` cleanup underneath it still matches, so only the comment is wrong. ########## sql/updates/33.sql: ########## @@ -0,0 +1,184 @@ +/* + * 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. + */ + +-- Relocate login credentials out of "user" into auth_provider. +-- +-- 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 + +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 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 - ' Review Comment: This is `33.sql`, but the abort message sends the operator to `32.sql` — which is the unrelated `user_warehouse` migration. The rebase renumbering carried the old number along. ```suggestion RAISE EXCEPTION 'migration 33: cannot promote "user".name to a login handle - ' ``` The `RAISE NOTICE` at `:118` says `migration 32` too. ########## amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala: ########## @@ -95,21 +99,33 @@ object AuthResource { ) } - def createAdminUser(): Unit = { - val adminUsername = UserSystemConfig.adminUsername - val adminPassword = UserSystemConfig.adminPassword + def createAdminUser(): Unit = + createAdminUser(UserSystemConfig.adminUsername.trim, UserSystemConfig.adminPassword.trim) - if (adminUsername.trim.nonEmpty && adminPassword.trim.nonEmpty) { - val existingUser = userDao.fetchByName(adminUsername) - if (existingUser.isEmpty) { - val user = new User - user.setName(adminUsername) - user.setEmail(adminUsername) - user.setRole(UserRoleEnum.ADMIN) - user.setPassword(new StrongPasswordEncryptor().encryptPassword(adminPassword)) - userDao.insert(user) - } + /** + * Bootstrap the configured admin account, doing nothing if it already exists. The credentials + * are parameters rather than reads of [[UserSystemConfig]] because those are object vals + * resolved once per JVM, which leaves the unconfigured case unreachable from a test. + */ + private[auth] def createAdminUser(adminUsername: String, adminPassword: String): Unit = { Review Comment: This overload's scaladoc says the credentials are parameters so the unconfigured case is reachable from a test — but nothing calls it; `AuthResourceSpec.scala:364-380` drives only the zero-arg form. `AdminUserResource.createLocalAccount` (`:147`) is the same story, with no caller in its spec. The cost: the guard you added at `:115` has no regression test, so reverting it to `fetchByName` would pass CI. Three tests close it — the empty-config return, the email-collision skip, and the 409 on a duplicate handle. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
