Fix adding roles when identity is username by singingwolfboy · Pull Request #686 · pallets-eco/flask-security-3.0

singingwolfboy · 2017-07-28T04:30:03Z

This resolves a bug that manifests under the following conditions:

app.config["SECURITY_USER_IDENTITY_ATTRIBUTES"] = ["username"]
SQLAlchemy datastore
User model has no "email" field
flask roles add <username> <role>

Traceback:

$ flask roles add singingwolfboy superuser
Traceback (most recent call last):
  File "/Users/singingwolfboy/.virtualenvs/bradley/lib/python3.6/site-packages/sqlalchemy/orm/base.py", line 379, in _entity_descriptor
    return getattr(entity, key)
AttributeError: type object 'User' has no attribute 'email'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/singingwolfboy/.virtualenvs/bradley/bin/flask", line 11, in <module>
    sys.exit(main())
  File "/Users/singingwolfboy/.virtualenvs/bradley/lib/python3.6/site-packages/flask/cli.py", line 514, in main
    cli.main(args=args, prog_name=name)
  File "/Users/singingwolfboy/.virtualenvs/bradley/lib/python3.6/site-packages/flask/cli.py", line 381, in main
    return AppGroup.main(self, *args, **kwargs)
  File "/Users/singingwolfboy/.virtualenvs/bradley/lib/python3.6/site-packages/click/core.py", line 697, in main
    rv = self.invoke(ctx)
  File "/Users/singingwolfboy/.virtualenvs/bradley/lib/python3.6/site-packages/click/core.py", line 1066, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/Users/singingwolfboy/.virtualenvs/bradley/lib/python3.6/site-packages/click/core.py", line 1066, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/Users/singingwolfboy/.virtualenvs/bradley/lib/python3.6/site-packages/click/core.py", line 895, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/Users/singingwolfboy/.virtualenvs/bradley/lib/python3.6/site-packages/click/core.py", line 535, in invoke
    return callback(*args, **kwargs)
  File "/Users/singingwolfboy/.virtualenvs/bradley/lib/python3.6/site-packages/click/decorators.py", line 17, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "/Users/singingwolfboy/.virtualenvs/bradley/lib/python3.6/site-packages/flask/cli.py", line 257, in decorator
    return __ctx.invoke(f, *args, **kwargs)
  File "/Users/singingwolfboy/.virtualenvs/bradley/lib/python3.6/site-packages/click/core.py", line 535, in invoke
    return callback(*args, **kwargs)
  File "/Users/singingwolfboy/.virtualenvs/bradley/lib/python3.6/site-packages/flask_security/cli.py", line 36, in wrapper
    fn(*args, **kwargs)
  File "/Users/singingwolfboy/.virtualenvs/bradley/lib/python3.6/site-packages/flask_security/cli.py", line 95, in roles_add
    user, role = _datastore._prepare_role_modify_args(user, role)
  File "/Users/singingwolfboy/.virtualenvs/bradley/lib/python3.6/site-packages/flask_security/datastore.py", line 121, in _prepare_role_modify_args
    user = self.find_user(email=user)
  File "/Users/singingwolfboy/.virtualenvs/bradley/lib/python3.6/site-packages/flask_security/datastore.py", line 254, in find_user
    return self.user_model.query.filter_by(**kwargs).first()
  File "/Users/singingwolfboy/.virtualenvs/bradley/lib/python3.6/site-packages/sqlalchemy/orm/query.py", line 1562, in filter_by
    for key, value in kwargs.items()]
  File "/Users/singingwolfboy/.virtualenvs/bradley/lib/python3.6/site-packages/sqlalchemy/orm/query.py", line 1562, in <listcomp>
    for key, value in kwargs.items()]
  File "/Users/singingwolfboy/.virtualenvs/bradley/lib/python3.6/site-packages/sqlalchemy/orm/base.py", line 383, in _entity_descriptor
    (description, key)
sqlalchemy.exc.InvalidRequestError: Entity '<class 'bradley.models.auth.User'>' has no property 'email'

I'm not sure what's the best way to write a test for this. None of the automated tests seem to handle the case where we have a nonstandard identity attribute.

jirikuncar · 2017-07-28T11:37:26Z

@singingwolfboy thanks for spotting it. Can you please also add a test case?

singingwolfboy · 2017-07-30T15:02:12Z

@jirikuncar In order to add a test case, I had to refactor the way the whole test suite works, to account for User models that don't have an "email" field at all. It took awhile, but it seems to be working now.

Ideally, the app fixture should be parameterized, so that it runs tests both for email-based identity and username-based identity User models. However, that would require a lot more work to fix up the existing tests that assume that email-based identity is the only option, so I left that out of this pull request. Tests that want to explicitly opt-in to username-based identity User models (like my new test) can depend on the username_app and username_datastore fixtures.

I'm sure this is way more of a change than you expected, but can you take a look and give me your thoughts and feedback?

jirikuncar · 2017-10-06T11:06:17Z

@singingwolfboy I am sorry for very late feedback. I am very in favor on making the identity identifier configurable. I would however try a bit lighter approach when defining new forms. I will post the related comments as code review.

jirikuncar · 2017-10-06T11:07:19Z

flask_security/core.py

        _('Invalid confirmation token.'), 'error'),
    'EMAIL_ALREADY_ASSOCIATED': (
        _('%(email)s is already associated with an account.'), 'error'),
+    'USERNAME_ALREADY_IN_USE': (


Could we go with something more generic like: IDENTIFIER_ALREADY_IN_USE?

jirikuncar · 2017-10-06T11:08:40Z

flask_security/core.py

        _('Email not provided'), 'error'),
    'INVALID_EMAIL_ADDRESS': (
        _('Invalid email address'), 'error'),
+    'USERNAME_NOT_PROVIDED': (


similarly here I would propose IDENTIFIER_NOT_PROVIDED

jirikuncar · 2017-10-06T11:09:28Z

flask_security/core.py

    'INVALID_EMAIL_ADDRESS': (
        _('Invalid email address'), 'error'),
+    'USERNAME_NOT_PROVIDED': (
+        _('Username not provided'), 'error'),


If we decide to be more generic the Username should be configurable %(identifier_name)s.

jirikuncar · 2017-10-06T11:10:20Z

flask_security/core.py

        _('Please reauthenticate to access this page.'), 'info'),
 }

-_default_forms = {


I would rather see this part unchanged and add the logic to the forms itself.

jirikuncar · 2017-10-06T11:11:34Z

flask_security/core.py

+        "SECURITY_USER_IDENTITY_ATTRIBUTES",
+        ["email"],
+    )
+    if ident_attrs == ["email"]:


What about more generic condition "email" in identity_attributes?

jirikuncar · 2017-10-06T11:12:38Z

flask_security/core.py

    ))

-    for key, value in _default_forms.items():
+    ident_attrs = app.config.get(


Please don't shorten the variable names - I would prefer identity_attributes for readability purposes.

jirikuncar · 2017-10-06T11:13:26Z

flask_security/datastore.py

    def _prepare_role_modify_args(self, user, role):
        if isinstance(user, string_types):
-            user = self.find_user(email=user)
+            user_kwargs = {attr: user for attr in get_identity_attributes()}


👍 this is very cool! I really like it - great idea 🎉

jirikuncar · 2017-10-06T11:14:59Z

flask_security/forms.py

+class IdentifierForm(Form):
+    def __init__(self, *args, **kwargs):
+        super(IdentifierForm, self).__init__(*args, **kwargs)
+        setattr(self, "identifier", getattr(self, self.identifier_field))


IMHO the default could be read from current_app.config[...]

Alternatively we could try something like:

class IdentifierField(Field): def __init__(...): # autodetect label and validators from current_app.config class AnyForm(Form): identifier = IdentifierField()

jirikuncar · 2017-10-06T11:19:15Z

tests/templates/_nav.html

@@ -1,6 +1,8 @@
-{%- if current_user.is_authenticated -%}
-  <p>Hello {{ current_user.email }}</p>


Maybe we can register new filter: security_user_attribute so we can do something like: {{ current_user|security_user_attribute }}.

jirikuncar · 2017-10-06T11:20:52Z

tests/utils.py

 def authenticate(
        client,
        email="[email protected]",
+        username="matt",


what about calling it identifier and deprecating email?

Fix adding roles when identity is username

30a2e59

Define fixtures for username-only User models

d9e199c

singingwolfboy force-pushed the cli-roles-add-username-identity branch from 47795c5 to 58952b4 Compare July 29, 2017 19:24

singingwolfboy added 2 commits July 29, 2017 15:31

Automatically set up forms based on identity attributes

33c6d67

Add test for add/remove role for username-only model

d57f6ec

singingwolfboy force-pushed the cli-roles-add-username-identity branch from 58952b4 to d57f6ec Compare July 29, 2017 19:31

Update form templates to use identifier form field

50fae9f

singingwolfboy force-pushed the cli-roles-add-username-identity branch 3 times, most recently from 838bff6 to ebdfa29 Compare July 30, 2017 13:59

Fixes for existing tests

02100c6

singingwolfboy force-pushed the cli-roles-add-username-identity branch from ebdfa29 to 02100c6 Compare July 30, 2017 14:13

Only one paramaterized fixture for datastore backend name

26751f2

jirikuncar suggested changes Oct 6, 2017

View reviewed changes

		@@ -1,6 +1,8 @@
		{%- if current_user.is_authenticated -%}
		<p>Hello {{ current_user.email }}</p>

Uh oh!

Conversation

singingwolfboy commented Jul 28, 2017

Uh oh!

jirikuncar commented Jul 28, 2017

Uh oh!

singingwolfboy commented Jul 30, 2017

Uh oh!

jirikuncar commented Oct 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

Comments