aglinxinyuan commented on code in PR #7055:
URL: https://github.com/apache/texera/pull/7055#discussion_r3752838568
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/admin/user/AdminUserResource.scala:
##########
@@ -73,14 +77,23 @@ class AdminUserResource {
@Path("/list")
@Produces(Array(MediaType.APPLICATION_JSON))
def list(): util.List[UserInfo] = {
+
+ val googleProvider = AUTH_PROVIDER.as("google_provider")
+
AdminUserResource.context
.select(
USER.UID,
USER.NAME,
USER.EMAIL,
- USER.GOOGLE_ID,
+ // 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 — 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"),
Review Comment:
The LOCAL handle is now what a user types to log in, and it can diverge from
`"user".name`: an external login rewrites the display name but never the
handle, and `33.sql` mints suffixed handles (`john-2`) for accounts that shared
a name. Nothing surfaces it, though — this projection returns `USER.NAME`, and
the migration's own NOTICE says the affected users *"cannot guess their handle
and must be told it or given a reset."*
So the only way for an operator to answer "what is this user's login
handle?" is a direct `auth_provider` query. The join machinery is already here;
a second alias makes it recoverable:
```scala
val localProvider = AUTH_PROVIDER.as("local_provider")
// ...
localProvider.PROVIDER_ID.as("loginHandle"),
// ...
.leftJoin(localProvider)
.on(localProvider.PROVIDER_TYPE.eq(ProviderTypeEnum.LOCAL))
.and(localProvider.UID.eq(USER.UID))
```
plus the matching `UserInfo` field in the same position — per the
positional-mapping warning added just above. Fine as a follow-up if you'd
rather keep this PR scoped, but it should land before `33.sql` runs anywhere
real.
##########
amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleAuthResource.scala:
##########
@@ -19,30 +19,38 @@
package org.apache.texera.web.resource.auth
-import com.google.api.client.googleapis.auth.oauth2.GoogleIdTokenVerifier
+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.SqlServer
-import org.apache.texera.dao.jooq.generated.enums.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.dao.jooq.generated.enums.ProviderTypeEnum
import org.apache.texera.web.model.http.response.TokenIssueResponse
-import org.apache.texera.web.resource.auth.GoogleAuthResource.userDao
import java.util.Collections
import javax.ws.rs._
import javax.ws.rs.core.MediaType
object GoogleAuthResource {
- private def userDao =
- new UserDao(
- SqlServer
- .getInstance()
- .createDSLContext()
- .configuration
+
+ /**
+ * 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.
+ */
+ private[auth] def profileOf(payload: GoogleIdToken.Payload): ExternalProfile
= {
+ val googleEmail = payload.getEmail
+ ExternalProfile(
+ ProviderTypeEnum.GOOGLE,
+ payload.getSubject,
+
Option(payload.get("name").asInstanceOf[String]).filter(_.nonEmpty).getOrElse(googleEmail),
Review Comment:
Two things about the payload reduction:
**`email_verified` is dropped.** `ExternalAuthProvisioner.provision` links a
new external identity to any account whose email matches, and claims a
placeholder while doing it — so an id token asserting an *unverified* address
is enough to take over an existing account. Google doesn't guarantee
`email_verified` is true; it can be false for Workspace/custom-domain accounts.
Pre-existing behavior for Google, but this PR promotes the path to *the*
generic one that IEEE/GitHub logins will reuse, so gating it here now is much
cheaper than gating it in three providers later.
**Null email.** `getEmail` feeds `userByEmailIgnoreCase` →
`EmailUtil.normalize`, which is `email.trim.toLowerCase` and NPEs on null.
`name` and `picture` are both guarded here; `email` is the one that isn't — and
it's also what `name` falls back to, so a null email additionally means a NOT
NULL violation on `"user".name`. Also pre-existing, but this method is now the
single place that guard belongs.
##########
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 =
Review Comment:
Jersey instantiates this resource per request by default, so a
per-*instance* `lazy val` still builds a `GoogleIdTokenVerifier` — and a
`NetHttpTransport`, which carries its own connection pool — on every login.
That's the same cost as the old inline construction inside `login`, so the
`lazy val` doesn't buy what it looks like it buys.
`GoogleIdTokenVerifier` is documented thread-safe, so moving it to the
companion object (alongside `profileOf`) makes it one per JVM.
##########
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
+ createLocalAccount("User" + random, 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)
Review Comment:
Two notes on this block:
**The `createLocalAccount` seam has no user.** Its doc says it was *"Split
out of `addUser` so the collision path is reachable"* — but it's a one-line
delegate with exactly one caller (line 139), and nothing in
`AdminUserResourceSpec` calls it. The collision path is tested directly against
`LocalAuthProvisioner` in `LocalAuthProvisionerSpec`, which is the better home
for it. `LocalAuthProvisioner`'s 2-arg `createLocalAccount` overload exists
only to feed this wrapper, so both layers can collapse into one call.
**The generated password is derivable from the generated handle.**
`createLocalAccount("User" + random, random)` makes the password the same UUID
as the visible username, so anyone who can read `/list` can authenticate as
that account. Pre-existing — it was `System.currentTimeMillis()` before, which
was worse — but the line is being rewritten here, and two independent values
cost nothing.
##########
amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala:
##########
@@ -0,0 +1,195 @@
+/*
+ * 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, 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
+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] =
Review Comment:
The scaladoc says this can't reuse `AuthResource.fetchUserByEmailIgnoreCase`
*"because it opens its own DSLContext"* — but `SqlServer.createDSLContext()` is
`def createDSLContext(): DSLContext = context`, i.e. it hands back the same
shared context rather than opening anything.
The real reason for the copy is transaction scope: this lookup has to run
inside the caller's transaction so it reads that transaction's own writes.
That's legitimate, but it's satisfiable without duplicating the query — give
`AuthResource.fetchUserByEmailIgnoreCase` a `DSLContext` overload and route
both call sites through it. As written, the case-insensitivity rule that both
copies document (correctly, and at length) has to stay in sync by hand.
##########
amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala:
##########
@@ -0,0 +1,195 @@
+/*
+ * 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, 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
+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 is taken to mean a concurrent login won
the race, so the
+ * 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: DataAccessException if e.sqlState() ==
SqlStates.UNIQUE_VIOLATION =>
+ 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())
+
+ 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 =>
Review Comment:
A few small things in `provision`:
**Redundant round trip (this line).** The
`select().from(USER).join(AUTH_PROVIDER)` above already fetched every `USER`
column into `record`, and this re-reads the same row by uid.
`AuthResource.retrieveUserByUsernameAndPassword` does the same join and maps in
place — `record.into(USER).into(classOf[User])` — which works here too and
saves a query on the hot returning-user path.
**Line 111 — `val claimed = user.getIsPlaceholder`** reads as "was already
claimed" but holds "is an *un*claimed placeholder", which makes `if (drifted ||
claimed)` below read backwards. `wasPlaceholder` would say it.
**Line 87 — `private def provision(profile: ExternalProfile) = {`** has no
declared return type, on a method whose inferred `User` comes out of a 25-line
two-branch `match`. Worth writing `: User`.
**Line 79** — stray blank line between `loginOrProvision`'s signature and
its `try`.
##########
common/auth/src/main/scala/org/apache/texera/auth/JwtAuth.scala:
##########
@@ -51,14 +51,29 @@ 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, 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
claims.setSubject(user.getName)
claims.setClaim("userId", user.getUid)
- claims.setClaim("googleId", user.getGoogleId)
claims.setClaim("email", user.getEmail)
claims.setClaim("role", user.getRole)
- claims.setClaim("googleAvatar", user.getGoogleAvatar)
+ claims.setClaim("googleAvatar", user.getAvatar)
+ googleId.foreach(claims.setClaim("googleId", _))
Review Comment:
Worth being explicit that this changes the token's *shape*, not just where
the value comes from: `googleId` used to be written unconditionally, so a
local-only user's token carried `"googleId": null`. It's now absent entirely.
`frontend/src/app/common/type/user.ts` declares `googleId?: string`, so the
read at `auth.service.ts:167` is fine either way. The one place it leaks is
`dashboard/service/user/flarum/flarum.service.ts:39,48`, which passes
`user.googleId` as the Flarum account password — `JSON.stringify` will now omit
the `password` key rather than send `null`. That path was already broken for
non-Google users, so it's a different flavour of broken rather than a
regression; flagging it in case anyone has Flarum enabled.
--
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]
