I now have the principal injection part of this working - thanks Romain for
your help and explanations. Progress is in my fork here:
https://github.com/jgallimore/tomee/tree/jwt-1.1 (changes here:
https://github.com/apache/tomee/compare/master...jgallimore:jwt-1.1?expand=1).
There are still a couple of TODOs to clean up, and 3 tests to get passing.
Any feedback is appreciated.
Jon
On Sat, Nov 3, 2018 at 9:10 AM Jonathan Gallimore <
jonathan.gallim...@gmail.com> wrote:
> Yep, got it. Thanks for the feedback - makes sense now.
>
> Cheers
>
> Jon
>
> On Fri, 2 Nov 2018, 16:46 Romain Manni-Bucau <rmannibu...@gmail.com wrote:
>
>> Answered hopefully "long enough" on dev@geronimo so will just do a short
>> one here and shout if not enough: ManagedSecurityService in cdi package of
>> openejb-core must make the getCurrentPrincipal contextual so hidden behind
>> a proxy. The proxied API must be Principal and JsonWebToken when available
>> (try { add if can load } catch { ignore } works as pattern). The proxy
>> instance can be created once for all app using the container loader or per
>> app using the app loader and avoiding to leak between apps since the API
>> can use different loaders.
>>
>> Le ven. 2 nov. 2018 14:44, Jonathan Gallimore <
>> jonathan.gallim...@gmail.com>
>> a écrit :
>>
>> > Thanks for the reply, but I am confused by your response. The PR I
>> > referenced adds a single test to the geronimo-jwt-auth project (
>> > https://github.com/apache/geronimo-jwt-auth/pull/3), based on
>> > org.eclipse.microprofile.jwt.tck.container.jaxrs.PrincipalInjectionTest
>> > from the TCK. It fails at present (hopefully we agree on that - my
>> results
>> > attached). The geronimo-jwt-auth project doesn't touch TomEE at all - it
>> > uses OWB/Meecrowave to run the MicroProfile JWT TCK. I have not modified
>> > the project config at all, so it is using the SecurityService code you
>> > previously posted. If this additional test were part of the MicroProfile
>> > JWT TCK (and I'm going to propose it), the Geronimo JWT Auth
>> implementation
>> > would *not* pass the TCK.
>> >
>> > I posted this here as I originally found the issue when continuing
>> > Roberto's efforts, but this has probably contributed to some confusion.
>> I
>> > would suggest we continue this over on the Geronimo and OWB lists to
>> avoid
>> > further confusion.
>> >
>> > Jon
>> >
>> > On Fri, Nov 2, 2018 at 12:46 PM Romain Manni-Bucau <
>> rmannibu...@gmail.com>
>> > wrote:
>> >
>> >> Hi
>> >>
>> >> Yes this is an owb misconfiguration/integration
>> >>
>> >> Geronimo is fine here so likely tomee owb spi to update as in geronimo
>> tck
>> >>
>> >> Le ven. 2 nov. 2018 10:42, Jonathan Gallimore <
>> >> jonathan.gallim...@gmail.com>
>> >> a écrit :
>> >>
>> >> > Thanks for the reply. I am still sure there is some sort of issue.
>> >> Putting
>> >> > TomEE to one side for the moment, I am able to reproduce this in the
>> >> > Geronimo JWT auth library as well. This PR includes a test to show
>> what
>> >> I
>> >> > mean: https://github.com/apache/geronimo-jwt-auth/pull/3.
>> >> >
>> >> > I can confirm that this change:
>> >> > https://github.com/apache/openwebbeans/pull/12 enables that new
>> test to
>> >> > pass.
>> >> >
>> >> > In short, if you @Inject JsonWebToken, or individual claims, or
>> >> > use @RolesAllowed, I think you're ok, but if you @Inject Principal,
>> you
>> >> > will most likely get the wrong principal because the instance is
>> cache
>> >> in a
>> >> > field in the org.apache.webbeans.portable.ProviderBasedProducer
>> class,
>> >> and
>> >> > that looks like a security issue.
>> >> >
>> >> > Jon
>> >> >
>> >> > On Tue, Oct 30, 2018 at 5:56 AM Romain Manni-Bucau <
>> >> rmannibu...@gmail.com>
>> >> > wrote:
>> >> >
>> >> > > Hi Jon,
>> >> > >
>> >> > > yes and no, idea is to be fast and for all producers it works
>> except
>> >> the
>> >> > > principal which is broken anyway in CDI 1.x so guess this was not
>> >> fixed
>> >> > >
>> >> > > in CDI 2 (tomee 8) we can impl it this way:
>> >> > >
>> >> > >
>> >> >
>> >>
>> https://github.com/apache/geronimo-jwt-auth/blob/master/src/test/java/org/apache/geronimo/microprofile/impl/jwtauth/tck/TckSecurityService.java
>> >> > >
>> >> > > Romain Manni-Bucau
>> >> > > @rmannibucau <https://twitter.com/rmannibucau> | Blog
>> >> > > <https://rmannibucau.metawerx.net/> | Old Blog
>> >> > > <http://rmannibucau.wordpress.com> | Github <
>> >> > > https://github.com/rmannibucau> |
>> >> > > LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
>> >> > > <
>> >> > >
>> >> >
>> >>
>> https://www.packtpub.com/application-development/java-ee-8-high-performance
>> >> > > >
>> >> > >
>> >> > >
>> >> > > Le mar. 30 oct. 2018 à 00:58, Jonathan Gallimore <
>> >> > > jonathan.gallim...@gmail.com> a écrit :
>> >> > >
>> >> > > > Here's a question, probably for Mark or Romain. If I turn the
>> proxy
>> >> > *off*
>> >> > > > in org.apache.webbeans.component.PrincipalBean, I'm finding that
>> I
>> >> get
>> >> > > the
>> >> > > > wrong principal injected sometimes. Specifically, I get the
>> >> whatever is
>> >> > > on
>> >> > > > the proxyInstance field here:
>> >> > > >
>> >> > > >
>> >> > >
>> >> >
>> >>
>> https://github.com/apache/openwebbeans/blob/trunk/webbeans-impl/src/main/java/org/apache/webbeans/portable/ProviderBasedProducer.java#L51
>> >> > > >
>> >> > > > Should this line (line 66)
>> >> > > >
>> >> > > >
>> >> > >
>> >> >
>> >>
>> https://github.com/apache/openwebbeans/blob/trunk/webbeans-impl/src/main/java/org/apache/webbeans/portable/ProviderBasedProducer.java#L66
>> >> > > > ,
>> >> > > > not simply be:
>> >> > > >
>> >> > > > return provider.get();
>> >> > > >
>> >> > > > as opposed to
>> >> > > >
>> >> > > > proxyInstance = provider.get(); ?
>> >> > > >
>> >> > > > That way, the proxyInstance field would never get set if proxy
>> mode
>> >> is
>> >> > > set
>> >> > > > to false. When proxy is true, this seems to work correctly
>> >> (although I
>> >> > > have
>> >> > > > other unrelated issues in TomEE).
>> >> > > >
>> >> > > > I can probably work around this some other way, but it seems to
>> me
>> >> like
>> >> > > > that behaviour isn't quite right.
>> >> > > >
>> >> > > > Trying to think of a way to test it - I can probably come up with
>> >> > > > something, but I'd appreciate some pointers. Happy to shift this
>> to
>> >> > > > openwebbeans-dev, and submit a PR. Replying here initially as I
>> ran
>> >> > into
>> >> > > > this while hacking on the JWT code.
>> >> > > >
>> >> > > > Jon
>> >> > > >
>> >> > > > On Wed, Oct 17, 2018 at 12:41 AM Roberto Cortez
>> >> > > > <radcor...@yahoo.com.invalid>
>> >> > > > wrote:
>> >> > > >
>> >> > > > > Please, go ahead. Let me know if need anything. Thanks!
>> >> > > > >
>> >> > > > > > On 16 Oct 2018, at 21:53, Jonathan Gallimore <
>> >> > > > > jonathan.gallim...@gmail.com> wrote:
>> >> > > > > >
>> >> > > > > > Any objection if I pick this up and have a go at the last
>> >> tests, or
>> >> > > is
>> >> > > > > > someone already working on this?
>> >> > > > > >
>> >> > > > > > On Thu, Sep 27, 2018 at 5:44 PM Romain Manni-Bucau <
>> >> > > > > rmannibu...@gmail.com>
>> >> > > > > > wrote:
>> >> > > > > >
>> >> > > > > >> Yep this feature. Then it must works since we support user
>> >> > principal
>> >> > > > if
>> >> > > > > the
>> >> > > > > >> jwt filter is corretly placed in the filter chain and we
>> must
>> >> > > inherit
>> >> > > > > from
>> >> > > > > >> the request principal.
>> >> > > > > >>
>> >> > > > > >> Le jeu. 27 sept. 2018 18:37, Roberto Cortez
>> >> > > > <radcor...@yahoo.com.invalid
>> >> > > > > >
>> >> > > > > >> a
>> >> > > > > >> écrit :
>> >> > > > > >>
>> >> > > > > >>> I guess you are referring to this, to remove the proxy?
>> >> > > > > >>>
>> >> > > > > >>>
>> >> > > > > >>
>> >> > > > >
>> >> > > >
>> >> > >
>> >> >
>> >>
>> https://github.com/apache/openwebbeans/commit/a21a949fb19247dcc39ee89292a1554b2cf1388e
>> >> > > > > >>> <
>> >> > > > > >>>
>> >> > > > > >>
>> >> > > > >
>> >> > > >
>> >> > >
>> >> >
>> >>
>> https://github.com/apache/openwebbeans/commit/a21a949fb19247dcc39ee89292a1554b2cf1388e
>> >> > > > > >>>>
>> >> > > > > >>>
>> >> > > > > >>> Yes, this one step.
>> >> > > > > >>>
>> >> > > > > >>> By default, we do inject the generic Principal of Tomcat.
>> We
>> >> > > probably
>> >> > > > > >> need
>> >> > > > > >>> to check first about the existence of a JWT Principal and
>> then
>> >> > > > fallback
>> >> > > > > >> to
>> >> > > > > >>> the Tomcat one. I think I know how to do it, I was just
>> >> trying to
>> >> > > > > broaden
>> >> > > > > >>> up the conversation about general integration with EE
>> >> security.
>> >> > > > > >>>
>> >> > > > > >>> Cheers,
>> >> > > > > >>> Roberto
>> >> > > > > >>>
>> >> > > > > >>>> On 26 Sep 2018, at 07:21, Romain Manni-Bucau <
>> >> > > rmannibu...@gmail.com
>> >> > > > >
>> >> > > > > >>> wrote:
>> >> > > > > >>>>
>> >> > > > > >>>> OWB enable to do it - we did it in geronimo impl to pass
>> tck
>> >> of
>> >> > > jwt
>> >> > > > > >> auth
>> >> > > > > >>>> spec.
>> >> > > > > >>>>
>> >> > > > > >>>> Le mer. 26 sept. 2018 03:28, Roberto Cortez
>> >> > > > > >> <radcor...@yahoo.com.invalid>
>> >> > > > > >>> a
>> >> > > > > >>>> écrit :
>> >> > > > > >>>>
>> >> > > > > >>>>> Hi,
>> >> > > > > >>>>>
>> >> > > > > >>>>> I’ve done some work to push our MP JWT implementation
>> from
>> >> 1.0
>> >> > to
>> >> > > > > 1.1.
>> >> > > > > >>>>>
>> >> > > > > >>>>> You can check it here:
>> >> > > > > >>>>> https://github.com/apache/tomee/pull/173 <
>> >> > > > > >>>>> https://github.com/apache/tomee/pull/173>
>> >> > > > > >>>>>
>> >> > > > > >>>>> There are still a couple of tests in the TCK that I have
>> to
>> >> fix
>> >> > > > and a
>> >> > > > > >>> few
>> >> > > > > >>>>> things that I would like to improve, but I think the
>> >> majority
>> >> > of
>> >> > > > the
>> >> > > > > >>> work
>> >> > > > > >>>>> is done.
>> >> > > > > >>>>>
>> >> > > > > >>>>> Some time ago, there was a discussion in the list about
>> how
>> >> to
>> >> > > > > >> integrate
>> >> > > > > >>>>> MP JWT with EE security:
>> >> > > > > >>>>>
>> >> > > > > >>>>>
>> >> > > > > >>>
>> >> > > > > >>
>> >> > > > >
>> >> > > >
>> >> > >
>> >> >
>> >>
>> http://tomee-openejb.979440.n4.nabble.com/Implementing-Microprofile-JWT-td4683212i40.html
>> >> > > > > >>>>> <
>> >> > > > > >>>>>
>> >> > > > > >>>
>> >> > > > > >>
>> >> > > > >
>> >> > > >
>> >> > >
>> >> >
>> >>
>> http://tomee-openejb.979440.n4.nabble.com/Implementing-Microprofile-JWT-td4683212i40.html
>> >> > > > > >>>>>>
>> >> > > > > >>>>>
>> >> > > > > >>>>> I believe we need to revisit that conversation and figure
>> >> out
>> >> > how
>> >> > > > to
>> >> > > > > >>> move
>> >> > > > > >>>>> forward.
>> >> > > > > >>>>>
>> >> > > > > >>>>> Right now for instance, we don’t support injecting a JWT
>> >> > > Principal
>> >> > > > > >> since
>> >> > > > > >>>>> it clashes with the predefined by CDI. Most likely, we
>> would
>> >> > need
>> >> > > > to
>> >> > > > > >>> plugin
>> >> > > > > >>>>> the JWT Principal lookup in TomcatSecurityService. I’m
>> not
>> >> sure
>> >> > > if
>> >> > > > we
>> >> > > > > >>> want
>> >> > > > > >>>>> to do it in that way, or if we want to think in something
>> >> else.
>> >> > > > > >>>>>
>> >> > > > > >>>>> Cheers,
>> >> > > > > >>>>> Roberto
>> >> > > > > >>>
>> >> > > > > >>>
>> >> > > > > >>
>> >> > > > >
>> >> > > > >
>> >> > > >
>> >> > >
>> >> >
>> >>
>> >
>>
>
