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

Fix openid.mod files for Jetty 12 #11907

Merged
merged 4 commits into from
Jun 18, 2024
Merged

Conversation

lachlan-roberts
Copy link
Contributor

  • we don't need ee8-10 variants of jetty-openid-baseloginservice.xml, jetty-openid.xml, or the ini template, just the core one is enough.
  • the ee10-openid.mod is pointless because it doesn't do anything other than depend on openid.mod so it has been removed.
  • ee8-openid.mod needed to specify its environment as ee8, was currently not working.

Signed-off-by: Lachlan Roberts <[email protected]>
Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

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

Please check ModulesTest.testOpenidModules(), which has not been updated and passes but it should not.

@lachlan-roberts
Copy link
Contributor Author

Please check ModulesTest.testOpenidModules(), which has not been updated and passes but it should not.

@sbordet it makes sense why this test is passing, it is using "ee8-openid", "ee9-openid", "openid", which all still exist (only ee10-openid was removed).

@sbordet
Copy link
Contributor

sbordet commented Jun 12, 2024

@lachlan-roberts the test is passing, but it's just spitting out errors, so it should not pass.
Please run it, and verify why it passes despite the errors.

Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

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

You have 2 options:

  1. Use the modules/openid/ directory properly.
  2. Don't copy the live jetty-home xml to the jetty-base xml.

@@ -12,7 +12,7 @@ lib/jetty-util-ajax-${jetty.version}.jar
lib/jetty-openid-${jetty.version}.jar

[files]
basehome:modules/openid/jetty-openid-baseloginservice.xml|etc/jetty-openid-baseloginservice.xml
basehome:etc/jetty-openid-baseloginservice.xml|etc/jetty-openid-baseloginservice.xml
Copy link
Contributor

Choose a reason for hiding this comment

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

ugh, don't copy a live XML from jetty-home to jetty-base.
The modules/openid/ directory exists for a reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it was useful, because if the xml was deleted from jetty-base/etc then it still works.
And the use of this XML is optional, but it would be annoying to have to manually copy it over when you need to use it.

Anyway I reverted this change so the file is back at modules/openid/jetty-openid-baseloginservice.xml.

@lachlan-roberts
Copy link
Contributor Author

@lachlan-roberts the test is passing, but it's just spitting out errors, so it should not pass.
Please run it, and verify why it passes despite the errors.

I checked the output of the test. The test is not setting up OpenID correctly, it just adding the module and verifying that it has the message Issuer was not configured in the logs, which comes from that big stacktrace.

We have tests like org.eclipse.jetty.ee10.tests.distribution.OpenIdTests and org.eclipse.jetty.ee9.tests.distribution.OpenIdTests which test OpenID with proper configuration and a webapp which uses it.

@lachlan-roberts lachlan-roberts merged commit 745cba4 into jetty-12.0.x Jun 18, 2024
10 checks passed
@lachlan-roberts lachlan-roberts deleted the jetty-12.0.x-openid branch June 18, 2024 05:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

4 participants