FELIX-3755: removing roles did not remove them as group member;
- remove the role from all groups it is member of;
- some additional cleanups and unused code removed.
git-svn-id: https://svn.apache.org/repos/asf/felix/trunk@1409119 13f79535-47bb-0310-9956-ffa450edef68
diff --git a/useradmin/useradmin/src/main/java/org/apache/felix/useradmin/impl/AuthorizationImpl.java b/useradmin/useradmin/src/main/java/org/apache/felix/useradmin/impl/AuthorizationImpl.java
index 2234ebc..0c93c19 100644
--- a/useradmin/useradmin/src/main/java/org/apache/felix/useradmin/impl/AuthorizationImpl.java
+++ b/useradmin/useradmin/src/main/java/org/apache/felix/useradmin/impl/AuthorizationImpl.java
@@ -21,8 +21,6 @@
import java.util.Iterator;
import java.util.List;
-
-import org.apache.felix.useradmin.impl.role.RoleImpl;
import org.osgi.service.useradmin.Authorization;
import org.osgi.service.useradmin.Role;
import org.osgi.service.useradmin.User;
@@ -86,7 +84,7 @@
Iterator rolesIter = m_roleManager.getRoles(null /* filter */).iterator();
while (rolesIter.hasNext()) {
- RoleImpl role = (RoleImpl) rolesIter.next();
+ Role role = (Role) rolesIter.next();
if (!Role.USER_ANYONE.equals(role.getName()) && m_roleChecker.isImpliedBy(role, m_user)) {
result.add(role.getName());
}
diff --git a/useradmin/useradmin/src/main/java/org/apache/felix/useradmin/impl/RoleChecker.java b/useradmin/useradmin/src/main/java/org/apache/felix/useradmin/impl/RoleChecker.java
index ced47ba..f0c4188 100644
--- a/useradmin/useradmin/src/main/java/org/apache/felix/useradmin/impl/RoleChecker.java
+++ b/useradmin/useradmin/src/main/java/org/apache/felix/useradmin/impl/RoleChecker.java
@@ -19,11 +19,8 @@
import java.util.ArrayList;
import java.util.List;
-
-import org.apache.felix.useradmin.impl.role.UserImpl;
import org.osgi.service.useradmin.Group;
import org.osgi.service.useradmin.Role;
-import org.osgi.service.useradmin.User;
/**
* Helper class to check for implied role memberships.
@@ -40,9 +37,7 @@
public boolean isImpliedBy(Role role, Role impliedRole) {
if (role instanceof Group) {
return isGroupImpliedBy((Group) role, impliedRole, new ArrayList());
- } else if (role instanceof User) {
- return isUserImpliedBy((User) role, impliedRole);
- } else {
+ } else /* if ((role instanceof User) || (role instanceof Role)) */ {
return isRoleImpliedBy(role, impliedRole);
}
}
@@ -72,10 +67,8 @@
if (requiredRole instanceof Group) {
seenGroups.add(requiredRole);
isImplied = isGroupImpliedBy((Group) requiredRole, impliedRole, seenGroups);
- } else if (requiredRole instanceof User) {
- isImplied = isUserImpliedBy((User) requiredRole, impliedRole);
- } else /* if (requiredRoles[i] instanceof RoleImpl) */ {
- isImplied = isRoleImpliedBy(requiredRole, impliedRole);
+ } else /* if ((requiredRole instanceof User) || (requiredRole instanceof Role)) */ {
+ isImplied = isRoleImpliedBy(requiredRole, impliedRole);
}
}
@@ -98,9 +91,7 @@
if (basicRole instanceof Group) {
seenGroups.add(basicRole);
isImplied = isGroupImpliedBy((Group) basicRole, impliedRole, seenGroups);
- } else if (basicRole instanceof UserImpl) {
- isImplied = isUserImpliedBy((User) basicRole, impliedRole);
- } else /* if (requiredRoles[i] instanceof RoleImpl) */ {
+ } else /* if ((basicRole instanceof User) || (basicRole instanceof Role)) */ {
isImplied = isRoleImpliedBy(basicRole, impliedRole);
}
}
@@ -109,17 +100,6 @@
}
/**
- * Verifies whether the given user is implied by the given role.
- *
- * @param user the user to check, cannot be <code>null</code>;
- * @param impliedRole the implied role to check for, cannot be <code>null</code>;
- * @return <code>true</code> if the given user is implied by the given role, <code>false</code> otherwise.
- */
- private boolean isUserImpliedBy(User user, Role impliedRole) {
- return Role.USER_ANYONE.equals(user.getName()) || (impliedRole != null && impliedRole.getName().equals(user.getName()));
- }
-
- /**
* Verifies whether the given role is implied by the given role.
*
* @param role the role to check, cannot be <code>null</code>;
diff --git a/useradmin/useradmin/src/main/java/org/apache/felix/useradmin/impl/RoleRepository.java b/useradmin/useradmin/src/main/java/org/apache/felix/useradmin/impl/RoleRepository.java
index 2348b55..b9666db 100644
--- a/useradmin/useradmin/src/main/java/org/apache/felix/useradmin/impl/RoleRepository.java
+++ b/useradmin/useradmin/src/main/java/org/apache/felix/useradmin/impl/RoleRepository.java
@@ -29,6 +29,7 @@
import org.apache.felix.useradmin.RoleRepositoryStore;
import org.apache.felix.useradmin.impl.role.RoleImpl;
import org.osgi.framework.Filter;
+import org.osgi.service.useradmin.Group;
import org.osgi.service.useradmin.Role;
import org.osgi.service.useradmin.UserAdminPermission;
@@ -44,33 +45,23 @@
/**
* {@inheritDoc}
*/
- public void roleAdded(Role role) {
- Iterator iterator = createListenerIterator();
- while (iterator.hasNext()) {
- ((RoleChangeListener) iterator.next()).roleAdded(role);
- }
- }
-
- /**
- * {@inheritDoc}
- */
- public void roleRemoved(Role role) {
- Iterator iterator = createListenerIterator();
- while (iterator.hasNext()) {
- ((RoleChangeListener) iterator.next()).roleRemoved(role);
- }
- }
-
- /**
- * {@inheritDoc}
- */
public void propertyAdded(Role role, Object key, Object value) {
Iterator iterator = createListenerIterator();
while (iterator.hasNext()) {
((RoleChangeListener) iterator.next()).propertyAdded(role, key, value);
}
}
-
+
+ /**
+ * {@inheritDoc}
+ */
+ public void propertyChanged(Role role, Object key, Object oldValue, Object newValue) {
+ Iterator iterator = createListenerIterator();
+ while (iterator.hasNext()) {
+ ((RoleChangeListener) iterator.next()).propertyChanged(role, key, oldValue, newValue);
+ }
+ }
+
/**
* {@inheritDoc}
*/
@@ -84,10 +75,20 @@
/**
* {@inheritDoc}
*/
- public void propertyChanged(Role role, Object key, Object oldValue, Object newValue) {
+ public void roleAdded(Role role) {
Iterator iterator = createListenerIterator();
while (iterator.hasNext()) {
- ((RoleChangeListener) iterator.next()).propertyChanged(role, key, oldValue, newValue);
+ ((RoleChangeListener) iterator.next()).roleAdded(role);
+ }
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ public void roleRemoved(Role role) {
+ Iterator iterator = createListenerIterator();
+ while (iterator.hasNext()) {
+ ((RoleChangeListener) iterator.next()).roleRemoved(role);
}
}
}
@@ -251,6 +252,9 @@
try {
if (m_store.removeRole(role)) {
+ // FELIX-3755: Remove the role as (required)member from all groups...
+ removeRoleFromAllGroups(role);
+
unwireChangeListener(role);
m_roleChangeReflector.roleRemoved(role);
@@ -278,7 +282,7 @@
m_listeners.remove(listener);
}
- /**
+ /**
* Starts this repository.
*/
public void start() {
@@ -292,7 +296,7 @@
e.printStackTrace();
}
}
-
+
/**
* Stops this repository, allowing it to clean up.
*/
@@ -304,7 +308,7 @@
// Ignore; nothing we can do about this here...
}
}
-
+
/**
* Creates a new iterator for iterating over all listeners.
*
@@ -325,7 +329,7 @@
securityManager.checkPermission(new UserAdminPermission(UserAdminPermission.ADMIN, null));
}
}
-
+
/**
* Returns whether or not the given role is a predefined role.
* <p>
@@ -338,6 +342,39 @@
private boolean isPredefinedRole(Role role) {
return Role.USER_ANYONE.equals(role.getName());
}
+
+ /**
+ * Removes a given role as (required)member from any groups it is member of.
+ *
+ * @param removedRole the role that is removed from the store already, cannot be <code>null</code>.
+ * @throws BackendException in case of problems accessing the store.
+ */
+ private void removeRoleFromAllGroups(Role removedRole) {
+ try {
+ Role[] roles = m_store.getAllRoles();
+ for (int i = 0; i < roles.length; i++) {
+ if (roles[i].getType() == Role.GROUP) {
+ Group group = (Group) roles[i];
+ // Don't check whether the given role is actually a member
+ // of the group, but let the group itself figure this out...
+ group.removeMember(removedRole);
+ }
+ }
+ }
+ catch (IOException e) {
+ throw new BackendException("Failed to get all roles!", e);
+ }
+ }
+
+ /**
+ * Unwires the given role to this repository so it no longer listens for its changes.
+ *
+ * @param role the role to unwire, cannot be <code>null</code>.
+ * @throws IllegalArgumentException in case the given object was not a {@link RoleImpl} instance.
+ */
+ private void unwireChangeListener(Object role) {
+ ((RoleImpl) role).setRoleChangeListener(null);
+ }
/**
* Wires the given role to this repository so it can listen for its changes.
@@ -353,15 +390,4 @@
}
return result;
}
-
- /**
- * Unwires the given role to this repository so it no longer listens for its changes.
- *
- * @param role the role to unwire, cannot be <code>null</code>.
- * @throws IllegalArgumentException in case the given object was not a {@link RoleImpl} instance.
- */
- private void unwireChangeListener(Object role) {
- RoleImpl result = (RoleImpl) role;
- result.setRoleChangeListener(null);
- }
}
diff --git a/useradmin/useradmin/src/main/java/org/apache/felix/useradmin/impl/role/GroupImpl.java b/useradmin/useradmin/src/main/java/org/apache/felix/useradmin/impl/role/GroupImpl.java
index 9346836..497cbce 100644
--- a/useradmin/useradmin/src/main/java/org/apache/felix/useradmin/impl/role/GroupImpl.java
+++ b/useradmin/useradmin/src/main/java/org/apache/felix/useradmin/impl/role/GroupImpl.java
@@ -17,7 +17,6 @@
package org.apache.felix.useradmin.impl.role;
import java.util.Collection;
-import java.util.Dictionary;
import java.util.HashMap;
import java.util.Map;
@@ -53,29 +52,19 @@
}
/**
- * Creates a new {@link GroupImpl} instance of type {@link Role#GROUP}.
- *
- * @param name the name of this group role, cannot be <code>null</code> or empty.
- */
- public GroupImpl(String name, Dictionary properties, Dictionary credentials) {
- super(Role.GROUP, name, properties, credentials);
-
- m_members = new HashMap();
- m_requiredMembers = new HashMap();
- }
-
- /**
* {@inheritDoc}
*/
public boolean addMember(Role role) {
checkPermissions();
+ String name = role.getName();
+
Object result;
synchronized (m_lock) {
- if (m_members.containsKey(role.getName()) || m_requiredMembers.containsKey(role.getName())) {
+ if (m_members.containsKey(name) || m_requiredMembers.containsKey(name)) {
return false;
}
- result = m_members.put(role.getName(), role);
+ result = m_members.put(name, role);
}
if (result == null) {
@@ -92,12 +81,14 @@
public boolean addRequiredMember(Role role) {
checkPermissions();
+ String name = role.getName();
+
Object result;
synchronized (m_lock) {
- if (m_requiredMembers.containsKey(role.getName()) || m_members.containsKey(role.getName())) {
+ if (m_requiredMembers.containsKey(name) || m_members.containsKey(name)) {
return false;
}
- result = m_requiredMembers.put(role.getName(), role);
+ result = m_requiredMembers.put(name, role);
}
if (result == null) {
@@ -140,17 +131,19 @@
public boolean removeMember(Role role) {
checkPermissions();
+ String name = role.getName();
+
String key = null;
Object result = null;
synchronized (m_lock) {
- if (m_requiredMembers.containsKey(role.getName())) {
+ if (m_requiredMembers.containsKey(name)) {
key = REQUIRED_MEMBER;
- result = m_requiredMembers.remove(role.getName());
+ result = m_requiredMembers.remove(name);
}
- else if (m_members.containsKey(role.getName())) {
+ else if (m_members.containsKey(name)) {
key = BASIC_MEMBER;
- result = m_members.remove(role.getName());
+ result = m_members.remove(name);
}
}
diff --git a/useradmin/useradmin/src/main/java/org/apache/felix/useradmin/impl/role/RoleImpl.java b/useradmin/useradmin/src/main/java/org/apache/felix/useradmin/impl/role/RoleImpl.java
index be153d0..576eee2 100644
--- a/useradmin/useradmin/src/main/java/org/apache/felix/useradmin/impl/role/RoleImpl.java
+++ b/useradmin/useradmin/src/main/java/org/apache/felix/useradmin/impl/role/RoleImpl.java
@@ -64,23 +64,6 @@
}
/**
- * Creates a new {@link RoleImpl} instance with a given type, name and properties.
- *
- * @param type the type of this role, should be {@link Role#ROLE}, {@link Role#USER} or {@link Role#GROUP};
- * @param name the name of this role, cannot be <code>null</code> or empty;
- * @param properties the initial properties of this role, cannot be <code>null</code>.
- */
- protected RoleImpl(int type, String name, Dictionary properties) {
- if (name == null || "".equals(name.trim())) {
- throw new IllegalArgumentException("Name cannot be null or empty!");
- }
- m_type = type;
- m_name = name;
- m_properties = new ObservableProperties(null, UserAdminPermission.CHANGE_PROPERTY, properties);
- m_properties.setDictionaryChangeListener(this);
- }
-
- /**
* {@inheritDoc}
*/
public final void entryAdded(Object key, Object value) {
diff --git a/useradmin/useradmin/src/main/java/org/apache/felix/useradmin/impl/role/UserImpl.java b/useradmin/useradmin/src/main/java/org/apache/felix/useradmin/impl/role/UserImpl.java
index eaf2911..d152af3 100644
--- a/useradmin/useradmin/src/main/java/org/apache/felix/useradmin/impl/role/UserImpl.java
+++ b/useradmin/useradmin/src/main/java/org/apache/felix/useradmin/impl/role/UserImpl.java
@@ -55,18 +55,6 @@
}
/**
- * Creates a new {@link UserImpl} instance with type {@link Role#USER}.
- *
- * @param name the name of this user role, cannot be <code>null</code> or empty.
- */
- protected UserImpl(int type, String name, Dictionary properties, Dictionary credentials) {
- super(type, name, properties);
-
- m_credentials = new ObservableProperties(UserAdminPermission.GET_CREDENTIAL, UserAdminPermission.CHANGE_CREDENTIAL, credentials);
- m_credentials.setDictionaryChangeListener(this);
- }
-
- /**
* {@inheritDoc}
*/
public Dictionary getCredentials() {
@@ -89,7 +77,7 @@
} else if (value instanceof String) {
s2 = (String) value;
} else {
- // Not a string or a byte-array!
+ // Not a string, nor a byte-array!
return false;
}
@@ -102,7 +90,7 @@
} else if (value instanceof String) {
b2 = ((String) value).getBytes();
} else {
- // Not a string or a byte-array!
+ // Not a string, nor a byte-array!
return false;
}
diff --git a/useradmin/useradmin/src/test/java/org/apache/felix/useradmin/impl/UserAdminImplTest.java b/useradmin/useradmin/src/test/java/org/apache/felix/useradmin/impl/UserAdminImplTest.java
index 4d895bf..9bca211 100644
--- a/useradmin/useradmin/src/test/java/org/apache/felix/useradmin/impl/UserAdminImplTest.java
+++ b/useradmin/useradmin/src/test/java/org/apache/felix/useradmin/impl/UserAdminImplTest.java
@@ -245,19 +245,6 @@
}
/**
- * Tests that creating a role with an invalid type does not succeed and yields an exception.
- */
- public void testCreateInvalidRoleTypeFail() {
- try {
- m_userAdmin.createRole("role1", Role.ROLE);
-
- fail("Expected an IllegalArgumentException!");
- } catch (IllegalArgumentException e) {
- // Ok; expected
- }
- }
-
- /**
* Tests that creating a role without a name does not succeed and yields an exception.
*/
public void testCreateInvalidRoleNameFail() {
@@ -271,11 +258,11 @@
}
/**
- * Tests that creating a role without a name does not succeed and yields an exception.
+ * Tests that creating a role with an invalid type does not succeed and yields an exception.
*/
- public void testCreateRoleWithEmptyNameFail() {
+ public void testCreateInvalidRoleTypeFail() {
try {
- m_userAdmin.createRole("", Role.USER);
+ m_userAdmin.createRole("role1", Role.ROLE);
fail("Expected an IllegalArgumentException!");
} catch (IllegalArgumentException e) {
@@ -306,6 +293,19 @@
}
/**
+ * Tests that creating a role without a name does not succeed and yields an exception.
+ */
+ public void testCreateRoleWithEmptyNameFail() {
+ try {
+ m_userAdmin.createRole("", Role.USER);
+
+ fail("Expected an IllegalArgumentException!");
+ } catch (IllegalArgumentException e) {
+ // Ok; expected
+ }
+ }
+
+ /**
* Tests that creating a {@link UserAdminImpl} with a null event dispatcher fails.
*/
public void testCreateUserAdminImplWithNullDispatcherFail() {
@@ -690,6 +690,75 @@
}
/**
+ * Tests that remove of a role also removes that role as member from any group (FELIX-3755).
+ */
+ public void testRemoveRoleRemovesItAsGroupMemberOk() {
+ Role user1 = m_userAdmin.createRole("user1", Role.USER);
+ Role user2 = m_userAdmin.createRole("user2", Role.USER);
+
+ Group group1 = (Group) m_userAdmin.createRole("group1", Role.GROUP);
+ group1.addMember(user1);
+
+ Group group2 = (Group) m_userAdmin.createRole("group2", Role.GROUP);
+ group2.addMember(user1);
+ group2.addMember(user2);
+
+ // Remove user...
+ m_userAdmin.removeRole(user1.getName());
+
+ // Retrieve an up-to-date instance of the first group...
+ group1 = (Group) m_userAdmin.getRole("group1");
+ assertNull(group1.getMembers());
+
+ // Retrieve an up-to-date instance of the second group...
+ group2 = (Group) m_userAdmin.getRole("group2");
+
+ Role[] members = group2.getMembers();
+ assertNotNull(members);
+ assertEquals(1, members.length);
+ assertEquals(user2, members[0]);
+ }
+
+ /**
+ * Tests that remove of a role also removes that role as required member from any group (FELIX-3755).
+ */
+ public void testRemoveRoleRemovesItAsRequiredGroupMemberOk() {
+ Role user1 = m_userAdmin.createRole("user1", Role.USER);
+ Role user2 = m_userAdmin.createRole("user2", Role.USER);
+
+ Group group1 = (Group) m_userAdmin.createRole("group1", Role.GROUP);
+ group1.addRequiredMember(user1);
+ group1.addMember(user2);
+
+ Group group2 = (Group) m_userAdmin.createRole("group2", Role.GROUP);
+ group2.addRequiredMember(user1);
+ group2.addRequiredMember(user2);
+
+ // Remove user...
+ m_userAdmin.removeRole(user1.getName());
+
+ // Retrieve an up-to-date instance of the group...
+ group1 = (Group) m_userAdmin.getRole("group1");
+
+ assertNull(group1.getRequiredMembers());
+
+ Role[] members = group1.getMembers();
+ assertNotNull(members);
+ assertEquals(1, members.length);
+ assertEquals(user2, members[0]);
+
+ // Retrieve an up-to-date instance of the group...
+ group2 = (Group) m_userAdmin.getRole("group2");
+
+ assertNull(group2.getMembers());
+
+ members = group2.getRequiredMembers();
+ assertNotNull(members);
+ assertEquals(1, members.length);
+ assertEquals(user2, members[0]);
+ }
+
+ /**
* Tests that remove a credential of a user works.
*/
public void testRemoveUserCredentialOk() {