Skip to content
This repository was archived by the owner on Jul 30, 2024. It is now read-only.

Fix adding roles when identity is username #686

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open

Fix adding roles when identity is username #686

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
30a2e59
Fix adding roles when identity is username
singingwolfboy Jul 28, 2017
d9e199c
Define fixtures for username-only User models
singingwolfboy Jul 29, 2017
33c6d67
Automatically set up forms based on identity attributes
singingwolfboy Jul 29, 2017
d57f6ec
Add test for add/remove role for username-only model
singingwolfboy Jul 29, 2017
50fae9f
Update form templates to use identifier form field
singingwolfboy Jul 29, 2017
02100c6
Fixes for existing tests
singingwolfboy Jul 29, 2017
26751f2
Only one paramaterized fixture for datastore backend name
singingwolfboy Jul 30, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 35 additions & 7 deletions flask_security/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,10 @@
from werkzeug.datastructures import ImmutableList
from werkzeug.local import LocalProxy

from .forms import ChangePasswordForm, ConfirmRegisterForm, \
ForgotPasswordForm, LoginForm, PasswordlessLoginForm, RegisterForm, \
from .forms import ChangePasswordForm, \
EmailConfirmRegisterForm, UsernameConfirmRegisterForm, \
ForgotPasswordForm, EmailLoginForm, UsernameLoginForm, \
PasswordlessLoginForm, EmailRegisterForm, UsernameRegisterForm, \
ResetPasswordForm, SendConfirmationForm
from .utils import config_value as cv
from .utils import _, get_config, hash_data, localize_callback, string_types, \
Expand Down Expand Up @@ -142,6 +144,8 @@
_('Invalid confirmation token.'), 'error'),
'EMAIL_ALREADY_ASSOCIATED': (
_('%(email)s is already associated with an account.'), 'error'),
'USERNAME_ALREADY_IN_USE': (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we go with something more generic like: IDENTIFIER_ALREADY_IN_USE?

_('%(username)s is already in use.'), 'error'),
'PASSWORD_MISMATCH': (
_('Password does not match'), 'error'),
'RETYPE_PASSWORD_MISMATCH': (
Expand Down Expand Up @@ -177,6 +181,10 @@
_('Email not provided'), 'error'),
'INVALID_EMAIL_ADDRESS': (
_('Invalid email address'), 'error'),
'USERNAME_NOT_PROVIDED': (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similarly here I would propose IDENTIFIER_NOT_PROVIDED

_('Username not provided'), 'error'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

'INVALID_USERNAME': (
_('Invalid username'), 'error'),
'PASSWORD_NOT_PROVIDED': (
_('Password not provided'), 'error'),
'PASSWORD_NOT_SET': (
Expand Down Expand Up @@ -205,10 +213,21 @@
_('Please reauthenticate to access this page.'), 'info'),
}

_default_forms = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

'login_form': LoginForm,
'confirm_register_form': ConfirmRegisterForm,
'register_form': RegisterForm,
_default_email_forms = {
'login_form': EmailLoginForm,
'confirm_register_form': EmailConfirmRegisterForm,
'register_form': EmailRegisterForm,
'forgot_password_form': ForgotPasswordForm,
'reset_password_form': ResetPasswordForm,
'change_password_form': ChangePasswordForm,
'send_confirmation_form': SendConfirmationForm,
'passwordless_login_form': PasswordlessLoginForm,
}

_default_username_forms = {
'login_form': UsernameLoginForm,
'confirm_register_form': UsernameConfirmRegisterForm,
'register_form': UsernameRegisterForm,
'forgot_password_form': ForgotPasswordForm,
'reset_password_form': ResetPasswordForm,
'change_password_form': ChangePasswordForm,
Expand Down Expand Up @@ -342,7 +361,16 @@ def _get_state(app, datastore, anonymous_user=None, **kwargs):
_unauthorized_callback=None
))

for key, value in _default_forms.items():
ident_attrs = app.config.get(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"SECURITY_USER_IDENTITY_ATTRIBUTES",
["email"],
)
if ident_attrs == ["email"]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about more generic condition "email" in identity_attributes?

default_forms = _default_email_forms
else:
default_forms = _default_username_forms

for key, value in default_forms.items():
if key not in kwargs or not kwargs[key]:
kwargs[key] = value

Expand Down
3 changes: 2 additions & 1 deletion flask_security/datastore.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,8 @@ def __init__(self, user_model, role_model):

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()}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

user = self.find_user(**user_kwargs)
if isinstance(role, string_types):
role = self.find_role(role)
return user, role
Expand Down
139 changes: 119 additions & 20 deletions flask_security/forms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@

