You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by David Nalley <da...@gnsa.us> on 2012/10/05 20:49:27 UTC
Re: git commit: CLOUDSTACK-121: Fixed "Incorrect username/domainId
login causes NullPointerException "
On Fri, Oct 5, 2012 at 2:36 PM, <al...@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 <ro...@citrix.com>
> Authored: Fri Oct 5 11:32:45 2012 -0700
> Committer: Alena Prokharchyk <al...@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.
Re: git commit: CLOUDSTACK-121: Fixed "Incorrect username/domainId
login causes NullPointerException "
Posted by David Nalley <da...@gnsa.us>.
On Fri, Oct 5, 2012 at 3:13 PM, Chip Childers <ch...@sungard.com> wrote:
> On Fri, Oct 5, 2012 at 2:49 PM, David Nalley <da...@gnsa.us> wrote:
>> On Fri, Oct 5, 2012 at 2:36 PM, <al...@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 <ro...@citrix.com>
>>> Authored: Fri Oct 5 11:32:45 2012 -0700
>>> Committer: Alena Prokharchyk <al...@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.
Re: git commit: CLOUDSTACK-121: Fixed "Incorrect username/domainId
login causes NullPointerException "
Posted by Chip Childers <ch...@sungard.com>.
On Fri, Oct 5, 2012 at 2:49 PM, David Nalley <da...@gnsa.us> wrote:
> On Fri, Oct 5, 2012 at 2:36 PM, <al...@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 <ro...@citrix.com>
>> Authored: Fri Oct 5 11:32:45 2012 -0700
>> Committer: Alena Prokharchyk <al...@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.