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 jaas demos #8689

Merged
merged 3 commits into from
Oct 11, 2022
Merged

Fix jaas demos #8689

merged 3 commits into from
Oct 11, 2022

Conversation

janbartel
Copy link
Contributor

There can only be a single jaas configuration file, so changed the demos to that instead of multiple ones (which were being ignored anyway).

@janbartel janbartel self-assigned this Oct 6, 2022
@@ -20,7 +20,6 @@ ext

[files]
basehome:modules/demo.d/ee10-demo-jaas.xml|webapps/ee10-demo-jaas.xml
basehome:modules/demo.d/ee10-demo-login.conf|etc/ee10-demo-login.conf
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this?

All the config files necessary were in the demo.d directory and copied to relevant places.
Why is now ee10-demo-login.conf an exception?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, the ee10-demo-login-conf file still references module xyz but should be ee10-xyz.

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 I made the problem clean in the opening comment of the PR? The problem is that you cannot have more than one jaas config file, so all of the eeX-demo-login.conf files were being ignored. All of the jaas configuration must go into a single file.

@@ -26,14 +26,14 @@
<!-- This is for convenience so that the src/etc/login.conf file can stay unmodified when copied to $jetty.home/etc directory -->
<jetty.base>${basedir}/src/main/config/modules/demo.d</jetty.base>
<!-- Mandatory. This system property tells JAAS where to find the login module configuration file -->
<java.security.auth.login.config>${basedir}/src/main/config/modules/demo.d/ee8-demo-login.conf</java.security.auth.login.config>
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand ee8 demos are derived from ee9 demos with some Maven magic.
As such we should just change the ee9 files, not the ee8 ones?

Copy link
Member

Choose a reason for hiding this comment

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

demos cannot be derived from ee9 demos.
they are including some very differents jars etc...
so I couldn;t do it for demos.

core-deploy

[files]
basehome:modules/demo.d/demo-login.conf|etc/demo-login.conf
Copy link
Contributor

Choose a reason for hiding this comment

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

This line was removed from the eeX versions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly. As there can only be a single config file.

org.eclipse.jetty.ee8.jaas.spi.PropertyFileLoginModule required
debug="true"
file="${jetty.base}/etc/ee8-demo-login.properties";
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is a core demo having eeX configuration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you look at jetty-home/src/main/resources/modules/demo.d/ you will see lots of other eeX related config files.

@@ -20,7 +20,6 @@ ext

[files]
basehome:modules/demo.d/ee10-demo-jaas.xml|webapps/ee10-demo-jaas.xml
basehome:modules/demo.d/ee10-demo-login.conf|etc/ee10-demo-login.conf
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, the ee10-demo-login-conf file still references module xyz but should be ee10-xyz.

@sbordet
Copy link
Contributor

sbordet commented Oct 6, 2022

@janbartel I'm not sure I follow these changes.

Build fails due to circular dependency.

If I want to just deploy the ee10 jaas demo, does it work?

And why the core demo has a configuration referring to eeX?

Are there distribution tests for these demos?

@janbartel
Copy link
Contributor Author

janbartel commented Oct 6, 2022

@sbordet as I've said, the problem I'm trying to fix is that you cannot have multiple jaas config files. Thus you cannot have one for ee8, one for ee9 and one for ee10. All config must be in a single file.

As for deploying a single webapp, yes I need to do a bit more work on that.

The circular dependency is this:

Caused by: org.codehaus.plexus.util.dag.CycleDetectedException: Edge between 'Vertex{label='org.eclipse.jetty:jetty-home:12.0.0-SNAPSHOT'}' and 'Vertex{label='org.eclipse.jetty.ee10:jetty-ee10-home:12.0.0-SNAPSHOT'}' introduces to cycle in the graph org.eclipse.jetty.ee10:jetty-ee10-home:12.0.0-SNAPSHOT --> org.eclipse.jetty.ee10.demos:jetty-ee10-demo-jaas-webapp:12.0.0-SNAPSHOT --> org.eclipse.jetty.ee10:jetty-ee10-maven-plugin:12.0.0-SNAPSHOT --> org.eclipse.jetty:jetty-home:12.0.0-SNAPSHOT --> org.eclipse.jetty.ee10:jetty-ee10-home:12.0.0-SNAPSHOT

