MfaProvider validation returns specific message. Do not restrict whit…

…espace in name [#150620737] https://www.pivotaltracker.com/story/show/150620737 Signed-off-by: Shash Reddy <sreddy@pivotal.io>
cloudfoundry · Oct 4, 2017 · 21e1af4 · 21e1af4
1 parent 8a8d957
commit 21e1af4
Show file tree

Hide file tree

Showing 17 changed files with 380 additions and 105 deletions.
diff --git a/model/src/main/java/org/cloudfoundry/identity/uaa/mfa_provider/GoogleMfaProviderConfig.java b/model/src/main/java/org/cloudfoundry/identity/uaa/mfa_provider/GoogleMfaProviderConfig.java
@@ -1,13 +1,37 @@
 package org.cloudfoundry.identity.uaa.mfa_provider;
 
+import com.fasterxml.jackson.annotation.JsonCreator;
 import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
 import com.fasterxml.jackson.annotation.JsonInclude;
 
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Set;
+
 @JsonInclude(JsonInclude.Include.NON_NULL)
 @JsonIgnoreProperties(ignoreUnknown = true)
 public class GoogleMfaProviderConfig extends AbstractMfaProviderConfig<GoogleMfaProviderConfig> {
 
-    public enum Algorithm { SHA256, SHA512 }
+    public enum Algorithm {
+        SHA256,
+        SHA512;
+
+        private static Map<String, GoogleMfaProviderConfig.Algorithm> namesMap = new HashMap();
+        static {
+            namesMap.put("SHA256", SHA256);
+            namesMap.put("SHA512", SHA512);
+        }
+
+        @JsonCreator
+        public static GoogleMfaProviderConfig.Algorithm forValue(String value) {
+            return namesMap.get(value);
+        }
+
+        public static Set<String> getStringaValues() {
+            return namesMap.keySet();
+        }
+
+    }
 
     private String providerDescription;
     private int digits = 6;

diff --git a/model/src/main/java/org/cloudfoundry/identity/uaa/mfa_provider/MfaProvider.java b/model/src/main/java/org/cloudfoundry/identity/uaa/mfa_provider/MfaProvider.java
@@ -18,6 +18,7 @@
 import java.util.Date;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.Set;
 
 import static org.cloudfoundry.identity.uaa.util.JsonUtils.getNodeAsBoolean;
 import static org.cloudfoundry.identity.uaa.util.JsonUtils.getNodeAsDate;
@@ -101,6 +102,10 @@ public String toValue() {
 
             return null; // or fail
         }
+
+        public static Set<String> getStringValues() {
+            return namesMap.keySet();
+        }
     }
 
     public T getConfig() {
@@ -148,25 +153,6 @@ public MfaProvider<T> setType(MfaProviderType type) {
         return this;
     }
 
-    public void validate() {
-        if(StringUtils.isEmpty(name)) {
-            throw new IllegalArgumentException("Provider name must be set");
-        }
-        if(name.length() > 255 || !name.matches("^[a-zA-Z0-9]*$")){
-            throw new IllegalArgumentException("Provider name invalid");
-        }
-        if(type == null) {
-            throw new IllegalArgumentException("Provider type must be set");
-        }
-        if(config == null) {
-            throw new IllegalArgumentException("Provider config must be set");
-        }
-        if(!StringUtils.hasText(identityZoneId)){
-            throw new IllegalArgumentException("Provider must belong to a zone");
-        }
-        config.validate();
-    }
-
     public static class MfaProviderDeserializer extends JsonDeserializer<MfaProvider> {
 
         @Override

diff --git a/model/src/test/java/org/cloudfoundry/identity/uaa/mfa_provider/MfaProviderTest.java b/model/src/test/java/org/cloudfoundry/identity/uaa/mfa_provider/MfaProviderTest.java
@@ -2,6 +2,7 @@
 
 import com.fasterxml.jackson.databind.JsonNode;
 import org.cloudfoundry.identity.uaa.util.JsonUtils;
+import org.cloudfoundry.identity.uaa.zone.IdentityZone;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
@@ -65,69 +66,6 @@ public void testDeserialize() {
 
     }
 
-    @Test
-    public void validateProviderNullConfig() throws ValidationException {
-        expectedException.expect(IllegalArgumentException.class);
-        expectedException.expectMessage("Provider config must be set");
-        MfaProvider<GoogleMfaProviderConfig> provider = createValidGoogleMfaProvider()
-                .setConfig(null);
-        provider.validate();
-    }
-
-    @Test
-    public void validateProviderEmptyName() throws ValidationException {
-        expectedException.expect(IllegalArgumentException.class);
-        expectedException.expectMessage("Provider name must be set");
-        MfaProvider provider = createValidGoogleMfaProvider()
-                .setName("");
-        provider.validate();
-    }
-
-    @Test
-    public void validateProviderInvalidNameTooLong() throws ValidationException {
-        expectedException.expect(IllegalArgumentException.class);
-        expectedException.expectMessage("Provider name invalid");
-        MfaProvider provider = createValidGoogleMfaProvider()
-                .setName(new RandomValueStringGenerator(256).generate());
-        provider.validate();
-    }
-    @Test
-    public void validateProviderInvalidNameWhitespace() throws ValidationException {
-        expectedException.expect(IllegalArgumentException.class);
-        expectedException.expectMessage("Provider name invalid");
-        MfaProvider provider = createValidGoogleMfaProvider()
-                .setName(" ");
-        provider.validate();
-    }
-
-    @Test
-    public void validateProviderInvalidNameSpecialChars() throws ValidationException {
-        expectedException.expect(IllegalArgumentException.class);
-        expectedException.expectMessage("Provider name invalid");
-        MfaProvider provider = createValidGoogleMfaProvider()
-                .setName("invalidName$");
-        provider.validate();
-    }
-
-
-    @Test
-    public void validateProviderNullType() throws ValidationException {
-        expectedException.expect(IllegalArgumentException.class);
-        expectedException.expectMessage("Provider type must be set");
-        MfaProvider provider = createValidGoogleMfaProvider()
-                .setType(null);
-        provider.validate();
-    }
-
-    @Test
-    public void validateProviderEmptyZone() throws ValidationException {
-        expectedException.expect(IllegalArgumentException.class);
-        expectedException.expectMessage("Provider must belong to a zone");
-        MfaProvider provider = createValidGoogleMfaProvider()
-                .setIdentityZoneId("");
-        provider.validate();
-    }
-
     @Test
     public void validateProviderActiveSetDefaultToTrue() {
         MfaProvider provider = createValidGoogleMfaProvider();
@@ -138,6 +76,7 @@ private MfaProvider createValidGoogleMfaProvider() {
         MfaProvider<GoogleMfaProviderConfig> res = new MfaProvider();
         res.setName(new RandomValueStringGenerator(5).generate())
                 .setConfig(createValidGoogleMfaConfig())
+                .setIdentityZoneId(IdentityZone.getUaa().getId())
                 .setType(MfaProvider.MfaProviderType.GOOGLE_AUTHENTICATOR);
         return res;
     }

diff --git a/...src/main/java/org/cloudfoundry/identity/uaa/mfa_provider/GeneralMfaProviderValidator.java b/...src/main/java/org/cloudfoundry/identity/uaa/mfa_provider/GeneralMfaProviderValidator.java
@@ -0,0 +1,40 @@
+package org.cloudfoundry.identity.uaa.mfa_provider;
+
+import org.springframework.util.StringUtils;
+
+public class GeneralMfaProviderValidator implements MfaProviderValidator{
+    private MfaProviderConfigValidator configValidator;
+
+    @Override
+    public void validate(MfaProvider mfaProvider) {
+        if(mfaProvider.getName() == null || StringUtils.isEmpty(mfaProvider.getName().trim())) {
+            throw new InvalidMfaProviderException("Provider name is required");
+        }
+        mfaProvider.setName(mfaProvider.getName().trim());
+        if(mfaProvider.getName().length() > 256) {
+            throw new InvalidMfaProviderException("Provider name cannot be longer than 256 characters");
+        }
+        if(!mfaProvider.getName().matches("^[a-zA-Z0-9]+[\\sa-zA-Z0-9]*$")){
+            throw new InvalidMfaProviderException("Provider name must be alphanumeric");
+        }
+        if(mfaProvider.getType() == null) {
+            throw new InvalidMfaProviderException("Provider type is required. Must be one of " + MfaProvider.MfaProviderType.getStringValues());
+        }
+        if(mfaProvider.getConfig() == null) {
+            throw new InvalidMfaProviderException("Provider config is required");
+        }
+        if(!StringUtils.hasText(mfaProvider.getIdentityZoneId())){
+            throw new InvalidMfaProviderException("Provider must belong to a zone");
+        }
+        try {
+            configValidator.validate(mfaProvider.getConfig());
+        } catch (InvalidMfaProviderConfigException e) {
+            throw new InvalidMfaProviderException("Invalid Config for MFA Provider. " + e.getMessage());
+        }
+    }
+
+
+    public void setConfigValidator(MfaProviderConfigValidator configValidator) {
+        this.configValidator = configValidator;
+    }
+}
diff --git a/...ain/java/org/cloudfoundry/identity/uaa/mfa_provider/GoogleMfaProviderConfigValidator.java b/...ain/java/org/cloudfoundry/identity/uaa/mfa_provider/GoogleMfaProviderConfigValidator.java
@@ -0,0 +1,17 @@
+package org.cloudfoundry.identity.uaa.mfa_provider;
+
+public class GoogleMfaProviderConfigValidator implements MfaProviderConfigValidator<GoogleMfaProviderConfig> {
+
+    @Override
+    public void validate(GoogleMfaProviderConfig mfaProviderConfig) throws InvalidMfaProviderConfigException {
+        if(mfaProviderConfig.getAlgorithm() == null) {
+            throw new InvalidMfaProviderConfigException("Algorithm must be one of " + GoogleMfaProviderConfig.Algorithm.getStringaValues());
+        }
+        if(mfaProviderConfig.getDigits() < 1) {
+            throw new InvalidMfaProviderConfigException("Digits must be greater than 0");
+        }
+        if(mfaProviderConfig.getDuration() < 1) {
+            throw new InvalidMfaProviderConfigException("Duration must be greater than 0");
+        }
+    }
+}
diff --git a/...in/java/org/cloudfoundry/identity/uaa/mfa_provider/InvalidMfaProviderConfigException.java b/...in/java/org/cloudfoundry/identity/uaa/mfa_provider/InvalidMfaProviderConfigException.java
@@ -0,0 +1,7 @@
+package org.cloudfoundry.identity.uaa.mfa_provider;
+
+public class InvalidMfaProviderConfigException extends Exception {
+    public InvalidMfaProviderConfigException(String message) {
+        super(message);
+    }
+}
diff --git a/...src/main/java/org/cloudfoundry/identity/uaa/mfa_provider/InvalidMfaProviderException.java b/...src/main/java/org/cloudfoundry/identity/uaa/mfa_provider/InvalidMfaProviderException.java
@@ -0,0 +1,15 @@
+package org.cloudfoundry.identity.uaa.mfa_provider;
+
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.cloudfoundry.identity.uaa.error.UaaException;
+import org.springframework.http.HttpStatus;
+
+public class InvalidMfaProviderException extends UaaException {
+
+    public final Log logger = LogFactory.getLog(InvalidMfaProviderException.class);
+    public InvalidMfaProviderException(String message) {
+        super("invalid_mfa_provider", message, HttpStatus.UNPROCESSABLE_ENTITY.value());
+        logger.debug("MfaProvider validation error. " + message);
+    }
+}
diff --git a/...src/main/java/org/cloudfoundry/identity/uaa/mfa_provider/JdbcMfaProviderProvisioning.java b/...src/main/java/org/cloudfoundry/identity/uaa/mfa_provider/JdbcMfaProviderProvisioning.java
@@ -24,15 +24,17 @@ public class JdbcMfaProviderProvisioning implements MfaProviderProvisioning, Sys
     public static final String CREATE_PROVIDER_SQL = "insert into mfa_providers(" + MFA_PROVIDER_FIELDS + ") values (?,?,?,?,?,?,?,?)";
     public static final String MFA_PROVIDER_BY_ID_QUERY = "select " + MFA_PROVIDER_FIELDS + " from mfa_providers " + "where id=? and identity_zone_id=?";
     protected final JdbcTemplate jdbcTemplate;
+    private MfaProviderValidator mfaProviderValidator;
     private MfaProviderMapper mapper = new MfaProviderMapper();
 
-    public JdbcMfaProviderProvisioning(JdbcTemplate jdbcTemplate) {
+    public JdbcMfaProviderProvisioning(JdbcTemplate jdbcTemplate, MfaProviderValidator mfaProviderValidator) {
         this.jdbcTemplate = jdbcTemplate;
+        this.mfaProviderValidator = mfaProviderValidator;
     }
 
     @Override
     public MfaProvider create(MfaProvider provider, String zoneId) {
-        provider.validate();
+        mfaProviderValidator.validate(provider);
         final String id = UUID.randomUUID().toString();
         try {
             jdbcTemplate.update(CREATE_PROVIDER_SQL, new PreparedStatementSetter() {

diff --git a/.../src/main/java/org/cloudfoundry/identity/uaa/mfa_provider/MfaProviderConfigValidator.java b/.../src/main/java/org/cloudfoundry/identity/uaa/mfa_provider/MfaProviderConfigValidator.java
@@ -0,0 +1,5 @@
+package org.cloudfoundry.identity.uaa.mfa_provider;
+
+public interface MfaProviderConfigValidator<T extends AbstractMfaProviderConfig>{
+    void validate(T mfaProviderConfig) throws InvalidMfaProviderConfigException;
+}
diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/mfa_provider/MfaProviderEndpoints.java b/server/src/main/java/org/cloudfoundry/identity/uaa/mfa_provider/MfaProviderEndpoints.java
@@ -8,6 +8,7 @@
 import org.springframework.http.HttpStatus;
 import org.springframework.http.ResponseEntity;
 import org.springframework.util.StringUtils;
+import org.springframework.web.bind.annotation.ExceptionHandler;
 import org.springframework.web.bind.annotation.RequestBody;
 import org.springframework.web.bind.annotation.RequestMapping;
 import org.springframework.web.bind.annotation.RestController;
@@ -21,6 +22,7 @@ public class MfaProviderEndpoints implements ApplicationEventPublisherAware{
     protected static Log logger = LogFactory.getLog(MfaProviderEndpoints.class);
     private ApplicationEventPublisher publisher;
     private MfaProviderProvisioning mfaProviderProvisioning;
+    private MfaProviderValidator mfaProviderValidator;
 
     @Override
     public void setApplicationEventPublisher(ApplicationEventPublisher applicationEventPublisher) {
@@ -30,15 +32,10 @@ public void setApplicationEventPublisher(ApplicationEventPublisher applicationEv
     @RequestMapping(method = POST)
     public ResponseEntity<MfaProvider> createMfaProvider(@RequestBody MfaProvider provider) {
         String zoneId = IdentityZoneHolder.get().getId();
-        try {
-            provider.setIdentityZoneId(zoneId);
-            provider.validate();
-            if(!StringUtils.hasText(provider.getConfig().getIssuer())){
-                provider.getConfig().setIssuer(IdentityZoneHolder.get().getName());
-            }
-        } catch (IllegalArgumentException e) {
-            logger.debug("MfaProvider [name"+provider.getName()+"] - Configuration validation error.", e);
-            return new ResponseEntity<>(provider, UNPROCESSABLE_ENTITY);
+        provider.setIdentityZoneId(zoneId);
+        mfaProviderValidator.validate(provider);
+        if(!StringUtils.hasText(provider.getConfig().getIssuer())){
+            provider.getConfig().setIssuer(IdentityZoneHolder.get().getName());
         }
         MfaProvider created = mfaProviderProvisioning.create(provider,zoneId);
         return new ResponseEntity<>(created, HttpStatus.CREATED);
@@ -51,4 +48,13 @@ public MfaProviderProvisioning getMfaProviderProvisioning() {
     public void setMfaProviderProvisioning(MfaProviderProvisioning mfaProviderProvisioning) {
         this.mfaProviderProvisioning = mfaProviderProvisioning;
     }
+
+    public void setMfaProviderValidator(MfaProviderValidator mfaProviderValidator) {
+        this.mfaProviderValidator = mfaProviderValidator;
+    }
+
+    @ExceptionHandler(InvalidMfaProviderException.class)
+    public ResponseEntity<InvalidMfaProviderException> handleInvalidMfaProviderException(InvalidMfaProviderException e) {
+        return new ResponseEntity<>(e, HttpStatus.UNPROCESSABLE_ENTITY);
+    }
 }
diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/mfa_provider/MfaProviderValidator.java b/server/src/main/java/org/cloudfoundry/identity/uaa/mfa_provider/MfaProviderValidator.java
@@ -0,0 +1,5 @@
+package org.cloudfoundry.identity.uaa.mfa_provider;
+
+public interface MfaProviderValidator {
+    void validate(MfaProvider mfaProvider);
+}