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-8131]: Aliases in credential stores should be case insensitive. #81

Merged
merged 1 commit into from Apr 4, 2017

Conversation

ehsavoie
Copy link

@ehsavoie ehsavoie commented Mar 16, 2017

Adding validation step to ensure that the alias is lower case.
Adding parameter on CredentialStore to allow disabling of this validation in case the store supports case sensitivity.

Jira: https://issues.jboss.org/browse/WFCORE-2556
JBEAP: https://issues.jboss.org/browse/JBEAP-8871
Test update PR : wildfly-security-incubator/wildfly#162

transformOperationAddress(operation);
super.execute(context, operation);
ModelNode transformedOperation = transformOperation(context, operation);
if(!CASE_SENSITIVE.resolveModelAttribute(context, context.readResourceFromRoot(context.getCurrentAddress().getParent()).getModel()).asBoolean()) {

Choose a reason for hiding this comment

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

context.readResourceFromRoot(....., false) to save cloning children that aren't needed.

@darranl
Copy link

darranl commented Mar 16, 2017

Sorry we probably should get the Jira issue moved to WFCORE and the commit messages updated for the new number. As we used to pull in the subsystem in WildFly we were using WFLY issues to make it easier for QE but now we have moved we should use the correct project.

@@ -1169,6 +1169,7 @@ elytron.server-ssl-context.ssl-session.invalidate=Invalidate the SSLSession (Not
# Credential Store #
####################
elytron.credential-store=Credential store to keep alias for sensitive information such as passwords for external services.
elytron.credential-store.case-sensitive=Case sensitivity of the credential store.

Choose a reason for hiding this comment

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

This needs more explanation.

super.execute(context, operation);
ModelNode transformedOperation = transformOperation(context, operation);
if(!CASE_SENSITIVE.resolveModelAttribute(context, context.readResourceFromRoot(context.getCurrentAddress().getParent()).getModel()).asBoolean()) {
ALIAS_NAME.resolveModelAttribute(context, transformedOperation).asString();

Choose a reason for hiding this comment

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

A reusable validator is nice, but this isn't a parameter, so the validator is providing a misleading message. I think we're better off providing a precise message, since this kind of constraint on resource names is not common and the reason why case matters isn't obvious.

if (sameAlias(context, operation)) {
context.addResource(PathAddress.EMPTY_ADDRESS, resource);
}
context.addResource(PathAddress.EMPTY_ADDRESS, resource);
return resource;
}

Choose a reason for hiding this comment

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

This doesn't need to be changed to merge this but I think this entire method override can be removed.

Adding validation step to ensure that  the alias is lower case.
Adding parameter on CredentialStore to allow disabling of this validation in case the store supports case sensitivity.
Copy link

@kabir kabir left a comment

Choose a reason for hiding this comment

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

https://issues.jboss.org/browse/JBEAP-8871 is recorded as resolved. Possibly because half the work came from the subsystem PR linked by 8871?

@darranl darranl merged commit 1081668 into wildfly-security-incubator:ladybird Apr 4, 2017
@ehsavoie ehsavoie deleted the WFLY-8131 branch October 6, 2017 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants