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

TOMEE-4351 - Jakarta Security 3.0 - OpenIdAuthenticationMechanismDefinition implementation #1178

Merged
merged 60 commits into from
Jun 17, 2024

Conversation

jungm
Copy link
Member

@jungm jungm commented May 31, 2024

Opening as a draft PR for now as it's still WIP

Doesn't work/WIP:

  • Token expiration checks (@AutoApplySession is used right now on the auth mechanism, I've mainly done this for testing purposes and will likely drop it)
  • redirectToOriginalResource=true, isn't implemented yet
  • Large parts of the code aren't covered by unit/integration tests

What does work:

  • Authentication against Keycloak (Both in a real app and in examples/openid-security tests) and https://github.com/mitreid-connect/OpenID-Connect-Java-Spring-Server (used in security TCK)
  • Passes TCK app-openid and app-openid2, app-openid3 tests redirectToOriginalResource which is not implemented yet:
    • [INFO] Reactor Summary for Jakarta Security TCK - main 3.0.1:
      [INFO] 
      [INFO] Jakarta Security TCK - main ........................ SUCCESS [  2.672 s]
      [INFO] common ............................................. SUCCESS [  0.699 s]
      [INFO] app-openid ......................................... SUCCESS [  6.441 s]
      [INFO] app-openid2 ........................................ SUCCESS [ 16.031 s]
      [INFO] app-openid3 ........................................ FAILURE [  6.057 s]
      [INFO] ------------------------------------------------------------------------
      [INFO] BUILD FAILURE
      [INFO] ------------------------------------------------------------------------
      

I've been working on this on and off for a couple of weeks now, with this PR getting quite big by now. So I'd highly appreciate if someone can take a look and give some feedback 🙂

@jungm
Copy link
Member Author

jungm commented Jun 2, 2024

redirectToOriginalResource=true works now and all openid modules of the TCK are passing:

[INFO] Reactor Summary for Jakarta Security TCK - main 3.0.1:
[INFO] 
[INFO] Jakarta Security TCK - main ........................ SUCCESS [  2.725 s]
[INFO] common ............................................. SUCCESS [  0.748 s]
[INFO] app-openid ......................................... SUCCESS [  6.556 s]
[INFO] app-openid2 ........................................ SUCCESS [ 15.742 s]
[INFO] app-openid3 ........................................ SUCCESS [ 14.100 s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  39.986 s
[INFO] Finished at: 2024-06-02T17:24:15+02:00
[INFO] ------------------------------------------------------------------------

@jungm
Copy link
Member Author

jungm commented Jun 4, 2024

Thanks @rzo1 for triggering a full build: https://ci-builds.apache.org/job/Tomee/view/tomee-10.x/job/pull-request-manual/106/

All tests are passing 🙂

@tandraschko
Copy link
Member

looks really good from a short review and tests are running; +1

great work!

@rzo1 rzo1 changed the title Jakarta Security 3.0 - OpenIdAuthenticationMechanismDefinition implementation TOMEE-4351 - Jakarta Security 3.0 - OpenIdAuthenticationMechanismDefinition implementation Jun 6, 2024
Copy link
Contributor

@rzo1 rzo1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good from a short review.

examples/security-openid/pom.xml Outdated Show resolved Hide resolved
examples/security-openid/pom.xml Show resolved Hide resolved
tomee/tomee-security/pom.xml Outdated Show resolved Hide resolved
@jungm
Copy link
Member Author

jungm commented Jun 12, 2024

what really needs a closer look and maybe needs to be discussed:

  • JWT Validation is using jose.4.j, this introduces a new dependency in all tomee flavours (wasn't in webprofile before). Maybe it needs to be added in some notice file?
  • Spec mentions a special variable that can be used in the annotation: ${baseURL}, I implemented this with producing an @nAmed String
  • I built a basic delegate in OpenIdAuthenticationMechanismDefinitionDelegate that automatically resolves the configuration from the openid provider
  • SavedRequest (originally from @logintocontinue) has been rewritten so I can serialize it for use in cookies
  • Spec is ambiguous on how to handle subjectTypeSupported, idTokenSigningAlgorithmsSupported and responseTypeSupported (See CompositeOpenIdProviderMetadata). A user can override these, but it's not obvious if that has been done or not. I handled these the same way soteria does, but it's probably worth a spec issue in the future?
  • Requests to openid provider are done using JAX-RS Client, maybe we want to use something else in TomEE? Really the only reason I chose this was because it's convenient

(See https://lists.apache.org/thread/sghf41f1z75gpnhpf236o1lrj1sl4vr8 for whole thread on mailing list)

@jungm jungm marked this pull request as ready for review June 12, 2024 10:56
@rzo1 rzo1 merged commit 8f0df13 into apache:main Jun 17, 2024
1 check passed
@rzo1
Copy link
Contributor

rzo1 commented Jun 17, 2024

Thanks @jungm

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

Successfully merging this pull request may close these issues.

3 participants