[Yanel-dev] Very poor performance of latest Yarep/Security Code

[basZero]

Hi Michael,

I've implemented the following which works as expected:
- fast performance, equal to the yanel version before the introduction of
BCrypt
- also does the autoupdate of passwords in old formats

Code Changes in org.wyona.security.impl.yarep.YarepUser.java:

Constructor public YarepUser(UserManager userManager, GroupManager
groupManager, Node node):
I just removed this section:
if(hashingAlgorithm != null && !hashingAlgorithm.startsWith("bcrypt")) {
  upgradeDoubleHash(this.hashedPassword, this.hashingAlgorithm);
}

and inserted it into a new overriding method:

@Override
public void save() throws AccessManagementException {
    try {
        // Check if we need to upgrade the password hash
        if(hashingAlgorithm != null &&
!hashingAlgorithm.startsWith("bcrypt")) {
            upgradeDoubleHash(this.hashedPassword, this.hashingAlgorithm);
        }
    } catch (Exception e) {
    }
    super.save();
}


The result is obvious, only if the user object gets saved and the password
is not yet in the new format (bcrypt), the hashed passwords gets upgraded
and then saved (calling save() of the YarepItem class).

This seems to me a very solid solution.

What do you think?

Do you need a git pull request for this?

This is NOT URGENT for me personally, as I have overwritten the class
YarepUser in my realm, but I think it is an essential patch for yanel and
users who are using yanel with a high user base.

Cheers,
Balz




On Fri, Aug 12, 2016 at 2:45 PM, basZero <baszero at gmail.com> wrote:

> Hi Michael,
>
> I found the root cause for the very poor performance!
> As you know of course, the user's passwords are encrypted and generated
> with the newly introduced bcrypt algorithm.
>
> When I did the upgrade to the latest Yanel version I was aware of this
> upgrade, however I somehow assumed that the password-update in the user XML
> would only be updated at LOGIN or when the user XML gets saved.
>
> My assumption was wrong.
>
> In the YarepUser.java class file I see in the constructor, that for each
> user object that gets created (even if it is created for read-only
> purposes), it is checked whether the password is stored in another format
> than bcrypt, and if so, the new password hash gets generated. And THIS
> consumes TIME!
>
> Code Line:
> upgradeDoubleHash(this.hashedPassword, this.hashingAlgorithm);
>
> I think it is a very bad approach to put that logic in the constructor,
> because reading an object means that you want to get an object as
> represented by a stored XML.
>
> A better way would be to put the logic into the save-logic of a user,
> because then it makes sense to store the new hashed password.
>
> What do you think?
>
> I will now code a workaround and let you know where I put it.
>
> Cheers, Balz
>
> On Thu, Aug 11, 2016 at 7:49 PM, Michael Wechner <
> michael.wechner at wyona.com> wrote:
>
>> Hi Balz
>>
>> I am currently on vacation (until August 17th), but what you describe
>> sounds really not good at all.
>>
>> I will have a look at it as soon as I will be back. Please keep us posted
>> in case you find something out in the meantime.
>>
>> Thanks
>>
>> Michael
>>
>> Am 11.08.16 um 09:59 schrieb basZero:
>>
>> Hi,
>>
>> please read this with high priority as it could be very critical to
>> performance of Yanel with many users:
>> https://github.com/wyona/security/issues/6
>>
>> Cheers, Bas
>>
>>
>>
>>
>> --
>> Yanel-development mailing list Yanel-development at wyona.com
>> http://mx2.wyona.com/cgi-bin/mailman/listinfo/yanel-development
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mx2.wyona.com/pipermail/yanel-development/attachments/20160812/732592c0/attachment-0001.html>