aglinxinyuan commented on code in PR #7055:
URL: https://github.com/apache/texera/pull/7055#discussion_r3694711824
##########
sql/texera_ddl.sql:
##########
@@ -78,6 +78,7 @@ 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;
Review Comment:
This drops the *type* but nothing drops the *table* — `auth_provider` is
missing from the DROP TABLE block above. Re-run the DDL on a DB that already
has it and you get: `DROP TYPE ... CASCADE` takes the `provider_type` column
off `auth_provider`, the table itself survives (`DROP TABLE "user" CASCADE`
only takes the FK), and `CREATE TABLE IF NOT EXISTS auth_provider` below is
then a no-op. You're left with a column-less, constraint-less table full of
stale rows.
Add `DROP TABLE IF EXISTS auth_provider CASCADE;` before the `"user"` drop,
and move this line down into the enum group at 87-89 where the other DROP TYPEs
live.
##########
amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala:
##########
@@ -128,9 +179,11 @@ class AuthResource {
user.setName(username)
user.setEmail(useremail)
user.setRole(UserRoleEnum.RESTRICTED)
- // hash the plain text password
- user.setPassword(new
StrongPasswordEncryptor().encryptPassword(userpassword))
- userDao.insert(user)
+ insertLocalUser(
Review Comment:
The guard above this (line 169) is still `userDao.fetchByName(username)` —
it checks the display name, but the handle now lives in
`auth_provider.provider_id`, and `insertLocalUser`'s own scaladoc says identity
is never re-derived from the mutable name.
`ExternalAuthProvisioner.refresh` rewrites `user.name` on every Google
login, so: alice registers locally (`name` = `provider_id` = "alice"), later
signs in with Google as "Alice Smith" and `user.name` moves, then bob registers
as "alice" — `fetchByName` finds nothing, and this insert hits
`uq_provider_identity`. That's a raw `DataAccessException` → 500, not the 406
you wanted. The mirror case rejects a handle that's actually free, because some
Google user happens to display that name.
`localHandleExists` is already defined at line 46 — that's the check
`register` wants. Worth also mapping 23505 → 409 here the way
`AdminUserResource.createLocalAccount` does; right now the same collision
returns two different statuses depending on the entry point.
##########
amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleAuthResource.scala:
##########
@@ -19,30 +19,40 @@
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.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),
+ googleEmail,
+ Some(
Review Comment:
`ExternalProfile.avatar`'s scaladoc says `None` means "the provider supplies
no avatar, so the user's existing avatar column is left untouched rather than
overwritten" — but this wraps unconditionally in `Some(...)` with a `""`
fallback, so `None` is unreachable and a token with no `picture` claim writes
`""` over whatever was stored. Same as the old behavior, so not a regression,
but it isn't what the new doc promises. Dropping the `Some(` and letting the
`Option` through makes the documented path real.
##########
amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala:
##########
@@ -0,0 +1,147 @@
+package org.apache.texera.web.resource.auth
Review Comment:
License header goes above the `package` line — this is the only `.scala`
file in the repo with it the other way round. RAT passes either way, so nothing
catches it.
##########
amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala:
##########
@@ -0,0 +1,147 @@
+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
+ * 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.
+ */
+
+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.jooq.DSLContext
+
+import java.time.OffsetDateTime
+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.
+ */
+final case class ExternalProfile(
+ providerType: ProviderTypeEnum,
+ providerId: String,
+ name: String,
+ email: String,
+ avatar: Option[String] = None
+)
+
+object ExternalAuthProvisioner {
+
+ /**
+ * 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) =>
+ // 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 =>
+ val user = Option(txUserDao.fetchOneByEmail(profile.email)) match {
Review Comment:
This links a verified external identity onto an existing account purely on
an email match, with no `email_verified` check. Inherited from the old
`GoogleAuthResource`, so not a regression — but this class is explicitly the
generic entry point for "github, IEEE accounts, etc.", and the first provider
that doesn't verify email turns this into account takeover. Worth an
`emailVerified: Boolean` on `ExternalProfile` (Google gives you
`email_verified` in the payload) before the second provider lands.
##########
sql/texera_ddl.sql:
##########
@@ -102,16 +104,25 @@ CREATE TABLE IF NOT EXISTS "user"
uid SERIAL PRIMARY KEY,
name VARCHAR(256) NOT NULL,
email VARCHAR(256) UNIQUE,
- password VARCHAR(256),
- google_id VARCHAR(256) UNIQUE,
- google_avatar VARCHAR(100),
+ avatar VARCHAR(100),
role user_role_enum NOT NULL DEFAULT 'INACTIVE',
comment TEXT,
account_creation_time TIMESTAMPTZ NOT NULL DEFAULT now(),
affiliation VARCHAR(128),
- joining_reason VARCHAR(500),
- -- check that either password or google_id is not null
- CONSTRAINT ck_nulltest CHECK ((password IS NOT NULL) OR (google_id IS NOT
NULL))
+ joining_reason VARCHAR(500)
+ );
+
+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),
+ 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),
+ CONSTRAINT ck_provider_credential CHECK ((provider_type = 'LOCAL') =
(password IS NOT NULL))
Review Comment:
Worth noting explicitly: the old `ck_nulltest` guaranteed every user had at
least one credential, and nothing replaces it — a `user` row with zero
`auth_provider` rows is now legal and simply can't log in. `30.sql` only `RAISE
NOTICE`s those. Probably the right call since it can't be a row-level check
anymore, just want it to be a deliberate decision rather than a side effect.
##########
amber/src/test/scala/org/apache/texera/web/auth/ExternalAuthProvisionerSpec.scala:
##########
@@ -0,0 +1,257 @@
+/*
+ * 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
Review Comment:
Package is `...web.resource.auth` but the file sits in `web/auth/`.
`AuthResourceSpec` and `GoogleAuthResourceSpec` are both under
`web/resource/auth/` — move this one next to them.
--
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]
