Skip to content

Commit

Permalink
Disallow deleting final user in neo4j-admin to match server
Browse files Browse the repository at this point in the history
 The server does not allow you to delete yourself. In neo4j-admin the running user is not identified, but the logged in user is assumed to be the admin. The closest role to match server behaviour is to disallow deleting the last user.
  • Loading branch information
craigtaverner committed Sep 23, 2016
1 parent e0267f4 commit c1fcc35
Show file tree
Hide file tree
Showing 4 changed files with 70 additions and 11 deletions.
Expand Up @@ -183,6 +183,10 @@ private void deleteUser( String username ) throws Throwable
{
BasicAuthManager authManager = getAuthManager();
authManager.getUser( username ); // Will throw error on missing user
if ( authManager.getAllUsernames().size() == 1 )
{
throw new IllegalArgumentException( "Deleting the only remaining user '" + username + "' is not allowed" );
}
if ( authManager.deleteUser( username ) )
{
outsideWorld.stdOutLine( "Deleted user '" + username + "'" );
Expand Down
Expand Up @@ -22,15 +22,13 @@
import org.junit.Before;

import java.io.File;
import java.io.IOException;
import java.nio.file.FileSystems;

import org.neo4j.commandline.admin.AdminTool;
import org.neo4j.commandline.admin.CommandLocator;
import org.neo4j.commandline.admin.OutsideWorld;
import org.neo4j.io.fs.DelegateFileSystemAbstraction;
import org.neo4j.io.fs.FileSystemAbstraction;
import org.neo4j.kernel.api.exceptions.InvalidArgumentsException;
import org.neo4j.logging.NullLogProvider;
import org.neo4j.server.security.auth.Credential;
import org.neo4j.server.security.auth.FileUserRepository;
Expand All @@ -40,6 +38,7 @@
import static org.mockito.Matchers.anyString;
import static org.mockito.Matchers.contains;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.reset;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
Expand All @@ -53,14 +52,17 @@ abstract class CommandTestBase
File confDir;
File homeDir;
OutsideWorld out;
AdminTool tool;

@Before
public void setup()
{
graphDir = testDir.graphDbDir();
confDir = ensureDir( "conf" );
homeDir = ensureDir( "home" );
out = mock( OutsideWorld.class );
resetOutsideWorldMock();
tool = new AdminTool( CommandLocator.fromServiceLocator(), out, true );
}

protected File ensureDir( String name )
Expand All @@ -75,7 +77,7 @@ protected File ensureDir( String name )

protected void resetOutsideWorldMock()
{
out = mock( OutsideWorld.class );
reset(out);
when( out.fileSystem() ).thenReturn( fileSystem );
}

Expand All @@ -84,9 +86,10 @@ private File authFile()
return new File( new File( new File( testDir.graphDbDir(), "data" ), "dbms" ), "auth" );
}

User createTestUser( String username, String password ) throws IOException, InvalidArgumentsException
User createTestUser( String username, String password ) throws Throwable
{
FileUserRepository users = new FileUserRepository( fileSystem, authFile(), NullLogProvider.getInstance() );
users.start();
User user =
new User.Builder( username, Credential.forPassword( password ) ).withRequiredPasswordChange( true )
.build();
Expand All @@ -109,7 +112,7 @@ String[] args(String... args)

protected abstract String command();

private String[] makeArgs( String subCommand, String... args )
protected String[] makeArgs( String subCommand, String... args )
{
String[] allArgs = new String[args.length + 2];
System.arraycopy( args, 0, allArgs, 2, args.length );
Expand All @@ -120,9 +123,7 @@ private String[] makeArgs( String subCommand, String... args )

void assertFailedSubCommand( String command, String[] args, String... errors )
{
// When running set password on a failing case (missing user, or other error)
resetOutsideWorldMock();
AdminTool tool = new AdminTool( CommandLocator.fromServiceLocator(), out, true );
tool.execute( graphDir.toPath(), confDir.toPath(), makeArgs( command, args ) );

// Then we get the expected error
Expand All @@ -136,9 +137,7 @@ void assertFailedSubCommand( String command, String[] args, String... errors )

void assertSuccessfulSubCommand( String command, String[] args, String... messages )
{
// When running set password on a successful case (user exists)
resetOutsideWorldMock();
AdminTool tool = new AdminTool( CommandLocator.fromServiceLocator(), out, true );
tool.execute( graphDir.toPath(), confDir.toPath(), makeArgs( command, args ));

// Then we get the expected output messages
Expand Down
Expand Up @@ -56,10 +56,30 @@ public void shouldFailToDeleteMissingUser() throws Throwable
}
}

@Test
public void shouldNotDeleteDefaultUser() throws Throwable
{
// Given - default user
createTestUser( "neo4j", "abc" );

// When
try
{
UsersCommand usersCommand = new UsersCommand( graphDir.toPath(), confDir.toPath(), out );
usersCommand.execute( new String[]{"delete", "neo4j"} );
}
catch ( CommandFailed e )
{
// Expect failure message
assertThat( e.getMessage(), containsString( "Deleting the only remaining user 'neo4j' is not allowed" ) );
}
}

@Test
public void shouldDeleteExistingUser() throws Throwable
{
// Given - existing user
createTestUser( "neo4j", "abc" );
createTestUser( "another", "abc" );

// When - creating new user
Expand Down
Expand Up @@ -19,6 +19,7 @@
*/
package org.neo4j.commandline.admin.security;

import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.RuleChain;
Expand All @@ -36,6 +37,18 @@ public class UsersCommandIT extends UsersCommandTestBase
@Rule
public RuleChain ruleChain = RuleChain.outerRule( testDir );

@Before
public void setup()
{
super.setup();
// the following line ensures that the test setup code (like creating test users) works on the same initial
// environment that the actual tested commands will encounter. In particular some auth state is created
// on demand in both the UserCommand and in the real server. We want that state created before the tests
// are run.
tool.execute( graphDir.toPath(), confDir.toPath(), makeArgs( "list") );
resetOutsideWorldMock();
}

@Test
public void shouldGetUsageErrorsWithNoSubCommand() throws Throwable
{
Expand Down Expand Up @@ -259,20 +272,43 @@ public void shouldNotDeleteMissingUser() throws Throwable
}

@Test
public void shouldDeleteDefaultUser() throws Throwable
public void shouldNotDeleteDefaultUserIfNoOtherUserExists() throws Throwable
{
// Given default state (only default user)

// When running 'delete' with correct parameters, expect error
assertFailedSubCommand( "delete", args("neo4j"), "Deleting the only remaining user 'neo4j' is not allowed" );
}

@Test
public void shouldDeleteDefaultUserIfAnotherUserExists() throws Throwable
{
// Given a user that requires password change
createTestUser( "another", "neo4j" );

// When running 'delete' with correct parameters, expect success
assertSuccessfulSubCommand( "delete", args("neo4j"), "Deleted user 'neo4j'" );
}

@Test
public void shouldDeleteExistingUser() throws Throwable
public void shouldNotDeleteExistingUserIfNoOtherUserExists() throws Throwable
{
// Given a user that requires password change
createTestUser( "another", "neo4j" );

// When running 'delete' with correct parameters, expect success
assertSuccessfulSubCommand( "delete", args("neo4j"), "Deleted user 'neo4j'" );

// When running 'delete' with correct parameters, expect error
assertFailedSubCommand( "delete", args("another"), "Deleting the only remaining user 'another' is not allowed" );
}

@Test
public void shouldDeleteExistingUser() throws Throwable
{
// Given the default user and another new user
createTestUser( "another", "neo4j" );

// When running 'create' with correct parameters, expect correct output
assertSuccessfulSubCommand( "delete", args("another"), "Deleted user 'another'" );
}
Expand Down

0 comments on commit c1fcc35

Please sign in to comment.