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

Add command for adding secret authentication for security realm. #82

Merged
merged 1 commit into from
Apr 11, 2016

Conversation

istraka
Copy link
Collaborator

@istraka istraka commented Apr 7, 2016

No description provided.

@@ -0,0 +1,89 @@
package org.wildfly.extras.creaper.commands.security.realms;


Copy link
Contributor

Choose a reason for hiding this comment

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

Oh come on :-)

@Ladicek
Copy link
Contributor

Ladicek commented Apr 7, 2016

Now thinking about the Base64 class -- we already depend on Guava and Guava has an implementation. So please use that.

@istraka
Copy link
Collaborator Author

istraka commented Apr 7, 2016

Thank you for objections. Issues are fixed.

}

/**
* {@link BaseEncoding#base64()} will be used.
Copy link
Contributor

Choose a reason for hiding this comment

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

How about saying This method will encode the provided {@code password} using Base64. ?

@Ladicek
Copy link
Contributor

Ladicek commented Apr 7, 2016

OK, looks good. Please address the remaining comments and squash.

@istraka
Copy link
Collaborator Author

istraka commented Apr 7, 2016

Done.

ModelNodeResult result = ops.readAttribute(TEST_SECRET_IDENTITY_ADDRESS, "value");
result.assertSuccess();
assertEquals("Password should be encoded properly.",
PASSWORD, new String(BaseEncoding.base64().decode(result.stringValue())));
Copy link
Contributor

Choose a reason for hiding this comment

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

Never ever do new String(byte[]), that uses the default charset. Please use new String(byte[], String|Charset).

@Ladicek
Copy link
Contributor

Ladicek commented Apr 7, 2016

Sorry for not noticing it earlier.

* This method will encode the provided {@code password} using Base64
*/
public Builder password(String password) throws IOException {
this.password = BaseEncoding.base64().encode(password.getBytes("UTF-8"));
Copy link
Contributor

Choose a reason for hiding this comment

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

One more thing: I think it should be documented that we're using UTF-8 here. Something like: For the purpose of the Base64 encoding, the {@code password} is treated as a sequence of bytes in the UTF-8 encoding.

Later, we could add an overload where one can specify the encoding themselves, but we don't need that now.

@istraka
Copy link
Collaborator Author

istraka commented Apr 11, 2016

Done.

@Ladicek Ladicek self-assigned this Apr 11, 2016
@Ladicek Ladicek added this to the 1.1.0 milestone Apr 11, 2016
@Ladicek Ladicek merged commit d90a557 into wildfly-extras:master Apr 11, 2016
@Ladicek
Copy link
Contributor

Ladicek commented Apr 11, 2016

OK, merged, thanks!

olukas pushed a commit to olukas/creaper that referenced this pull request Apr 12, 2017
fix: correct order of removing resources (because of capability)
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.

2 participants