Skip to content

Commit

Permalink
Don't throw when querying for authorization
Browse files Browse the repository at this point in the history 
If a provider threw an exception during authorization it would result
in an overall error, even though another provider could provide
authorization.

Now the result from all providers will be properly aggregated.
  • Loading branch information
@OliviaYtterbrink
OliviaYtterbrink committed Sep 28, 2018
1 parent e490af2 commit 4882f73
Show file tree
Hide file tree
Showing 4 changed files with 61 additions and 28 deletions.
2 changes: 1 addition & 1 deletion ...curity/src/main/java/org/neo4j/server/security/enterprise/auth/InternalFlatFileRealm.java
View file
Expand Up @@ -343,7 +343,7 @@ public boolean supports( AuthenticationToken token )
}

@Override
protected AuthorizationInfo doGetAuthorizationInfo( PrincipalCollection principals ) throws AuthenticationException
protected AuthorizationInfo doGetAuthorizationInfo( PrincipalCollection principals )
{
if ( !authorizationEnabled )
{
Expand Down
8 changes: 2 additions & 6 deletions enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/LdapRealm.java
View file
Expand Up @@ -384,13 +384,9 @@ protected AuthorizationInfo doGetAuthorizationInfo( PrincipalCollection principa
}
catch ( AuthorizationException e )
{
securityLog.error( withRealm( "Failed to get authorization info: '%s' caused by '%s'",
securityLog.warn( withRealm( "Failed to get authorization info: '%s' caused by '%s'",
e.getMessage(), e.getCause().getMessage() ) );
if ( isAuthorizationExceptionAnLdapReadTimeout( e ) )
{
throw new AuthProviderTimeoutException( LDAP_READ_TIMEOUT_CLIENT_MESSAGE, e );
}
throw new AuthProviderFailedException( LDAP_AUTHORIZATION_FAILURE_CLIENT_MESSAGE, e );
return null;
}
}

Expand Down
12 changes: 6 additions & 6 deletions ...prise/security/src/test/java/org/neo4j/server/security/enterprise/auth/LdapRealmTest.java
View file
Expand Up @@ -50,7 +50,6 @@
import javax.naming.directory.SearchResult;
import javax.naming.ldap.LdapContext;

import org.neo4j.graphdb.security.AuthProviderFailedException;
import org.neo4j.kernel.configuration.Config;
import org.neo4j.server.security.enterprise.configuration.SecuritySettings;
import org.neo4j.server.security.enterprise.log.SecurityLog;
Expand All @@ -59,6 +58,7 @@
import static java.util.Collections.singletonList;
import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.CoreMatchers.hasItems;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertThat;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyString;
Expand All @@ -72,8 +72,8 @@
public class LdapRealmTest
{
Config config = mock( Config.class );
SecurityLog securityLog = mock( SecurityLog.class );
SecureHasher secureHasher = new SecureHasher();
private SecurityLog securityLog = mock( SecurityLog.class );
private SecureHasher secureHasher = new SecureHasher();

@Rule
public ExpectedException expectedException = ExpectedException.none();
Expand Down Expand Up @@ -442,11 +442,11 @@ public void shouldLogFailedAuthorizationQueries()
when( jndiLdapContectFactory.getUrl() ).thenReturn( "ldap://myserver.org:12345" );

// When
assertException( () -> realm.doGetAuthorizationInfo( new SimplePrincipalCollection( "olivia", "LdapRealm" ) ),
AuthProviderFailedException.class );
AuthorizationInfo info = realm.doGetAuthorizationInfo( new SimplePrincipalCollection( "olivia", "LdapRealm" ) );

// Then
verify( securityLog ).error( contains( "{LdapRealm}: Failed to get authorization info: " +
assertNull( info );
verify( securityLog ).warn( contains( "{LdapRealm}: Failed to get authorization info: " +
"'LDAP naming error while attempting to retrieve authorization for user [olivia].'" +
" caused by 'Simulated failure'"
) );
Expand Down
67 changes: 52 additions & 15 deletions .../src/test/java/org/neo4j/server/security/enterprise/auth/integration/bolt/LdapAuthIT.java
View file
Expand Up @@ -61,6 +61,7 @@
import org.neo4j.graphdb.config.Setting;
import org.neo4j.internal.kernel.api.security.AuthSubject;
import org.neo4j.io.fs.FileSystemAbstraction;
import org.neo4j.kernel.api.exceptions.InvalidArgumentsException;
import org.neo4j.kernel.api.exceptions.Status;
import org.neo4j.kernel.impl.factory.GraphDatabaseFacade;
import org.neo4j.kernel.impl.proc.Procedures;
Expand Down Expand Up @@ -107,7 +108,7 @@ interface TimeoutTests
"objectClass: organization\n\n" ) ),
},
loadedSchemas = {
@LoadSchema( name = "nis", enabled = true ),
@LoadSchema( name = "nis" ),
} )
@CreateLdapServer(
transports = {@CreateTransport( protocol = "LDAP", port = 10389, address = "0.0.0.0" ),
Expand All @@ -128,9 +129,9 @@ interface TimeoutTests
@ApplyLdifFiles( "ldap_test_data.ldif" )
public class LdapAuthIT extends EnterpriseAuthenticationTestBase
{
public static final String LDAP_ERROR_MESSAGE_INVALID_CREDENTIALS = "LDAP: error code 49 - INVALID_CREDENTIALS";
public static final String NON_ROUTABLE_IP = "192.0.2.0"; // Ip in the TEST-NET-1 range, reserved for documentation...
public static final String REFUSED_IP = "127.0.0.1"; // "0.6.6.6";
private static final String LDAP_ERROR_MESSAGE_INVALID_CREDENTIALS = "LDAP: error code 49 - INVALID_CREDENTIALS";
private static final String NON_ROUTABLE_IP = "192.0.2.0"; // Ip in the TEST-NET-1 range, reserved for documentation...
private static final String REFUSED_IP = "127.0.0.1"; // "0.6.6.6";
private final String MD5_HASHED_abc123 = "{MD5}6ZoYxCjLONXyYIU2eJIuAw==";
// Hashed 'abc123' (see ldap_test_data.ldif)

Expand Down Expand Up @@ -451,13 +452,7 @@ public void shouldBeAbleToLoginNativelyAndAuthorizeWithLdap() throws Throwable
String ldapReaderUser = "neo";
String nativePassword = "nativePassword";

// this is ugly, but cannot be resolved until embedded gets security
GraphDatabaseFacade gds = (GraphDatabaseFacade) server.graphDatabaseService();
EnterpriseAuthAndUserManager authManager =
gds.getDependencyResolver().resolveDependency( EnterpriseAuthAndUserManager.class );

authManager.getUserManager( AuthSubject.AUTH_DISABLED, true )
.newUser( ldapReaderUser, nativePassword, false );
createNativeUser( ldapReaderUser, nativePassword );

// Then
// login user 'neo' with native auth provider and test that LDAP authorization gives correct permission
Expand Down Expand Up @@ -599,7 +594,7 @@ public void shouldTimeoutIfLdapServerDoesNotRespond() throws Throwable
.andThen( settings -> settings.put( SecuritySettings.ldap_read_timeout, "1s" ) ) );

assertAuth( "neo", "abc123" );
assertLdapAuthorizationTimeout();
assertReadFails( "neo", "" );
}
}

Expand All @@ -617,7 +612,7 @@ public void shouldTimeoutIfLdapServerDoesNotRespondWithoutConnectionPooling() th
} ) );

