----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60205/#review178685 -----------------------------------------------------------
Fix it, then Ship it! ambari-server/src/main/java/org/apache/ambari/server/security/authorization/Users.java Lines 75-78 (patched) <https://reviews.apache.org/r/60205/#comment252841> 100 retries? That could be a lot :) ... typically, I'd say a handful is enough here. Especially since you're catching every exception type. ambari-server/src/main/java/org/apache/ambari/server/security/authorization/Users.java Lines 1261 (patched) <https://reviews.apache.org/r/60205/#comment252835> sp: failure ambari-server/src/main/java/org/apache/ambari/server/security/authorization/Users.java Lines 1274 (patched) <https://reviews.apache.org/r/60205/#comment252834> sp: failure ambari-server/src/main/java/org/apache/ambari/server/security/authorization/Users.java Lines 1285-1295 (patched) <https://reviews.apache.org/r/60205/#comment252837> The else isn't needed since you return null above in the if-statement. ambari-server/src/main/java/org/apache/ambari/server/security/authorization/Users.java Lines 1345 (patched) <https://reviews.apache.org/r/60205/#comment252842> I think this method does what you want; it re-runs the command to increment the value in the case of entity being updated behind the scenes. The question is whether it needs to be done for any merge or just for the login attempt value. I'm fine leaving it as-is, especially since user values don't get updated that often, right? ambari-server/src/main/java/org/apache/ambari/server/security/authorization/Users.java Lines 1355 (patched) <https://reviews.apache.org/r/60205/#comment252838> Out of curiousity, any reason you didn't catch the OptimisticLockException directly (or a Runtime exception directly)? Throwable covers everything, yes, but seems kind of broad. - Jonathan Hurley On June 22, 2017, 10:39 a.m., Robert Levas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60205/ > ----------------------------------------------------------- > > (Updated June 22, 2017, 10:39 a.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/audit/event/LoginAuditEvent.java > 9583b84cdb > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariServer.java > 01920f86d6 > > 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/audit/LoginAuditEventTest.java > ac79967ea2 > > 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/3/ > > > Testing > ------- > > Manually tested in cluster > > #Local test results: > ``` > INFO] ------------------------------------------------------------------------ > [INFO] BUILD SUCCESS > [INFO] > ------------------------------------------------------------------------ > [INFO] Total time: 29:50 min > [INFO] Finished at: 2017-06-21T15:43:10-04:00 > [INFO] Final Memory: 86M/1822M > [INFO] > ------------------------------------------------------------------------ > ``` > > #Jasper test results: PENDING > > > Thanks, > > Robert Levas > >
