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.

Sign up for GitHub

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

Jump to bottom

[WFLY-9300] Add test for ejb security propagation using PLAIN #10472

Closed
wants to merge 1 commit into from
Closed

[WFLY-9300] Add test for ejb security propagation using PLAIN #10472

wants to merge 1 commit into from

Conversation

bmaxwell
Copy link
Contributor

@bmaxwell bmaxwell commented Sep 5, 2017

@bstansberry bstansberry added the core-upgrade-needed PR requires a wildfly-core change to be merged and integrated first label Sep 5, 2017
rsvoboda
rsvoboda suggested changes Sep 6, 2017
View reviewed changes
Copy link
Contributor

@rsvoboda rsvoboda left a comment

Choose a reason for hiding this comment

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

Please change log.info to log.debug, @ctomc doesn't like too much output on CI servers ;)

Looking at test scenarios it feels to me a bit optimistic, just one user & role for web / ejb. Historically there were issues (I think login modules or krb stuff) when more users with different roles hit the test deployment.
I would prefer more defensive tests with 2+ users with different roles or multiple roles per user for both ejb and web way to make sure all the things are properly propagated.

@bmaxwell
Copy link
Contributor Author

bmaxwell commented Sep 6, 2017

I think I have all of the log.infos out. There are actually 2 users with different roles. ejbUser has ejbRole, webUser has ejb and web role. I started some more changes to validate the roles further.

@bstansberry bstansberry removed the core-upgrade-needed PR requires a wildfly-core change to be merged and integrated first label Sep 9, 2017
@bstansberry
Copy link
Contributor

retest this please

@rsvoboda
Copy link
Contributor

@bmaxwell I meant 2 web users + 2 ejb users to be sure first web/ejb authenticated user is not sharing permissions with second web/ejb user. In any case thanks for increasing the coverage!

@bstansberry bstansberry added the security Particular care related to security aspects of the PR is appropriate. label Dec 6, 2017
@kabir
Copy link
Contributor

kabir commented Feb 9, 2018

@bmaxwell This needs a rebase (I've not reviewed beyond that)

@kabir
Copy link
Contributor

kabir commented Aug 2, 2018

@bmaxwell Closing due to inactivity on fixing this

@kabir kabir closed this Aug 2, 2018
@rpelisse
Copy link
Contributor

@kabir I've rebased on behalf of @bmaxwell who appears to have not has the time to do it. I've setup a new PR that thus superseeds this PR11527 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rsvoboda rsvoboda rsvoboda requested changes

Assignees
No one assigned
Labels
security Particular care related to security aspects of the PR is appropriate.
Projects
None yet
Milestone
No milestone
5 participants
@bmaxwell @bstansberry @rsvoboda @kabir @rpelisse