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

Drop @LoginConfig #182

Open
rmannibucau opened this issue Apr 13, 2020 · 13 comments
Open

Drop @LoginConfig #182

rmannibucau opened this issue Apr 13, 2020 · 13 comments
Milestone

Comments

@rmannibucau
Copy link

Follow up issue of #70 (hoping the scope of this one is clear and narrowed :))

This annotation has a lot of pitfalls:

  1. it is not defined in a way which enables it to be portable,
  2. it requires to hardcode the security config in the application (same as @*Definition are never used anywhere else than in demo in JakartaEE land, we should learn from this error),
  3. it is not needed to make it work (only case it can be relevant is for a container providing all spec, even the ones the user don't need/want - container philosophy - and in this case the vendor can trivially provide a way to disable it if needed so the toggle is not in jwt-auth spec level).

So overall, there is not a single case which justify this annotation and it does not bring any configuration the implementation would require so there is no blocker to drop it (note that dropping an annotation keeps the application functional without recompilation in terms of API).

Side note: adding a mp-config key to enable/disable this spec can still be done as a consequence of the removal of this annotation IMHO.

@sberyozkin
Copy link
Contributor

@rmannibucau Sure, this is more concrete, no doubt about it :-)

@sberyozkin sberyozkin added this to the JWT-2.0 milestone Apr 14, 2020
@sberyozkin
Copy link
Contributor

sberyozkin commented Apr 14, 2020

Hey @rmannibucau I've added a 2.0 milestone, though I'll resist as much as possible :-) because we have the users (and likely not only us) using it, unfortunately with more than MP-JWT, so there has to be a strong reason to make them update their applications. Few quick questions:

it is not defined in a way which enables it to be portable,

What is not portable about sticking it to the JAX-RS Application ?

it requires to hardcode the security config in the application

Do you mean its optional realm property ?

it is not needed to make it work

MP-JWT ? I certainly know one project where authMethod=MP-JWT is checked.

I wonder if a starting point can be to drop the references to all but MP-JWT method, and make MP-JWT a default value. IMHO it can be more realistic than just dropping LoginConfig

Cheers

@rmannibucau
Copy link
Author

there has to be a strong reason to make them update their applications

It is an annotation so if not in the classpath it is ignored, no update to do :).

What is not portable about sticking it to the JAX-RS Application ?

It is not defined yet ;). It also has the issue to conflict with other EE specs (servlet and security out of my mind) which have their own security mecanism/annotations. Who wins? Who wraps who?

Do you mean its optional realm property ?

No, the jwt-auth activation itself. With all the proxies we have these days in k8s and friends, it is likely we'll have deployment with JWT Auth and deployments without when going accross cloud providers, using a mp-config toggle is a neat solution to be able to deactivate that feature and can completly replace the annotation.

MP-JWT ? I certainly know one project where authMethod=MP-JWT is checked.

I guess it is the case of a project going beyond the spec (keycloak usage?) and this would be solved the same way with the mp-config toggle for this app (+ redhat ecosystem using jandex does not need the annotation to exist to see it ;)).

@sberyozkin
Copy link
Contributor

Hi Romain,

It is an annotation so if not in the classpath it is ignored, no update to do :).

But the legacy project is relying on a user application telling it via LoginConfig what it wants

What is not portable about sticking it to the JAX-RS Application ? It is not defined yet ;)

Then its docs can be updated

using a mp-config toggle is a neat solution to be able to deactivate that feature and can completly replace the annotation.

It can - but we are talking about the new projects here.

guess it is the case of a project going beyond the spec (keycloak usage?)

No, it is not related to KC.

Some immediate clarity would definitely helped though. We can say:

  • the use of LoginConfig is optional - the implementations which depend on it may check JAX-RS Application
  • remove the list of non MP JWT methods from the docs and make MP-JWT a default value

making optional should really work for all

@rmannibucau
Copy link
Author

@sberyozkin before saying I agree or not, can you clarify you propose to make this annotation support optional as "implementation can or not support it" (I'm ok with it) or something else?

@sberyozkin
Copy link
Contributor

@rmannibucau Yes, optional would imply it, but I'm not 100% to be honest right now.

@rmannibucau
Copy link
Author

@sberyozkin nothing urges but think we should try to relax that soon, if bothersome (= we get negative feedback of making it optional) we could rethink about an useful and not conflicting configuration (can be as simple as enabling to handle a JAXRS interceptors set programmatically in an user feature to enable/disable that, it could even enable to make it dynamic in apps compared to today ;)).

@sberyozkin
Copy link
Contributor

@rmannibucau Are you OK with renaming this issue to Make @LoginConfig optional ? Once we fix this one we can open Deprecate LoginConfig if needed, if having optional will not be enough.

@rmannibucau
Copy link
Author

@sberyozkin works for me (maybe with another wording since "make @loginConfig optional" sounds like it can be there or not whereas we want to emphasis it can be supported or not).

@sberyozkin
Copy link
Contributor

sberyozkin commented Apr 17, 2020

@rmannibucau Making it optional is the very first step - once it is optional it effectively means no one has to support it but only those containers which prefer it will do it.

By the way, the package.info has:

LoginConfig: an annotation that is used by a JAX-RS Application to define the authentication method and associated security realm

So there is a reference to Application. So not sure how to untangle it, because as soon as we make it optional, some users will start saying, wait, we already use it, as well some implementers may assert the same.

But in any case, I'm fine with updating its docs as the 1step as suggested earlier (drop the ref to all but MP JWT methods)

@rmannibucau
Copy link
Author

Yep works for me.

Side note: resources are used by jaxrs apps but not always referenced so javadoc does not remove all the ambiguity and clearly encourages the one with EE ecosystem so good to slowly move to dropping it from default required api.

@sberyozkin
Copy link
Contributor

CC @teddyjtorres.

@dentonmwood
Copy link

dentonmwood commented Mar 5, 2021

Was there a decision reached on this? We're adding MP-JWT for the first time to an app running on WildFly, and I need to know if we should continue to use web.xml or use @LoginConfig and dump our web.xml file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants