Yicong-Huang commented on code in PR #7055: URL: https://github.com/apache/texera/pull/7055#discussion_r3746979494
########## amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala: ########## @@ -0,0 +1,193 @@ +/* + * 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 = { + + try { + provision(profile) + } catch { + case e: org.jooq.exception.DataAccessException if e.sqlState() == "23505" => Review Comment: `LocalAuthProvisioner.UNIQUE_VIOLATION` (`:45`) already names this SQLSTATE, one file away in the same package. Widen it to `private[auth]` and refer to it here, so there stays one definition. Both files are new in this PR, so this copy is being created rather than inherited. ########## common/auth/src/main/scala/org/apache/texera/auth/JwtAuth.scala: ########## @@ -51,14 +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 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. Review Comment: `AuthResource.register` also omits `googleId` (`:186`, `:201`), and its token goes straight to the browser. A reader applying the criterion stated here would read those two call sites as bugs. They are fine, for a different reason worth naming: ```suggestion * `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. ``` ########## amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala: ########## @@ -19,61 +19,61 @@ package org.apache.texera.web.resource.auth -import org.apache.texera.auth.JwtAuth.{TOKEN_EXPIRE_TIME_IN_MINUTES, jwtClaims, jwtToken} +import com.typesafe.scalalogging.Logger +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 -import org.apache.texera.dao.jooq.generated.Tables.USER -import org.apache.texera.dao.jooq.generated.enums.UserRoleEnum +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 import org.apache.texera.web.resource.auth.AuthResource._ import org.jooq.impl.DSL -import org.jasypt.util.password.StrongPasswordEncryptor import java.time.Instant import java.time.temporal.ChronoUnit import javax.ws.rs._ import javax.ws.rs.core.MediaType object AuthResource { + private val logger: Logger = Logger(classOf[AuthResource]) - private def userDao = - new UserDao( - SqlServer - .getInstance() - .createDSLContext() - .configuration - ) + private def context = SqlServer.getInstance().context + private def userDao = new UserDao(context.configuration) Review Comment: Nothing references this any more — `fetchByName` and `update` were its last two callers and both go away in this diff. Worth dropping it together with the `UserDao` import at `:29`, which it is the only user of. `build.sbt` sets `javacOptions` only, so no Scala `-Wunused` pass will flag it for you. ########## amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala: ########## @@ -0,0 +1,193 @@ +/* + * 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. Review Comment: `loginOrProvision` can now run two transactions, so "a single transaction" no longer holds. The retry that resolved the concurrent-login 500 is also undescribed, which makes it read as incidental rather than load-bearing. ```suggestion * 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. ``` ########## 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 + ) Review Comment: Two unique constraints can fire inside this transaction and both land here: `user_email_key` from `txUserDao.insert` at `:88`, and `uq_provider_identity` from `txAuthDao.insert` at `:95`. So two concurrent registrations sharing an email but not a username end with the loser told its handle is taken, when the handle is free. The scaladoc at `:77-78` already scopes this mapping to the handle race, so re-checking after the rollback makes the message match the contract you wrote: ```suggestion 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) ``` ########## common/auth/src/test/scala/org/apache/texera/auth/JwtAuthSpec.scala: ########## @@ -33,26 +33,43 @@ class JwtAuthSpec extends AnyFlatSpec with Matchers { user.setUid(42) user.setName("alice") user.setEmail("[email protected]") - user.setGoogleId("g-123") - user.setGoogleAvatar("avatar-blob") + user.setAvatar("avatar-blob") user.setRole(UserRoleEnum.ADMIN) user } "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("googleId") shouldBe "g-123" claims.getClaimValueAsString("email") shouldBe "[email protected]" claims.getClaimValueAsString("googleAvatar") shouldBe "avatar-blob" claims.getClaimValueAsString("role") shouldBe UserRoleEnum.ADMIN.name } + // 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()) + 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()).hasClaim("googleId") shouldBe false + } + + it should "carry the googleId claim when a provider id is supplied" in { + 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 = { Review Comment: `jwtClaims` no longer takes an `expireInDays`, so this parameter is unread. The two calls below therefore evaluate the identical expression: the test asserts one thing twice, under a name promising a contract the signature can no longer express. The claim worth keeping is that the window comes from `AuthConfig.jwtExpirationMinutes`. One call and a name without the "ignoring the expireInDays argument" clause says exactly that. -- 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]
