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

[WFLY-11787] Test checks if the identity sets by HttpServletRequest.login() is propagated into the secured EJB. #12131

Closed
wants to merge 1 commit into from

Conversation

dcihak
Copy link
Contributor

@dcihak dcihak commented Mar 1, 2019

Upstream JIRA issue: https://issues.jboss.org/browse/WFLY-11787

Test configures Elytron to use Identity Propagation.
Test deploys application with the secured servlet and checks if the identity sets by HttpServletRequest.login() is propagated into the secured EJB.

@dcihak
Copy link
Contributor Author

dcihak commented Mar 4, 2019

Retest this please

@dcihak
Copy link
Contributor Author

dcihak commented Mar 7, 2019

@spyrkob Could you please review this PR?

@dcihak
Copy link
Contributor Author

dcihak commented Mar 12, 2019

Can we merge this?

@bstansberry
Copy link
Contributor

@darranl or @fjuma Please review.

@spyrkob
Copy link
Contributor

spyrkob commented Mar 14, 2019

I'm not sure, but I think the HelloBean will be called locally rather than remotely in the test, so the fix won't actually get tested.

@fjuma
Copy link
Contributor

fjuma commented Mar 20, 2019

@dcihak Were you able to verify that the test fails before the identity propagation changes (i.e., before Elytron 1.8.0.Final) and passes after?

servletElytronDomainSetup.setup(managementClient, containerId);

// /subsystem=elytron/sasl-authentication-factory=auth-test:add(sasl-server-factory=configured,security-domain=auth-test,mechanism-configurations=[{mechanism-name=BASIC}])
ModelNode addSaslAuthentication = createOpNode("subsystem=elytron/sasl-authentication-factory=" + getSecurityDomainName(), ADD);
Copy link
Contributor

Choose a reason for hiding this comment

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

Really I think it would be nice to get all of these operations into org.wildfly.test.security.common.AbstractElytronSetupTask to create ConfigurableElement instances so the same resources can be created for further test cases.

That is probably a task outside this PR as it looks like we also need to update AbstractElytronSetupTask to also extend SnapshotRestoreSetupTask or at least make use of that in the tests so the setup tasks can avoid deleting resources.

@darranl
Copy link
Contributor

darranl commented Apr 10, 2019

@dcihak Did you have a chance to look into the question from @fjuma - some of these recent bug reports were quite difficult to reproduce in the first place so it would be good to confirm if the test case really does test the underlying issue that was fixed.

@wildfly-ci wildfly-ci added the deps-ok Dependencies have been checked, and there are no significant changes label May 3, 2019
Copy link
Contributor

@darranl darranl left a comment

Choose a reason for hiding this comment

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

Also a couple of missing copyright headers but the previous comments still remain, there are some questions re if this really tests the scenario that was fixed.

@@ -0,0 +1,6 @@
package org.jboss.as.test.integration.web.security.identity.propagation.deployment;
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing copyright header

@@ -0,0 +1,17 @@
package org.jboss.as.test.integration.web.security.identity.propagation.deployment;
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing copyright header

@darranl
Copy link
Contributor

darranl commented Dec 19, 2019

I am going to close this one as it has had no updates since the original questions were asked, please feel free to either re-open or send in a new PR with the details requested.

@darranl darranl closed this Dec 19, 2019
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
Projects
None yet
6 participants