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

[WFCORE-6411] Make it possible to use JaasSecurityRealm via a custom-realm resource #5587

Merged
merged 1 commit into from
Feb 15, 2024

Conversation

Skyllarr
Copy link
Contributor

@Skyllarr Skyllarr commented Jul 17, 2023

@github-actions github-actions bot added the deps-ok Dependencies have been checked, and there are no significant changes label Jul 17, 2023
@Skyllarr Skyllarr force-pushed the WFCORE-6411 branch 2 times, most recently from 20e5f0e to 7ea6ab5 Compare July 25, 2023 12:40
@Skyllarr Skyllarr marked this pull request as ready for review July 25, 2023 13:19
@Skyllarr Skyllarr requested a review from fjuma July 26, 2023 14:46
File jarFile = new File(tmpDir.getRoot(), "testJaasCustomRealm.jar");
jar.as(ZipExporter.class).exportTo(jarFile, true);
CLIWrapper cli = new CLIWrapper(true);
cli.sendLine("module add --name=" + "jaasWrapperModule " + " --resources=" + TestSuiteEnvironment.getSystemProperty("jboss.dist", null) + "/modules/system/layers/base/org/wildfly/extension/elytron " + " --dependencies=org.wildfly.extension.elytron,org.wildfly.security.elytron");
Copy link
Contributor

Choose a reason for hiding this comment

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

"module add --name=" + "jaasWrapperModule " + " --resources="

-> "module add --name=jaasWrapperModule --resources=" ?

"/modules/system/layers/base/org/wildfly/extension/elytron " + " --dependencies=org.wildfly.extension.elytron,org.wildfly.security.elytron"

-> "/modules/system/layers/base/org/wildfly/extension/elytron --dependencies=org.wildfly.extension.elytron,org.wildfly.security.elytron" ?

jar.as(ZipExporter.class).exportTo(jarFile, true);
CLIWrapper cli = new CLIWrapper(true);
cli.sendLine("module add --name=" + "jaasWrapperModule " + " --resources=" + TestSuiteEnvironment.getSystemProperty("jboss.dist", null) + "/modules/system/layers/base/org/wildfly/extension/elytron " + " --dependencies=org.wildfly.extension.elytron,org.wildfly.security.elytron");
cli.sendLine("module add --name=" + "jaasLoginModule " + " --resources=" + jarFile.getAbsolutePath() + " --dependencies=org.wildfly.security.elytron");
Copy link
Contributor

Choose a reason for hiding this comment

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

"module add --name=" + "jaasLoginModule " + " --resources="

-> "module add --name=jaasLoginModule --resources=" ?