Having fixed the coordinates of the jetty-maven-plugin in the jetty-ee10-demo-jaas-webapp project, there is now a circular reference around jetty-home and the jetty-maven-plugin via the webapp. We seem to have lost the ability to be able to use the plugin for our webapp projects?!

@olamy
Copy link
Member

olamy commented Oct 6, 2022

No idea why there is now a circular dependency having just changed some .mod files, will look at that.

you changed a pom as well ;) and that's probably the reason.
probably something such:
maven-plugin need jetty-home, jetty-home need jaas-demo but jaas-demo needs maven-plugin

@sbordet
Copy link
Contributor

sbordet commented Oct 6, 2022

@janbartel I still don't understand.
If I only deploy the ee10 jaas demo, there will be a single file ee10-demo-login.conf, no?
That "there must be only one jaas config file" is already true if I only deploy one eeX jaas demo, whatever it is.

Are you trying to make the ee-10-demo module depend on the equivalent core module just in case someone deploys multiple eeX jaas demos?

@janbartel
Copy link
Contributor Author

@olamy but jetty-maven-plugin doesn't need jetty-home. Only the integration tests for the jetty-maven-plugin need jetty-home. If they were moved out of jetty-maven-plugin then there wouldn't be a problem.

@janbartel
Copy link
Contributor Author

@sbordet you need to be able to deploy demos and have it work. Currently if you do that, then all of the jaas demos are broken. As you point out, you also need to be able to deploy just a single eeX-demo as well.

@olamy
Copy link
Member

olamy commented Oct 7, 2022

@olamy but jetty-maven-plugin doesn't need jetty-home. Only the integration tests for the jetty-maven-plugin need jetty-home. If they were moved out of jetty-maven-plugin then there wouldn't be a problem.

which is real pain when developing/debugging it... :( as you have to build multiple modules first
I'm not sure why adding jetty-eeX-maven-plugin to jaas-demo.
As long as you have distribution tests covering jaas-eeX-demo it's fine

@janbartel
Copy link
Contributor Author

@olamy but jetty-maven-plugin doesn't need jetty-home. Only the integration tests for the jetty-maven-plugin need jetty-home. If they were moved out of jetty-maven-plugin then there wouldn't be a problem.

which is real pain when developing/debugging it... :( as you have to build multiple modules first I'm not sure why adding jetty-eeX-maven-plugin to jaas-demo. As long as you have distribution tests covering jaas-eeX-demo it's fine

@olamy I'm not adding the maven plugin to the demo webapps, it was always there ;) and it used to work. I think it's a reasonable expectation that you could run our demo webapps just like you could run any other webapp with jetty.

@janbartel janbartel requested review from sbordet and olamy October 10, 2022 01:14
@olamy
Copy link
Member

olamy commented Oct 10, 2022

@olamy but jetty-maven-plugin doesn't need jetty-home. Only the integration tests for the jetty-maven-plugin need jetty-home. If they were moved out of jetty-maven-plugin then there wouldn't be a problem.

which is real pain when developing/debugging it... :( as you have to build multiple modules first I'm not sure why adding jetty-eeX-maven-plugin to jaas-demo. As long as you have distribution tests covering jaas-eeX-demo it's fine

@olamy I'm not adding the maven plugin to the demo webapps, it was always there ;) and it used to work. I think it's a reasonable expectation that you could run our demo webapps just like you could run any other webapp with jetty.

oh right. correct that;s weird that should fail as well in jetty-10.0.x branch.
We have been lucky :P.Sounds like a maven bug as it's similar as jetty-10.0.x branch except here there is one more module in the stack (e.g jetty-ee10-home)

@janbartel janbartel merged commit 756cf21 into jetty-12.0.x Oct 11, 2022
@janbartel janbartel deleted the jetty-12.0.x-fix-jaas-demos branch October 11, 2022 22:33
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.

4 participants