Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Repeating @PermissionsAllowed annotations totally disable method authentication #44185

Closed
AlexanderUkhta opened this issue Oct 30, 2024 · 13 comments · Fixed by #44238
Closed
Assignees
Labels
Milestone

Comments

@AlexanderUkhta
Copy link

AlexanderUkhta commented Oct 30, 2024

Describe the bug

Currently using the latest Quarkus 3.15.1.

I have my security annotations being used in the resource interface. While the method is annotated with a single @PermissionsAllowed annotation or is marked as @Authenticated - it all works fine. If the method is annotated with repeating @PermissionsAllowed, any auth or permission validation does not work (Got 200 OK on any request).

Please see my test resource with comments below:

@Path("/security_test")
interface TestResourceInterface {
    @GET
    @Path("/one")
    @PermissionsAllowed("zoneA:view")
    suspend fun one(): String     // works fine

    @GET
    @Path("/two")
    @PermissionsAllowed("zoneB:view", "zoneB:update")
    suspend fun two(): String      // works fine

    @GET
    @Path("/three")
    @PermissionsAllowed("zoneC:view")
    @PermissionsAllowed("zoneC:create")
    suspend fun three(): String      // does not work at all

    @GET
    @Path("/four")
    @Authenticated
    suspend fun four(): String       // works fine
}

If I put the @Authenticated additionally on the interface level - it all works fine. However, I can't use the @Authenticated annotation this way due to some limitations of my resource generator and because I still need to keep some api methods public.

Expected behavior

Repeating @PermissionsAllowed annotations on a method should work as multiple permissions, that all are needed to access the api method.

Actual behavior

Interface method, which is annotated with repeating @PermissionsAllowed and the interface is not marked with @Authenticated - always returns 200 OK.

How to Reproduce?

No response

Output of uname -a or ver

Darwin Kernel Version 21.6.0: Mon Jun 24 00:56:10 PDT 2024; root:xnu-8020.240.18.709.2~1/RELEASE_X86_64 x86_64

Output of java -version

openjdk version "21.0.4" 2024-07-16 LTS OpenJDK Runtime Environment Corretto-21.0.4.7.1 (build 21.0.4+7-LTS) OpenJDK 64-Bit Server VM Corretto-21.0.4.7.1 (build 21.0.4+7-LTS, mixed mode, sharing)

Quarkus version or git rev

3.15.1

Build tool (ie. output of mvnw --version or gradlew --version)

------------------------------------------------------------ Gradle 8.6 ------------------------------------------------------------ Build time: 2024-02-02 16:47:16 UTC Revision: d55c486870a0dc6f6278f53d21381396d0741c6e Kotlin: 1.9.20 Groovy: 3.0.17 Ant: Apache Ant(TM) version 1.10.13 compiled on January 4 2023 JVM: 18.0.2 (Amazon.com Inc. 18.0.2+9-FR) OS: Mac OS X 12.7.6 x86_64

Additional information

No response

@AlexanderUkhta AlexanderUkhta added the kind/bug Something isn't working label Oct 30, 2024
Copy link

quarkus-bot bot commented Oct 30, 2024

/cc @geoand (kotlin)

@geoand
Copy link
Contributor

geoand commented Oct 30, 2024

cc @michalvavrik

@michalvavrik
Copy link
Member

michalvavrik commented Oct 30, 2024

Before I start, https://quarkus.io/guides/security-authorize-web-endpoints-reference#endpoint-security-annotations-and-jakarta-rest-inheritance says you should only use security annotations on implementations. In order to not break existing behavior, we kept these of interface checks, that worked in past, working. That also makes user friendly validation exception really hard, although it is possible.

Repeating @PersmissionsAllowed annotations totally disable method authentication

We have great many tests like these. I think your scenario differs and in order to determine where, I need to run this.
@AlexanderUkhta while behavior you describe is intriguing, issue description is not enough because I don't know how your implementation looks like, neither I know how your configuration is setup. If you want me to look at this, can you provide something I can run or at least relevant code snippes (like interface + implementation + info about config setup like proactivate enabled/disabled )?