@AfterClass
public static void cleanUp() throws Exception {
CLIWrapper cli = new CLIWrapper(true);
cli.sendLine("module remove --name=" + "jaasWrapperModule");
Copy link
Contributor

Choose a reason for hiding this comment

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

"module remove --name=" + "jaasWrapperModule"

-> "module remove --name=jaasWrapperModule" ?

And the same for the next line.

@@ -0,0 +1,6 @@
Entry1 {
org.wildfly.security.auth.TestLoginModule required;
Copy link
Contributor

Choose a reason for hiding this comment

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

org.wildfly.security.auth

It seems the content of this config file is not used as the module is in a different package.

@yersan yersan added Feature This PR adds a new feature to WildFly missing-reqs This PR is missing external requirements before it can be merged labels Jul 28, 2023
Copy link
Collaborator

@yersan yersan left a comment

Choose a reason for hiding this comment

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

Just a minor note, I understand this PR will need to add the required transformers and bump the Elytron model.
And one suggestion, if it is in progress, you can use a draft to express the current PR status/intention, Jobs are also executed on Draft PRs.

@Skyllarr Skyllarr force-pushed the WFCORE-6411 branch 2 times, most recently from 8d70ddd to 6fac6ad Compare August 7, 2023 16:23
@Skyllarr
Copy link
Contributor Author

Skyllarr commented Aug 7, 2023

Just a minor note, I understand this PR will need to add the required transformers and bump the Elytron model. And one suggestion, if it is in progress, you can use a draft to express the current PR status/intention, Jobs are also executed on Draft PRs.

Hi @yersan , actually this PR does not introduce any new elements to the schema so it does not need the transformers or elytron bump model.

This feature is to allow users to use a code via a resource that is available in older versions. This is done so that older versions of the server can use this code when it is backported, without making changes to the model. It is not a draft anymore, though now I am having issues with the galleon and bootable-jar profiles that I need to figure out

@wildfly-ci

This comment was marked as outdated.

@wildfly-ci

This comment was marked as outdated.

@wildfly-ci

This comment was marked as outdated.

@wildfly-ci

This comment was marked as outdated.

@wildfly-ci

This comment was marked as outdated.

@wildfly-ci

This comment was marked as outdated.

@wildfly-ci

This comment was marked as outdated.

@wildfly-ci

This comment was marked as outdated.

@wildfly-ci

This comment was marked as outdated.

@Skyllarr Skyllarr force-pushed the WFCORE-6411 branch 2 times, most recently from 1b975a2 to 18e516f Compare August 8, 2023 10:50
@yersan
Copy link
Collaborator

yersan commented Feb 12, 2024

@Skyllarr

The module exclusion PR now fails in HostExcludesTestCase, probably for the same reason as wildfly/wildfly#17044 (comment) ?

Yes, we have already bumped the WildFly Core Kernel version but we have not released it so the WildFly 31 exclusion is not yet available on WildFly Full.

@Skyllarr
Copy link
Contributor Author

@yersan Understood. So then, since my WFLY PR with the module exclusion has to be merged before this one IIUC, and it needs WFCORE upgrade to pass the CI, then this PR can not make it into the wildfly-core release this week, right?

@Skyllarr
Copy link
Contributor Author

@yersan I have rebased this PR and created a combined WFLY PR here: wildfly/wildfly#17622 .

The combined PR also contains a commit for https://issues.redhat.com/browse/WFLY-19030 , I hope that is correct. Thanks again!

@wildfly-ci

This comment was marked as outdated.

@wildfly-ci

This comment was marked as outdated.

@wildfly-ci

This comment was marked as outdated.

@yersan
Copy link
Collaborator

yersan commented Feb 13, 2024

@Skyllarr this got a conflict, could you take a look and rebase it again? thanks

@Skyllarr
Copy link
Contributor Author

@yersan This is now rebased

@wildfly-ci

This comment was marked as outdated.

@wildfly-ci

This comment was marked as outdated.

@wildfly-ci

This comment was marked as outdated.

@Skyllarr
Copy link
Contributor Author

@yersan

https://ci.wildfly.org/buildConfiguration/WF_WildFlyCoreIntegrationExperiments/417090 (Linux integration) https://ci.wildfly.org/buildConfiguration/WF_WildFlyCoreIntegrationExperiments/417091 (Microprofile) https://ci.wildfly.org/buildConfiguration/WF_WildFlyCoreIntegrationExperiments/417092 (Bootable JAR) https://ci.wildfly.org/buildConfiguration/WF_WildFlyCoreIntegrationExperiments/417120 (Galleon) https://ci.wildfly.org/buildConfiguration/WF_WildFlyCoreIntegrationExperiments/417093 (Galleon) https://ci.wildfly.org/buildConfiguration/WF_WildFlyCoreIntegrationExperiments/417094 (WildFly Preview JDK 17) https://ci.wildfly.org/buildConfiguration/WF_WildFlyCoreIntegrationExperimentsWindows/417096 (Windows)

After rebasing, the Galleon integration job has failed twice. It needs some investigation.

@yersan Looking... The error seems to be: java.lang.AssertionError: Some expected to be unused modules have been provisioned in test-all-layers: javax.xml.stream.api,org.apache.qpid.proton,org.wildfly.extension.elytron.jaas-realm,sun.jdk,sun.scripting which is weird because I put it in the NOT_REFERENCED and not NOT_USED . And I did not touch the org.apache.qpid.proton or sun.jdk,sun.scripting ..

@Skyllarr
Copy link
Contributor Author

Hm there was a change in wildfly repo also: https://github.com/wildfly/wildfly/pull/17618/files where the sun.scripting and org.apache.qpid.proton are moved

@yersan
Copy link
Collaborator

yersan commented Feb 14, 2024

@Skyllarr for those two, we could have your wildfly PR rebased on top of main. I don't have any clue about the other two yet.

@Skyllarr
Copy link
Contributor Author

Skyllarr commented Feb 14, 2024

@Skyllarr for those two, we could have your wildfly PR rebased on top of main. I don't have any clue about the other two yet.

@yersan I've rebased the WFLY PR, there were no conflicts but maybe only that rebase was missing not sure

@Skyllarr
Copy link
Contributor Author

I can reproduce the failure in wildfly repo on my branch even after rebasing, so looking

@Skyllarr
Copy link
Contributor Author

@yersan I've rebased and updated this branc hand the wildfly branch wildfly/wildfly#17622 . The LayersTestCase in wildfly-core and wildfly are now passing locally for me with mvn clean install -Dts.layers -Dtest=LayersTestCase.

@yersan yersan removed the missing-reqs This PR is missing external requirements before it can be merged label Feb 14, 2024
<dependencies>
<module name="java.logging"/>
<module name="java.xml"/>
<module name="javax.xml.stream.api"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

The javax.xml.stream.api is deprecated and should not be used here. According to the note on this module:

<!--
This module is deprecated and any use of it should be replaced
with java.xml JPMS module provided by Java SE.
-->

it should be replaced by java.xml, which is already in the org.wildfly.extension.elytron.jaas-realm dependency list.

Copy link
Contributor

Choose a reason for hiding this comment

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

@yersan Since @Skyllarr is now on PTO, I've submitted a new PR that contains these changes and removes the javax.xml.stream.api dependency from org.wildfly.extension.elytron.jaas-realm:

#5861

@@ -31,6 +31,7 @@ public class LayersTestCase {
// This is the expected set of un-referenced modules found when scanning
// the test-standalone-reference configuration.
private static final String[] NOT_REFERENCED = {
"javax.xml.stream.api",
Copy link
Collaborator

Choose a reason for hiding this comment

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

The usage of this module is unexpected. This change can be removed by removing the javax.xml.stream.api dependency from org.wildfly.extension.elytron.jaas-realm

Copy link
Contributor

Choose a reason for hiding this comment

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

Removed in #5861

@wildfly-ci
Copy link

Core -> WildFly Preview Integration Build 13302 outcome was UNKNOWN using a merge of bb0806b
Summary: Canceled (Tests passed: 2400, ignored: 45; exit code 143 (Step: Build & test full (Maven)) (new)) Build time: 01:06:52

@wildfly-ci
Copy link

Core -> Full Integration Build 13479 outcome was UNKNOWN using a merge of bb0806b
Summary: Canceled (Tests passed: 4525, ignored: 65; exit code 143 (Step: Build & test full (Maven)) (new)) Build time: 01:10:51

@wildfly-ci
Copy link

Core -> Full Integration Build 13239 outcome was UNKNOWN using a merge of bb0806b
Summary: Canceled (Tests passed: 1615, ignored: 28; exit code 143 (Step: Build & test full (Maven)) (new)) Build time: 01:07:40

@yersan yersan merged commit bb0806b into wildfly:main Feb 15, 2024
5 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deps-ok Dependencies have been checked, and there are no significant changes Feature This PR adds a new feature to WildFly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants