Always commit user transactions on save when modifying the security model via the UI
authorBruno Sinou <bsinou@argeo.org>
Mon, 12 Sep 2016 12:08:04 +0000 (12:08 +0000)
committerBruno Sinou <bsinou@argeo.org>
Mon, 12 Sep 2016 12:08:04 +0000 (12:08 +0000)
git-svn-id: https://svn.argeo.org/commons/trunk@9113 4cfe0d0a-d680-48aa-b62c-e0a02a3f76cc

13 files changed:
org.argeo.security.ui.admin/plugin.xml
org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/UserAdminWrapper.java
org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/commands/DeleteGroups.java
org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/commands/DeleteUsers.java
org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/commands/NewGroup.java
org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/commands/NewUser.java
org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/commands/SaveArgeoUser.java
org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/commands/UserTransactionHandler.java
org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/parts/GroupMainPage.java
org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/parts/UserBatchUpdateWizard.java
org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/parts/UserEditor.java
org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/parts/UserMainPage.java
org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/providers/UserTransactionProvider.java

index 28ee5ee7ef54462e6f2a49cbd4cfa24fb75792da..2cf0ba210191824bea9c1854a47d791327b15bbf 100644 (file)
                                <parameter name="param.commandId" value="transaction.commit" />
                                <visibleWhen>
                                        <with variable="org.argeo.security.ui.admin.userTransactionState">
-                                                       <equals value="status.active" />
+                                               <equals value="status.active" />
                                        </with>
                                </visibleWhen>
                        </command>
index 8306e155234aedd1f6abc9c1643e40117922b65e..d443a3d47ce3c1a4b4e3c8109d7cc02334585796 100644 (file)
@@ -10,24 +10,27 @@ import org.argeo.cms.CmsException;
 import org.osgi.service.useradmin.UserAdminEvent;
 import org.osgi.service.useradmin.UserAdminListener;
 