@michalvavrik
Copy link
Member

Honestly:

    @GET
    @Path("/three")
    @PermissionsAllowed("zoneC:view")
    @PermissionsAllowed("zoneC:create")
    suspend fun three(): String      // does not work at all

doesn't give me information, because I don't know what permissions your identity has. Sorry for stupid question, but do you understand that your identity needs both zoneC:view and zoneC:create and you will only be granted access if your identity has both? I am just checking.

@michalvavrik
Copy link
Member

michalvavrik commented Oct 30, 2024

So Got 200 OK on any request, you basically say that permission check is not applied. I'll check when you provide above-mentioned config.

@michalvavrik
Copy link
Member

michalvavrik commented Oct 30, 2024

One more thing @AlexanderUkhta , are you using Quarkus REST or RESTEasy Classic? Better provide a reproducer.

@AlexanderUkhta
Copy link
Author

you will only be granted access if your identity has both?

Sure, as I've written above - that was the expected behavior, but the access is granted anyway.

I'll check when you provide above-mentioned config.

I'll send you a link with an example a bit later.

@michalvavrik
Copy link
Member

you will only be granted access if your identity has both?

Sure, as I've written above - that was the expected behavior, but the access is granted anyway.

Sure, sorry, I missed that.

I'll check when you provide above-mentioned config.

I'll send you a link with an example a bit later.

Thanks. It doesn't need to be minimal, just something that can be run (even after adjustments).

@michalvavrik
Copy link
Member

@AlexanderUkhta I think I have guessed it. Better not spend time on it and don't create the reproducer. Please forget about the reproducer. Let's just confirm me 2 things:

  • it works if you move annotation down to the implementation
  • you are using Quarkus REST

@sberyozkin
Copy link
Member

@AlexanderUkhta
Copy link
Author

AlexanderUkhta commented Oct 30, 2024

@michalvavrik

you are using Quarkus REST

Yes, you're right.

it works if you move annotation down to the implementation

I've just tried to move my @PermissionsAllowed annotation to the implementation. Let's look at the snippets below.

  1. The case with the single annotation on the impl method.

My interface now:

import io.quarkus.security.Authenticated
import io.quarkus.security.PermissionsAllowed
import jakarta.ws.rs.GET
import jakarta.ws.rs.Path

@Path("/security_test")
interface TestResourceInterface {
    @GET
    @Path("/three")
    suspend fun three(): String
}

