incubator-cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From David Nalley <da...@gnsa.us>
Subject Re: git commit: CLOUDSTACK-121: Fixed "Incorrect username/domainId login causes NullPointerException "
Date Fri, 05 Oct 2012 19:14:56 GMT
On Fri, Oct 5, 2012 at 3:13 PM, Chip Childers <chip.childers@sungard.com> wrote:
> On Fri, Oct 5, 2012 at 2:49 PM, David Nalley <david@gnsa.us> wrote:
>> On Fri, Oct 5, 2012 at 2:36 PM,  <alena1108@apache.org> wrote:
>>> Updated Branches:
>>>   refs/heads/4.0 8ba6c8b17 -> f7ebb76f5
>>>
>>>
>>> CLOUDSTACK-121: Fixed "Incorrect username/domainId login causes NullPointerException
"
>>>
>>>
>>> Project: http://git-wip-us.apache.org/repos/asf/incubator-cloudstack/repo
>>> Commit: http://git-wip-us.apache.org/repos/asf/incubator-cloudstack/commit/f7ebb76f
>>> Tree: http://git-wip-us.apache.org/repos/asf/incubator-cloudstack/tree/f7ebb76f
>>> Diff: http://git-wip-us.apache.org/repos/asf/incubator-cloudstack/diff/f7ebb76f
>>>
>>> Branch: refs/heads/4.0
>>> Commit: f7ebb76f57a0c88c8b379aacb1a4e3fd653a325f
>>> Parents: 8ba6c8b
>>> Author: Rohit Yadav <rohit.yadav@citrix.com>
>>> Authored: Fri Oct 5 11:32:45 2012 -0700
>>> Committer: Alena Prokharchyk <alena.prokharchyk@citrix.com>
>>> Committed: Fri Oct 5 11:33:21 2012 -0700
>>>
>>> ----------------------------------------------------------------------
>>>  api/src/com/cloud/user/UserAccount.java           |    2 ++
>>>  server/src/com/cloud/user/AccountManagerImpl.java |   15 ++++++++-------
>>>  2 files changed, 10 insertions(+), 7 deletions(-)
>>> ----------------------------------------------------------------------
>>>
>>>
>>> http://git-wip-us.apache.org/repos/asf/incubator-cloudstack/blob/f7ebb76f/api/src/com/cloud/user/UserAccount.java
>>> ----------------------------------------------------------------------
>>> diff --git a/api/src/com/cloud/user/UserAccount.java b/api/src/com/cloud/user/UserAccount.java
>>> index 734e16b..2a6bd4f 100644
>>> --- a/api/src/com/cloud/user/UserAccount.java
>>> +++ b/api/src/com/cloud/user/UserAccount.java
>>> @@ -56,4 +56,6 @@ public interface UserAccount {
>>>      String getRegistrationToken();
>>>
>>>      boolean isRegistered();
>>> +
>>> +    int getLoginAttempts();
>>>  }
>>>
>>> http://git-wip-us.apache.org/repos/asf/incubator-cloudstack/blob/f7ebb76f/server/src/com/cloud/user/AccountManagerImpl.java
>>> ----------------------------------------------------------------------
>>> diff --git a/server/src/com/cloud/user/AccountManagerImpl.java b/server/src/com/cloud/user/AccountManagerImpl.java
>>> index fa9fafb..4d3f45c 100755
>>> --- a/server/src/com/cloud/user/AccountManagerImpl.java
>>> +++ b/server/src/com/cloud/user/AccountManagerImpl.java
>>> @@ -1862,24 +1862,25 @@ public class AccountManagerImpl implements AccountManager,
AccountService, Manag
>>>              }
>>>
>>>              UserAccount userAccount = _userAccountDao.getUserAccount(username,
domainId);
>>> -            UserAccountVO user = _userAccountDao.findById(userAccount.getId());
>>> -            if (user != null) {
>>> -                if ((user.getState().toString()).equals("enabled")) {
>>> -                    if (!isInternalAccount(user.getType())) {
>>> +            if (userAccount != null) {
>>> +                if (userAccount.getState().equalsIgnoreCase(Account.State.enabled.toString()))
{
>>> +                    if (!isInternalAccount(userAccount.getType())) {
>>>                          //Internal accounts are not disabled
>>> -                        int attemptsMade = user.getLoginAttempts() + 1;
>>> +                        int attemptsMade = userAccount.getLoginAttempts() +
1;
>>>                          if (attemptsMade < _allowedLoginAttempts) {
>>>                              updateLoginAttempts(userAccount.getId(), attemptsMade,
false);
>>>                              s_logger.warn("Login attempt failed. You have "
+ ( _allowedLoginAttempts - attemptsMade ) + " attempt(s) remaining");
>>>                          } else {
>>>                              updateLoginAttempts(userAccount.getId(), _allowedLoginAttempts,
true);
>>> -                            s_logger.warn("User " + user.getUsername() + " has
been disabled due to multiple failed login attempts." +
>>> +                            s_logger.warn("User " + userAccount.getUsername()
+ " has been disabled due to multiple failed login attempts." +
>>>                                      " Please contact admin.");
>>>                          }
>>>                      }
>>>                  } else {
>>> -                    s_logger.info("User " + user.getUsername() + " is disabled/locked");
>>> +                    s_logger.info("User " + userAccount.getUsername() + " is
disabled/locked");
>>>                  }
>>> +            } else {
>>> +                s_logger.warn("Authentication failure: No user with name " +
username + " for domainId " + domainId);
>>>              }
>>>              return null;
>>>          }
>>>
>>
>> This is a bit of information leakage? I mean we are essentially
>> telling someone what usernames don't exist, which by process of
>> elimination means that folks can determine what user accounts are
>> valid.
>>
>
> Aren't these just log statements?  That means they are within the
> system, and therefore aren't exposing anything new to an operator.

Doh, yes- you are right.

Mime
View raw message