----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6858/#review10998 -----------------------------------------------------------
server/src/com/cloud/configuration/Config.java <https://reviews.apache.org/r/6858/#comment23575> The global config should read as incorrect.login.attempts.allowed server/src/com/cloud/user/AccountManagerImpl.java <https://reviews.apache.org/r/6858/#comment23576> Should be _allowedLoginAttempts instead Refer http://docs.cloudstack.org/CloudStack_Documentation/Design_Documents/Coding_Conventions server/src/com/cloud/user/AccountManagerImpl.java <https://reviews.apache.org/r/6858/#comment23577> Use NumbersUtil to set a default value. See example below String value = configs.get(Config.AccountCleanupInterval.key()); _cleanupInterval = NumbersUtil.parseInt(value, 60 * 60 * 24); // 1 day. server/src/com/cloud/user/AccountManagerImpl.java <https://reviews.apache.org/r/6858/#comment23578> The function signature should be something like this :- updateLoginAttempts(Long accountId, int failedLoginAttempts , boolean isDisableAllowed) server/src/com/cloud/user/AccountManagerImpl.java <https://reviews.apache.org/r/6858/#comment23586> Can you please refer to other dao functions to see how to use transactions and how to lock rows server/src/com/cloud/user/AccountManagerImpl.java <https://reviews.apache.org/r/6858/#comment23585> Dont you have to do this for all the users belonging to this account ? server/src/com/cloud/user/AccountManagerImpl.java <https://reviews.apache.org/r/6858/#comment23580> hardcoding to 2 scares me. I would say create a function boolean isInternalAccount(Long accountId) server/src/com/cloud/user/AccountManagerImpl.java <https://reviews.apache.org/r/6858/#comment23583> Can you assume that you will always get acct object here ? It can also be null correct ? server/src/com/cloud/user/AccountManagerImpl.java <https://reviews.apache.org/r/6858/#comment23579> Move this to configure function server/src/com/cloud/user/AccountManagerImpl.java <https://reviews.apache.org/r/6858/#comment23581> The value can be 0 meaning for the 1st incorrect login u disable the account server/src/com/cloud/user/AccountManagerImpl.java <https://reviews.apache.org/r/6858/#comment23582> There shouldnt be - 1 here. server/src/com/cloud/user/AccountManagerImpl.java <https://reviews.apache.org/r/6858/#comment23584> Please say - Account is disabled. - Nitin Mehta On Sept. 3, 2012, 2 p.m., saksham srivastava wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6858/ > ----------------------------------------------------------- > > (Updated Sept. 3, 2012, 2 p.m.) > > > Review request for cloudstack, Devdeep Singh, Rajesh Battala, and Alena > Prokharchyk. > > > Description > ------- > > Added global setting login.attempts.allowed which defines the maximum > incorrect password attempts allowed. > Also after the maximum attempts are reached the user account is disabled. > > > This addresses bug CS-10219. > > > Diffs > ----- > > core/src/com/cloud/user/UserAccountVO.java 5e7c018 > server/src/com/cloud/configuration/Config.java ebcd070 > server/src/com/cloud/user/AccountManagerImpl.java 38153f3 > setup/db/create-schema.sql fa933e3 > setup/db/db/schema-302to40.sql aaf23e6 > > Diff: https://reviews.apache.org/r/6858/diff/ > > > Testing > ------- > > Verified locally. > > > Thanks, > > saksham srivastava > >