My implementation is sth like (let's assume, that auditor is just some module, which suspend methods are to be called):

import io.quarkus.security.PermissionsAllowed
import my.audit.package.BaseAuditEvent
import my.audit.package.LoggingAuditor

class TestResource(
    private val auditor: LoggingAuditor
): TestResourceInterface {

    @PermissionsAllowed("permission:view")
    override suspend fun three(): String = auditor.withAudit(BaseAuditEvent.someEvent) {
        "ok"
    }
}

And that is my test (kerberos auth is used and roles/permissions are stored in db):

@QuarkusTest
class SomeTest {
    @BeforeEach
    fun setUp() {  // some kerberos test pre-configuration
        RestAssured.replaceFiltersWith(
            SpnegoAuthFilter(PRINCIPAL, "someUser"),  
            RequestLoggingFilter(),
            ResponseLoggingFilter()
        )
    }

    @Test
    fun `user with not all required permissions should not be allowed to consume api`() {
        val role = createRole()
        assignPermissions(role, "permission:create")
        assignUser(role, PREDEFINED_USER)
        setupRestAssured(PREDEFINED_USER)

        When {
            get("/security_test/three")
        } Then {
            statusCode(403)
        }
    }
}

Above everything is fine.

  1. The case with the repeating annotations on the impl method.

Now only my method annotations quantity is changed:

    @PermissionsAllowed("permission:view")
    @PermissionsAllowed("permission:create")
    override suspend fun three(): String = auditor.withAudit(BaseAuditEvent.someEvent) {
        "ok"
    }

In this case I've got an exception:

java.lang.IllegalStateException: The current thread cannot be blocked: vert.x-eventloop-thread-2 @coroutine#13
	at io.smallrye.mutiny.operators.uni.UniBlockingAwait.await(UniBlockingAwait.java:30)
	at io.smallrye.mutiny.groups.UniAwait.atMost(UniAwait.java:65)
	at io.smallrye.mutiny.groups.UniAwait.indefinitely(UniAwait.java:46)
	at io.quarkus.security.identity.SecurityIdentity.checkPermissionBlocking(SecurityIdentity.java:126)
	at io.quarkus.security.runtime.interceptor.check.PermissionSecurityCheck$3.checkPermissions(PermissionSecurityCheck.java:194)
	at io.quarkus.security.runtime.interceptor.check.PermissionSecurityCheck$3.checkPermissions(PermissionSecurityCheck.java:168)
	at io.quarkus.security.runtime.interceptor.check.PermissionSecurityCheck.apply(PermissionSecurityCheck.java:50)
	at io.quarkus.security.runtime.interceptor.SecurityConstrainer.check(SecurityConstrainer.java:77)
	at io.quarkus.security.runtime.interceptor.SecurityHandler.handle(SecurityHandler.java:46)
	at io.quarkus.security.runtime.interceptor.PermissionsAllowedInterceptor.intercept(PermissionsAllowedInterceptor.java:26)
	at io.quarkus.security.runtime.interceptor.PermissionsAllowedInterceptor_Bean.intercept(Unknown Source)
	at io.quarkus.arc.impl.InterceptorInvocation.invoke(InterceptorInvocation.java:42)
	at io.quarkus.arc.impl.AroundInvokeInvocationContext.proceed(AroundInvokeInvocationContext.java:70)
	at io.quarkus.arc.impl.AroundInvokeInvocationContext.proceed(AroundInvokeInvocationContext.java:62)
	at io.quarkus.resteasy.reactive.server.runtime.StandardSecurityCheckInterceptor.intercept(StandardSecurityCheckInterceptor.java:44)
	at io.quarkus.resteasy.reactive.server.runtime.StandardSecurityCheckInterceptor_PermissionsAllowedInterceptor_Bean.intercept(Unknown Source)
	at io.quarkus.arc.impl.InterceptorInvocation.invoke(InterceptorInvocation.java:42)
	at io.quarkus.arc.impl.AroundInvokeInvocationContext.perform(AroundInvokeInvocationContext.java:30)
	at io.quarkus.arc.impl.InvocationContexts.performAroundInvoke(InvocationContexts.java:27)
	at tech.inno.lcm.authz.TestResource_Subclass.three(Unknown Source)
	at tech.inno.lcm.authz.TestResourceInterface$quarkuscoroutineinvoker$three_ecf4e964fe2c1e096a50494ac09d978f06fc9357.invokeCoroutine(Unknown Source)
	at org.jboss.resteasy.reactive.server.runtime.kotlin.CoroutineInvocationHandler$handle$job$1.invokeSuspend(CoroutineInvocationHandler.kt:48)
	at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
	at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:104)
	at org.jboss.resteasy.reactive.server.runtime.kotlin.VertxDispatcher.dispatch$lambda$0(ApplicationCoroutineScope.kt:45)
	at io.vertx.core.impl.ContextInternal.dispatch(ContextInternal.java:270)
	at io.vertx.core.impl.ContextInternal.dispatch(ContextInternal.java:252)
	at io.vertx.core.impl.ContextInternal.lambda$runOnContext$0(ContextInternal.java:50)
	at io.netty.util.concurrent.AbstractEventExecutor.runTask(AbstractEventExecutor.java:173)
	at io.netty.util.concurrent.AbstractEventExecutor.safeExecute(AbstractEventExecutor.java:166)
	at io.netty.util.concurrent.SingleThreadEventExecutor.runAllTasks(SingleThreadEventExecutor.java:469)
	at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:569)
	at io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:994)
	at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74)
	at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
	at java.base/java.lang.Thread.run(Thread.java:1583)

