[SYNCOPE-1309] Clean up JWT after submitting self update
authorFrancesco Chicchiriccò <ilgrosso@apache.org>
Mon, 28 May 2018 15:12:07 +0000 (17:12 +0200)
committerFrancesco Chicchiriccò <ilgrosso@apache.org>
Tue, 29 May 2018 09:40:34 +0000 (11:40 +0200)
15 files changed:
client/console/src/main/java/org/apache/syncope/client/console/rest/UserSelfRestClient.java
client/enduser/src/main/java/org/apache/syncope/client/enduser/SyncopeEnduserSession.java
client/enduser/src/main/java/org/apache/syncope/client/enduser/resources/UserSelfChangePassword.java
client/enduser/src/main/java/org/apache/syncope/client/enduser/resources/UserSelfUpdateResource.java
common/lib/src/main/java/org/apache/syncope/common/lib/AnyOperations.java
common/rest-api/src/main/java/org/apache/syncope/common/rest/api/service/UserSelfService.java
core/logic/src/main/java/org/apache/syncope/core/logic/UserLogic.java
core/persistence-api/src/main/java/org/apache/syncope/core/persistence/api/dao/ConfDAO.java
core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/dao/JPAAccessTokenDAO.java
core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/dao/JPAConfDAO.java
core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/pushpull/AzurePullActions.java
core/rest-cxf/src/main/java/org/apache/syncope/core/rest/cxf/service/UserSelfServiceImpl.java
core/spring/src/main/java/org/apache/syncope/core/spring/security/AuthDataAccessor.java
fit/core-reference/src/test/java/org/apache/syncope/fit/console/AbstractConsoleITCase.java
fit/core-reference/src/test/java/org/apache/syncope/fit/core/UserSelfITCase.java

index c796a88..8b81f14 100644 (file)
@@ -25,7 +25,7 @@ public class UserSelfRestClient extends BaseRestClient {
     private static final long serialVersionUID = 100731599744900931L;
 
     public void changePassword(final String password) {
-        getService(UserSelfService.class).changePassword(password);
+        getService(UserSelfService.class).mustChangePassword(password);
     }
 
 }
index 0238072..9f8fe09 100644 (file)
@@ -18,7 +18,6 @@
  */
 package org.apache.syncope.client.enduser;
 
