Yicong-Huang commented on code in PR #7055:
URL: https://github.com/apache/texera/pull/7055#discussion_r3694395793
##########
common/auth/src/main/scala/org/apache/texera/auth/JwtAuth.scala:
##########
@@ -55,10 +55,9 @@ object JwtAuth {
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("avatar", user.getAvatar)
Review Comment:
**Cross-language contract break — no frontend changes ship with this PR.**
JWT claims have three readers: this writer, the Scala `JwtParser`, and the
hand-written TS reader
`frontend/src/app/common/service/user/auth.service.ts:167-168`, which still
reads `.googleId` and `.googleAvatar`. After this PR both are `undefined`:
- Renaming the `googleAvatar` claim to `avatar` makes **every user's
avatar** `undefined` — `coeditor-user-icon`, `admin-user`, `user-icon`,
`workflow-execution-history`, `computing-unit-selection`, and the dashboard
cards all read `googleAvatar`.
- Removing the `googleId` claim entirely is more than a rename:
`frontend/src/app/dashboard/service/user/flarum/flarum.service.ts:39,48` uses
`user.googleId` as the **Flarum forum SSO password**, so forum login breaks and
there is no `googleId` left to repoint it at.
The admin `UserInfo` field rename (`googleAvatar`→`avatar`) breaks
`admin-user.component.html:196` the same way. Backend CI is green because the
TS mirror is not compiled against Scala. Resolve in this PR: either keep the
claim/field names, or update the frontend mirror (`auth.service.ts`,
`common/type/user.ts`, the admin component) in lockstep and decide where
Flarum's credential comes from now.
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowExecutionsResource.scala:
##########
@@ -582,7 +582,7 @@ class WorkflowExecutionsResource {
WORKFLOW_EXECUTIONS.VID,
WORKFLOW_EXECUTIONS.CUID,
USER.NAME,
- USER.GOOGLE_AVATAR,
+ USER.AVATAR,
Review Comment:
Second site of the same `fetchInto` column/field mismatch — alias here too:
```suggestion
USER.AVATAR.as("googleAvatar"),
```
##########
amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala:
##########
@@ -35,53 +36,103 @@ 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)
+
+ private val passwordEncryptor = new StrongPasswordEncryptor
+
+ private def localHandleExists(handle: String): Boolean = {
+ context.fetchExists(
+ context
+ .selectFrom(AUTH_PROVIDER)
+ .where(AUTH_PROVIDER.PROVIDER_TYPE.eq(ProviderTypeEnum.LOCAL))
+ .and(AUTH_PROVIDER.PROVIDER_ID.eq(handle))
)
+ }
+
+ //TODO ASSERT THAT ALL USERS WERE MIGRATED CORRECTLY AND CHECK
Review Comment:
Leftover shouting placeholder — please remove or complete it before merge.
(advisory)
##########
common/auth/src/main/scala/org/apache/texera/auth/JwtParser.scala:
##########
@@ -62,17 +62,15 @@ object JwtParser extends LazyLogging {
// call writes Integer; widen via Number to handle both cases.
val userId = claims.getClaimValue("userId", classOf[Number]).intValue()
val role =
UserRoleEnum.valueOf(claims.getClaimValue("role").asInstanceOf[String])
- val googleId = claims.getClaimValue("googleId", classOf[String])
- val googleAvatar = claims.getClaimValue("googleAvatar", classOf[String])
+ val googleAvatar = claims.getClaimValue("avatar", classOf[String])
Review Comment:
The local is still named `googleAvatar` but now holds the provider-neutral
`avatar` claim, so the `google` prefix misleads. Suggest renaming it (and
updating `setAvatar(googleAvatar)` below) to `avatar`. Same stale-name pattern
in `DashboardResource` and `ComputingUnitManagingResource`
(`ownerGoogleAvatar`). (advisory)
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowExecutionsResource.scala:
##########
@@ -321,7 +321,7 @@ object WorkflowExecutionsResource {
WORKFLOW_EXECUTIONS.VID,
WORKFLOW_EXECUTIONS.CUID,
USER.NAME,
- USER.GOOGLE_AVATAR,
+ USER.AVATAR,
Review Comment:
This projects the renamed column `avatar` into
`.fetchInto(classOf[WorkflowExecutionEntry])`, but the case-class field is
still `googleAvatar` (line 527). jOOQ's `DefaultRecordMapper` matches by
normalized column name, and `avatar` does not match `googleavatar`, so the
avatar is left null on every execution-history row. It worked before the rename
because the column was `google_avatar`. Alias the projection to preserve the
field mapping (and the frontend field name):
```suggestion
USER.AVATAR.as("googleAvatar"),
```
Same fix is needed at line 585. Please also add a spec asserting the avatar
value in the execution-history query — no current test observes it.
##########
amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala:
##########
@@ -35,53 +36,103 @@ 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)
+
+ private val passwordEncryptor = new StrongPasswordEncryptor
+
+ private def localHandleExists(handle: String): Boolean = {
+ context.fetchExists(
+ context
+ .selectFrom(AUTH_PROVIDER)
+ .where(AUTH_PROVIDER.PROVIDER_TYPE.eq(ProviderTypeEnum.LOCAL))
+ .and(AUTH_PROVIDER.PROVIDER_ID.eq(handle))
)
+ }
+
+ //TODO ASSERT THAT ALL USERS WERE MIGRATED CORRECTLY AND CHECK
/**
* Retrieve exactly one User from databases with the given username and
password.
* The password is used to validate against the hashed password stored in
the db.
*
- * @param name String
+ * @param username String
* @param password String, plain text password
* @return
*/
- def retrieveUserByUsernameAndPassword(name: String, password: String):
Option[User] = {
- if (password == null) return None
- if (name == null) return None
- Option(
- SqlServer
- .getInstance()
- .createDSLContext()
- .select()
- .from(USER)
- .where(USER.NAME.eq(name))
- .fetchOneInto(classOf[User])
- ).filter(user => new StrongPasswordEncryptor().checkPassword(password,
user.getPassword))
- }
+ def retrieveUserByUsernameAndPassword(username: String, password: String):
Option[User] = {
+ if (password == null || username == null) return None
- def createAdminUser(): Unit = {
- val adminUsername = UserSystemConfig.adminUsername
- val adminPassword = UserSystemConfig.adminPassword
+ val record = context
+ .select()
+ .from(AUTH_PROVIDER)
+ .join(USER)
+ .on(USER.UID.eq(AUTH_PROVIDER.UID))
+ .where(AUTH_PROVIDER.PROVIDER_TYPE.eq(ProviderTypeEnum.LOCAL))
+ .and(AUTH_PROVIDER.PROVIDER_ID.eq(username))
+ .fetchOne()
- 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)
+ Option(record).flatMap(r => {
+ val encryptedPassword = r.get(AUTH_PROVIDER.PASSWORD)
+ if (passwordEncryptor.checkPassword(password, encryptedPassword)) {
+ Some(r.into(USER).into(classOf[User]))
+ } else {
+ None
}
+ })
+ }
+
+ /**
+ * Create a user together with the LOCAL credential it logs in with. The
handle is passed
+ * explicitly rather than read off `user.getName`, so that identity is
never re-derived
+ * from the mutable display name.
+ */
+ private def insertLocalUser(user: User, handle: String, hashedPassword:
String): Unit = {
Review Comment:
`insertLocalUser` and `AdminUserResource.createLocalAccount` are
near-identical "insert `User` + insert a LOCAL `AuthProvider` in one
transaction" sequences (with `ExternalAuthProvisioner.upsertProvider` a third
partial variant). Consider extracting one shared helper so a future change to
how a LOCAL credential is created lands once instead of diverging across
resources. (advisory)
##########
sql/updates/30.sql:
##########
@@ -0,0 +1,154 @@
+/*
+ * 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.
+ */
+
+\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
+$$;
+
+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;
+BEGIN
+ 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));
+
+SELECT string_agg(uid::TEXT, ', ')
+INTO orphans
+FROM "user"
+WHERE password IS NULL AND google_id IS NULL;
+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 29: cannot promote "user".name to a login
handle - '
Review Comment:
This is `30.sql`, but the user-facing message names the wrong migration:
```suggestion
RAISE EXCEPTION 'migration 30: cannot promote "user".name to a login
handle - '
```
The `RAISE NOTICE` at line 92 has the same `migration 29` text, and the file
is missing a trailing newline. (polish)
--
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]