Seems to be a problem with a blocking call in io.quarkus.security.runtime.interceptor.check.PermissionSecurityCheck#checkPermissions(io.quarkus.security.identity.SecurityIdentity, java.security.Permission[][]), doesn't it:

@michalvavrik
Copy link
Member

Thank you for confirming @AlexanderUkhta , and thanks for reporting this.

In this case I've got an exception:

that's this one I think #44068, maybe have a look, I provided there context

@AlexanderUkhta AlexanderUkhta changed the title Repeating @PersmissionsAllowed annotations totally disable method authentication Repeating @PermissionsAllowed annotations totally disable method authentication Oct 30, 2024
@michalvavrik
Copy link
Member

We don't want to support security annotations on interfaces, but what you describe here is bad user experience and we can definitely do better. I'll take care of it this or next week.

@michalvavrik michalvavrik self-assigned this Oct 30, 2024
@quarkus-bot quarkus-bot bot added this to the 3.17 - main milestone Oct 31, 2024
@gsmet gsmet modified the milestones: 3.17 - main, 3.16.2 Nov 5, 2024
@gsmet gsmet modified the milestones: 3.16.2, 3.15.2 Nov 13, 2024
benkard pushed a commit to benkard/quarkus-googlecloud-jsonlogging that referenced this issue Nov 13, 2024
…us-googlecloud-jsonlogging!24)

