search

collective / collective.pwexpiry

Emulate Active Directory password complexity requirements.
1 stars 6 forks source link

Standarize messages #37

Closed frapell closed 5 years ago

frapell commented 5 years ago

The reason for these changes is to improve security. The way this works now is, Plone will give its standard error when someone enters an invalid password:

image

If you enter your pw several times wrong and your account gets locked, and now you enter your password correctly, the error changes:

image

This allows for an attacker to use brute force and find the actual password for a user.

This PR solves this by standarizing the same error message, wether the password is incorrect or the user was locked:

image

frapell commented 5 years ago

@frisi Can you improve both error messages translations? I marked them as fuzzy

https://github.com/collective/collective.pwexpiry/blob/84a3426bec6d79570e79435435c01fd54ea05331/collective/pwexpiry/locales/de/LC_MESSAGES/collective.pwexpiry.po#L35 and https://github.com/collective/collective.pwexpiry/blob/84a3426bec6d79570e79435435c01fd54ea05331/collective/pwexpiry/locales/de/LC_MESSAGES/collective.pwexpiry.po#L48

@giacomos Could you improve the error message for the italian translation? I marked them as fuzzy

https://github.com/collective/collective.pwexpiry/blob/84a3426bec6d79570e79435435c01fd54ea05331/collective/pwexpiry/locales/it/LC_MESSAGES/collective.pwexpiry.po#L33 and https://github.com/collective/collective.pwexpiry/blob/84a3426bec6d79570e79435435c01fd54ea05331/collective/pwexpiry/locales/it/LC_MESSAGES/collective.pwexpiry.po#L45

@hvelarde Can you improve the error message for the portuguese translation? I marked them as fuzzy

https://github.com/collective/collective.pwexpiry/blob/84a3426bec6d79570e79435435c01fd54ea05331/collective/pwexpiry/locales/pt_BR/LC_MESSAGES/collective.pwexpiry.po#L32 and https://github.com/collective/collective.pwexpiry/blob/84a3426bec6d79570e79435435c01fd54ea05331/collective/pwexpiry/locales/pt_BR/LC_MESSAGES/collective.pwexpiry.po#L44

frapell commented 5 years ago

@frisi You are right, I had removed it because I thought it wasn't needed, but now that you mention, I think you are right, and it is back.

The message now looks like this, whether the account is actually locked, or the user just entered a wrong password:

image

Also, there is no new logic to review, only a change in the error message shown.

frapell commented 5 years ago

@frisi Did you have a chance to review this? I am in need to merge this and release a new version... Thanks !

frapell commented 5 years ago

@frisi Well, I thought so as well, however i18ndude was not able to recognize that they were coming from a different message factory... In logged_in.cpy I tried doing this:

from Products.CMFPlone import PloneMessageFactory as pmf_
from collective.pwexpiry.config import _

And then I used _() only for the new messages, which includes the part of being blocked, and pmf_() for the unaltered messages that already exist in plone domain.

This didn't work, i18ndude continued to pull all the translations as if they were from collective.pwexpiry.

I did however, went to plone.app.locales to pull the original translations to be the same...

Do you have any suggestion ?

frisi commented 5 years ago

i can remember that i had a similar problem and discussion with @mauritsvanrees on another package (but can't find where to look that up right now :-( )

iirc i18ndude relies on the pattern _( for extracting the messageids. so as long as you name the foreign factory differently the messageids do not get extracted. (eg. _plone = MessageFactory("plone") should do the trick

mauritsvanrees commented 5 years ago

@frisi is correct.

frapell commented 5 years ago

@mauritsvanrees Do you know if this should work in .vpy files as well? I have copied the skins/plone_login/login_formvalidate.vpy file from CMFPlone into skins/pwexpiry and what I did now is replace ```from collective.pwexpiry.config import ``` with

from zope.i18nmessageid import MessageFactory

_p = MessageFactory("collective.pwexpiry")
_ = MessageFactory('plone')

And then I am using _() with all the unchanged messages, except the ones with the "your account might be locked" part, where I am using _p()

However i18ndude still extracts all messages under the collective.pwexpiry domain...

Also, I tried calling the rebuild-pot for the "plone" domain, and it extracts the ones for collective.pwexpiry too...

mauritsvanrees commented 5 years ago

First of all, you cannot use _p in skin scripts. It will throw an Unauthorized exception because it starts with an underscore. The exception to this rule is a single underscore, as that is white listed somewhere in Zope. So you would need to rename _p.

i18ndude does not analyse the Python code to see which domain a MessageFactory is. It simply looks for an underscore function that is called. It can only look for messages of a specific domain in templates and zcml/xml.

Options:

frapell commented 5 years ago

@mauritsvanrees Thank you very much, that did the trick...

@frisi all ready

frapell commented 5 years ago

@hvelarde and @giacomos Could you guys update your translations? I am in need of cutting a new release with this fix... thanks !

frisi commented 5 years ago

do you have an idea why tests are failing @frapell ?

frapell commented 5 years ago

@frisi There was an issue with version pins on each Plone version, it is fixed now.

frapell commented 5 years ago

@hvelarde @frisi This is incredibly frustrating, Do you know why build 80 https://travis-ci.org/collective/collective.pwexpiry/builds/458089772 would fail while build 79 https://travis-ci.org/collective/collective.pwexpiry/builds/458089751 worked just fine? they are both the same tests for the same commit.

In my local instance, tests are passing fine for 4.3, 5.0 and 5.1

I need this merged and a new release for this package, and I already wasted almost the whole day today :(

frisi commented 5 years ago

hey @frapell.

great you added additional tests!

sorry, i'm not really familiar with robot tests. just had a quick look at the failing test which says:

==============================================================================
Test User Locked P5                                                           
==============================================================================
Test Email Error Message With Wrong Password                          | FAIL |
ValueError: Element locator 'collective pwexpiry validity_period' did not match any elements.

if the input shows up on your local tests i'd say go ahead, merge and release. the missing test can be fixed later. afaict your pr solves most of the failing tests and did not introduce new problems.

frapell commented 5 years ago

@frisi Yeah, and as I said, build 79 pass fine, while build 80 breaks, and they are both builds for the same commit, so if the test is really failing, it should fail on both builds... I don't know, maybe the test speed needs to be slowed down... maybe @hvelarde would chime in and share his thoughts since he spends a lot of time with setups like this, and might have a better understanding...

In any case, I cannot merge until you approve it :) so when you have a chance, could you? Thanks

frapell commented 5 years ago

@frisi great! I was trying to find a "Restart" button somewhere around the travis UI but for the life of me, I couldn't... and now when I go there, I totally see it... thanks for that.

hvelarde commented 5 years ago

@frapell I'm sorry, I'm very busy ATM.