-import java.security.AccessControlException;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -137,7 +136,7 @@ public class SyncopeEnduserSession extends WebSession {
         if (isAuthenticated()) {
             try {
                 client.logout();
-            } catch (AccessControlException e) {
+            } catch (Exception e) {
                 LOG.debug("Unexpected exception while logging out", e);
             } finally {
                 client = null;
index 9d8ed9a..2d476e8 100644 (file)
@@ -53,9 +53,7 @@ public class UserSelfChangePassword extends BaseResource {
                 throw new Exception("A new correct password should be provided");
             }
             SyncopeEnduserSession.get().getService(UserSelfService.class).
-                    changePassword(parameters.get("newPassword")[0]);
-
-            final String responseMessage = new StringBuilder().append("Password changed correctly").toString();
+                    mustChangePassword(parameters.get("newPassword")[0]);
 
             response.setTextEncoding(StandardCharsets.UTF_8.name());
 
@@ -63,7 +61,7 @@ public class UserSelfChangePassword extends BaseResource {
 
                 @Override
                 public void writeData(final Attributes attributes) throws IOException {
-                    attributes.getResponse().write(responseMessage);
+                    attributes.getResponse().write("Password changed correctly");
                 }
             });
 
index 221b744..26bad61 100644 (file)
@@ -151,14 +151,22 @@ public class UserSelfUpdateResource extends BaseUserSelfResource {
                 }
                 // create diff patch
                 UserPatch userPatch = AnyOperations.diff(userTO, selfTO, false);
-                // update user by patch
-                Response res = SyncopeEnduserSession.get().
-                        getService(userTO.getETagValue(), UserSelfService.class).update(userPatch);
-
-                buildResponse(response, res.getStatus(), res.getStatusInfo().getFamily().equals(
-                        Response.Status.Family.SUCCESSFUL)
-                                ? "User [" + userTO.getUsername() + "] successfully updated"
-                                : "ErrorMessage{{ " + res.getStatusInfo().getReasonPhrase() + " }}");
+                if (userPatch.isEmpty()) {
+                    // nothing to do
+                    buildResponse(response,
+                            Response.Status.OK.getStatusCode(),
+                            "No need to update [" + selfTO.getUsername() + "]");
+                } else {
+                    // update user by patch
+                    Response coreResponse = SyncopeEnduserSession.get().
+                            getService(userTO.getETagValue(), UserSelfService.class).update(userPatch);
+
+                    buildResponse(response,
+                            coreResponse.getStatus(),
+                            coreResponse.getStatusInfo().getFamily() == Response.Status.Family.SUCCESSFUL
+                            ? "User [" + selfTO.getUsername() + "] successfully updated"
+                            : "ErrorMessage{{ " + coreResponse.getStatusInfo().getReasonPhrase() + " }}");
+                }
             } else {
                 LOG.warn(
                         "Incoming update request [{}] is not compliant with form customization rules."
index c82dc2d..d6b7777 100644 (file)
 package org.apache.syncope.common.lib;
 
 import java.util.Collection;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Set;
+import java.util.stream.Collectors;
 import org.apache.commons.lang3.SerializationUtils;
 import org.apache.commons.lang3.StringUtils;
 import org.apache.commons.lang3.tuple.Pair;
@@ -55,6 +57,8 @@ public final class AnyOperations {
 
     private static final Logger LOG = LoggerFactory.getLogger(AnyOperations.class);
 
+    private static final Set<String> NULL_SINGLETON = Collections.singleton(null);
+
     private AnyOperations() {
         // empty constructor for static utility classes
     }
@@ -71,29 +75,6 @@ public final class AnyOperations {
     }
 
     private static void diff(
-            final MembershipTO updated,
-            final MembershipTO original,
-            final MembershipPatch result,
-            final boolean incremental) {
-
-        // check same key
-        if (updated.getGroupKey() == null && original.getGroupKey() != null
-                || (updated.getGroupKey() != null && !updated.getGroupKey().equals(original.getGroupKey()))) {
-
-            throw new IllegalArgumentException("Memberships must be the same");
-        }
-        result.setGroup(updated.getGroupKey());
-
-        // 1. plain attributes
-        result.getPlainAttrs().clear();
-        result.getPlainAttrs().addAll(updated.getPlainAttrs());
-
-        // 2. virtual attributes
-        result.getVirAttrs().clear();
-        result.getVirAttrs().addAll(updated.getVirAttrs());
-    }
-
-    private static void diff(
             final AnyTO updated, final AnyTO original, final AnyPatch result, final boolean incremental) {
 
         // check same key
@@ -226,13 +207,18 @@ public final class AnyOperations {
         Map<String, MembershipTO> updatedMembs = EntityTOUtils.buildMembershipMap(updated.getMemberships());
         Map<String, MembershipTO> originalMembs = EntityTOUtils.buildMembershipMap(original.getMemberships());
 
-        updatedMembs.entrySet().stream().
-                filter(entry -> (!originalMembs.containsKey(entry.getKey()))).
-                forEachOrdered(entry -> {
-                    result.getMemberships().add(new MembershipPatch.Builder().
-                            operation(PatchOperation.ADD_REPLACE).group(entry.getValue().getGroupKey()).
-                            build());
-                });
+        updatedMembs.forEach((key, value) -> {
+            MembershipPatch membershipPatch = new MembershipPatch.Builder().
+                    operation(PatchOperation.ADD_REPLACE).group(value.getGroupKey()).build();
+
+            diff(value, membershipPatch);
+
+            if (!originalMembs.containsKey(key)
+                    || (!membershipPatch.getPlainAttrs().isEmpty() || !membershipPatch.getVirAttrs().isEmpty())) {
+
+                result.getMemberships().add(membershipPatch);
+            }
+        });
 
         if (!incremental) {
             originalMembs.keySet().stream().filter(membership -> !updatedMembs.containsKey(membership)).
@@ -245,6 +231,20 @@ public final class AnyOperations {
         return result;
     }
 
+    private static void diff(
+            final MembershipTO updated,
+            final MembershipPatch result) {
+
+        // 1. plain attributes
+        result.getPlainAttrs().addAll(updated.getPlainAttrs().stream().
+                filter(attrTO -> !attrTO.getValues().isEmpty() && NULL_SINGLETON.equals(attrTO.getValues())).
+                collect(Collectors.toSet()));
+
+        // 2. virtual attributes
+        result.getVirAttrs().clear();
+        result.getVirAttrs().addAll(updated.getVirAttrs());
+    }
+
     /**
      * Calculate modifications needed by first in order to be equal to second.
      *
@@ -329,24 +329,18 @@ public final class AnyOperations {
         Map<String, MembershipTO> updatedMembs = EntityTOUtils.buildMembershipMap(updated.getMemberships());
         Map<String, MembershipTO> originalMembs = EntityTOUtils.buildMembershipMap(original.getMemberships());
 
-        updatedMembs.entrySet().stream().
-                map(entry -> {
-                    MembershipPatch membershipPatch = new MembershipPatch.Builder().
-                            operation(PatchOperation.ADD_REPLACE).group(entry.getValue().getGroupKey()).build();
-                    MembershipTO omemb;
-                    if (originalMembs.containsKey(entry.getKey())) {
-                        // get the original membership
-                        omemb = originalMembs.get(entry.getKey());
-                    } else {
-                        // create an empty one to generate the patch
-                        omemb = new MembershipTO.Builder().group(entry.getKey()).build();
-                    }
-                    diff(entry.getValue(), omemb, membershipPatch, incremental);
-                    return membershipPatch;
-                }).
-                forEachOrdered(membershipPatch -> {
-                    result.getMemberships().add(membershipPatch);
-                });
+        updatedMembs.forEach((key, value) -> {
+            MembershipPatch membershipPatch = new MembershipPatch.Builder().
+                    operation(PatchOperation.ADD_REPLACE).group(value.getGroupKey()).build();
+
+            diff(value, membershipPatch);
+
+            if (!originalMembs.containsKey(key)
+                    || (!membershipPatch.getPlainAttrs().isEmpty() || !membershipPatch.getVirAttrs().isEmpty())) {
+
+                result.getMemberships().add(membershipPatch);
+            }
+        });
 
         if (!incremental) {
             originalMembs.keySet().stream().filter(membership -> !updatedMembs.containsKey(membership)).
@@ -416,7 +410,7 @@ public final class AnyOperations {
                 if (removed != null && removed.getSchemaInfo() != null) {
                     patch.getAttrTO().setSchemaInfo(removed.getSchemaInfo());
                 }
-                if (patch.getOperation() == PatchOperation.ADD_REPLACE) {
+                if (patch.getOperation() == PatchOperation.ADD_REPLACE && !patch.getAttrTO().getValues().isEmpty()) {
                     rwattrs.put(patch.getAttrTO().getSchema(), patch.getAttrTO());
                 }
             }
index 656e912..fd1665a 100644 (file)
@@ -237,9 +237,9 @@ public interface UserSelfService extends JAXRSService {
         @SecurityRequirement(name = "BasicAuthentication"),
         @SecurityRequirement(name = "Bearer") })
     @POST
-    @Path("changePassword")
+    @Path("mustChangePassword")
     @Produces({ MediaType.APPLICATION_JSON, SyncopeConstants.APPLICATION_YAML, MediaType.APPLICATION_XML })
-    Response changePassword(String password);
+    Response mustChangePassword(String password);
 
     /**
      * Provides answer for the security question configured for user matching the given username, if any.
index d094652..dace468 100644 (file)
@@ -41,7 +41,9 @@ import org.apache.syncope.common.lib.types.AnyTypeKind;
 import org.apache.syncope.common.lib.types.ClientExceptionType;
 import org.apache.syncope.common.lib.types.PatchOperation;
 import org.apache.syncope.common.lib.types.StandardEntitlement;
+import org.apache.syncope.core.persistence.api.dao.AccessTokenDAO;
 import org.apache.syncope.core.persistence.api.dao.AnySearchDAO;
+import org.apache.syncope.core.persistence.api.dao.ConfDAO;
 import org.apache.syncope.core.persistence.api.dao.NotFoundException;
 import org.apache.syncope.core.persistence.api.dao.search.OrderByClause;
 import org.apache.syncope.core.persistence.api.dao.search.SearchCond;
@@ -69,6 +71,12 @@ public class UserLogic extends AbstractAnyLogic<UserTO, UserPatch> {
     protected AnySearchDAO searchDAO;
 
     @Autowired
+    protected ConfDAO confDAO;
+
+    @Autowired
+    protected AccessTokenDAO accessTokenDAO;
+
+    @Autowired
     protected UserDataBinder binder;
 
     @Autowired
@@ -160,7 +168,18 @@ public class UserLogic extends AbstractAnyLogic<UserTO, UserPatch> {
     public ProvisioningResult<UserTO> selfUpdate(final UserPatch userPatch, final boolean nullPriorityAsync) {
         UserTO userTO = binder.getAuthenticatedUserTO();
         userPatch.setKey(userTO.getKey());
-        return doUpdate(userPatch, true, nullPriorityAsync);
+        ProvisioningResult<UserTO> updated = doUpdate(userPatch, true, nullPriorityAsync);
+
+        // Ensures that, if the self update above moves the user into a status from which no authentication
+        // is possible, the existing Access Token is clean up to avoid issues with future authentications
+        if (!confDAO.getValuesAsStrings("authentication.statuses").contains(updated.getEntity().getStatus())) {
+            String accessToken = accessTokenDAO.findByOwner(updated.getEntity().getUsername()).getKey();
+            if (accessToken != null) {
+                accessTokenDAO.delete(accessToken);
+            }
+        }
+
+        return updated;
     }
 
     @PreAuthorize("hasRole('" + StandardEntitlement.USER_UPDATE + "')")
@@ -259,7 +278,7 @@ public class UserLogic extends AbstractAnyLogic<UserTO, UserPatch> {
     }
 
     @PreAuthorize("hasRole('" + StandardEntitlement.MUST_CHANGE_PASSWORD + "')")
-    public ProvisioningResult<UserTO> changePassword(final String password, final boolean nullPriorityAsync) {
+    public ProvisioningResult<UserTO> mustChangePassword(final String password, final boolean nullPriorityAsync) {
         UserPatch userPatch = new UserPatch();
         userPatch.setPassword(new PasswordPatch.Builder().value(password).build());
         userPatch.setMustChangePassword(new BooleanReplacePatchItem.Builder().value(false).build());
index b150aaa..af6f8c4 100644 (file)
@@ -19,6 +19,7 @@
 package org.apache.syncope.core.persistence.api.dao;
 
 import java.util.Optional;
+import java.util.List;
 import org.apache.syncope.core.persistence.api.entity.conf.CPlainAttr;
 import org.apache.syncope.core.persistence.api.entity.conf.Conf;
 
@@ -26,6 +27,8 @@ public interface ConfDAO extends DAO<Conf> {
 
     Optional<? extends CPlainAttr> find(String key);
 
+    List<String> getValuesAsStrings(String key);
+
     <T> T find(String key, T defaultValue);
 
     Conf get();
index 2ecc44e..59c69d8 100644 (file)
@@ -109,12 +109,13 @@ public class JPAAccessTokenDAO extends AbstractDAO<AccessToken> implements Acces
         return query.getResultList();
     }
 
-    @Override
     @Transactional(rollbackFor = Throwable.class)
+    @Override
     public AccessToken save(final AccessToken accessToken) {
         return entityManager().merge(accessToken);
     }
 
+    @Transactional(rollbackFor = Throwable.class)
     @Override
     public void delete(final String key) {
         AccessToken accessToken = find(key);
index 29d161e..35d07d2 100644 (file)
@@ -19,6 +19,8 @@
 package org.apache.syncope.core.persistence.jpa.dao;
 
 import java.util.Optional;
+import java.util.Collections;
+import java.util.List;
 import org.apache.syncope.core.persistence.api.dao.ConfDAO;
 import org.apache.syncope.core.persistence.api.entity.conf.CPlainAttr;
 import org.apache.syncope.core.persistence.api.entity.conf.Conf;
@@ -52,6 +54,13 @@ public class JPAConfDAO extends AbstractDAO<Conf> implements ConfDAO {
 
     @Transactional(readOnly = true)
     @Override
+    public List<String> getValuesAsStrings(final String key) {
+        Optional<? extends CPlainAttr> attr = find(key);
+        return attr.isPresent() ? attr.get().getValuesAsStrings() : Collections.<String>emptyList();
+    }
+
+    @Transactional(readOnly = true)
+    @Override
     public <T> T find(final String key, final T defaultValue) {
         Optional<? extends CPlainAttr> result = find(key);
         if (!result.isPresent()) {
index 3b3d786..67e8fd3 100644 (file)
@@ -80,10 +80,6 @@ public class AzurePullActions implements PullActions {
 
     private final Map<EntityTO, String> azureRefs = new HashMap<>();
 
-    protected String getEmailAttrName() {
-        return "mailNickname";
-    }
-
     protected String getAzureUserIdSchema() {
         return "AzureUserId";
     }
@@ -217,5 +213,4 @@ public class AzurePullActions implements PullActions {
             }
         }
     }
-
 }
index 4a08852..ed6cd69 100644 (file)
@@ -90,8 +90,8 @@ public class UserSelfServiceImpl extends AbstractServiceImpl implements UserSelf
     }
 
     @Override
-    public Response changePassword(final String password) {
-        ProvisioningResult<UserTO> updated = logic.changePassword(password, isNullPriorityAsync());
+    public Response mustChangePassword(final String password) {
+        ProvisioningResult<UserTO> updated = logic.mustChangePassword(password, isNullPriorityAsync());
         return modificationResponse(updated);
     }
 
index 691ff36..1cc5b61 100644 (file)
@@ -210,8 +210,7 @@ public class AuthDataAccessor {
                 throw new DisabledException("User " + user.getUsername() + " is suspended");
             }
 
-            Optional<? extends CPlainAttr> authStatuses = confDAO.find("authentication.statuses");
-            if (authStatuses.isPresent() && !authStatuses.get().getValuesAsStrings().contains(user.getStatus())) {
+            if (!confDAO.getValuesAsStrings("authentication.statuses").contains(user.getStatus())) {
                 throw new DisabledException("User " + user.getUsername() + " not allowed to authenticate");
             }
 
@@ -405,8 +404,7 @@ public class AuthDataAccessor {
                 throw new DisabledException("User " + username + " is suspended");
             }
 
-            Optional<? extends CPlainAttr> authStatuses = confDAO.find("authentication.statuses");
-            if (authStatuses.isPresent() && !authStatuses.get().getValuesAsStrings().contains(user.getStatus())) {
+            if (!confDAO.getValuesAsStrings("authentication.statuses").contains(user.getStatus())) {
                 throw new DisabledException("User " + username + " not allowed to authenticate");
             }
 
index f9ab81b..b72288a 100644 (file)
@@ -31,7 +31,6 @@ import org.apache.wicket.markup.html.list.ListItem;
 import org.apache.wicket.util.tester.FormTester;
 import org.apache.wicket.util.tester.WicketTester;
 import org.apache.wicket.util.visit.IVisit;
-import org.apache.wicket.util.visit.IVisitor;
 import org.junit.jupiter.api.BeforeAll;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -78,7 +77,7 @@ public abstract class AbstractConsoleITCase {
 
         Component component = TESTER.getComponentFromLastRenderedPage(searchPath);
         return (component instanceof MarkupContainer ? MarkupContainer.class.cast(component) : component.getPage()).
-                visitChildren(ListItem.class, (final ListItem<?> object, final IVisit<Component> visit) -> {
+                visitChildren(ListItem.class, (ListItem<?> object, IVisit<Component> visit) -> {
                     try {
                         Method getter = PropertyResolver.getPropertyGetter(property, object.getModelObject());
                         if (getter != null && getter.invoke(object.getModelObject()).equals(key)) {
@@ -95,18 +94,14 @@ public abstract class AbstractConsoleITCase {
 
         Component component = TESTER.getComponentFromLastRenderedPage(searchPath);
         return (component instanceof MarkupContainer ? MarkupContainer.class.cast(component) : component.getPage()).
-                visitChildren(ListItem.class, new IVisitor<ListItem<?>, Component>() {
-
-                    @Override
-                    public void component(final ListItem<?> object, final IVisit<Component> visit) {
-                        try {
-                            Method getter = PropertyResolver.getPropertyGetter(property, object.getModelObject());
-                            if (getter != null && getter.invoke(object.getModelObject()) != null) {
-                                visit.stop(object);
-                            }
-                        } catch (Exception e) {
-                            LOG.error("Error invoke method", e);
+                visitChildren(ListItem.class, (ListItem<?> object, IVisit<Component> visit) -> {
+                    try {
+                        Method getter = PropertyResolver.getPropertyGetter(property, object.getModelObject());
+                        if (getter != null && getter.invoke(object.getModelObject()) != null) {
+                            visit.stop(object);
                         }
+                    } catch (Exception e) {
+                        LOG.error("Error invoke method", e);
                     }
                 });
     }
index c37c1c7..48d9d55 100644 (file)
@@ -393,7 +393,7 @@ public class UserSelfITCase extends AbstractITCase {
         }
 
         // 3. change password
-        vivaldiClient.getService(UserSelfService.class).changePassword("password123");
+        vivaldiClient.getService(UserSelfService.class).mustChangePassword("password123");
 
         // 4. verify it worked
         self = clientFactory.create("vivaldi", "password123").self();