You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@atlas.apache.org by Hemanth Yamijala <yh...@gmail.com> on 2016/04/06 06:25:25 UTC
Re: Review Request 45029: ATLAS-494 : Authentication
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45029/#review127257
-----------------------------------------------------------
webapp/src/main/java/org/apache/atlas/util/PropertiesUtil.java (line 38)
<https://reviews.apache.org/r/45029/#comment190487>
Using Properties instead of a HashMap will probably cut down a lot of code in the getProperty and related methods. *Can be done later*
webapp/src/main/java/org/apache/atlas/util/PropertiesUtil.java (line 52)
<https://reviews.apache.org/r/45029/#comment190478>
Extract System.getProperties() so that it doesn't get called repeatedly in a loop. Seeing the implementation of System.getProperties, there's a security manager check, which I don't know how expensive it is. So would be good to avoid.
webapp/src/main/java/org/apache/atlas/util/PropertiesUtil.java (line 136)
<https://reviews.apache.org/r/45029/#comment190488>
Do we need this? I don't see it currently used. Moreover, if we want to change the underlying implementation to a Properties, this would cause a problem. I prefer not adding it unless required. Exposing lesser of the internals will allow more flexibility later.
webapp/src/main/java/org/apache/atlas/util/XMLPropertiesUtil.java (line 55)
<https://reviews.apache.org/r/45029/#comment190490>
The file format with property elements having name and value children seems very similar to Hadoop configuration. Maybe the Hadoop Configuration object itself can be reused here. *Can do this later*
webapp/src/main/java/org/apache/atlas/web/dao/UserDao.java (line 44)
<https://reviews.apache.org/r/45029/#comment190491>
Can this file be refreshed? If a new user password is added to the file, how will that get loaded into Atlas server?
webapp/src/main/java/org/apache/atlas/web/dao/UserDao.java (line 61)
<https://reviews.apache.org/r/45029/#comment190492>
Please print the exception, by including the Exception object 'e' in the LOG.error method. This way we would know what the problem is and it would help debugging. The other thing to include in the error message would be the name of the configuration file. If the path is wrong etc, we would know it very clearly.
webapp/src/main/java/org/apache/atlas/web/dao/UserDao.java (line 68)
<https://reviews.apache.org/r/45029/#comment190493>
Should we add an empty check?
webapp/src/main/java/org/apache/atlas/web/dao/UserDao.java (line 79)
<https://reviews.apache.org/r/45029/#comment190494>
Please avoid exposing internals as far as possible. If required for testing, you could do two things: 1) restrict access by making it package or protected. 2) Mark the method with an annotation that it is exposed only for testing.. We use @VisibleForTesting.
webapp/src/main/java/org/apache/atlas/web/model/User.java (line 29)
<https://reviews.apache.org/r/45029/#comment190495>
Since we don't have any of these options (email, firstname, lastname), can we remove them? Or if they are used by the LDAP or AD models, maybe we should keep them.. Please let me know.
webapp/src/main/java/org/apache/atlas/web/security/AtlasADAuthenticationProvider.java (line 59)
<https://reviews.apache.org/r/45029/#comment190524>
There's a lot of repeat code between this and the LDAP authentication. I would strongly suggest this duplication is cut down. *This can be done later*
webapp/src/main/java/org/apache/atlas/web/security/AtlasAuthenticationProvider.java (line 31)
<https://reviews.apache.org/r/45029/#comment190527>
Please add some tests for this. You could focus on file based auth method tests for now, I guess.
webapp/src/main/java/org/apache/atlas/web/security/AtlasAuthenticationProvider.java (line 60)
<https://reviews.apache.org/r/45029/#comment190498>
Since the authentication method does not change for a run of the Atlas Server, can we avoid loading the configuration in each call to this method? Essentially cache the authentication method?
webapp/src/main/java/org/apache/atlas/web/security/AtlasAuthenticationProvider.java (line 86)
<https://reviews.apache.org/r/45029/#comment190503>
The Spring documentation of AuthenticationProvider says that an AuthenticationException should be thrown if authentication fails rather than return null.
webapp/src/main/java/org/apache/atlas/web/security/AtlasAuthenticationProvider.java (line 93)
<https://reviews.apache.org/r/45029/#comment190500>
I guess this should call super.supports.
webapp/src/main/java/org/apache/atlas/web/security/AtlasFileAuthenticationProvider.java (line 33)
<https://reviews.apache.org/r/45029/#comment190506>
Either of the two cases user is null or password is null is handled by the UserService code. So, my understanding is this code will never be hit. Yes? If that's the case, we could avoid this handling here?
webapp/src/main/java/org/apache/atlas/web/security/AtlasFileAuthenticationProvider.java (line 43)
<https://reviews.apache.org/r/45029/#comment190507>
Please print user name in the login message
webapp/src/main/java/org/apache/atlas/web/security/AtlasFileAuthenticationProvider.java (line 46)
<https://reviews.apache.org/r/45029/#comment190509>
We don't call getAuthenticationWithGrantedAuthority in File based authentication case. Is there a reason for the difference?
webapp/src/main/java/org/apache/atlas/web/security/AtlasLdapAuthenticationProvider.java (line 62)
<https://reviews.apache.org/r/45029/#comment190522>
Much of this code can be done once and reused, right? If yes, can we set it up like that?.. Also the code blocks can be broken into smaller methods for better readability. In this fragment, I can see methods to build the LdapContextSource, The AuthorititesPopulator, the SearchFilter etc.
webapp/src/main/java/org/apache/atlas/web/security/AtlasLdapAuthenticationProvider.java (line 113)
<https://reviews.apache.org/r/45029/#comment190519>
Appears in this case it is an error and thus should throw an Exception (like if password is empty, etc)
webapp/src/main/java/org/apache/atlas/web/security/AtlasLdapAuthenticationProvider.java (line 117)
<https://reviews.apache.org/r/45029/#comment190526>
Why not throw AtlasAuthenticationException directly here. Also, please include the original exception object for debugging.
webapp/src/test/java/org/apache/atlas/web/security/FileAuthenticationLoginTest.java (line 29)
<https://reviews.apache.org/r/45029/#comment190528>
This test class is not testing FileAuthentication. It is at best a UserDao test.
webapp/src/test/java/org/apache/atlas/web/security/FileAuthenticationLoginTest.java (line 45)
<https://reviews.apache.org/r/45029/#comment190530>
I don't still see the point of this test. It is essentially saying something like Map.put('key', 'value'). assertFalse(Map.get('key').equals('value1'))
What you are interested in only is that the right value is returned for a key, and that is tested by testUserwithValidLoginAndPassword. So, there's no need for this test.
webapp/src/test/java/org/apache/atlas/web/security/FileAuthenticationLoginTest.java (line 62)
<https://reviews.apache.org/r/45029/#comment190529>
File is incorrectly formatted.
- Hemanth Yamijala
On April 5, 2016, 6:25 a.m., Nixon Rodrigues wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45029/
> -----------------------------------------------------------
>
> (Updated April 5, 2016, 6:25 a.m.)
>
>
> Review request for atlas, Erik Bergenholtz, Shwetha GS, Suma Shivaprasad, and Hemanth Yamijala.
>
>
> Bugs: ATLAS-494
> https://issues.apache.org/jira/browse/ATLAS-494
>
>
> Repository: atlas
>
>
> Description
> -------
>
> **Problem Statement**
>
> Atlas needs an authentication mechanism besides kerberos.
> *Requirements*
> Provide ability to authenticate against local file
> Provide Authentication that supports LDAP
> For consistency, configuration should use exactly the same configuration (properties) as Ranger & Knox
>
>
> Diffs
> -----
>
> dashboard/public/css/login.css PRE-CREATION
> dashboard/public/modules/home/views/header.html 896cfb4
> distro/src/conf/atlas-application.properties 29df5b3
> distro/src/conf/users-credentials.properties PRE-CREATION
> pom.xml 226529d
> webapp/pom.xml 85c9471
> webapp/src/main/java/org/apache/atlas/util/PropertiesUtil.java PRE-CREATION
> webapp/src/main/java/org/apache/atlas/util/XMLPropertiesUtil.java PRE-CREATION
> webapp/src/main/java/org/apache/atlas/web/dao/UserDao.java PRE-CREATION
> webapp/src/main/java/org/apache/atlas/web/filters/AtlasAuthenticationEntryPoint.java PRE-CREATION
> webapp/src/main/java/org/apache/atlas/web/model/User.java PRE-CREATION
> webapp/src/main/java/org/apache/atlas/web/security/AtlasADAuthenticationProvider.java PRE-CREATION
> webapp/src/main/java/org/apache/atlas/web/security/AtlasAbstractAuthenticationProvider.java PRE-CREATION
> webapp/src/main/java/org/apache/atlas/web/security/AtlasAuthenticationException.java PRE-CREATION
> webapp/src/main/java/org/apache/atlas/web/security/AtlasAuthenticationProvider.java PRE-CREATION
> webapp/src/main/java/org/apache/atlas/web/security/AtlasFileAuthenticationProvider.java PRE-CREATION
> webapp/src/main/java/org/apache/atlas/web/security/AtlasLdapAuthenticationProvider.java PRE-CREATION
> webapp/src/main/java/org/apache/atlas/web/service/UserService.java PRE-CREATION
> webapp/src/main/resources/atlas-admin-site.xml PRE-CREATION
> webapp/src/main/resources/spring-security.xml PRE-CREATION
> webapp/src/main/webapp/WEB-INF/applicationContext.xml PRE-CREATION
> webapp/src/main/webapp/WEB-INF/web.xml 855a068
> webapp/src/main/webapp/login.jsp PRE-CREATION
> webapp/src/test/java/org/apache/atlas/web/security/FileAuthenticationLoginTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/45029/diff/
>
>
> Testing
> -------
>
> Done.
>
> Some existing test case were failing for master.
>
> Failed tests:
> HiveLineageJerseyResourceIT.setUp:52->setupInstances:169->table:234->BaseResourceIT.createInstance:129 ยป AtlasService
>
> Tests run: 72, Failures: 1, Errors: 0, Skipped: 5
>
>
> Tests run: 6, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 28.161 sec <<< FAILURE! - in org.apache.atlas.repository.graph.GraphBackedMetadataRepositoryDeleteEntitiesTest
> testDisconnectUnidirectionalArrayReferenceFromClassType(org.apache.atlas.repository.graph.GraphBackedMetadataRepositoryDeleteEntitiesTest) Time elapsed: 0.756 sec <<< FAILURE!
> org.apache.atlas.repository.RepositoryException: org.apache.atlas.typesystem.types.ValueConversionException$NullConversionException: For field 'columns'
> at org.apache.atlas.typesystem.types.ClassType.convert(ClassType.java:157)
> at org.apache.atlas.typesystem.types.ClassType.convert(ClassType.java:45)
> at org.apache.atlas.typesystem.types.DataTypes$ArrayType.convert(DataTypes.java:490)
> at org.apache.atlas.typesystem.types.DataTypes$ArrayType.convert(DataTypes.java:459)
> at org.apache.atlas.typesystem.persistence.StructInstance.set(StructInstance.java:122)
> at org.apache.atlas.repository.graph.GraphToTypedInstanceMapper.mapVertexToArrayInstance(GraphToTypedInstanceMapper.java:196)
> at org.apache.atlas.repository.graph.GraphToTypedInstanceMapper.mapVertexToAttribute(GraphToTypedInstanceMapper.java:122)
> at org.apache.atlas.repository.graph.GraphToTypedInstanceMapper.mapVertexToInstance(GraphToTypedInstanceMapper.java:95)
> at org.apache.atlas.repository.graph.GraphToTypedInstanceMapper.mapGraphToTypedInstance(GraphToTypedInstanceMapper.java:74)
> at org.apache.atlas.repository.graph.GraphBackedMetadataRepository.getEntityDefinition(GraphBackedMetadataRepository.java:144)
> at org.apache.atlas.GraphTransactionInterceptor.invoke(GraphTransactionInterceptor.java:42)
> at org.apache.atlas.repository.graph.GraphBackedMetadataRepositoryDeleteEntitiesTest.testDisconnectUnidirectionalArrayReferenceFromClassType(GraphBackedMetadataRepositoryDeleteEntitiesTest.java:324)
>
>
> Thanks,
>
> Nixon Rodrigues
>
>