Ma77Ball commented on code in PR #5199:
URL: https://github.com/apache/texera/pull/5199#discussion_r3321709534
##########
config-service/src/main/scala/org/apache/texera/service/resource/ConfigResource.scala:
##########
@@ -28,8 +28,15 @@ import org.apache.texera.config.{AuthConfig,
ComputingUnitConfig, GuiConfig, Use
@Produces(Array(MediaType.APPLICATION_JSON))
class ConfigResource {
+ // The frontend loads /config/gui and /config/user-system as an
APP_INITIALIZER
+ // (GuiConfigService.load() in gui-config.service.ts) — i.e. before any
login. They
+ // must answer unauthenticated callers so the login page can render. PR
#5049 left
+ // @RolesAllowed on both endpoints, which once RolesAllowedDynamicFeature was
+ // registered started returning 403 during bootstrap and broke the whole
app; that
+ // PR was reverted in #5173. @PermitAll keeps enforcement on for the rest of
the
Review Comment:
With this change both endpoints in `ConfigResource` are `@PermitAll`, and a
grep shows no other `@RolesAllowed` resource anywhere in config-service. So
registering `RolesAllowedDynamicFeature` in `ConfigService.run` has **no
enforcement effect on any current endpoint** — config-service is fully public
either way. That makes the comment "keeps enforcement on for the rest of the
service" inaccurate: there is no protected endpoint here. It is fine as
future-proofing, but consider rewording to say it guards future `@RolesAllowed`
endpoints rather than implying anything is enforced today.
##########
workflow-compiling-service/src/main/scala/org/apache/texera/service/WorkflowCompilingService.scala:
##########
@@ -90,6 +95,20 @@ class WorkflowCompilingService extends
Application[WorkflowCompilingServiceConfi
}
object WorkflowCompilingService {
+ // Registers JWT auth, @Auth injection, and @RolesAllowed enforcement.
+ def registerAuthFeatures(environment: Environment): Unit = {
Review Comment:
`registerAuthFeatures` is now duplicated verbatim here and in
`ComputingUnitManagingService`, and inlined a third time in
`ConfigService.run`. The same JWT-auth + `AuthValueFactoryProvider.Binder` +
`RolesAllowedDynamicFeature` triplet also appears in FileService,
AccessControlService, TexeraWebApplication, and ComputingUnitMaster. Consider
one shared helper in `common/auth` (e.g. `JerseyAuth.register(environment)`) so
the auth pipeline cannot drift between services. At minimum, `ConfigService`
should call the same helper instead of inlining it, for consistency with the
two services that now extract it.
##########
config-service/src/test/scala/org/apache/texera/service/ConfigServiceRunSpec.scala:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.service
+
+import io.dropwizard.core.setup.Environment
+import io.dropwizard.jersey.setup.JerseyEnvironment
+import io.dropwizard.jetty.MutableServletContextHandler
+import io.dropwizard.jetty.setup.ServletEnvironment
+import org.glassfish.jersey.server.filter.RolesAllowedDynamicFeature
+import org.mockito.Mockito.{mock, verify, when}
+import org.scalatest.flatspec.AnyFlatSpec
+import org.scalatest.matchers.should.Matchers
+
+class ConfigServiceRunSpec extends AnyFlatSpec with Matchers {
+
+ // Verifies that the @RolesAllowed annotations on ConfigResource are actually
+ // enforced by Jersey, which requires RolesAllowedDynamicFeature to be
+ // registered on the Jersey environment.
+ "ConfigService.run" should "register RolesAllowedDynamicFeature on the
Jersey environment" in {
+ val jersey = mock(classOf[JerseyEnvironment])
+ val servlets = mock(classOf[ServletEnvironment])
+ val context = mock(classOf[MutableServletContextHandler])
+ val env = mock(classOf[Environment])
+ when(env.jersey).thenReturn(jersey)
+ when(env.servlets).thenReturn(servlets)
+ when(env.getApplicationContext).thenReturn(context)
+
+ val service = new ConfigService
+ // run() reaches into SqlServer near the end to preload defaults; that
throws
+ // here because no real DB is wired up. By that point all Jersey
registrations
+ // have already executed, so the verification below is still valid.
+ intercept[Exception] {
Review Comment:
This test asserts the registration only indirectly: it depends on
`SqlServer.getInstance()` throwing because no DB was wired. `SqlServer` is a
process-wide singleton, so if any earlier-running test in this module calls
`SqlServer.initConnection(...)`, `getInstance()` will not throw here and
`run()` will proceed into the real preload/transaction path — making this test
outcome depend on test ordering. `intercept[Exception]` is broad enough that it
will usually still throw *somewhere*, but it is proving
"RolesAllowedDynamicFeature was registered" via an incidental DB failure.
Prefer the seam approach used in `ComputingUnitManagingServiceRunSpec` (verify
against a mocked `JerseyEnvironment`) rather than running all of `run()`.
##########
config-service/src/test/scala/org/apache/texera/service/resource/ConfigResourceAuthSpec.scala:
##########
@@ -0,0 +1,93 @@
+/*
+ * 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.service.resource
+
+import com.fasterxml.jackson.databind.ObjectMapper
+import com.fasterxml.jackson.module.scala.DefaultScalaModule
+import io.dropwizard.jackson.Jackson
+import io.dropwizard.testing.junit5.ResourceExtension
+import jakarta.annotation.security.RolesAllowed
+import jakarta.ws.rs.core.MediaType
+import jakarta.ws.rs.{GET, Path, Produces}
+import org.apache.texera.auth.JwtAuthFilter
+import org.glassfish.jersey.server.filter.RolesAllowedDynamicFeature
+import org.scalatest.BeforeAndAfterAll
+import org.scalatest.flatspec.AnyFlatSpec
+import org.scalatest.matchers.should.Matchers
+
+// Wires ConfigResource through the same Jersey auth pipeline production uses
+// (JwtAuthFilter + RolesAllowedDynamicFeature) and fires HTTP requests with no
+// Authorization header. Regression guard for the bootstrap break that caused
+// PR #5049 to be reverted in #5173: /config/gui and /config/user-system are
+// loaded by the frontend's APP_INITIALIZER before any login, so they must
+// return 200 to unauthenticated callers even with role enforcement enabled.
+class ConfigResourceAuthSpec extends AnyFlatSpec with Matchers with
BeforeAndAfterAll {
+
+ // Companion to ConfigResource that's deliberately @RolesAllowed, so the same
+ // setup also proves the feature actually rejects when it should — a 200 on
+ // the @PermitAll endpoints would otherwise be consistent with the feature
+ // being silently no-op'd.
+ // Mirror production's mapper: ConfigService bootstraps Dropwizard's default
mapper
+ // (Jackson.newObjectMapper) and registers DefaultScalaModule on top. Same
call here.
+ private val testMapper: ObjectMapper =
+ Jackson.newObjectMapper().registerModule(DefaultScalaModule)
+
+ private val resources: ResourceExtension = ResourceExtension
+ .builder()
+ .setMapper(testMapper)
+ .addProvider(classOf[JwtAuthFilter])
Review Comment:
The class comment says this wires `ConfigResource` "through the same Jersey
auth pipeline production uses", but production registers `JwtAuthFilter` via
`new AuthDynamicFeature(classOf[JwtAuthFilter])`, whereas here it is added as a
plain global `@Provider`. `AuthDynamicFeature` binds the filter per-method
based on auth annotations; a global provider runs on every request. For these
specific assertions (no `Authorization` header → no `SecurityContext` → 200 on
`@PermitAll`, 403 on `@RolesAllowed`) the result is the same, so the test is
valid — just not byte-for-byte the production wiring the comment claims.
##########
computing-unit-managing-service/build.sbt:
##########
@@ -34,6 +34,13 @@ Universal / mappings := AddMetaInfLicenseFiles.distMappings(
// Dependency Versions
val dropwizardVersion = "4.0.7"
+val mockitoVersion = "5.4.0"
+
+// Test Dependencies
+libraryDependencies ++= Seq(
Review Comment:
Minor consistency: this adds a separate test-deps block with only
`scalatest` + `mockito`, whereas config-service and workflow-compiling-service
declare a consolidated test block (scalamock, scalatest, dropwizard-testing,
mockito-core, assertj, junit-interface) and `Global / concurrentRestrictions +=
Tags.limit(Tags.Test, 1)`. The current spec only needs scalatest+mockito so it
compiles, but consider matching the sibling services block — and note that
without the `Tags.limit` line CU tests are not serialized the way the other two
services are.
##########
config-service/src/main/scala/org/apache/texera/service/resource/ConfigResource.scala:
##########
@@ -28,8 +28,15 @@ import org.apache.texera.config.{AuthConfig,
ComputingUnitConfig, GuiConfig, Use
@Produces(Array(MediaType.APPLICATION_JSON))
class ConfigResource {
+ // The frontend loads /config/gui and /config/user-system as an
APP_INITIALIZER
+ // (GuiConfigService.load() in gui-config.service.ts) — i.e. before any
login. They
+ // must answer unauthenticated callers so the login page can render. PR
#5049 left
+ // @RolesAllowed on both endpoints, which once RolesAllowedDynamicFeature was
+ // registered started returning 403 during bootstrap and broke the whole
app; that
+ // PR was reverted in #5173. @PermitAll keeps enforcement on for the rest of
the
+ // service while explicitly whitelisting these two pre-login endpoints.
@GET
- @RolesAllowed(Array("REGULAR", "ADMIN"))
+ @PermitAll
Review Comment:
Heads up on what `@PermitAll` here exposes (pre-existing, not introduced by
this PR): `getGuiConfig` returns `defaultLocalUser.password` (line 63), so
marking `/config/gui` `@PermitAll` formally serves the default local-user
password to fully unauthenticated callers. On `main` this endpoint was already
public — `RolesAllowedDynamicFeature` was never registered there, so the old
`@RolesAllowed` was dead code — so this is not a *new* exposure. But since this
PR cements the access policy, it is worth deciding whether the default password
belongs in a pre-login payload at all; if something needs it before login,
consider returning only the username (or a boolean) and resolving the password
server-side.
--
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]