_default_field_labels = {
'email': _('Email Address'),
'username': _('Username'),
'password': _('Password'),
'remember_me': _('Remember Me'),
'login': _('Login'),
Expand Down Expand Up @@ -61,12 +62,18 @@ class Email(ValidatorMixin, validators.Email):
pass


class Regexp(ValidatorMixin, validators.Regexp):
pass


class Length(ValidatorMixin, validators.Length):
pass


email_required = Required(message='EMAIL_NOT_PROVIDED')
email_validator = Email(message='INVALID_EMAIL_ADDRESS')
username_required = Required(message='USERNAME_NOT_PROVIDED')
username_validator = Regexp(r"[A-Za-z0-9_]+", message='INVALID_USERNAME')
password_required = Required(message='PASSWORD_NOT_PROVIDED')
password_length = Length(min=6, max=128, message='PASSWORD_INVALID_LENGTH')

Expand All @@ -81,19 +88,34 @@ def unique_user_email(form, field):
raise ValidationError(msg)


def unique_user_username(form, field):
if _datastore.get_user(field.data) is not None:
msg = get_message('USERNAME_ALREADY_IN_USE', username=field.data)[0]
raise ValidationError(msg)


def valid_user_email(form, field):
form.user = _datastore.get_user(field.data)
if form.user is None:
raise ValidationError(get_message('USER_DOES_NOT_EXIST')[0])


valid_user_username = valid_user_email


class Form(BaseForm):
def __init__(self, *args, **kwargs):
if current_app.testing:
self.TIME_LIMIT = None
super(Form, self).__init__(*args, **kwargs)


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively we could try something like:

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

class AnyForm(Form):
    identifier = IdentifierField()



class EmailFormMixin():
email = StringField(
get_form_field_label('email'),
Expand All @@ -105,12 +127,41 @@ class UserEmailFormMixin():
email = StringField(
get_form_field_label('email'),
validators=[email_required, email_validator, valid_user_email])
identifier_field = "email"


class UniqueEmailFormMixin():
email = StringField(
get_form_field_label('email'),
validators=[email_required, email_validator, unique_user_email])
identifier_field = "email"


class UsernameFormMixin():
username = StringField(
get_form_field_label('username'),
validators=[username_required, username_validator])


class UserUsernameFormMixin():
user = None
username = StringField(
get_form_field_label('username'),
validators=[
username_required, username_validator, valid_user_username
]
)
identifier_field = "username"


class UniqueUsernameFormMixin():
username = StringField(
get_form_field_label('username'),
validators=[
username_required, username_validator, unique_user_username
]
)
identifier_field = "username"


class PasswordFormMixin():
Expand Down Expand Up @@ -150,11 +201,12 @@ def is_field_and_user_attr(member):
hasattr(_datastore.user_model, member.name)

fields = inspect.getmembers(form, is_field_and_user_attr)
return dict((key, value.data) for key, value in fields)
return dict((key, value.data) for key, value in fields
if key != "identifier")


class SendConfirmationForm(Form, UserEmailFormMixin):
"""The default forgot password form"""
"""The default send confirmation form"""

submit = SubmitField(get_form_field_label('send_confirmation'))

Expand All @@ -172,20 +224,34 @@ def validate(self):
return True


class ForgotPasswordForm(Form, UserEmailFormMixin):
class AbstractForgotPasswordForm(IdentifierForm):
"""The default forgot password form"""

submit = SubmitField(get_form_field_label('recover_password'))

def validate(self):
if not super(ForgotPasswordForm, self).validate():
if not super(AbstractForgotPasswordForm, self).validate():
return False
if requires_confirmation(self.user):
self.email.errors.append(get_message('CONFIRMATION_REQUIRED')[0])
self.identifier.errors.append(
get_message('CONFIRMATION_REQUIRED')[0]
)
return False
return True


class EmailForgotPasswordForm(AbstractForgotPasswordForm, UserEmailFormMixin):
pass


class UsernameForgotPasswordForm(AbstractForgotPasswordForm,
UserUsernameFormMixin):
pass


ForgotPasswordForm = EmailForgotPasswordForm


class PasswordlessLoginForm(Form, UserEmailFormMixin):
"""The passwordless login form"""

Expand All @@ -203,18 +269,16 @@ def validate(self):
return True


class LoginForm(Form, NextFormMixin):
class AbstractLoginForm(IdentifierForm, NextFormMixin):
"""The default login form"""

email = StringField(get_form_field_label('email'),
validators=[Required(message='EMAIL_NOT_PROVIDED')])
password = PasswordField(get_form_field_label('password'),
validators=[password_required])
remember = BooleanField(get_form_field_label('remember_me'))
submit = SubmitField(get_form_field_label('login'))

def __init__(self, *args, **kwargs):
super(LoginForm, self).__init__(*args, **kwargs)
super(AbstractLoginForm, self).__init__(*args, **kwargs)
if not self.next.data:
self.next.data = request.args.get('next', '')
self.remember.default = config_value('DEFAULT_REMEMBER_ME')
Expand All @@ -227,42 +291,77 @@ def __init__(self, *args, **kwargs):
self.password.description = html

def validate(self):
if not super(LoginForm, self).validate():
if not super(AbstractLoginForm, self).validate():
return False

self.user = _datastore.get_user(self.email.data)
self.user = _datastore.get_user(self.identifier.data)

if self.user is None:
self.email.errors.append(get_message('USER_DOES_NOT_EXIST')[0])
self.identifier.errors.append(
get_message('USER_DOES_NOT_EXIST')[0]
)
return False
if not self.user.password:
self.password.errors.append(get_message('PASSWORD_NOT_SET')[0])
self.identifier.errors.append(get_message('PASSWORD_NOT_SET')[0])
return False
if not verify_and_update_password(self.password.data, self.user):
self.password.errors.append(get_message('INVALID_PASSWORD')[0])
return False
if requires_confirmation(self.user):
self.email.errors.append(get_message('CONFIRMATION_REQUIRED')[0])
self.identifier.errors.append(
get_message('CONFIRMATION_REQUIRED')[0]
)
return False
if not self.user.is_active:
self.email.errors.append(get_message('DISABLED_ACCOUNT')[0])
self.identifier.errors.append(get_message('DISABLED_ACCOUNT')[0])
return False
return True


class ConfirmRegisterForm(Form, RegisterFormMixin,
UniqueEmailFormMixin, NewPasswordFormMixin):
class EmailLoginForm(AbstractLoginForm, UserEmailFormMixin):
pass


class UsernameLoginForm(AbstractLoginForm, UserUsernameFormMixin):
pass


LoginForm = EmailLoginForm


class EmailConfirmRegisterForm(IdentifierForm, RegisterFormMixin,
UniqueEmailFormMixin, NewPasswordFormMixin):
pass


class RegisterForm(ConfirmRegisterForm, PasswordConfirmFormMixin,
NextFormMixin):
class UsernameConfirmRegisterForm(IdentifierForm, RegisterFormMixin,
UniqueUsernameFormMixin,
NewPasswordFormMixin):
pass


ConfirmRegisterForm = EmailConfirmRegisterForm


class EmailRegisterForm(EmailConfirmRegisterForm, PasswordConfirmFormMixin,
NextFormMixin):
def __init__(self, *args, **kwargs):
super(EmailRegisterForm, self).__init__(*args, **kwargs)
if not self.next.data:
self.next.data = request.args.get('next', '')


class UsernameRegisterForm(UsernameConfirmRegisterForm,
PasswordConfirmFormMixin, NextFormMixin):
def __init__(self, *args, **kwargs):
super(RegisterForm, self).__init__(*args, **kwargs)
super(UsernameRegisterForm, self).__init__(*args, **kwargs)
if not self.next.data:
self.next.data = request.args.get('next', '')


RegisterForm = EmailRegisterForm


class ResetPasswordForm(Form, NewPasswordFormMixin, PasswordConfirmFormMixin):
"""The default reset password form"""

Expand Down
2 changes: 1 addition & 1 deletion flask_security/templates/security/login_user.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
<h1>{{ _('Login') }}</h1>
<form action="{{ url_for_security('login') }}" method="POST" name="login_user_form">
{{ login_user_form.hidden_tag() }}
{{ render_field_with_errors(login_user_form.email) }}
{{ render_field_with_errors(login_user_form.identifier) }}
{{ render_field_with_errors(login_user_form.password) }}
{{ render_field_with_errors(login_user_form.remember) }}
{{ render_field(login_user_form.next) }}
Expand Down
2 changes: 1 addition & 1 deletion flask_security/templates/security/register_user.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
<h1>{{ _('Register') }}</h1>
<form action="{{ url_for_security('register') }}" method="POST" name="register_user_form">
{{ register_user_form.hidden_tag() }}
{{ render_field_with_errors(register_user_form.email) }}
{{ render_field_with_errors(register_user_form.identifier) }}
{{ render_field_with_errors(register_user_form.password) }}
{% if register_user_form.password_confirm %}
{{ render_field_with_errors(register_user_form.password_confirm) }}
Expand Down
Loading