-
Notifications
You must be signed in to change notification settings - Fork 282
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
[ELY-2612] removed duplicates in BasicScramSelfTest #2002
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution @sugan0tech, I've added some feedback.
tests/base/src/test/java/org/wildfly/security/sasl/scram/BasicScramSelfTest.java
Outdated
Show resolved
Hide resolved
tests/base/src/test/java/org/wildfly/security/sasl/scram/BasicScramSelfTest.java
Outdated
Show resolved
Hide resolved
tests/base/src/test/java/org/wildfly/security/sasl/scram/BasicScramSelfTest.java
Outdated
Show resolved
Hide resolved
fc97f93
to
c739f22
Compare
@fjuma I have updated the requested changes. Thanks 👍 . BTW i can see the same changes required in other section of code too. |
tests/base/src/test/java/org/wildfly/security/sasl/scram/BasicScramSelfTest.java
Outdated
Show resolved
Hide resolved
tests/base/src/test/java/org/wildfly/security/sasl/scram/BasicScramSelfTest.java
Outdated
Show resolved
Hide resolved
tests/base/src/test/java/org/wildfly/security/sasl/scram/BasicScramSelfTest.java
Outdated
Show resolved
Hide resolved
c739f22
to
9248c65
Compare
.setPassword("pencil".toCharArray()) | ||
.build(); | ||
CallbackHandler clientHandler = createClientCallbackHandler("wrong", "pencil".toCharArray()); | ||
testAuthentication(SaslMechanismInformation.Names.SCRAM_SHA_1, saslServer, clientHandler, "user", EMPTY); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this line:
final SaslServer saslServer =
new SaslServerBuilder(ScramSaslServerFactory.class, SaslMechanismInformation.Names.SCRAM_SHA_1)
.setUserName("user")
.setPassword("pencil".toCharArray())
.build();
We are configuring a username ("user") and password ("pencil") to be used on the server side.
In this line:
CallbackHandler clientHandler = createClientCallbackHandler("wrong", "pencil".toCharArray());
We are specifying the username ("wrong") and password ("pencil") to be used on the client side.
In this line:
testAuthentication(SaslMechanismInformation.Names.SCRAM_SHA_1, saslServer, clientHandler, "user", EMPTY);
We are specifying the authorizationId
to be used. Here, we are using the same username that was specified in the server side configuration above ("user").
So in the common performAuthenticationTest
method that's being added below, we need to make sure we use different variables for the server side and client side configuration, e.g., username
and password
could be used for the server side configuration and clientUsername
and clientPassword
could be used for the client side configuration. We can update the method to take these 4 parameters.
You'll then need to go through each test that's been updated here and make sure the performAuthenticationTest
is being called with the right parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fjuma I have updated those changes.
9248c65
to
9226989
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @sugan0tech, looks good!
https://issues.redhat.com/browse/ELY-2612