Consider making some security annotations regular CDI interceptor bindings

[mkouba]

Currently, annotations like @Authenticated, @PermissionsAllowed and @RunAsUser are not CDI interceptor bindings.

While it allows for more flexible implementation (and implementation-agnostic javadoc), the implementation is usually CDI-based and we have to do weird things to make it work (annotation transformations, InterceptorBindingRegistrar, etc.).

It could be reasonable and practical to make these annotations regular interceptor bindings and possibly explain in the javadoc that they can be used even outside CDI?

[michalvavrik]

Just so that we don't forget about it, if the annotation @RunAsUser becomes the interceptor binding, we need to allow use it on classes, because our RunAsUserInterceptor is a class interceptor. @mkouba had a good suggestion that users can do:

@RunAsUser(user = "foo")
class Foo {

   // --> run as foo
   @Scheduled(every = "1s")
   void everySecond() {
     // ..do something
   }

   // --> run as foo
   @Scheduled(every = "10s")
   void everyTenSecond() {
     // ..do something
   }
}

so we little changes in validation and testing, we can do that safely.

[michalvavrik]

\cc @sberyozkin

[sberyozkin]

What would happen with

@RunAsUser(user = "foo")
class Foo {
   @Authenticated
   void doSomething() {
     // ..do something
   }
}

?

[mkouba]

What would happen with

@RunAsUser(user = "foo")
class Foo {
   @Authenticated
   void doSomething() {
     // ..do something
   }
}

?

It depends on interceptor priority. Hm, currently both the interceptors have the priority of value io.quarkus.security.spi.runtime.SecurityHandlerConstants.SECURITY_INTERCEPTOR_PRIORITY so the behavior would be random. But we could easily change the priorities...

[michalvavrik]

I am aware of the priority collision. I didn't think it made sense to do:

@Scheduled
@Authenticated
@RunAsUser(user = "foo")
void doSomething() { ...}

hence I didn't introduce a new constant.

[michalvavrik]

What would happen with

I think the answer it - validation should fail the build. There is no @Scheduled.

[sberyozkin]

I have to admit I'm not exactly sure of the possible side-effects, several projects have only this set of interfaces brought in via this project's dependency, so that users, if they need to, can add for example quarkus-oidc

[manovotn]

Just as a side note, I have to say I find it funny how we've come a full circle here from justifying why it cannot be this way to why it can - #82 (comment) :-)

That said, I am still for making it an interceptor binding if there isn't a strong reason against it. It makes for cleaner code and users can clearly see the annotation is linked to CDI in a certain way 👍

[sberyozkin]

I don't know why Stuart decided to have this API created as a separate project, but it is here now, and despite the complexities, it works well in Quarkus.

I'm not sure bringing in CDI into the pure API project is critical

if there isn't a strong reason against it.

The problem is, how can we know ?

[sberyozkin]

It makes for cleaner code

Sure, I can understand why it can be easier for Quarkus

and users can clearly see the annotation is linked to CDI in a certain way

IMHO they won't care how it is implemented, they just want @Authenticated and friends to work as required

[michalvavrik]

I don't want to make judgement on the whole proposal, because I am not sure it is one and the same thing, but I agree with this statement.

IMHO they won't care how it is implemented, they just want @Authenticated and friends to work as required

I would expect that most users don't care and that we shouldn't give any guarantees on implementation details (how we perform these checks). For now, we have only one, but very significant exception to the CDI interceptor binding, which are endpoints that most often does not rely on the CDI interceptors. There is not many situations where it makes sense to not use CDI interceptors.

[sberyozkin]

Sure, I'm really the last person who should be talking about pros and cons of implementing them as CDI inteceptors, I'm simply concerned, possibly unnecessarily, that having such association being made right here can have impact that we do not envisage right now.

[mkouba]

IMHO they won't care how it is implemented, they just want @Authenticated and friends to work as required

If user don't care how it's implemented then I would not expect them to mind the presence of interceptor binding annotaion either ;-).

I'm simply concerned, possibly unnecessarily, that having such association being made right here can have impact that we do not envisage right now.

We might add a note to the javadoc to clarify that there are no implementation guarantees.

That said, I would not mind if you close this issue as not planned. My original intent was to simplify the implementation...

[manovotn]

That's right.
I am not pushing it either; we can just leave it here for now or even close the issue.

You guys are the security experts and if you want to keep it as is, that is a fair call :⁠-⁠)