Copilot commented on code in PR #5743: URL: https://github.com/apache/texera/pull/5743#discussion_r3464125657
########## common/auth/src/test/scala/org/apache/texera/auth/RoleAnnotationEnforcerSpec.scala: ########## @@ -0,0 +1,252 @@ +/* + * 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.auth + +import ch.qos.logback.classic.{Level, Logger => LogbackLogger} +import jakarta.annotation.security.{DenyAll, PermitAll, RolesAllowed} +import jakarta.ws.rs.{DELETE, GET, HEAD, OPTIONS, PATCH, POST, PUT} +import org.glassfish.jersey.server.ResourceConfig +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers +import org.slf4j.LoggerFactory + +class RoleAnnotationEnforcerSpec extends AnyFlatSpec with Matchers { + + "findUnannotatedEndpoints" should "return nothing when every HTTP method is annotated" in { + RoleAnnotationEnforcer.findUnannotatedEndpoints( + Seq(classOf[RoleAnnotationEnforcerSpec.FullyAnnotatedResource]) + ) shouldBe empty + } + + it should "flag an HTTP method with no security annotation" in { + val violations = + RoleAnnotationEnforcer.findUnannotatedEndpoints( + Seq(classOf[RoleAnnotationEnforcerSpec.PartiallyAnnotatedResource]) + ) + violations should have size 1 + violations.head should endWith("#openEndpoint") + } + + it should "treat a class-level annotation as covering every method" in { + RoleAnnotationEnforcer.findUnannotatedEndpoints( + Seq(classOf[RoleAnnotationEnforcerSpec.ClassLevelResource]) + ) shouldBe empty + } + + it should "accept @PermitAll and @DenyAll, not only @RolesAllowed" in { + RoleAnnotationEnforcer.findUnannotatedEndpoints( + Seq(classOf[RoleAnnotationEnforcerSpec.PermitAndDenyResource]) + ) shouldBe empty + } + + it should "ignore methods that are not HTTP-mapped" in { + // helper has no @RolesAllowed but is not a JAX-RS endpoint, so it is not a hole + RoleAnnotationEnforcer.findUnannotatedEndpoints( + Seq(classOf[RoleAnnotationEnforcerSpec.NonEndpointMethodResource]) + ) shouldBe empty + } + + it should "return nothing when given no resources" in { + RoleAnnotationEnforcer.findUnannotatedEndpoints(Seq.empty) shouldBe empty + } + + it should "report every hole across multiple resources as fully-qualified Class#method" in { + val violations = RoleAnnotationEnforcer.findUnannotatedEndpoints( + Seq( + classOf[RoleAnnotationEnforcerSpec.PartiallyAnnotatedResource], + classOf[RoleAnnotationEnforcerSpec.MultiHoleResource] + ) + ) + violations should contain allOf ( + s"${classOf[RoleAnnotationEnforcerSpec.PartiallyAnnotatedResource].getName}#openEndpoint", + s"${classOf[RoleAnnotationEnforcerSpec.MultiHoleResource].getName}#put", + s"${classOf[RoleAnnotationEnforcerSpec.MultiHoleResource].getName}#patch" + ) + } + + it should "detect verbs beyond GET/POST/DELETE via the @HttpMethod meta-annotation" in { + val violations = RoleAnnotationEnforcer.findUnannotatedEndpoints( + Seq(classOf[RoleAnnotationEnforcerSpec.AllVerbsUnannotatedResource]) + ) + violations.map(_.split("#").last) should contain theSameElementsAs + Seq("put", "patch", "head", "options") + } + + it should "treat a security annotation inherited from a superclass method as covering it" in { + RoleAnnotationEnforcer.findUnannotatedEndpoints( + Seq(classOf[RoleAnnotationEnforcerSpec.InheritsAnnotatedEndpoint]) + ) shouldBe empty + } + + it should "let a subclass class-level annotation cover an inherited unannotated endpoint" in { + RoleAnnotationEnforcer.findUnannotatedEndpoints( + Seq(classOf[RoleAnnotationEnforcerSpec.SecuredSubclass]) + ) shouldBe empty + } + + it should "flag an inherited unannotated endpoint against the scanned subclass" in { + RoleAnnotationEnforcer.findUnannotatedEndpoints( + Seq(classOf[RoleAnnotationEnforcerSpec.UnsecuredSubclass]) + ) should contain( + s"${classOf[RoleAnnotationEnforcerSpec.UnsecuredSubclass].getName}#inheritedWrite" + ) + } + + it should "deduplicate when the same resource is scanned more than once" in { + RoleAnnotationEnforcer.findUnannotatedEndpoints( + Seq.fill(3)(classOf[RoleAnnotationEnforcerSpec.PartiallyAnnotatedResource]) + ) should have size 1 + } + + "enforce" should "throw when an endpoint is unannotated" in { + val ex = intercept[IllegalStateException] { + RoleAnnotationEnforcer.enforce( + Seq(classOf[RoleAnnotationEnforcerSpec.PartiallyAnnotatedResource]), + "TestService" + ) + } + ex.getMessage should include("TestService") + ex.getMessage should include("openEndpoint") + } + + "enforce" should "not throw when every endpoint is annotated" in { + noException should be thrownBy RoleAnnotationEnforcer.enforce( + Seq(classOf[RoleAnnotationEnforcerSpec.FullyAnnotatedResource]), + "TestService" + ) + } + + it should "list every offending endpoint in the thrown message" in { + val ex = intercept[IllegalStateException] { + RoleAnnotationEnforcer.enforce( + Seq(classOf[RoleAnnotationEnforcerSpec.MultiHoleResource]), + "TestService" + ) + } + ex.getMessage should include("#put") + ex.getMessage should include("#patch") + } + + it should "not throw when given no resources" in { + noException should be thrownBy RoleAnnotationEnforcer.enforce(Seq.empty, "TestService") + } + + "enforce(ResourceConfig)" should "pass when every registered resource is annotated" in { + val resourceConfig = new ResourceConfig() + resourceConfig.register(classOf[RoleAnnotationEnforcerSpec.FullyAnnotatedResource]) + noException should be thrownBy RoleAnnotationEnforcer.enforce(resourceConfig, "TestService") + } + + it should "throw when a registered resource class has an unannotated endpoint" in { + val resourceConfig = new ResourceConfig() + resourceConfig.register(classOf[RoleAnnotationEnforcerSpec.PartiallyAnnotatedResource]) + val ex = intercept[IllegalStateException] { + RoleAnnotationEnforcer.enforce(resourceConfig, "TestService") + } + ex.getMessage should include("TestService") + ex.getMessage should include("openEndpoint") + } + + it should "throw when a resource registered as an instance has an unannotated endpoint" in { + val resourceConfig = new ResourceConfig() + resourceConfig.register(new RoleAnnotationEnforcerSpec.PartiallyAnnotatedResource) + an[IllegalStateException] should be thrownBy + RoleAnnotationEnforcer.enforce(resourceConfig, "TestService") + } + + it should "still fail closed when error logging is disabled" in { + // enforce logs the violation at error level before throwing. With error + // logging suppressed, the scala-logging `isErrorEnabled` guard takes its + // disabled branch; enforcement must still throw rather than silently pass. Review Comment: The comment mentions a scala-logging `isErrorEnabled` guard, but `RoleAnnotationEnforcer.enforce` unconditionally calls `logger.error(message)` (no explicit guard). This makes the rationale misleading; the test is really about ensuring enforcement still throws even when error logging is turned off. -- 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]
