Ma77Ball commented on code in PR #5199: URL: https://github.com/apache/texera/pull/5199#discussion_r3322381572
########## 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]) + .addProvider(classOf[RolesAllowedDynamicFeature]) + .addResource(new ConfigResource) + .addResource(new ConfigResourceAuthSpec.ProtectedProbe) + .build() + + override protected def beforeAll(): Unit = resources.before() + override protected def afterAll(): Unit = resources.after() + + "GET /config/gui" should "return 200 without an Authorization header" in { + val response = resources.target("/config/gui").request(MediaType.APPLICATION_JSON).get() + response.getStatus shouldBe 200 + } + + "GET /config/user-system" should "return 200 without an Authorization header" in { + val response = + resources.target("/config/user-system").request(MediaType.APPLICATION_JSON).get() + response.getStatus shouldBe 200 + } + + "GET an @RolesAllowed endpoint" should "return 403 without an Authorization header" in { + // Sanity: with no SecurityContext set by JwtAuthFilter, RolesAllowedDynamicFeature + // must reject. Catches the case where the feature is registered but somehow + // disabled (e.g. swallowed exception during setup). + val response = + resources.target("/auth-probe").request(MediaType.APPLICATION_JSON).get() + response.getStatus shouldBe 403 + } +} + +object ConfigResourceAuthSpec { + @Path("/auth-probe") + @Produces(Array(MediaType.APPLICATION_JSON)) + class ProtectedProbe { Review Comment: …and move the companion comment here, where it belongs: ```suggestion // A deliberately @RolesAllowed companion to ConfigResource, 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. @Path("/auth-probe") @Produces(Array(MediaType.APPLICATION_JSON)) class ProtectedProbe { ``` ########## 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. Review Comment: ```suggestion // These two endpoints are fetched by the frontend during app initialization, // before any login, so they must answer unauthenticated callers — hence @PermitAll. // They are the only endpoints in this resource, so role enforcement gates nothing // here; @PermitAll is what keeps them reachable when role enforcement is enabled. ``` I don't think this comment accurately represents what the `@PermitAll` decorator is doing here. I suggest changing it to this and avoiding any mention of reverted PRs. ########## 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. Review Comment: This comment actually describes `ProtectedProbe`. I suggest removing it here and moving it to the probe class (see next suggestion). ```suggestion ``` ########## 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. Review Comment: Stale comment: `ConfigResource` no longer has `@RolesAllowed` — both endpoints are `@PermitAll`. The feature is registered to guard *future* `@RolesAllowed` endpoints, which is what this should say. ```suggestion // ConfigResource's own endpoints are @PermitAll, but the service still registers // RolesAllowedDynamicFeature so that any @RolesAllowed endpoint is enforced by // Jersey. This verifies that registration actually happens. ``` ########## config-service/src/main/scala/org/apache/texera/service/ConfigService.scala: ########## @@ -71,6 +72,9 @@ class ConfigService extends Application[ConfigServiceConfiguration] with LazyLog new io.dropwizard.auth.AuthValueFactoryProvider.Binder(classOf[SessionUser]) ) + // Enforce @RolesAllowed annotations on resource methods + environment.jersey.register(classOf[RolesAllowedDynamicFeature]) Review Comment: Minor / consistency: the other two services factor this JWT + `@Auth` + `RolesAllowedDynamicFeature` block into a `registerAuthFeatures` helper, but `ConfigService` inlines it. Consider sharing the same helper so the three don't drift apart. ########## computing-unit-managing-service/src/test/scala/org/apache/texera/service/ComputingUnitManagingServiceRunSpec.scala: ########## @@ -0,0 +1,48 @@ +/* + * 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.auth.{AuthDynamicFeature, AuthValueFactoryProvider} +import io.dropwizard.core.setup.Environment +import io.dropwizard.jersey.setup.JerseyEnvironment +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 ComputingUnitManagingServiceRunSpec extends AnyFlatSpec with Matchers { + + // Verifies that the @RolesAllowed annotations on resource methods are actually + // enforced by Jersey, which requires RolesAllowedDynamicFeature, AuthDynamicFeature, + // and AuthValueFactoryProvider.Binder to be registered on the Jersey environment. + "ComputingUnitManagingService.registerAuthFeatures" should "register auth + RolesAllowedDynamicFeature on the Jersey environment" in { Review Comment: **Bigger picture (all three `*RunSpec`s):** these use mocks to check that `register(...)` was called, basically "the setup code ran the setup line." They don't prove any endpoint is actually protected. The catch: `RolesAllowedDynamicFeature` only guards endpoints that have `@RolesAllowed` anything; without it, it is **public**. So if someone later adds an endpoint and forgets the annotation, it's silently open and these tests still pass. (Everything's annotated today, so nothing's broken; there's just no test guarding against that mistake.) `ConfigResourceAuthSpec` already shows the better approach: spin up the real resource with `ResourceExtension` and send a real request. Suggest one of those per service instead **no token → `403`**, **valid `REGULAR` token → `200`**. That actually tests the behavior. > [!NOTE] > This also kills a flaky test. `ConfigServiceRunSpec` relies on `run()` crashing on a missing DB (`intercept[Exception]`), but `SqlServer` is a shared singleton; if another test connects to a DB first, the crash doesn't happen, and this test breaks depending on test order. A real-request test doesn't touch the DB, so the problem goes away. -- 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]