-/** Centralize interaction with the UserAdmin in this bundle */
+/** Centralise interaction with the UserAdmin in this bundle */
 public class UserAdminWrapper extends
                org.argeo.cms.util.useradmin.UserAdminWrapper {
-       // private Log log = LogFactory.getLog(UserAdminWrapper.class);
+
+       // First effort to simplify UX while managing users and groups
+       public final static boolean COMMIT_ON_SAVE = true;
 
        // Registered listeners
        List<UserAdminListener> listeners = new ArrayList<UserAdminListener>();
 
        /**
-        * Overwrite the normal begin transaction behaviour to also notify the UI.
-        * Must be called from the UI Thread.
+        * Starts a transaction if necessary. Should always been called together
+        * with {@link UserAdminWrapper#commitOrNotifyTransactionStateChange()} once
+        * the security model changes have been performed.
         */
        public UserTransaction beginTransactionIfNeeded() {
                try {
                        UserTransaction userTransaction = getUserTransaction();
                        if (userTransaction.getStatus() == Status.STATUS_NO_TRANSACTION) {
                                userTransaction.begin();
-                               UiAdminUtils.notifyTransactionStateChange(userTransaction);
+                               // UiAdminUtils.notifyTransactionStateChange(userTransaction);
                        }
                        return userTransaction;
                } catch (Exception e) {
@@ -35,13 +38,33 @@ public class UserAdminWrapper extends
                }
        }
 
+       /**
+        * Depending on the current application configuration, it will either commit
+        * the current transaction or throw a notification that the transaction
+        * state has changed (In the later case, it must be called from the UI
+        * thread).
+        */
+       public void commitOrNotifyTransactionStateChange() {
+               try {
+                       UserTransaction userTransaction = getUserTransaction();
+                       if (userTransaction.getStatus() == Status.STATUS_NO_TRANSACTION)
+                               return;
+
+                       if (UserAdminWrapper.COMMIT_ON_SAVE)
+                               userTransaction.commit();
+                       else
+                               UiAdminUtils.notifyTransactionStateChange(userTransaction);
+               } catch (Exception e) {
+                       throw new CmsException("Unable to clean transaction", e);
+               }
+       }
+
        // TODO implement safer mechanism
        public void addListener(UserAdminListener userAdminListener) {
                if (!listeners.contains(userAdminListener))
                        listeners.add(userAdminListener);
        }
 
-       // Expose this?
        public void removeListener(UserAdminListener userAdminListener) {
                if (listeners.contains(userAdminListener))
                        listeners.remove(userAdminListener);
index 868aa0fc7f6a3239ad5591f1b2cb315d63f4b94b..df5d430abd1fad48dee70330d558819dec39c415 100644 (file)
@@ -71,10 +71,8 @@ public class DeleteGroups extends AbstractHandler {
                UserAdmin userAdmin = userAdminWrapper.getUserAdmin();
                IWorkbenchPage iwp = HandlerUtil.getActiveWorkbenchWindow(event)
                                .getActivePage();
-
                for (Group group : groups) {
                        String groupName = group.getName();
-
                        // TODO find a way to close the editor cleanly if opened. Cannot be
                        // done through the UserAdminListeners, it causes a
                        // java.util.ConcurrentModificationException because disposing the
@@ -82,11 +80,16 @@ public class DeleteGroups extends AbstractHandler {
                        IEditorPart part = iwp.findEditor(new UserEditorInput(groupName));
                        if (part != null)
                                iwp.closeEditor(part, false);
-
                        userAdmin.removeRole(groupName);
+               }
+               userAdminWrapper.commitOrNotifyTransactionStateChange();
+
+               // Update the view
+               for (Group group : groups) {
                        userAdminWrapper.notifyListeners(new UserAdminEvent(null,
                                        UserAdminEvent.ROLE_REMOVED, group));
                }
+
                return null;
        }
 
index 87da43cdf14f7495c14ee3ba45837baf9cb014d3..e583bef53bd610b5d99af00ab2266271536760d1 100644 (file)
@@ -81,7 +81,6 @@ public class DeleteUsers extends AbstractHandler {
 
                for (User user : users) {
                        String userName = user.getName();
-
                        // TODO find a way to close the editor cleanly if opened. Cannot be
                        // done through the UserAdminListeners, it causes a
                        // java.util.ConcurrentModificationException because disposing the
@@ -89,8 +88,11 @@ public class DeleteUsers extends AbstractHandler {
                        IEditorPart part = iwp.findEditor(new UserEditorInput(userName));
                        if (part != null)
                                iwp.closeEditor(part, false);
-
                        userAdmin.removeRole(userName);
+               }
+               userAdminWrapper.commitOrNotifyTransactionStateChange();
+
+               for (User user : users) {
                        userAdminWrapper.notifyListeners(new UserAdminEvent(null,
                                        UserAdminEvent.ROLE_REMOVED, user));
                }
index f075fbc31c3aed0929f3e1ef8dfa0621c88a4f40..75b9b0b33e4e52e06d675014c644d096a42c0d2e 100644 (file)
@@ -95,6 +95,7 @@ public class NewGroup extends AbstractHandler {
                                String descStr = descriptionTxt.getText();
                                if (EclipseUiUtils.notEmpty(descStr))
                                        props.put(LdifName.description.name(), descStr);
+                               userAdminWrapper.commitOrNotifyTransactionStateChange();
                                userAdminWrapper.notifyListeners(new UserAdminEvent(null,
                                                UserAdminEvent.ROLE_CREATED, group));
                                return true;
index 1a85253ad9238a4e83cc0a428c93f7141d72e19f..c04c83562f3cad02fdbbed82a40b5eedf4cd728e 100644 (file)
@@ -106,8 +106,8 @@ public class NewUser extends AbstractHandler {
                        if (!canFinish())
                                return false;
                        String username = mainUserInfo.getUsername();
+                       userAdminWrapper.beginTransactionIfNeeded();
                        try {
-                               userAdminWrapper.beginTransactionIfNeeded();
                                User user = (User) userAdminWrapper.getUserAdmin().createRole(
                                                getDn(username), Role.USER);
 
@@ -132,7 +132,7 @@ public class NewUser extends AbstractHandler {
 
                                char[] password = mainUserInfo.getPassword();
                                user.getCredentials().put(null, password);
-
+                               userAdminWrapper.commitOrNotifyTransactionStateChange();
                                userAdminWrapper.notifyListeners(new UserAdminEvent(null,
                                                UserAdminEvent.ROLE_CREATED, user));
                                return true;
index 131a7c4e3a06dc4111a0b5d146a4b9f33ea4458a..61d8a7daf3c422166c16a43138f92ee6c97d8a03 100644 (file)
@@ -34,7 +34,6 @@ public class SaveArgeoUser extends AbstractHandler {
                try {
                        IWorkbenchPart iwp = HandlerUtil.getActiveWorkbenchWindow(event)
                                        .getActivePage().getActivePart();
-
                        if (!(iwp instanceof IEditorPart))
                                return null;
                        IEditorPart editor = (IEditorPart) iwp;
index 27b530f0743399e6f8804beb5de402159831c62d..236584ca3fcf16bb0a68403f8d65c764b01bdb64 100644 (file)
@@ -69,13 +69,13 @@ public class UserTransactionHandler extends AbstractHandler {
                        UiAdminUtils.notifyTransactionStateChange(userTransaction);
                        // Try to remove invalid thread access errors when managing users.
                        // HandlerUtil.getActivePart(event).getSite().getShell().getDisplay()
-                       //              .asyncExec(new Runnable() {
-                       //                      @Override
-                       //                      public void run() {
-                       //                              UiAdminUtils
-                       //                                              .notifyTransactionStateChange(userTransaction);
-                       //                      }
-                       //              });
+                       // .asyncExec(new Runnable() {
+                       // @Override
+                       // public void run() {
+                       // UiAdminUtils
+                       // .notifyTransactionStateChange(userTransaction);
+                       // }
+                       // });
 
                } catch (CmsException e) {
                        throw e;
index 4a441a1a2d4a3c19651d7dfef7ba4e28528150bb..fe56679db3e6c2b70982e8ad406f49393c98b444 100644 (file)
@@ -19,6 +19,9 @@ import java.util.ArrayList;
 import java.util.Iterator;
 import java.util.List;
 
+import javax.transaction.UserTransaction;
+
+import org.argeo.cms.CmsException;
 import org.argeo.cms.util.useradmin.UserAdminUtils;
 import org.argeo.eclipse.ui.ColumnDefinition;
 import org.argeo.eclipse.ui.EclipseUiUtils;
@@ -131,7 +134,8 @@ public class GroupMainPage extends FormPage implements ArgeoNames {
                        @Override
                        public void initialize(IManagedForm form) {
                                super.initialize(form);
-                               listener = editor.new MainInfoListener(parent.getDisplay(), this);
+                               listener = editor.new MainInfoListener(parent.getDisplay(),
+                                               this);
                                userAdminWrapper.addListener(listener);
                        }
 
@@ -294,11 +298,8 @@ public class GroupMainPage extends FormPage implements ArgeoNames {
                        @SuppressWarnings("unchecked")
                        Iterator<User> it = ((IStructuredSelection) selection).iterator();
                        List<User> users = new ArrayList<User>();
-                       // StringBuilder builder = new StringBuilder();
                        while (it.hasNext()) {
                                User currUser = it.next();
-                               // String groupName = UserAdminUtils.getUsername(currGroup);
-                               // builder.append(groupName).append("; ");
                                users.add(currUser);
                        }
 
@@ -306,6 +307,7 @@ public class GroupMainPage extends FormPage implements ArgeoNames {
                        for (User user : users) {
                                group.removeMember(user);
                        }
+                       userAdminWrapper.commitOrNotifyTransactionStateChange();
                        userAdminWrapper.notifyListeners(new UserAdminEvent(null,
                                        UserAdminEvent.ROLE_CHANGED, group));
                }
@@ -328,7 +330,8 @@ public class GroupMainPage extends FormPage implements ArgeoNames {
                @Override
                public void initialize(IManagedForm form) {
                        super.initialize(form);
-                       listener = editor.new GroupChangeListener(userViewer.getDisplay(), GroupMembersPart.this);
+                       listener = editor.new GroupChangeListener(userViewer.getDisplay(),
+                                       GroupMembersPart.this);
                        userAdminWrapper.addListener(listener);
                }
 
@@ -414,15 +417,23 @@ public class GroupMainPage extends FormPage implements ArgeoNames {
                                        return;
                                }
                                userAdminWrapper.beginTransactionIfNeeded();
-                               // TODO implement the dirty state
                                myGroup.addMember(newGroup);
+                               userAdminWrapper.commitOrNotifyTransactionStateChange();
                                userAdminWrapper.notifyListeners(new UserAdminEvent(null,
                                                UserAdminEvent.ROLE_CHANGED, myGroup));
                        } else if (role.getType() == Role.USER) {
                                // TODO check if the group is already member of this group
-                               userAdminWrapper.beginTransactionIfNeeded();
+                               UserTransaction transaction = userAdminWrapper
+                                               .beginTransactionIfNeeded();
                                User user = (User) role;
                                myGroup.addMember(user);
+                               if (UserAdminWrapper.COMMIT_ON_SAVE)
+                                       try {
+                                               transaction.commit();
+                                       } catch (Exception e) {
+                                               throw new CmsException("Cannot commit transaction "
+                                                               + "after user group membership update", e);
+                                       }
                                userAdminWrapper.notifyListeners(new UserAdminEvent(null,
                                                UserAdminEvent.ROLE_CHANGED, myGroup));
                        }
index 80c817ebeb6b993d5321fbd085fce05adec89fd5..31b2042b6ec14d2ec91eabefc5f43d2af0697c50 100644 (file)
@@ -18,7 +18,6 @@ import org.argeo.eclipse.ui.EclipseUiUtils;
 import org.argeo.eclipse.ui.parts.LdifUsersTable;
 import org.argeo.jcr.ArgeoNames;
 import org.argeo.osgi.useradmin.LdifName;
-import org.argeo.security.ui.admin.internal.UiAdminUtils;
 import org.argeo.security.ui.admin.internal.UserAdminWrapper;
 import org.argeo.security.ui.admin.internal.providers.CommonNameLP;
 import org.argeo.security.ui.admin.internal.providers.DomainNameLP;
@@ -140,18 +139,16 @@ public class UserBatchUpdateWizard extends Wizard {
 
                @SuppressWarnings("unchecked")
                protected void doUpdate() {
-                       UserTransaction userTransaction = userAdminWrapper
-                                       .beginTransactionIfNeeded();
+                       userAdminWrapper.beginTransactionIfNeeded();
                        try {
                                for (User user : usersToUpdate) {
                                        // the char array is emptied after being used.
                                        user.getCredentials().put(null, newPwd.clone());
                                }
-                               userTransaction.commit();
-                               UiAdminUtils.notifyTransactionStateChange(userTransaction);
+                               userAdminWrapper.commitOrNotifyTransactionStateChange();
                        } catch (Exception e) {
-                               throw new CmsException(
-                                               "Cannot perform batch update on users", e);
+                               throw new CmsException("Cannot perform batch update on users",
+                                               e);
                        } finally {
                                UserTransaction ut = userAdminWrapper.getUserTransaction();
                                try {
@@ -495,8 +492,8 @@ public class UserBatchUpdateWizard extends Wizard {
                                        roles = userAdminWrapper.getUserAdmin().getRoles(
                                                        builder.toString());
                                } catch (InvalidSyntaxException e) {
-                                       throw new CmsException(
-                                                       "Unable to get roles with filter: " + filter, e);
+                                       throw new CmsException("Unable to get roles with filter: "
+                                                       + filter, e);
                                }
                                List<User> users = new ArrayList<User>();
                                for (Role role : roles)
index 0fae9f49afa6184b423a0fa209b211bc619faccf..6c0731d01d26902bfc01c21df9be724039c6e7a2 100644 (file)
@@ -121,6 +121,7 @@ public class UserEditor extends FormEditor {
        public void doSave(IProgressMonitor monitor) {
                userAdminWrapper.beginTransactionIfNeeded();
                commitPages(true);
+               userAdminWrapper.commitOrNotifyTransactionStateChange();
                firePropertyChange(PROP_DIRTY);
                userAdminWrapper.notifyListeners(new UserAdminEvent(null,
                                UserAdminEvent.ROLE_REMOVED, user));
index 39a80a2e75c1e83c66a87cc0177f8b69ca3572d7..db6eb538fe059635393edfa3781e20bb4055c917 100644 (file)
@@ -228,7 +228,7 @@ public class UserMainPage extends FormPage implements ArgeoNames {
                                                || !password2.getText().equals("")) {
                                        if (password1.getText().equals(password2.getText())) {
                                                char[] newPassword = password1.getText().toCharArray();
-                                               userAdminWrapper.beginTransactionIfNeeded();
+                                               // userAdminWrapper.beginTransactionIfNeeded();
                                                user.getCredentials().put(null, newPassword);
                                                password1.setText("");
                                                password2.setText("");
@@ -416,25 +416,17 @@ public class UserMainPage extends FormPage implements ArgeoNames {
                        @SuppressWarnings("unchecked")
                        Iterator<Group> it = ((IStructuredSelection) selection).iterator();
                        List<Group> groups = new ArrayList<Group>();
-                       // StringBuilder builder = new StringBuilder();
                        while (it.hasNext()) {
                                Group currGroup = it.next();
-                               // String groupName = UserAdminUtils.getUsername(currGroup);
-                               // builder.append(groupName).append("; ");
                                groups.add(currGroup);
                        }
 
-                       // if (!MessageDialog.openQuestion(
-                       // HandlerUtil.getActiveShell(event),
-                       // "Re",
-                       // "Are you sure that you want to delete these users?\n"
-                       // + builder.substring(0, builder.length() - 2)))
-                       // return null;
-
                        userAdminWrapper.beginTransactionIfNeeded();
                        for (Group group : groups) {
                                group.removeMember(user);
-                               // sectionPart.refresh();
+                       }
+                       userAdminWrapper.commitOrNotifyTransactionStateChange();
+                       for (Group group : groups) {
                                userAdminWrapper.notifyListeners(new UserAdminEvent(null,
                                                UserAdminEvent.ROLE_CHANGED, group));
                        }
@@ -476,17 +468,10 @@ public class UserMainPage extends FormPage implements ArgeoNames {
                        if (role.getType() == Role.GROUP) {
                                // TODO check if the user is already member of this group
 
-                               // Remove invalid thread access errors when managing users.
-                               // myUserAdminWrapper.beginTransactionIfNeeded();
-                               event.display.asyncExec(new Runnable() {
-                                       @Override
-                                       public void run() {
-                                               myUserAdminWrapper.beginTransactionIfNeeded();
-                                       }
-                               });
-
+                               myUserAdminWrapper.beginTransactionIfNeeded();
                                Group group = (Group) role;
                                group.addMember(myUser);
+                               userAdminWrapper.commitOrNotifyTransactionStateChange();
                                myUserAdminWrapper.notifyListeners(new UserAdminEvent(null,
                                                UserAdminEvent.ROLE_CHANGED, group));
                        }
index 8c8fe7234827afd70c1bc79b305595bad58a6fef..4ba304bb5f09082896ad951cf0befe1ce2e7919a 100644 (file)
@@ -28,13 +28,13 @@ public class UserTransactionProvider extends AbstractSourceProvider {
 
        @Override
        public String[] getProvidedSourceNames() {
-               return new String[] { TRANSACTION_STATE };
+               return new String[] { TRANSACTION_STATE};
        }
 
        @Override
        public Map<String, String> getCurrentState() {
                Map<String, String> currentState = new HashMap<String, String>(1);
-               currentState.put(TRANSACTION_STATE, getInternalCurrentState());
+                       currentState.put(TRANSACTION_STATE, getInternalCurrentState());
                return currentState;
        }