From 3c0a1450eec516f77e500681e8f41db5d13eeabe Mon Sep 17 00:00:00 2001 From: Henrik Nyman Date: Tue, 24 May 2016 16:50:06 +0200 Subject: [PATCH] Make role management atomic - Add com.googlecode.thread-weaver for concurrency tests to deterministically test concurrent updates with a sequential flow - Add atomicity tests for addUserToRole and deleteUser - Share common functionality in AbstractRoleRepository (this also increases test coverage) --- enterprise/security/pom.xml | 21 ++ .../auth/AbstractRoleRepository.java | 218 ++++++++++++++++++ .../enterprise/auth/FileRoleRepository.java | 167 +------------- .../enterprise/auth/FileUserRealm.java | 72 ++++-- .../security/enterprise/auth/RoleManager.java | 16 ++ .../security/enterprise/auth/RoleRecord.java | 3 +- .../enterprise/auth/RoleRepository.java | 4 +- .../enterprise/auth/RoleSerialization.java | 4 +- .../enterprise/auth/ShiroAuthManager.java | 10 + .../enterprise/auth/FileUserRealmTest.java | 198 ++++++++++++++++ .../auth/InMemoryRoleRepository.java | 128 +--------- 11 files changed, 529 insertions(+), 312 deletions(-) create mode 100644 enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/AbstractRoleRepository.java create mode 100644 enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/FileUserRealmTest.java diff --git a/enterprise/security/pom.xml b/enterprise/security/pom.xml index db8d22a8f88bd..0f0f7d13f49c5 100644 --- a/enterprise/security/pom.xml +++ b/enterprise/security/pom.xml @@ -104,6 +104,27 @@ 1.0 test + + + com.googlecode.thread-weaver + threadweaver + 0.2 + test + + + + org.slf4j + slf4j-nop + 1.7.21 + test + + + + org.apache.commons + commons-lang3 + 3.4 + test + diff --git a/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/AbstractRoleRepository.java b/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/AbstractRoleRepository.java new file mode 100644 index 0000000000000..d71dfb10be211 --- /dev/null +++ b/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/AbstractRoleRepository.java @@ -0,0 +1,218 @@ +/* + * Copyright (c) 2002-2016 "Neo Technology," + * Network Engine for Objects in Lund AB [http://neotechnology.com] + * + * This file is part of Neo4j. + * + * Neo4j is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ +package org.neo4j.server.security.enterprise.auth; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.SortedSet; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentSkipListSet; + +import org.neo4j.kernel.lifecycle.LifecycleAdapter; +import org.neo4j.server.security.auth.exception.ConcurrentModificationException; + +public abstract class AbstractRoleRepository extends LifecycleAdapter implements RoleRepository +{ + // TODO: We could improve concurrency by using a ReadWriteLock + + /** Quick lookup of roles by name */ + protected final Map rolesByName = new ConcurrentHashMap<>(); + private final Map> rolesByUsername = new ConcurrentHashMap<>(); + + /** Master list of roles */ + protected volatile List roles = new ArrayList<>(); + + @Override + public RoleRecord findByName( String name ) + { + return rolesByName.get( name ); + } + + @Override + public Set findByUsername( String username ) + { + return rolesByUsername.get( username ); + } + + @Override + public void create( RoleRecord role ) throws IllegalArgumentException, IOException + { + if ( !isValidName( role.name() ) ) + { + throw new IllegalArgumentException( "'" + role.name() + "' is not a valid role name." ); + } + + synchronized (this) + { + // Check for existing role + for ( RoleRecord other : roles ) + { + if ( other.name().equals( role.name() ) ) + { + throw new IllegalArgumentException( "The specified role already exists" ); + } + } + + roles.add( role ); + + saveRoles(); + + rolesByName.put( role.name(), role ); + + populateUserMap( role ); + } + } + + @Override + public void update( RoleRecord existingRole, RoleRecord updatedRole ) throws ConcurrentModificationException, IOException + { + // Assert input is ok + if ( !existingRole.name().equals( updatedRole.name() ) ) + { + throw new IllegalArgumentException( "updated role has a different name" ); + } + + synchronized (this) + { + // Copy-on-write for the roles list + List newRoles = new ArrayList<>(); + boolean foundRole = false; + for ( RoleRecord other : roles ) + { + if ( other.equals( existingRole ) ) + { + foundRole = true; + newRoles.add( updatedRole ); + } else + { + newRoles.add( other ); + } + } + + if ( !foundRole ) + { + throw new ConcurrentModificationException(); + } + + roles = newRoles; + + saveRoles(); + + rolesByName.put( updatedRole.name(), updatedRole ); + + removeFromUserMap( existingRole ); + populateUserMap( updatedRole ); + } + } + + @Override + public boolean delete( RoleRecord role ) throws IOException + { + boolean foundRole = false; + synchronized (this) + { + // Copy-on-write for the roles list + List newRoles = new ArrayList<>(); + for ( RoleRecord other : roles ) + { + if ( other.name().equals( role.name() ) ) + { + foundRole = true; + } else + { + newRoles.add( other ); + } + } + + if ( foundRole ) + { + roles = newRoles; + + saveRoles(); + + rolesByName.remove( role.name() ); + } + + removeFromUserMap( role ); + } + return foundRole; + } + + /** + * Override this in the implementing class to persist roles + * @throws IOException + */ + abstract protected void saveRoles() throws IOException; + + @Override + public int numberOfRoles() + { + return roles.size(); + } + + @Override + public void removeUserFromAllRoles( String username ) throws ConcurrentModificationException, IOException + { + synchronized ( this ) + { + Set roles = rolesByUsername.get( username ); + if ( roles != null ) + { + // Since update() is modifying the set we create a copy for the iteration + List rolesToRemoveFrom = new ArrayList<>( roles ); + for ( String roleName : rolesToRemoveFrom ) + { + RoleRecord role = rolesByName.get( roleName ); + RoleRecord newRole = role.augment().withoutUser( username ).build(); + update( role, newRole ); + } + } + } + } + + protected void populateUserMap( RoleRecord role ) + { + for ( String username : role.users() ) + { + SortedSet memberOfRoles = rolesByUsername.get( username ); + if ( memberOfRoles == null ) + { + memberOfRoles = new ConcurrentSkipListSet<>(); + rolesByUsername.put( username, memberOfRoles ); + } + memberOfRoles.add( role.name() ); + } + } + + protected void removeFromUserMap( RoleRecord role ) + { + for ( String username : role.users() ) + { + SortedSet memberOfRoles = rolesByUsername.get( username ); + if ( memberOfRoles != null ) + { + memberOfRoles.remove( role.name() ); + } + } + } +} diff --git a/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/FileRoleRepository.java b/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/FileRoleRepository.java index 55f2e86b94b1f..d25544568be3e 100644 --- a/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/FileRoleRepository.java +++ b/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/FileRoleRepository.java @@ -24,13 +24,7 @@ import java.nio.file.Path; import java.util.ArrayList; import java.util.List; -import java.util.Map; -import java.util.Set; -import java.util.SortedSet; -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.ConcurrentSkipListSet; -import org.neo4j.kernel.lifecycle.LifecycleAdapter; import org.neo4j.logging.Log; import org.neo4j.logging.LogProvider; import org.neo4j.server.security.auth.exception.ConcurrentModificationException; @@ -43,21 +37,12 @@ * JVM restarts and crashes. */ // TODO: Extract shared code with FileUserRepository -public class FileRoleRepository extends LifecycleAdapter implements RoleRepository +public class FileRoleRepository extends AbstractRoleRepository { private final Path roleFile; - // TODO: We could improve concurrency by using a ReadWriteLock - - /** Quick lookup of roles by name */ - private final Map rolesByName = new ConcurrentHashMap<>(); - private final Map> rolesByUsername = new ConcurrentHashMap<>(); - private final Log log; - /** Master list of roles */ - private volatile List roles = new ArrayList<>(); - private final RoleSerialization serialization = new RoleSerialization(); public FileRoleRepository( Path file, LogProvider logProvider ) @@ -66,18 +51,6 @@ public FileRoleRepository( Path file, LogProvider logProvider ) this.log = logProvider.getLog( getClass() ); } - @Override - public RoleRecord findByName( String name ) - { - return rolesByName.get( name ); - } - - @Override - public Set findByUsername( String username ) - { - return rolesByUsername.get( username ); - } - @Override public void start() throws Throwable { @@ -88,119 +61,15 @@ public void start() throws Throwable } @Override - public void create( RoleRecord role ) throws IllegalArgumentException, IOException - { - if ( !isValidName( role.name() ) ) - { - throw new IllegalArgumentException( "'" + role.name() + "' is not a valid role name." ); - } - - synchronized (this) - { - // Check for existing role - for ( RoleRecord other : roles ) - { - if ( other.name().equals( role.name() ) ) - { - throw new IllegalArgumentException( "The specified role already exists" ); - } - } - - roles.add( role ); - - saveRolesToFile(); - - rolesByName.put( role.name(), role ); - - populateUserMap( role ); - } - } - - @Override - public void update( RoleRecord existingRole, RoleRecord updatedRole ) throws ConcurrentModificationException, IOException - { - // Assert input is ok - if ( !existingRole.name().equals( updatedRole.name() ) ) - { - throw new IllegalArgumentException( "updated role has a different name" ); - } - - synchronized (this) - { - // Copy-on-write for the roles list - List newRoles = new ArrayList<>(); - boolean foundRole = false; - for ( RoleRecord other : roles ) - { - if ( other.equals( existingRole ) ) - { - foundRole = true; - newRoles.add( updatedRole ); - } else - { - newRoles.add( other ); - } - } - - if ( !foundRole ) - { - throw new ConcurrentModificationException(); - } - - roles = newRoles; - - saveRolesToFile(); - - rolesByName.put( updatedRole.name(), updatedRole ); - - removeFromUserMap( existingRole ); - populateUserMap( updatedRole ); - } - } - - @Override - public boolean delete( RoleRecord role ) throws IOException - { - boolean foundRole = false; - synchronized (this) - { - // Copy-on-write for the roles list - List newRoles = new ArrayList<>(); - for ( RoleRecord other : roles ) - { - if ( other.name().equals( role.name() ) ) - { - foundRole = true; - } else - { - newRoles.add( other ); - } - } - - if ( foundRole ) - { - roles = newRoles; - - saveRolesToFile(); - - rolesByName.remove( role.name() ); - } - - removeFromUserMap( role ); - } - return foundRole; - } - - @Override - public int numberOfRoles() + public boolean isValidName( String name ) { - return roles.size(); + return name.matches( "^[a-zA-Z0-9_]+$" ); } @Override - public boolean isValidName( String name ) + protected void saveRoles() throws IOException { - return name.matches( "^[a-zA-Z0-9_]+$" ); + saveRolesToFile(); } private void saveRolesToFile() throws IOException @@ -244,30 +113,4 @@ private void loadRolesFromFile() throws IOException populateUserMap( role ); } } - - private void populateUserMap( RoleRecord role ) - { - for ( String username : role.users() ) - { - SortedSet memberOfRoles = rolesByUsername.get( username ); - if ( memberOfRoles == null ) - { - memberOfRoles = new ConcurrentSkipListSet<>(); - rolesByUsername.put( username, memberOfRoles ); - } - memberOfRoles.add( role.name() ); - } - } - - private void removeFromUserMap( RoleRecord role ) - { - for ( String username : role.users() ) - { - SortedSet memberOfRoles = rolesByUsername.get( username ); - if ( memberOfRoles != null ) - { - memberOfRoles.remove( role.name() ); - } - } - } } diff --git a/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/FileUserRealm.java b/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/FileUserRealm.java index 3090de48bab7a..3e5380ed68daa 100644 --- a/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/FileUserRealm.java +++ b/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/FileUserRealm.java @@ -154,7 +154,7 @@ User newUser( String username, String initialPassword, boolean requirePasswordCh return user; } - RoleRecord newRole( String roleName, String... users ) throws IOException, IllegalCredentialsException + RoleRecord newRole( String roleName, String... users ) throws IOException { assertValidRoleName( roleName ); for (String username : users) @@ -175,33 +175,31 @@ void addUserToRole( String username, String roleName ) throws IOException assertValidUsername( username ); assertValidRoleName( roleName ); - // TODO: FIXME This is not atomic. E.g. the user could be deleted between here and updating the role - User user = userRepository.findByName( username ); - if ( user == null ) + synchronized ( this ) { - throw new IllegalArgumentException( "User " + username + " does not exist." ); - } + User user = userRepository.findByName( username ); + if ( user == null ) + { + throw new IllegalArgumentException( "User " + username + " does not exist." ); + } - // TODO: FIXME This is not atomic. E.g. the role could be created between here and create - // In that case we would get an IllegalArgumentException, but what we want is a - // ConcurrentModificationException and then retry. We should use a RoleRepositoty.createOrUpdate instead. - RoleRecord role = roleRepository.findByName( roleName ); - if ( role == null ) - { - RoleRecord newRole = new RoleRecord( roleName, username ); - roleRepository.create( newRole ); - } - else - { - RoleRecord newRole = role.augment().withUser( username ).build(); - try + RoleRecord role = roleRepository.findByName( roleName ); + if ( role == null ) { - roleRepository.update( role, newRole ); + throw new IllegalArgumentException( "Role " + roleName + " does not exist." ); } - catch ( ConcurrentModificationException e ) + else { - // Try again - addUserToRole( username, roleName ); + RoleRecord newRole = role.augment().withUser( username ).build(); + try + { + roleRepository.update( role, newRole ); + } + catch ( ConcurrentModificationException e ) + { + // Try again + addUserToRole( username, roleName ); + } } } } @@ -211,6 +209,34 @@ void removeUserFromRole( String username, String rolename ) throws IOException // TODO } + boolean deleteUser( String username ) throws IOException + { + boolean result = false; + synchronized ( this ) + { + User user = userRepository.findByName( username ); + if ( user != null && userRepository.delete( user ) ) + { + removeUserFromAllRoles( username ); + result = true; + } + } + return result; + } + + private void removeUserFromAllRoles( String username ) throws IOException + { + try + { + roleRepository.removeUserFromAllRoles( username ); + } + catch ( ConcurrentModificationException e ) + { + // Try again + removeUserFromAllRoles( username ); + } + } + private void assertValidUsername( String name ) { if ( !userRepository.isValidName( name ) ) diff --git a/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/RoleManager.java b/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/RoleManager.java index 0b3f8d3e5a371..2fb0943968d5a 100644 --- a/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/RoleManager.java +++ b/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/RoleManager.java @@ -23,7 +23,23 @@ public interface RoleManager { + /** + * Add a user to a role. The role has to exist. + * + * @param username + * @param roleName + * @throws IllegalArgumentException if the role does not exist + * @throws IOException + */ void addUserToRole( String username, String roleName ) throws IOException; + /** + * Remove a user from a role. + * + * @param username + * @param roleName + * @throws IllegalArgumentException if the username or the role does not exist + * @throws IOException + */ void removeUserFromRole( String username, String roleName ) throws IOException; } diff --git a/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/RoleRecord.java b/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/RoleRecord.java index d7083f9388000..d1c79f605af77 100644 --- a/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/RoleRecord.java +++ b/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/RoleRecord.java @@ -111,7 +111,7 @@ public String toString() public static class Builder { private String name; - private SortedSet users; + private SortedSet users = new TreeSet<>(); public Builder() { } @@ -124,6 +124,7 @@ public Builder( RoleRecord base ) public Builder withName( String name ) { this.name = name; return this; } public Builder withUsers( SortedSet users ) { this.users = users; return this; } public Builder withUser( String user ) { this.users.add( user ); return this; } + public Builder withoutUser( String user ) { this.users.remove( user ); return this; } public RoleRecord build() { diff --git a/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/RoleRepository.java b/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/RoleRepository.java index 7974ca4ad1cc1..73f00075e669e 100644 --- a/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/RoleRepository.java +++ b/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/RoleRepository.java @@ -37,7 +37,7 @@ public interface RoleRepository extends Lifecycle /** * Create a role, given that the roles token is unique. * @param role the new role object - * @throws IllegalArgumentException if the role name is not valid + * @throws IllegalArgumentException if the role name is not valid or the role name already exists */ void create( RoleRecord role ) throws IllegalArgumentException, IOException; @@ -60,4 +60,6 @@ public interface RoleRepository extends Lifecycle /** Utility for API consumers to tell if #create() will accept a given role name */ boolean isValidName( String name ); + + void removeUserFromAllRoles( String username ) throws ConcurrentModificationException, IOException; } diff --git a/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/RoleSerialization.java b/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/RoleSerialization.java index e29769ed80c71..04d44e44d0250 100644 --- a/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/RoleSerialization.java +++ b/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/RoleSerialization.java @@ -84,7 +84,7 @@ private RoleRecord deserializeRole( String line, int lineNumber ) throws FormatE } return new RoleRecord.Builder() .withName( parts[0] ) - .withUsers( deserializeUsers( parts[1], lineNumber ) ) + .withUsers( deserializeUsers( parts[1] ) ) .build(); } @@ -93,7 +93,7 @@ private String serialize( SortedSet users ) return joinCollection( userSeparator, users ); } - private SortedSet deserializeUsers( String part, int lineNumber ) throws FormatException + private SortedSet deserializeUsers( String part ) throws FormatException { String[] splits = part.split( userSeparator, -1 ); diff --git a/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/ShiroAuthManager.java b/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/ShiroAuthManager.java index 6cae290ddd0b9..ea96c23b26319 100644 --- a/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/ShiroAuthManager.java +++ b/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/ShiroAuthManager.java @@ -169,12 +169,22 @@ public void setPassword( AuthSubject authSubject, String username, String passwo @Override public void addUserToRole( String username, String roleName ) throws IOException { + assertAuthEnabled(); realm.addUserToRole( username, roleName ); } @Override public void removeUserFromRole( String username, String roleName ) throws IOException { + assertAuthEnabled(); realm.removeUserFromRole( username, roleName ); } + + @Override + public boolean deleteUser( String username ) throws IOException + { + assertAuthEnabled(); + return realm.deleteUser( username ); + } + } diff --git a/enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/FileUserRealmTest.java b/enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/FileUserRealmTest.java new file mode 100644 index 0000000000000..0c9e28770808b --- /dev/null +++ b/enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/FileUserRealmTest.java @@ -0,0 +1,198 @@ +/* + * Copyright (c) 2002-2016 "Neo Technology," + * Network Engine for Objects in Lund AB [http://neotechnology.com] + * + * This file is part of Neo4j. + * + * Neo4j is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ +package org.neo4j.server.security.enterprise.auth; + +import com.google.testing.threadtester.ClassInstrumentation; +import com.google.testing.threadtester.CodePosition; +import com.google.testing.threadtester.Instrumentation; +import com.google.testing.threadtester.InterleavedRunner; +import com.google.testing.threadtester.MainRunnableImpl; +import com.google.testing.threadtester.RunResult; +import com.google.testing.threadtester.SecondaryRunnableImpl; +import com.google.testing.threadtester.ThreadedTest; +import com.google.testing.threadtester.ThreadedTestRunner; +import org.junit.Test; + +import java.util.Arrays; + +import org.neo4j.server.security.auth.InMemoryUserRepository; +import org.neo4j.server.security.auth.User; +import org.neo4j.server.security.auth.UserRepository; + +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; + +public class FileUserRealmTest +{ + RoleRepository roleRepository; + UserRepository userRepository; + + private static final String USERNAME = "neo4j"; + private static final String ROLE = "admin"; + + public FileUserRealmTest() throws Exception + { + super(); + setup(); + } + + private void setup() throws Exception + { + roleRepository = new InMemoryRoleRepository(); + userRepository = new InMemoryUserRepository(); + userRepository.create( new User.Builder().withName( USERNAME ).build() ); + roleRepository.create( new RoleRecord.Builder().withName( ROLE ).build() ); + } + + @Test + public void testThreadedTests() throws Exception + { + ThreadedTestRunner runner = new ThreadedTestRunner(); + + try + { + runner.runTests( getClass(), FileUserRealm.class ); + } + // We need to work around an issue that we do not get failures from the test framework + catch ( final RuntimeException e ) + { + final Throwable root = org.apache.commons.lang3.exception.ExceptionUtils.getRootCause( e ); + if ( root instanceof AssertionError ) + { + throw (AssertionError) root; + } + else + { + throw e; + } + } + } + + @ThreadedTest + public void addUserToRoleShouldBeAtomic() throws Exception + { + // Create a code position for where we want to break in the main thread + CodePosition codePosition = getCodePositionAfterCall( "addUserToRole", "findByName" ); + + FileUserRealm realm = new FileUserRealm( userRepository, roleRepository ); + + // When + RunResult result = InterleavedRunner.interleave( + new AdminMain( realm, addUserToRoleOp ), + new AdminSecond( deleteUserOp ), + Arrays.asList( codePosition ) ); + result.throwExceptionsIfAny(); + + // Then + RoleRecord role = roleRepository.findByName( ROLE ); + assertNull( "User " + USERNAME + " should be deleted!", userRepository.findByName( USERNAME ) ); + assertNotNull( "Role " + ROLE + " should exist!", role ); + assertTrue( "Users assigned to role " + ROLE + " should be empty!", role.users().isEmpty() ); + } + + @ThreadedTest + public void deleteUserShouldBeAtomic() throws Exception + { + // Create a code position for where we want to break in the main thread + CodePosition codePosition = getCodePositionAfterCall( "deleteUser", "findByName" ); + + FileUserRealm realm = new FileUserRealm( userRepository, roleRepository ); + + // When + RunResult result = InterleavedRunner.interleave( + new AdminMain( realm, deleteUserOp ), + new AdminSecond( addUserToRoleOp ), + Arrays.asList( codePosition ) ); + + // Then + assertNull( "User " + USERNAME + " should be deleted!", userRepository.findByName( USERNAME ) ); + assertTrue( result.getSecondaryException() instanceof IllegalArgumentException ); + assertTrue( result.getSecondaryException().getMessage().equals( "User " + USERNAME + " does not exist." ) ); + } + + private CodePosition getCodePositionAfterCall( String caller, String called ) + { + ClassInstrumentation instrumentation = Instrumentation.getClassInstrumentation( FileUserRealm.class ); + CodePosition codePosition = instrumentation.afterCall( caller, called ); + return codePosition; + } + + private class AdminMain extends MainRunnableImpl + { + private FileUserRealm realm; + private RealmOperation op; + + public AdminMain( FileUserRealm realm, RealmOperation op ) + { + this.realm = realm; + this.op = op; + } + + @Override + public Class getClassUnderTest() + { + return FileUserRealm.class; + } + + @Override + public FileUserRealm getMainObject() + { + return realm; + } + + @Override + public void run() throws Exception + { + op.doOp( realm ); + } + } + + private class AdminSecond extends SecondaryRunnableImpl + { + private FileUserRealm realm; + private RealmOperation op; + + public AdminSecond( RealmOperation op ) + { + this.op = op; + } + + @Override + public void initialize( AdminMain main ) throws Exception + { + realm = main.getMainObject(); + } + + @Override + public void run() throws Exception + { + op.doOp( realm ); + } + } + + private interface RealmOperation + { + void doOp( FileUserRealm realm ) throws Exception; + } + + private RealmOperation addUserToRoleOp = ( realm ) -> realm.addUserToRole( USERNAME, ROLE ); + private RealmOperation deleteUserOp = ( realm ) -> realm.deleteUser( USERNAME ); +} diff --git a/enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/InMemoryRoleRepository.java b/enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/InMemoryRoleRepository.java index 22e7d1b161e71..791862f471724 100644 --- a/enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/InMemoryRoleRepository.java +++ b/enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/InMemoryRoleRepository.java @@ -19,108 +19,11 @@ */ package org.neo4j.server.security.enterprise.auth; -import java.util.Map; -import java.util.Set; -import java.util.SortedSet; -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.ConcurrentSkipListSet; - -import org.neo4j.kernel.api.security.exception.IllegalCredentialsException; -import org.neo4j.kernel.lifecycle.LifecycleAdapter; -import org.neo4j.server.security.auth.exception.ConcurrentModificationException; +import java.io.IOException; /** A role repository implementation that just stores roles in memory */ -public class InMemoryRoleRepository extends LifecycleAdapter implements RoleRepository +public class InMemoryRoleRepository extends AbstractRoleRepository { - private final ConcurrentHashMap roles = new ConcurrentHashMap<>(); - private final Map> rolesByUsername = new ConcurrentHashMap<>(); - - @Override - public RoleRecord findByName( String name ) - { - return roles.get( name ); - } - - @Override - public Set findByUsername( String username ) - { - return rolesByUsername.get( username ); - } - - @Override - public void create( RoleRecord role ) - { - synchronized (this) - { - // Check for existing role or token - for ( RoleRecord other : roles.values() ) - { - if ( other.name().equals( role.name() ) ) - { - throw new IllegalArgumentException( "The specified role already exists" ); - } - } - - roles.put( role.name(), role ); - - populateUserMap( role ); - } - } - - @Override - public void update( RoleRecord existingRole, RoleRecord updatedRole ) throws ConcurrentModificationException - { - // Assert input is ok - if ( !existingRole.name().equals( updatedRole.name() ) ) - { - throw new IllegalArgumentException( "updated role has a different name" ); - } - - synchronized (this) - { - boolean foundRole = false; - for ( RoleRecord other : roles.values() ) - { - if ( other.equals( existingRole ) ) - { - foundRole = true; - } - } - - if ( !foundRole ) - { - throw new ConcurrentModificationException(); - } - - roles.put( updatedRole.name(), updatedRole ); - - removeFromUserMap( existingRole ); - populateUserMap( updatedRole ); - } - } - - @Override - public boolean delete( RoleRecord role ) - { - synchronized (this) - { - boolean removedRole = roles.remove( role.name() ) != null; - - if ( removedRole ) - { - removeFromUserMap( role ); - } - - return removedRole; - } - } - - @Override - public int numberOfRoles() - { - return roles.size(); - } - @Override public boolean isValidName( String name ) { @@ -128,30 +31,9 @@ public boolean isValidName( String name ) return true; } - private void populateUserMap( RoleRecord role ) - { - for ( String username : role.users() ) - { - SortedSet membersOfRole = rolesByUsername.get( username ); - if ( membersOfRole == null ) - { - membersOfRole = new ConcurrentSkipListSet<>(); - rolesByUsername.put( username, membersOfRole ); - } - membersOfRole.add( role.name() ); - } - } - - private void removeFromUserMap( RoleRecord role ) + @Override + protected void saveRoles() throws IOException { - for ( String username : role.users() ) - { - SortedSet membersOfRole = rolesByUsername.get( username ); - if ( membersOfRole != null ) - { - membersOfRole.remove( role.name() ); - } - } + // Nothing to do } - }