HumanSignal / label-studio

Label Studio is a multi-type data labeling and annotation tool with standardized output format
https://labelstud.io
Apache License 2.0
18.34k stars 2.3k forks source link

More extensive password validation #5778

Closed staubertTim closed 4 months ago

staubertTim commented 5 months ago

Password validation is only done for a minimum length of 8 characters. With this "testtest" ist a valid password. I want to be able to easily apply more custom validators next to the current standard Django validators. Where can I add my custom Djang validator so that it is actually used?

jombooth commented 4 months ago

Hi @staubertTim,

You can add additional validators here, assuming you're building LS from source: https://github.com/HumanSignal/label-studio/blob/ec2663411eb345930c7f3c7320e20d69c42a3f65/label_studio/core/settings/base.py#L291-L298

I would have thought testtest would be blocked by CommonPasswordValidator, but not sure on that.

staubertTim commented 4 months ago

Hi @jombooth,

thank you for taking the time to answer! That's what I thought as well so I went ahead and changed those - WITH NO EFFECT. I'm afraid these Django Validators are not even used. You can verify that by using these Validators and then building from source:

AUTH_PASSWORD_VALIDATORS = [ {'NAME': 'django.contrib.auth.password_validation.UserAttributeSimilarityValidator'}, { 'NAME': 'django.contrib.auth.password_validation.MinimumLengthValidator', 'OPTIONS': {'min_length': 10}, }, {'NAME': 'django.contrib.auth.password_validation.CommonPasswordValidator'}, {'NAME': 'django.contrib.auth.password_validation.NumericPasswordValidator'}, ]

Normally Label Studio Passwords need to be 8 characters long and I would expect that they now need to be ten characters long - which is NOT the case, although this parameter is build in by default into the MinimumLengthValidator.

I then traced error messages from the frontend and realized that password checking is done in label_studio/users/forms.py in the class UserSignupForm and the clean_password method where the only applied check is:

if len(password) < PASS_MIN_LENGTH: raise forms.ValidationError(PASS_LENGTH_ERROR)

Once I changed PASS_MIN_LENGTH to 10 I could actually only use passwords that are ten characters long. I will now try to monkey-patch this method in my docker build. But still I think it is suboptimal to have these Django validators and not apply them (if I am not mistaken).

Is this intended or a bug? Happy to hear your thoughts.