Yicong-Huang commented on code in PR #7055:
URL: https://github.com/apache/texera/pull/7055#discussion_r3739210687
##########
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.
+ */
+ def jwtClaims(user: User, expireInDays: Int, googleId: Option[String] =
None): JwtClaims = {
Review Comment:
`expireInDays` is never read — the body hardcodes
`TOKEN_EXPIRE_TIME_IN_MINUTES` at `:78` — and it names days for a minutes
value. Dead and misleading at once.
It is pre-existing, so I would normally leave it. I raise it because you are
already rewriting this signature to add `googleId`. Dropping the parameter in
the same edit is nearly free; later it is its own PR across every call site.
Behavior is unchanged either way, since callers pass the constant the body
already uses.
##########
amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala:
##########
@@ -19,61 +19,61 @@
package org.apache.texera.web.resource.auth
+import com.typesafe.scalalogging.Logger
import org.apache.texera.auth.JwtAuth.{TOKEN_EXPIRE_TIME_IN_MINUTES,
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)
/**
* 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
Review Comment:
States the type where a description belongs — the sibling `@param password`
line below at least says what the value is.
```suggestion
* @param username the LOCAL login handle to authenticate
```
##########
amber/src/test/scala/org/apache/texera/web/resource/auth/GoogleAuthResourceSpec.scala:
##########
@@ -0,0 +1,177 @@
+/*
+ * 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.google.api.client.googleapis.auth.oauth2.GoogleIdToken
+import org.apache.texera.common.config.UserSystemConfig
+import org.apache.texera.dao.MockTexeraDB
+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.scalatest.flatspec.AnyFlatSpec
+import org.scalatest.matchers.should.Matchers
+import org.scalatest.{BeforeAndAfterAll, BeforeAndAfterEach}
+
+import javax.ws.rs.NotAuthorizedException
+
+/**
+ * Integration spec for [[GoogleAuthResource]] against embedded Postgres.
+ *
+ * Token verification is the one part that cannot run here — it needs a
Google-signed JWT and a
+ * network round trip — so the suite overrides `verifiedPayload` and drives
the resource with
+ * payloads built by hand. What it pins down is everything downstream of
verification: how a
+ * Google payload becomes an [[ExternalProfile]] (the name fallback and the
avatar reduced to
+ * its last path segment) and that an unverifiable credential is a 401 rather
than a crash.
Review Comment:
The "rather than a crash" half stopped holding when the 401 handling was
reverted. `verifiedPayload` (`GoogleAuthResource.scala:75-76`) has no exception
handling now, and this suite's stub overrides that very method — so the crash
path is the one thing the suite cannot observe.
The 401-on-`None` half is real and worth keeping.
```suggestion
* its last path segment) and that a credential Google does not verify is a
401.
```
##########
sql/changelog.xml:
##########
@@ -68,10 +68,16 @@
<sqlFile path="sql/updates/31.sql"/>
</changeSet>
+ <!-- Split auth credentials out of "user" into auth_provider, and store
the provider's
+ full avatar URL instead of a Google-specific fragment -->
Review Comment:
This describes the avatar change that was withdrawn. `32.sql:24-26` now says
the opposite: the rename is in place and "does not change what any user's
avatar resolves to".
The changelog is what a DBA reads to know what a migration does, so it is
worth keeping honest.
```suggestion
<!-- Split auth credentials out of "user" into auth_provider, and rename
the
Google-specific google_avatar column to the provider-neutral avatar
-->
```
##########
amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala:
##########
@@ -95,21 +99,33 @@ object AuthResource {
)
}
- def createAdminUser(): Unit = {
- val adminUsername = UserSystemConfig.adminUsername
- val adminPassword = UserSystemConfig.adminPassword
+ def createAdminUser(): Unit =
+ createAdminUser(UserSystemConfig.adminUsername.trim,
UserSystemConfig.adminPassword.trim)
- 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)
- }
+ /**
+ * Bootstrap the configured admin account, doing nothing if it already
exists. The credentials
+ * are parameters rather than reads of [[UserSystemConfig]] because those
are object vals
+ * resolved once per JVM, which leaves the unconfigured case unreachable
from a test.
+ */
+ private[auth] def createAdminUser(adminUsername: String, adminPassword:
String): Unit = {
+ if (adminUsername.isEmpty || adminPassword.isEmpty) return
+
+ if (LocalAuthProvisioner.handleExists(adminUsername)) return
+
+ if (userDao.fetchOneByEmail(adminUsername) != null) {
Review Comment:
This guard is case-sensitive, so it misses the collision it exists to catch.
`"user".email` is a plain case-sensitive UNIQUE (`texera_ddl.sql:108`), so a
configured admin username differing only in casing from a stored email finds
nothing here. `createLocalAccount` below then inserts a second account,
violating nothing. The admin lands in an empty ADMIN account beside the real
one — the outcome this warning exists to prevent.
`fetchUserByEmailIgnoreCase` is already in this object at `:81`, and it is
the rule you established at `ExternalAuthProvisioner.scala:52-58`.
```suggestion
if (fetchUserByEmailIgnoreCase(adminUsername) != null) {
```
Worth a test too — `AuthResourceSpec.scala:364-380` covers only the insert
and idempotence paths, so nothing reaches this branch today.
--
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]