assertAuth( "neo", "abc123" );
assertLdapAuthorizationTimeout();
assertReadFails( "neo", "" );
}
}

Expand All @@ -631,7 +626,7 @@ public void shouldFailIfLdapSearchFails() throws Throwable
.andThen( settings -> settings.put( SecuritySettings.ldap_read_timeout, "1s" ) ) );

assertAuth( "neo", "abc123" );
assertLdapAuthorizationFailed();
assertReadFails( "neo", "" );
}
}

Expand Down Expand Up @@ -920,6 +915,32 @@ public void shouldBeAbleToAuthorizeUsingNativeWithLdapEnabled() throws Throwable
assertReadSucceeds();
}

@Test
public void shouldBeAbleToAuthorizeUsingNativeWhenLdapFails() throws Throwable
{
restartNeo4jServerWithOverriddenSettings( settings ->
{
settings.put( SecuritySettings.auth_providers,
SecuritySettings.LDAP_REALM_NAME + "," + SecuritySettings.NATIVE_REALM_NAME );
settings.put( SecuritySettings.ldap_server, "ldap://" + REFUSED_IP );
settings.put( SecuritySettings.native_authentication_enabled, "true" );
settings.put( SecuritySettings.native_authorization_enabled, "true" );
settings.put( SecuritySettings.ldap_authentication_enabled, "true" );
settings.put( SecuritySettings.ldap_authorization_enabled, "true" );
settings.put( SecuritySettings.ldap_authorization_use_system_account, "true" );
} );

// Given
// we have a native 'simon' that is read only
createNativeUser( "simon", createdUserPassword, PredefinedRoles.READER );

// When
assertAuth( "simon", createdUserPassword );

// Then
assertReadSucceeds();
}

@Test
public void shouldNotLogErrorsFromLdapRealmWhenLoginSuccessfulInNativeRealmAndNativeFirst() throws Throwable
{
Expand Down Expand Up @@ -1367,6 +1388,22 @@ private void changeLDAPGroup( String username, Object credentials, String group
modifyLDAPAttribute( username, credentials, "gidnumber", gid );
}

private void createNativeUser( String username, String password, String... roles ) throws IOException, InvalidArgumentsException
{
GraphDatabaseFacade gds = (GraphDatabaseFacade) server.graphDatabaseService();
EnterpriseAuthAndUserManager authManager =
gds.getDependencyResolver().resolveDependency( EnterpriseAuthAndUserManager.class );

authManager.getUserManager( AuthSubject.AUTH_DISABLED, true )
.newUser( username, password, false );

for ( String role : roles )
{
authManager.getUserManager( AuthSubject.AUTH_DISABLED, true )
.addRoleToUser( role, username );
}
}

private class DirectoryServiceWaitOnSearch implements AutoCloseable
{
private final Interceptor waitOnSearchInterceptor;
Expand Down Expand Up @@ -1531,7 +1568,7 @@ private File fileFromResources( String path )

private void resetProperty( String property, String value )
{
if ( property != null )
if ( value == null )
{
System.clearProperty( property );
}
Expand Down

0 comments on commit 4882f73

Please sign in to comment.