This MR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [io.quarkus:quarkus-extension-processor](https://github.com/quarkusio/quarkus) |  | patch | `3.16.1` -> `3.16.3` |
| [io.quarkus:quarkus-extension-maven-plugin](https://github.com/quarkusio/quarkus) | build | patch | `3.16.1` -> `3.16.3` |
| [io.quarkus:quarkus-bom](https://github.com/quarkusio/quarkus) | import | patch | `3.16.1` -> `3.16.3` |
| [io.quarkus:quarkus-maven-plugin](https://github.com/quarkusio/quarkus) | build | patch | `3.16.1` -> `3.16.3` |

---

### Release Notes

<details>
<summary>quarkusio/quarkus</summary>

### [`v3.16.3`](quarkusio/quarkus@3.16.2...3.16.3)

[Compare Source](quarkusio/quarkus@3.16.2...3.16.3)

### [`v3.16.2`](https://github.com/quarkusio/quarkus/releases/tag/3.16.2)

[Compare Source](quarkusio/quarkus@3.16.1...3.16.2)

##### Complete changelog

-   [#&#8203;34824](quarkusio/quarkus#34824) - AmazonLambdaRecorder Handler Discovery Erroneously Considers Decorators
-   [#&#8203;38086](quarkusio/quarkus#38086) - Documentation about `RecordCodecProvider` in MongoDB with Panache
-   [#&#8203;42149](quarkusio/quarkus#42149) - Upgrade Postgres 16
-   [#&#8203;44039](quarkusio/quarkus#44039) - WebSockets Next: create a new event loop context for each client
-   [#&#8203;44132](quarkusio/quarkus#44132) - Update getting-started-reactive.adoc
-   [#&#8203;44149](quarkusio/quarkus#44149) - Fix Config Error Screen
-   [#&#8203;44152](quarkusio/quarkus#44152) - Do not throw NPE in AfterAll interceptor if application didn't start
-   [#&#8203;44155](quarkusio/quarkus#44155) - Amazon Lambda - Support decorators
-   [#&#8203;44156](quarkusio/quarkus#44156) - Make OidcRequestContextProperties modifiable
-   [#&#8203;44178](quarkusio/quarkus#44178) - Bump com.fasterxml.jackson:jackson-bom from 2.18.0 to 2.18.1
-   [#&#8203;44183](quarkusio/quarkus#44183) - Properly apply the update recipes in version order
-   [#&#8203;44184](quarkusio/quarkus#44184) - Add Support for Trusted Proxy Detection on Forwarded Requests
-   [#&#8203;44185](quarkusio/quarkus#44185) - Repeating `@PermissionsAllowed` annotations totally disable method authentication
-   [#&#8203;44189](quarkusio/quarkus#44189) - Small improvements to the Deploying to Google Cloud guide
-   [#&#8203;44190](quarkusio/quarkus#44190) - ResteasyReactiveProcessor#setupEndpoints reports duplicate endpoints when a rest client matches a resource
-   [#&#8203;44191](quarkusio/quarkus#44191) - Update PostgreSQL image to 17
-   [#&#8203;44201](quarkusio/quarkus#44201) - Broken link
-   [#&#8203;44202](quarkusio/quarkus#44202) - Broken link?
-   [#&#8203;44203](quarkusio/quarkus#44203) - Make OidcRequestContextProperties modifiable
-   [#&#8203;44204](quarkusio/quarkus#44204) - Fix broken doc links
-   [#&#8203;44207](quarkusio/quarkus#44207) - Bump bouncycastle.version from 1.78.1 to 1.79
-   [#&#8203;44209](quarkusio/quarkus#44209) - Bump io.quarkus.develocity:quarkus-project-develocity-extension from 1.1.6 to 1.1.7
-   [#&#8203;44221](quarkusio/quarkus#44221) - Add extension description for websockets next
-   [#&#8203;44227](quarkusio/quarkus#44227) - Add stork-configuration-generator as an annotationProcessorPath
-   [#&#8203;44229](quarkusio/quarkus#44229) - ContainerResponseFilter with `@Priority(Integer.MIN_VALUE)` will be actually invoked with max priority
-   [#&#8203;44232](quarkusio/quarkus#44232) - Quartz: use a more reasonable default for quarkus.quartz.thread-count
-   [#&#8203;44235](quarkusio/quarkus#44235) - 3.16: `@WithTestResource` starts all test resources (regression)
-   [#&#8203;44237](quarkusio/quarkus#44237) - Properly implement priority of ContainerResponseFilter
-   [#&#8203;44238](quarkusio/quarkus#44238) - Refactor SecurityTransformerUtils to consider repeated annotations
-   [#&#8203;44239](quarkusio/quarkus#44239) - Replace oidc auth facebook screenshots with generic ones
-   [#&#8203;44244](quarkusio/quarkus#44244) - Bump `quarkiverse-parent` to 18
-   [#&#8203;44245](quarkusio/quarkus#44245) - Delete disabled job
-   [#&#8203;44248](quarkusio/quarkus#44248) - Ignore client interfaces when detecting duplicate endpoints
-   [#&#8203;44263](quarkusio/quarkus#44263) - Quarkus Dev UI - Clicking on gRPC - Services - service implementation class  Uncaught exception received by Vert.x
-   [#&#8203;44277](quarkusio/quarkus#44277) - Dev UI Open in IDE make sure lineNumber is in quotes
-   [#&#8203;44279](quarkusio/quarkus#44279) - Limit `MATCHING_RESOURCES` TestResources to the test that declares them
-   [#&#8203;44281](quarkusio/quarkus#44281) - Included pages within a fragment ignores rendered=false property.
-   [#&#8203;44298](quarkusio/quarkus#44298) - Qute: fix rendered=false if a fragment includes nested fragment
-   [#&#8203;44300](quarkusio/quarkus#44300) - When testing request payload is populated with string "null" if enable-reflection-free-serializers enabled
-   [#&#8203;44309](quarkusio/quarkus#44309) - Avoid deserializing null nodes in reflection free Jackson serialization
-   [#&#8203;44316](quarkusio/quarkus#44316) - Duplicated field serialization using the generated reflection free Jackson serializers
-   [#&#8203;44317](quarkusio/quarkus#44317) - Avoid duplicated field serialization in reflection free Jackson serializers
-   [#&#8203;44321](quarkusio/quarkus#44321) - Use Java 21 by default in the Deploying to Google Cloud guide
-   [#&#8203;44322](quarkusio/quarkus#44322) - Explain in MongoDB docs that records are supported
-   [#&#8203;44324](quarkusio/quarkus#44324) - Take config annotation when trying to match test resources

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever MR is behind base branch, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this MR and you won't be reminded about these updates again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box

---

This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4yNC4wIiwidXBkYXRlZEluVmVyIjoiMzQuMjQuMCJ9-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants