feat: password complexity option on DB Auth #1687

dpgaspar · 2021-09-06T12:50:25Z

Description

New feature to support password complexity validation to AUTH database users.

Adds the following new optional config keys:

FAB_PASSWORD_COMPLEXITY_VALIDATOR: Hook for a custom function to validate password complexity
FAB_PASSWORD_COMPLEXITY_ENABLED: Enables new FAB default password complexity validator

This PR also adds partial mypy type checking and a small test refactor

ADDITIONAL INFORMATION

Has associated issue: [feature request] Password policy/Complexity #1576
Is CRUD MVC related.
Is Auth, RBAC security related.
Changes the security db schema.
Introduces new feature
Removes existing feature

codecov · 2021-09-06T12:53:25Z

Codecov Report

Merging #1687 (642b9da) into master (4d7dad0) will increase coverage by 0.12%.
The diff coverage is 96.77%.

@@            Coverage Diff             @@
##           master    #1687      +/-   ##
==========================================
+ Coverage   76.43%   76.55%   +0.12%     
==========================================
  Files          55       55              
  Lines        8032     8061      +29     
==========================================
+ Hits         6139     6171      +32     
+ Misses       1893     1890       -3

Flag	Coverage Δ
python	`76.55% <96.77%> (+0.12%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
flask_appbuilder/security/views.py	`62.02% <91.30%> (+0.27%)`	⬆️
flask_appbuilder/exceptions.py	`100.00% <100.00%> (ø)`
flask_appbuilder/models/base.py	`74.03% <100.00%> (+0.29%)`	⬆️
flask_appbuilder/security/forms.py	`100.00% <100.00%> (ø)`
flask_appbuilder/validators.py	`100.00% <100.00%> (ø)`
flask_appbuilder/views.py	`65.68% <0.00%> (+0.42%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d7dad0...642b9da. Read the comment docs.

dpgaspar · 2021-09-07T09:23:15Z

@ashb @potiuk

kamalkeshavani-aiinside · 2021-09-07T09:25:02Z

docs/security.rst

+- At least 3 Lowercase letters
+- At least 1 special character
+- At least 2 numeric digits
+- At least 10 total digits


It should be 'At least 10 total characters'.

fixed, thks!

LGTM, it has all the options I was looking for.

villebro

One personal opinion + one minor improvement proposal

villebro · 2021-09-07T10:03:55Z

flask_appbuilder/security/forms.py

@@ -5,6 +5,7 @@

 from ..fieldwidgets import BS3PasswordFieldWidget, BS3TextFieldWidget
 from ..forms import DynamicForm
+from ..validators import PasswordComplexityValidator


Just an opinion, but while we're on the topic of improving typing/readability, I somehow always find fully qualified imports more pleasant to read:

from flask_appbuilder.validators import PasswordComplexityValidator

over

from ..validators import PasswordComplexityValidator

Currently I prefer them also, but I'm following the current code base "convention", I plan to change them on a different PR

Agree. Relative imports are evil.

Coming as strong advise in Airflow 2.2: https://github.com/apache/airflow/blob/main/docs/apache-airflow/modules_management.rst#dont-use-relative-imports

villebro · 2021-09-07T10:20:52Z

flask_appbuilder/validators.py

+password_complexity_regex = re.compile(
+    r"""(
+    ^(?=.*[A-Z].*[A-Z])                # at least two capital letters
+    (?=.*[!@#$&*])                     # at least one of these special characters


To not "force" users to use a highly restricted set of special characters, would it be better to do a negative match for non-special characters here? Something like

(?!([a-zA-Z0-9])).

villebro

LGTM

potiuk · 2021-09-07T10:52:14Z

Oh cool. did not have time to follow up on this one. let me take a look.

potiuk · 2021-09-07T10:55:42Z

flask_appbuilder/models/base.py

@@ -47,6 +47,13 @@ class BaseInterface(object):
    def __init__(self, obj: Type[Any]):
        self.obj = obj

+    def __getattr__(self, name: str) -> Any:


Nice. I did not know about it !

potiuk · 2021-09-07T11:06:24Z

flask_appbuilder/tests/security/test_password_complexity.py

+from ..base import FABTestCase
+
+
+class PasswordComplexityTestCase(FABTestCase):


Suggestion: Maybe you could use https://pypi.org/project/parameterized/ here?

And few other places.

did not know that one, only this: https://docs.pytest.org/en/6.2.x/parametrize.html#pytest-mark-parametrize-parametrizing-test-functions.

added, looks better and it encourages to write more test cases (which I did) :)

potiuk · 2021-09-07T11:20:03Z

flask_appbuilder/validators.py

+                password_complexity_validator(field.data)
+            except PasswordComplexityValidationError as exc:
+                raise ValidationError(str(exc))
+        if current_app.config.get("FAB_PASSWORD_COMPLEXITY_ENABLED", False):


I think the current set of configuration variables is a bit confusing.

When I see FAB_PASSWORD_COMPLEXITY_ENABLED - I think this parameter configures password complexity check in general, so I expect that if FAB_PASSWORD_COMPLEXITY_ENABLED is "false" then no validation is done (even if I have my own custom validator configured in FAB_PASSWORD_COMPLEXITY_VALIDATOR).

Currently, in order to enable ONLY custom validator you have to have effectively this combination:

FAB_PASSWORD_COMPLEXITY_ENABLED = false FAB_PASSWORD_COMPLEXITY_VALIDATOR = your_validator

This is pretty confusing (because I'd expect FAB_PASSWORD_COMPLEXITY_ENABLED = false disables validation of password completely.

I think two one of the two approaches would be better:

a) FAB_PASSWORD_COMPLEXITY_ENABLED - enables validation in general, and FAB_PASSWORD_COMPLEXITY_VALIDATOR replaces the default validator with the custom one. In this case you cannot run both custom and default validation at the same time

b) FAB_PASSWORD_COMPLEXITY_ENABLED - should be renamed to FAB_PASSWORD_DEFAULT_COMPLEXITY_VALIDATOR - to not make impression that whole feature is disabled by setting it to false

I have slight preference for option a) (I think it's better to always run one validator - either default or custom, you can always derive your custom validator from the default one and use it internally) - but I think both a) and b) are better than current proposal.

Very good point, went for option a)

Cool. Looks great now. Then I have one more NIT comment :)

potiuk · 2021-09-07T11:22:29Z

Thanks for that @dpgaspar - I had some other pressing issues in the meantime, so I did not pick it up as promised, sorry for that. I hoe you find my comments useful

dpgaspar · 2021-09-07T13:14:59Z

Thanks for that @dpgaspar - I had some other pressing issues in the meantime, so I did not pick it up as promised, sorry for that. I hoe you find my comments useful

No worries at all, I had this one on my TODO list also. Thank you for the review and yes the comments were great, looks much better now!

potiuk

Small NIT after choosing option a)

potiuk · 2021-09-07T14:18:00Z

flask_appbuilder/validators.py

+                password_complexity_validator(field.data)
+            except PasswordComplexityValidationError as exc:
+                raise ValidationError(str(exc))
+        if current_app.config.get("FAB_PASSWORD_COMPLEXITY_ENABLED", False):


Cool. Looks great now. Then I have one more NIT comment :)

docs/config.rst

Co-authored-by: Jarek Potiuk <jarek@potiuk.com>

potiuk · 2021-09-07T15:47:07Z

Fantastic!

potiuk · 2021-09-07T23:18:23Z

🎉 🎉 🎉 🎉 🎉

feat: password complexity option on DB Auth

486859f

dpgaspar added 4 commits September 6, 2021 14:29

drop wtforms dependency on this feature

08d23d1

add tests

9943faf

fix tests

1d1a281

fix tests

ef54f07

dpgaspar marked this pull request as ready for review September 6, 2021 17:02

dpgaspar added 2 commits September 6, 2021 22:10

remove password policy warning field description

041bb74

lint

f73e85a

kamalkeshavani-aiinside reviewed Sep 7, 2021

View reviewed changes

dpgaspar added 2 commits September 7, 2021 10:37

address comment

c5747ed

change quickhowto

307053c

villebro reviewed Sep 7, 2021

View reviewed changes

accept more special characters

f43f703

villebro approved these changes Sep 7, 2021

View reviewed changes

potiuk reviewed Sep 7, 2021

View reviewed changes

dpgaspar added 3 commits September 7, 2021 13:21

fir regex and config key logic

53a51cd

fix test and docs with new config logic

a37ad3d

improve unittest use parameterized

dfd336e

potiuk reviewed Sep 7, 2021

View reviewed changes

Update docs/config.rst

642b9da

Co-authored-by: Jarek Potiuk <jarek@potiuk.com>

potiuk approved these changes Sep 7, 2021

View reviewed changes

dpgaspar merged commit c46cac3 into master Sep 7, 2021

dpgaspar deleted the feat/password-complexity-hook branch September 7, 2021 22:50

dpgaspar mentioned this pull request Sep 14, 2021

release: 3.3.3 #1699

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: password complexity option on DB Auth #1687

feat: password complexity option on DB Auth #1687

dpgaspar commented Sep 6, 2021 •

edited

Loading

codecov bot commented Sep 6, 2021 •

edited

Loading

dpgaspar commented Sep 7, 2021

kamalkeshavani-aiinside Sep 7, 2021

dpgaspar Sep 7, 2021

kamalkeshavani-aiinside Sep 7, 2021

villebro left a comment

villebro Sep 7, 2021

dpgaspar Sep 7, 2021

potiuk Sep 7, 2021

potiuk Sep 7, 2021

villebro Sep 7, 2021

dpgaspar Sep 7, 2021

villebro left a comment

potiuk commented Sep 7, 2021

potiuk Sep 7, 2021

potiuk Sep 7, 2021

potiuk Sep 7, 2021

dpgaspar Sep 7, 2021

dpgaspar Sep 7, 2021 •

edited

Loading

potiuk Sep 7, 2021 •

edited

Loading

dpgaspar Sep 7, 2021

potiuk Sep 7, 2021

potiuk commented Sep 7, 2021

dpgaspar commented Sep 7, 2021 •

edited

Loading

potiuk left a comment

potiuk Sep 7, 2021

potiuk commented Sep 7, 2021

potiuk commented Sep 7, 2021

		from ..base import FABTestCase


		class PasswordComplexityTestCase(FABTestCase):

feat: password complexity option on DB Auth #1687

feat: password complexity option on DB Auth #1687

Conversation

dpgaspar commented Sep 6, 2021 • edited Loading

Description

ADDITIONAL INFORMATION

codecov bot commented Sep 6, 2021 • edited Loading

Codecov Report

dpgaspar commented Sep 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

potiuk commented Sep 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpgaspar Sep 7, 2021 • edited Loading

Choose a reason for hiding this comment

potiuk Sep 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

potiuk commented Sep 7, 2021

dpgaspar commented Sep 7, 2021 • edited Loading

potiuk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

potiuk commented Sep 7, 2021

potiuk commented Sep 7, 2021

dpgaspar commented Sep 6, 2021 •

edited

Loading

codecov bot commented Sep 6, 2021 •

edited

Loading

dpgaspar Sep 7, 2021 •

edited

Loading

potiuk Sep 7, 2021 •

edited

Loading

dpgaspar commented Sep 7, 2021 •

edited

Loading