You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by GitBox <gi...@apache.org> on 2021/01/28 14:07:18 UTC
[GitHub] [sling-org-apache-sling-serviceusermapper] anchela commented on a change in pull request #2: SLING-10099 : Validators should all agree for a service user to be valid and should be configurable
anchela commented on a change in pull request #2:
URL: https://github.com/apache/sling-org-apache-sling-serviceusermapper/pull/2#discussion_r566112312
##########
File path: src/main/java/org/apache/sling/serviceusermapping/impl/ServiceUserMapperImpl.java
##########
@@ -455,38 +486,54 @@ private boolean isValidUser(final String userId, final String serviceName, final
log.debug("isValidUser: userId is null -> invalid");
return false;
}
- if ( !userValidators.isEmpty() || require) {
- for (final ServiceUserValidator validator : userValidators) {
- if ( validator.isValid(userId, serviceName, subServiceName) ) {
- log.debug("isValidUser: Validator {} accepts userId [{}] -> valid", validator, userId);
- return true;
+ List<ServiceUserValidator> validators = getUserValidators();
+ if (!validators.isEmpty()) {
+ for (final ServiceUserValidator validator : validators) {
+ if (!validator.isValid(userId, serviceName, subServiceName)) {
+ log.debug("isValidUser: Validator {} doesn't accept userId [{}] -> invalid", validator, userId);
+ return false;
}
}
- log.debug("isValidUser: No validator accepted userId [{}] -> invalid", userId);
- return false;
+ log.debug("isValidUser: All validators accepted userId [{}] -> valid", userId);
} else {
log.debug("isValidUser: No active validators for userId [{}] -> valid", userId);
- return true;
}
+ return !require || !validators.isEmpty();
}
private boolean areValidPrincipals(final Iterable<String> principalNames, final String serviceName, final String subServiceName, boolean require) {
if (principalNames == null) {
log.debug("areValidPrincipals: principalNames are null -> invalid");
return false;
}
- if ( !principalsValidators.isEmpty() || require ) {
- for (final ServicePrincipalsValidator validator : principalsValidators) {
- if ( validator.isValid(principalNames, serviceName, subServiceName) ) {
- log.debug("areValidPrincipals: Validator {} accepts principal names [{}] -> valid", validator, principalNames);
- return true;
+ List<ServicePrincipalsValidator> validators = getPrincipalsValidators();
+ if (!validators.isEmpty() || require) {
+ for (final ServicePrincipalsValidator validator : validators) {
+ if (!validator.isValid(principalNames, serviceName, subServiceName)) {
+ log.debug("areValidPrincipals: Validator {} doesn't accept principal names [{}] -> invalid", validator, principalNames);
+ return false;
}
}
- log.debug("areValidPrincipals: No validator accepted principal names [{}] -> invalid", principalNames);
- return false;
+ log.debug("areValidPrincipals: All validators accepted principal names [{}] -> valid", principalNames);
} else {
log.debug("areValidPrincipals: No active validators for principal names [{}] -> valid", principalNames);
- return true;
+ }
+ return !require || !validators.isEmpty();
+ }
+
+ private List<ServiceUserValidator> getUserValidators() {
Review comment:
maybe add NotNull annotation
##########
File path: src/main/java/org/apache/sling/serviceusermapping/impl/ServiceUserMapperImpl.java
##########
@@ -455,38 +486,54 @@ private boolean isValidUser(final String userId, final String serviceName, final
log.debug("isValidUser: userId is null -> invalid");
return false;
}
- if ( !userValidators.isEmpty() || require) {
- for (final ServiceUserValidator validator : userValidators) {
- if ( validator.isValid(userId, serviceName, subServiceName) ) {
- log.debug("isValidUser: Validator {} accepts userId [{}] -> valid", validator, userId);
- return true;
+ List<ServiceUserValidator> validators = getUserValidators();
+ if (!validators.isEmpty()) {
+ for (final ServiceUserValidator validator : validators) {
+ if (!validator.isValid(userId, serviceName, subServiceName)) {
+ log.debug("isValidUser: Validator {} doesn't accept userId [{}] -> invalid", validator, userId);
+ return false;
}
}
- log.debug("isValidUser: No validator accepted userId [{}] -> invalid", userId);
- return false;
+ log.debug("isValidUser: All validators accepted userId [{}] -> valid", userId);
} else {
log.debug("isValidUser: No active validators for userId [{}] -> valid", userId);
- return true;
}
+ return !require || !validators.isEmpty();
}
private boolean areValidPrincipals(final Iterable<String> principalNames, final String serviceName, final String subServiceName, boolean require) {
if (principalNames == null) {
log.debug("areValidPrincipals: principalNames are null -> invalid");
return false;
}
- if ( !principalsValidators.isEmpty() || require ) {
- for (final ServicePrincipalsValidator validator : principalsValidators) {
- if ( validator.isValid(principalNames, serviceName, subServiceName) ) {
- log.debug("areValidPrincipals: Validator {} accepts principal names [{}] -> valid", validator, principalNames);
- return true;
+ List<ServicePrincipalsValidator> validators = getPrincipalsValidators();
+ if (!validators.isEmpty() || require) {
+ for (final ServicePrincipalsValidator validator : validators) {
+ if (!validator.isValid(principalNames, serviceName, subServiceName)) {
+ log.debug("areValidPrincipals: Validator {} doesn't accept principal names [{}] -> invalid", validator, principalNames);
+ return false;
}
}
- log.debug("areValidPrincipals: No validator accepted principal names [{}] -> invalid", principalNames);
- return false;
+ log.debug("areValidPrincipals: All validators accepted principal names [{}] -> valid", principalNames);
} else {
log.debug("areValidPrincipals: No active validators for principal names [{}] -> valid", principalNames);
- return true;
+ }
+ return !require || !validators.isEmpty();
+ }
+
+ private List<ServiceUserValidator> getUserValidators() {
+ if (presentValidators.containsAll(requiredValidators)) {
+ return userValidators;
+ } else {
+ return Collections.emptyList();
+ }
+ }
+
+ private List<ServicePrincipalsValidator> getPrincipalsValidators() {
Review comment:
maybe add NotNull annotation
also that's almost a duplication of getUserValidators.
##########
File path: src/main/java/org/apache/sling/serviceusermapping/impl/ServiceUserMapperImpl.java
##########
@@ -455,38 +486,54 @@ private boolean isValidUser(final String userId, final String serviceName, final
log.debug("isValidUser: userId is null -> invalid");
return false;
}
- if ( !userValidators.isEmpty() || require) {
- for (final ServiceUserValidator validator : userValidators) {
- if ( validator.isValid(userId, serviceName, subServiceName) ) {
- log.debug("isValidUser: Validator {} accepts userId [{}] -> valid", validator, userId);
- return true;
+ List<ServiceUserValidator> validators = getUserValidators();
+ if (!validators.isEmpty()) {
+ for (final ServiceUserValidator validator : validators) {
+ if (!validator.isValid(userId, serviceName, subServiceName)) {
+ log.debug("isValidUser: Validator {} doesn't accept userId [{}] -> invalid", validator, userId);
+ return false;
}
}
- log.debug("isValidUser: No validator accepted userId [{}] -> invalid", userId);
- return false;
+ log.debug("isValidUser: All validators accepted userId [{}] -> valid", userId);
} else {
log.debug("isValidUser: No active validators for userId [{}] -> valid", userId);
- return true;
}
+ return !require || !validators.isEmpty();
Review comment:
same as below
##########
File path: src/main/java/org/apache/sling/serviceusermapping/impl/ServiceUserMapperImpl.java
##########
@@ -85,7 +87,13 @@
"default mapping is applied which uses the service user \"serviceuser--\" + bundleId + [\"--\" + subServiceName]")
boolean user_enable_default_mapping() default true;
+ @AttributeDefinition(name = "Require Validation",
+ description = "Do validators have to be present and validate the service users?")
boolean require_validation() default false;
+
+ @AttributeDefinition(name = "Required Validators",
+ description = "A list of required validators ids. If not present and require validation is on not user will be valid")
Review comment:
that's a bit hard to read.... i would either put on in quotes 'on' or replace _on_ by _enabled_.
then: 'not user will be valid'. shouldn't that read 'no userid and no principal name will be valid'?
##########
File path: src/main/java/org/apache/sling/serviceusermapping/impl/ServiceUserMapperImpl.java
##########
@@ -455,38 +486,54 @@ private boolean isValidUser(final String userId, final String serviceName, final
log.debug("isValidUser: userId is null -> invalid");
return false;
}
- if ( !userValidators.isEmpty() || require) {
- for (final ServiceUserValidator validator : userValidators) {
- if ( validator.isValid(userId, serviceName, subServiceName) ) {
- log.debug("isValidUser: Validator {} accepts userId [{}] -> valid", validator, userId);
- return true;
+ List<ServiceUserValidator> validators = getUserValidators();
+ if (!validators.isEmpty()) {
+ for (final ServiceUserValidator validator : validators) {
+ if (!validator.isValid(userId, serviceName, subServiceName)) {
+ log.debug("isValidUser: Validator {} doesn't accept userId [{}] -> invalid", validator, userId);
+ return false;
}
}
- log.debug("isValidUser: No validator accepted userId [{}] -> invalid", userId);
- return false;
+ log.debug("isValidUser: All validators accepted userId [{}] -> valid", userId);
} else {
log.debug("isValidUser: No active validators for userId [{}] -> valid", userId);
- return true;
}
+ return !require || !validators.isEmpty();
}
private boolean areValidPrincipals(final Iterable<String> principalNames, final String serviceName, final String subServiceName, boolean require) {
if (principalNames == null) {
log.debug("areValidPrincipals: principalNames are null -> invalid");
return false;
}
- if ( !principalsValidators.isEmpty() || require ) {
- for (final ServicePrincipalsValidator validator : principalsValidators) {
- if ( validator.isValid(principalNames, serviceName, subServiceName) ) {
- log.debug("areValidPrincipals: Validator {} accepts principal names [{}] -> valid", validator, principalNames);
- return true;
+ List<ServicePrincipalsValidator> validators = getPrincipalsValidators();
+ if (!validators.isEmpty() || require) {
+ for (final ServicePrincipalsValidator validator : validators) {
+ if (!validator.isValid(principalNames, serviceName, subServiceName)) {
+ log.debug("areValidPrincipals: Validator {} doesn't accept principal names [{}] -> invalid", validator, principalNames);
+ return false;
}
}
- log.debug("areValidPrincipals: No validator accepted principal names [{}] -> invalid", principalNames);
- return false;
+ log.debug("areValidPrincipals: All validators accepted principal names [{}] -> valid", principalNames);
} else {
log.debug("areValidPrincipals: No active validators for principal names [{}] -> valid", principalNames);
- return true;
+ }
+ return !require || !validators.isEmpty();
Review comment:
that's a bit hard to read.... i think it would be easier if 'validators' would be renamed to reflect the fact that this are only the required-and-satisfied-validators..... in other words.... some hint that allows to easily understand that if 'validators' are empty not all required ones are present.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org