From c13da2a3a91edd21b54db7563727d009def37d3e Mon Sep 17 00:00:00 2001 From: Bruno Sinou Date: Mon, 12 Sep 2016 12:08:04 +0000 Subject: [PATCH] Always commit user transactions on save when modifying the security model via the UI git-svn-id: https://svn.argeo.org/commons/trunk@9113 4cfe0d0a-d680-48aa-b62c-e0a02a3f76cc --- org.argeo.security.ui.admin/plugin.xml | 2 +- .../ui/admin/internal/UserAdminWrapper.java | 35 +++++++++++++++---- .../admin/internal/commands/DeleteGroups.java | 9 +++-- .../admin/internal/commands/DeleteUsers.java | 6 ++-- .../ui/admin/internal/commands/NewGroup.java | 1 + .../ui/admin/internal/commands/NewUser.java | 4 +-- .../internal/commands/SaveArgeoUser.java | 1 - .../commands/UserTransactionHandler.java | 14 ++++---- .../admin/internal/parts/GroupMainPage.java | 25 +++++++++---- .../internal/parts/UserBatchUpdateWizard.java | 15 ++++---- .../ui/admin/internal/parts/UserEditor.java | 1 + .../ui/admin/internal/parts/UserMainPage.java | 27 ++++---------- .../providers/UserTransactionProvider.java | 4 +-- 13 files changed, 83 insertions(+), 61 deletions(-) diff --git a/org.argeo.security.ui.admin/plugin.xml b/org.argeo.security.ui.admin/plugin.xml index 28ee5ee7e..2cf0ba210 100644 --- a/org.argeo.security.ui.admin/plugin.xml +++ b/org.argeo.security.ui.admin/plugin.xml @@ -112,7 +112,7 @@ - + diff --git a/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/UserAdminWrapper.java b/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/UserAdminWrapper.java index 8306e1552..d443a3d47 100644 --- a/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/UserAdminWrapper.java +++ b/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/UserAdminWrapper.java @@ -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 listeners = new ArrayList(); /** - * 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); diff --git a/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/commands/DeleteGroups.java b/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/commands/DeleteGroups.java index 868aa0fc7..df5d430ab 100644 --- a/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/commands/DeleteGroups.java +++ b/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/commands/DeleteGroups.java @@ -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; } diff --git a/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/commands/DeleteUsers.java b/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/commands/DeleteUsers.java index 87da43cdf..e583bef53 100644 --- a/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/commands/DeleteUsers.java +++ b/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/commands/DeleteUsers.java @@ -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)); } diff --git a/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/commands/NewGroup.java b/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/commands/NewGroup.java index f075fbc31..75b9b0b33 100644 --- a/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/commands/NewGroup.java +++ b/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/commands/NewGroup.java @@ -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; diff --git a/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/commands/NewUser.java b/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/commands/NewUser.java index 1a85253ad..c04c83562 100644 --- a/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/commands/NewUser.java +++ b/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/commands/NewUser.java @@ -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; diff --git a/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/commands/SaveArgeoUser.java b/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/commands/SaveArgeoUser.java index 131a7c4e3..61d8a7daf 100644 --- a/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/commands/SaveArgeoUser.java +++ b/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/commands/SaveArgeoUser.java @@ -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; diff --git a/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/commands/UserTransactionHandler.java b/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/commands/UserTransactionHandler.java index 27b530f07..236584ca3 100644 --- a/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/commands/UserTransactionHandler.java +++ b/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/commands/UserTransactionHandler.java @@ -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; diff --git a/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/parts/GroupMainPage.java b/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/parts/GroupMainPage.java index 4a441a1a2..fe56679db 100644 --- a/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/parts/GroupMainPage.java +++ b/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/parts/GroupMainPage.java @@ -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 it = ((IStructuredSelection) selection).iterator(); List users = new ArrayList(); - // 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)); } diff --git a/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/parts/UserBatchUpdateWizard.java b/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/parts/UserBatchUpdateWizard.java index 80c817ebe..31b2042b6 100644 --- a/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/parts/UserBatchUpdateWizard.java +++ b/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/parts/UserBatchUpdateWizard.java @@ -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 users = new ArrayList(); for (Role role : roles) diff --git a/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/parts/UserEditor.java b/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/parts/UserEditor.java index 0fae9f49a..6c0731d01 100644 --- a/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/parts/UserEditor.java +++ b/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/parts/UserEditor.java @@ -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)); diff --git a/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/parts/UserMainPage.java b/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/parts/UserMainPage.java index 39a80a2e7..db6eb538f 100644 --- a/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/parts/UserMainPage.java +++ b/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/parts/UserMainPage.java @@ -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 it = ((IStructuredSelection) selection).iterator(); List groups = new ArrayList(); - // 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)); } diff --git a/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/providers/UserTransactionProvider.java b/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/providers/UserTransactionProvider.java index 8c8fe7234..4ba304bb5 100644 --- a/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/providers/UserTransactionProvider.java +++ b/org.argeo.security.ui.admin/src/org/argeo/security/ui/admin/internal/providers/UserTransactionProvider.java @@ -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 getCurrentState() { Map currentState = new HashMap(1); - currentState.put(TRANSACTION_STATE, getInternalCurrentState()); + currentState.put(TRANSACTION_STATE, getInternalCurrentState()); return currentState; } -- 2.30.2