> On June 20, 2017, 7:45 a.m., Sebastian Toader wrote: > > ambari-server/src/main/java/org/apache/ambari/server/security/authentication/AmbariAuthenticationEventHandlerImpl.java > > Lines 109-117 (patched) > > <https://reviews.apache.org/r/60205/diff/1/?file=1753543#file1753543line109> > > > > Add to audit log the number of login attemps.
Is there a special `with` call for that or do you mean that this information should be added to the reason string? > On June 20, 2017, 7:45 a.m., Sebastian Toader wrote: > > ambari-server/src/main/java/org/apache/ambari/server/security/authentication/AmbariBasicAuthenticationFilter.java > > Line 120 (original), 102 (patched) > > <https://reviews.apache.org/r/60205/diff/1/?file=1753545#file1753545line120> > > > > Is it a valid scenario to not have an event handler passed in? No, so I guess I should throw and exception here or allow the NPE to be thrown. > On June 20, 2017, 7:45 a.m., Sebastian Toader wrote: > > ambari-server/src/main/java/org/apache/ambari/server/security/authentication/AmbariBasicAuthenticationFilter.java > > Line 121 (original), 103 (patched) > > <https://reviews.apache.org/r/60205/diff/1/?file=1753545#file1753545line121> > > > > If the audit logging is factored out into an event handler we have to > > ensure that event handler is always instantiaded otherwise the won't have > > audit logs related to logins. correct... so we should ensure that the eventHander has been set. > On June 20, 2017, 7:45 a.m., Sebastian Toader wrote: > > ambari-server/src/main/java/org/apache/ambari/server/security/authorization/Users.java > > Lines 1275 (patched) > > <https://reviews.apache.org/r/60205/diff/1/?file=1753553#file1753553line1275> > > > > Wouldn't it be simpler to handle this within a transaction instead of > > optimistic locking? I am not sure if transactions are sufficient here. After discussing with @jhurley, this seems like the way to go to ensure the counter is updated properly in a concurrent update scenario. > On June 20, 2017, 7:45 a.m., Sebastian Toader wrote: > > ambari-server/src/main/java/org/apache/ambari/server/security/authorization/Users.java > > Lines 1300 (patched) > > <https://reviews.apache.org/r/60205/diff/1/?file=1753553#file1753553line1300> > > > > I think it's a valid scenario that a user gets deleted by an admin in > > parallel so it's not an unexpected situtation. good point. > On June 20, 2017, 7:45 a.m., Sebastian Toader wrote: > > ambari-server/src/main/java/org/apache/ambari/server/security/authorization/Users.java > > Lines 1304 (patched) > > <https://reviews.apache.org/r/60205/diff/1/?file=1753553#file1753553line1304> > > > > Why is this needed? The ```userEntity = userDAO.findByPK(userID);``` > > provides an up to date entity. (Unless user was updated in the db directly > > or by some named JPQL query) I needed this for my test since I was updating the DB directly, but I didn't know if normal caching would cause an issue as well. Should I remove this and assume all will work ok? > On June 20, 2017, 7:45 a.m., Sebastian Toader wrote: > > ambari-server/src/main/java/org/apache/ambari/server/security/authorization/Users.java > > Lines 1349 (patched) > > <https://reviews.apache.org/r/60205/diff/1/?file=1753553#file1753553line1349> > > > > Shouldn't use optimistic locking here as well? Nice call.. thanks. Exception should be handled here as well. Thanks for catching that. - Robert ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60205/#review178344 ----------------------------------------------------------- On June 19, 2017, 4:26 p.m., Robert Levas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60205/ > ----------------------------------------------------------- > > (Updated June 19, 2017, 4:26 p.m.) > > > Review request for Ambari, Attila Magyar, Balázs Bence Sári, Eugene > Chekanskiy, Jonathan Hurley, Laszlo Puskas, and Sebastian Toader. > > > Bugs: AMBARI-21216 > https://issues.apache.org/jira/browse/AMBARI-21216 > > > Repository: ambari > > > Description > ------- > > Add support for consecutive login failure accounting where as log-in failures > should increment the {{users.consecutive_failures}} and successful > authentication attempts should reset the value to 0. > > Concurrency should be kept in mind to handle simultaneous authentication > attempts for the same user. > > > Diffs > ----- > > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariServer.java > 01920f86d6 > ambari-server/src/main/java/org/apache/ambari/server/orm/dao/UserDAO.java > 0e28e50709 > > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/UserEntity.java > 66e9003873 > > ambari-server/src/main/java/org/apache/ambari/server/security/authentication/AmbariAuthenticationEventHandler.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/security/authentication/AmbariAuthenticationEventHandlerImpl.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/security/authentication/AmbariAuthenticationException.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/security/authentication/AmbariBasicAuthenticationFilter.java > ac3e15fa0f > > ambari-server/src/main/java/org/apache/ambari/server/security/authentication/AmbariJWTAuthenticationFilter.java > fca8b29fe2 > > ambari-server/src/main/java/org/apache/ambari/server/security/authentication/UserNotFoundException.java > f6c4bcf2a7 > > ambari-server/src/main/java/org/apache/ambari/server/security/authentication/kerberos/AmbariKerberosAuthenticationFilter.java > 1b001ec020 > > ambari-server/src/main/java/org/apache/ambari/server/security/authorization/AmbariLdapAuthenticationProvider.java > 6137b68e99 > > ambari-server/src/main/java/org/apache/ambari/server/security/authorization/AmbariLdapAuthoritiesPopulator.java > 5c482a1a13 > > ambari-server/src/main/java/org/apache/ambari/server/security/authorization/AmbariLocalUserProvider.java > 517efe49b8 > > ambari-server/src/main/java/org/apache/ambari/server/security/authorization/InvalidUsernamePasswordCombinationException.java > db8238184b > > ambari-server/src/main/java/org/apache/ambari/server/security/authorization/Users.java > 35eb255fcb > > ambari-server/src/main/java/org/apache/ambari/server/security/authorization/internal/AmbariInternalAuthenticationProvider.java > c57bdf1a99 > > ambari-server/src/main/java/org/apache/ambari/server/security/authorization/jwt/JwtAuthenticationFilter.java > 3c3a446a61 > ambari-server/src/main/resources/Ambari-DDL-Derby-CREATE.sql 0c86591010 > ambari-server/src/main/resources/Ambari-DDL-MySQL-CREATE.sql 83b1f48049 > ambari-server/src/main/resources/Ambari-DDL-Oracle-CREATE.sql 215c01d7b7 > ambari-server/src/main/resources/Ambari-DDL-Postgres-CREATE.sql 40ba709d1d > ambari-server/src/main/resources/Ambari-DDL-SQLAnywhere-CREATE.sql > 35951f142b > ambari-server/src/main/resources/Ambari-DDL-SQLServer-CREATE.sql b7244abee2 > ambari-server/src/main/resources/webapp/WEB-INF/spring-security.xml > bdbf0de3fa > > ambari-server/src/test/java/org/apache/ambari/server/security/authentication/AmbariBasicAuthenticationFilterTest.java > 18c4ccee77 > > ambari-server/src/test/java/org/apache/ambari/server/security/authentication/AmbariJWTAuthenticationFilterTest.java > 961e65dfbb > > ambari-server/src/test/java/org/apache/ambari/server/security/authentication/kerberos/AmbariKerberosAuthenticationFilterTest.java > 5503fac6ac > > ambari-server/src/test/java/org/apache/ambari/server/security/authorization/AmbariAuthorizationProviderDisableUserTest.java > 33100dd33b > > ambari-server/src/test/java/org/apache/ambari/server/security/authorization/AmbariLdapAuthenticationProviderForDNWithSpaceTest.java > 1bf122e0a1 > > ambari-server/src/test/java/org/apache/ambari/server/security/authorization/AmbariLdapAuthenticationProviderTest.java > d9eb3350fe > > ambari-server/src/test/java/org/apache/ambari/server/security/authorization/AmbariLocalUserProviderTest.java > 65a5400dc7 > > > Diff: https://reviews.apache.org/r/60205/diff/1/ > > > Testing > ------- > > Manually tested in cluster > > #Local test results: > ``` > [INFO] > ------------------------------------------------------------------------ > [INFO] BUILD SUCCESS > [INFO] > ------------------------------------------------------------------------ > [INFO] Total time: 30:58 min > [INFO] Finished at: 2017-06-19T15:16:18-04:00 > [INFO] Final Memory: 108M/1760M > [INFO] > ------------------------------------------------------------------------ > ``` > > #Jasper test results: PENDING > > > Thanks, > > Robert Levas > >
