Hi,

The module by default saving passwords for all users into 'password_policy_history' table, which is not required. I think, it should save passwords only for users have role added to password policy.

Could you please confirm, is that a bug or something you don't support ?

Thanks,
Balraj

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

BalrajB’s picture

Issue tags: +If this is bug, +need to add this patch
FileSize
1 KB

If this is bug, need to add this patch (file attached)

BalrajB’s picture

Priority: Normal » Major
Status: Active » Needs review

Need to review above patch.

Status: Needs review » Needs work
BalrajB’s picture

Thanks for quick reply. that was my fault, did not upload correct patch. Correct one is here, lets see if still fail.

BalrajB’s picture

Status: Needs work » Needs review
erikwebb’s picture

The balance here is that if you need to add any new roles with history constraints, they won't be usable at first.

I'm not opposed to this change, but I'd want to make sure we are gaining something here.

BalrajB’s picture

The main advantage of above patch is:
on hight traffic site, the 'password_policy_history' table would become massive ( because this would add password history for all users) which is not good. and by above solution you can still save password history once you add a role to same or new policy.

Is that answer your question ?

erikwebb’s picture

I guess my thought was really why you would use a password policy for only a minority of your users. That would be the use case for this patch. If a majority of users are already using a policy, then the overhead would be minimal in the event you wanted to add a policy to those unrestricted roles.

AohRveTPV’s picture

Issue summary: View changes
Issue tags: -

In my opinion it would be better to not store old password hashes at all by default. Password hashes are sensitive data and add security risk. Worse, administrators who do not use the history constraint may not expect or be aware that they are being stored. So I think it would also be good to store hashes only for users to whom a history constraint applies, and store them only once the constraint is enabled.

If administrators want to store old passwords in case they later decide to enable a history constraint, there could be a setting to enable storage of old password hashes for all users.

The benefit of storing old password hashes by default without administrator knowledge is limited anyway, because they are only stored since the module was enabled and not before. I am guessing the typical use case for Password Policy is to install it, immediately configure policies, and enable them. It is probably not so much to install Password Policy, wait for a password history to accumulate, then configure and enable password policies.

However, changing the default behavior of password history in 7.x-1.x/6.x-1.x could cause confusion and risks causing problems in a stable release. I think more would need to be changed than in the patch (e.g., README.txt and a configuration setting). So I currently would be in favor of implementing this change by default in 7.x-2.x and leaving the 7.x-1.x/6.x-1.x behavior as it is. Maybe it could be implemented in 7.x-1.x/6.x-1.x later as a non-default option.

AohRveTPV’s picture

Version: 6.x-1.6 » 7.x-2.x-dev
Status: Needs review » Active
lisotton’s picture

I tried to implement something more generic, which will not store the history for users that don't match any of the configured policy.

We have a case where 99% of the users authenticate with CAS module and the CAS module generate a new password every time a user authenticate, so a new record is created in the "password_policy_history" for every authentication.

lisotton’s picture