Yicong-Huang commented on code in PR #7055: URL: https://github.com/apache/texera/pull/7055#discussion_r3751064987
########## amber/src/main/scala/org/apache/texera/web/resource/auth/LocalAuthProvisioner.scala: ########## @@ -0,0 +1,144 @@ +/* + * 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 => + 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) + } + } + + /** + * 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 = { + val hashedPassword = hashPassword(rawPassword) + + try { + SqlServer.withTransaction(SqlServer.getInstance().createDSLContext()) { ctx => + new UserDao(ctx.configuration()).update(user) + + val auth = new AuthProvider + auth.setUid(user.getUid) + auth.setProviderType(ProviderTypeEnum.LOCAL) + auth.setProviderId(handle) + auth.setPassword(hashedPassword) + new AuthProviderDao(ctx.configuration()).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 + ) Review Comment: The constraint that fires here is neither of the two the sibling handler weighs. This path leaves the email alone and inserts `(uid, LOCAL, handle)`. So a second registration claiming the same placeholder collides on `PRIMARY KEY (uid, provider_type)`, and its loser is told its handle is taken when the handle is free. `createLocalAccount` at `:98-102` now re-checks before naming a cause; this copy did not follow — the divergence the folding request at `:113` was about, arriving before the follow-up issue does. ```suggestion 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) ``` ########## amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleAuthResource.scala: ########## @@ -53,67 +61,34 @@ class GoogleAuthResource { @Path("/clientid") def getClientId: String = 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 + * 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 Review Comment: Two fragments: the middle sentence has no main verb, and "kept a method" is missing its "as". ```suggestion * 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 ``` ########## amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala: ########## @@ -0,0 +1,194 @@ +/* + * 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. 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. Review Comment: This wording is mine from last round, and it overclaims — my error, not yours. A `23505` can also come from `refresh` + `update` at `:102-104` writing an email another account already holds. That is not a concurrent login, and the re-run fails identically, so the doc should not promise that every unique violation resolves. ```suggestion * 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. ``` -- 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]
