diff --git a/features/steps/failed_registration.py b/features/steps/failed_registration.py index ffb4f40..323226c 100755 --- a/features/steps/failed_registration.py +++ b/features/steps/failed_registration.py @@ -40,5 +40,5 @@ def when_form_submit(context): @then('the new user will not be registered') def then_user_registered(context): # just one user exists, the first one - user_query = User.query.filter_by(username=context.test_user['username']) - assert user_query.count() == 1 + user = User.get(username=context.test_user['username']) + assert user is not None diff --git a/lastuser_core/models/__init__.py b/lastuser_core/models/__init__.py index 01db8cc..71486b9 100644 --- a/lastuser_core/models/__init__.py +++ b/lastuser_core/models/__init__.py @@ -1,6 +1,7 @@ # -*- coding: utf-8 -*- -from coaster.sqlalchemy import TimestampMixin, BaseMixin, BaseScopedNameMixin, UuidMixin # Imported from here by other models # NOQA +# Imported from here by other models +from coaster.sqlalchemy import TimestampMixin, BaseMixin, BaseScopedNameMixin, UuidMixin # NOQA from coaster.db import db @@ -47,7 +48,13 @@ def merge_users(user1, user2): else: keep_user, merge_user = user2, user1 - # 1. Inspect all tables for foreign key references to merge_user and switch to keep_user. + # 1. Release the username + if not keep_user.username: + if merge_user.username: + keep_user.username = merge_user.username + merge_user.username = None + + # 2. Inspect all tables for foreign key references to merge_user and switch to keep_user. for model in db.Model.__subclasses__(): if model != User: # a. This is a model and it's not the User model. Does it have a migrate_user classmethod? @@ -57,12 +64,10 @@ def merge_users(user1, user2): elif hasattr(model, 'user_id') and hasattr(model, 'query'): for row in model.query.filter_by(user_id=merge_user.id).all(): row.user_id = keep_user.id - # 2. Add merge_user's uuid to olduserids. Commit session. + # 3. Add merge_user's uuid to olduserids. Commit session. db.session.add(UserOldId(id=merge_user.uuid, user=keep_user)) - # 3. Mark merge_user as merged. Commit session. + # 4. Mark merge_user as merged. Commit session. merge_user.status = USER_STATUS.MERGED - # 4. Release the username - merge_user.username = None # 5. Commit all of this db.session.commit() diff --git a/lastuser_core/models/user.py b/lastuser_core/models/user.py index 051568d..85315ff 100644 --- a/lastuser_core/models/user.py +++ b/lastuser_core/models/user.py @@ -5,37 +5,156 @@ from werkzeug import check_password_hash, cached_property import bcrypt import phonenumbers -from sqlalchemy import or_, event, DDL +from sqlalchemy import or_ from sqlalchemy.orm import defer, deferred from sqlalchemy.ext.hybrid import hybrid_property from sqlalchemy.ext.associationproxy import association_proxy -from coaster.utils import newsecret, newpin, valid_username, require_one_of +from coaster.utils import newsecret, newpin, valid_username, require_one_of, LabeledEnum from coaster.sqlalchemy import make_timestamp_columns, failsafe_add, add_primary_relationship -from baseframe import _ +from baseframe import _, __ from . import db, BaseMixin, UuidMixin -__all__ = ['User', 'UserEmail', 'UserEmailClaim', 'PasswordResetRequest', 'UserExternalId', +__all__ = ['Name', 'User', 'UserEmail', 'UserEmailClaim', 'PasswordResetRequest', 'UserExternalId', 'UserPhone', 'UserPhoneClaim', 'Team', 'Organization', 'UserOldId', 'USER_STATUS'] -class USER_STATUS: - ACTIVE = 0 # Regular, active user - SUSPENDED = 1 # Suspended account - MERGED = 2 # Merged into another user - INVITED = 3 # Invited to make an account, doesn't have one yet +class Name(UuidMixin, BaseMixin, db.Model): + """ + Manage common namespace between the User and Organization models. + """ + __tablename__ = 'name' + __uuid_primary_key__ = True + __name_length__ = 63 + + #: The "username" assigned to a user or organization (length limit 63 to fit DNS label limit) + name = db.Column(db.Unicode(__name_length__), nullable=False, unique=True) + # Only one of the following three may be set: + #: User that owns this name (limit one per user) + user_id = db.Column(None, db.ForeignKey('user.id', ondelete='CASCADE'), unique=True, nullable=True) + user = db.relationship('User', backref=db.backref('_name', uselist=False, cascade='all, delete-orphan')) + #: Organization that owns this name (limit one per organization) + org_id = db.Column(None, db.ForeignKey('organization.id', ondelete='CASCADE'), unique=True, nullable=True) + org = db.relationship('Organization', backref=db.backref('_name', uselist=False, cascade='all, delete-orphan')) + #: Reserved name (not assigned to any party) + reserved = db.Column(db.Boolean, nullable=False, default=False, index=True) + + __table_args__ = ( + db.CheckConstraint( + db.case([(user_id != None, 1)], else_=0) + + db.case([(org_id != None, 1)], else_=0) + + db.case([(reserved == True, 1)], else_=0) == 1, + name='username_owner_check'), # NOQA + db.Index('ix_name_name_lower', db.func.lower(name).label('name_lower'), + unique=True, postgresql_ops={'name_lower': 'varchar_pattern_ops'}) + ) + + @property + def owner(self): + return self.user or self.org + + @owner.setter + def owner(self, value): + if isinstance(value, User): + self.user = value + self.org = None + elif isinstance(value, Organization): + self.user = None + self.org = value + else: + raise ValueError(value) + self.reserved = False + + @classmethod + def get(cls, name): + return cls.query.filter(db.func.lower(Name.name) == db.func.lower(name)).one_or_none() + + @classmethod + def validate_name_candidate(cls, name): + """ + Check if a name is available, returning one of several error codes, or None if all is okay: + + * ``blank``: No name supplied + * ``invalid``: Invalid characters in name + * ``long``: Name is longer than allowed size + * ``reserved``: Name is reserved + * ``user``: Name is assigned to a user + * ``org``: Name is assigned to an organization + """ + if not name: + return 'blank' + elif not valid_username(name): + return 'invalid' + elif len(name) > cls.__name_length__: + return 'long' + existing = cls.get(name) + if existing: + if existing.reserved: + return 'reserved' + elif existing.user_id: + return 'user' + elif existing.org_id: + return 'org' + + @classmethod + def is_available_name(cls, name): + if valid_username(name) and len(name) <= cls.__name_length__: + if cls.query.filter(db.func.lower(cls.name) == db.func.lower(name)).isempty(): + return True + return False + + @db.validates('name') + def validate_name(self, key, value): + if not valid_username(value): + raise ValueError("Invalid username: " + value) + # We don't check for existence in the db since this validator only + # checks for valid syntax. To confirm the name is actually available, + # the caller must call :meth:`is_available_name` or attempt to commit + # to the db and catch IntegrityError. + return value + + +class SharedNameMixin(object): + """ + Common methods between User and Organization to link to Name + """ + # The `name` property in User and Organization is not over here because + # of what seems to be a SQLAlchemy bug: we can't override the expression + # (both models need separate expressions) without triggering an inspection + # of the `_name` relationship, which does not exist yet as the backrefs + # are only fully setup when module loading is finished. + # Doc: https://docs.sqlalchemy.org/en/latest/orm/extensions/hybrid.html#reusing-hybrid-properties-across-subclasses + + def is_valid_name(self, value): + if not valid_username(value): + return False + existing = Name.get(value) + if existing and existing.owner != self: + return False + return True + + def validate_name_candidate(self, name): + if name and name == self.name: + return + return Name.validate_name_candidate(name) + +class USER_STATUS(LabeledEnum): + ACTIVE = (0, 'active', __("Active")) # Regular, active user + SUSPENDED = (1, 'suspended', __("Suspended")) # Suspended account + MERGED = (2, 'merged', __("Merged")) # Merged into another user + INVITED = (3, 'invited', __("Invited")) # Invited to make an account, doesn't have one yet -class User(UuidMixin, BaseMixin, db.Model): + +class User(SharedNameMixin, UuidMixin, BaseMixin, db.Model): __tablename__ = 'user' + __title_length__ = 80 userid = db.synonym('buid') # XXX: Deprecated, still here for Baseframe compatibility #: The user's fullname - fullname = db.Column(db.Unicode(80), default=u'', nullable=False) + fullname = db.Column(db.Unicode(__title_length__), default=u'', nullable=False) #: Alias for the user's fullname title = db.synonym('fullname') - #: The user's username, backend store - _username = db.Column('username', db.Unicode(80), unique=True, nullable=True) #: Bcrypt hash of the user's password pw_hash = db.Column(db.String(80), nullable=True) #: Timestamp for when the user's password last changed @@ -51,7 +170,7 @@ class User(UuidMixin, BaseMixin, db.Model): #: User avatar (URL to browser-ready image) avatar = db.Column(db.Unicode(250), nullable=True) - #: Client id that created this account + #: Client id that created this user account client_id = db.Column(None, db.ForeignKey('client.id', use_alter=True, name='user_client_id_fkey'), nullable=True) #: If this user was created by a client app via the API, record it here @@ -66,6 +185,11 @@ class User(UuidMixin, BaseMixin, db.Model): #: Other user accounts that were merged into this user account oldusers = association_proxy('oldids', 'olduser') + __table_args__ = ( + db.Index('ix_user_fullname_lower', db.func.lower(fullname).label('fullname_lower'), + postgresql_ops={'fullname_lower': 'varchar_pattern_ops'}), + ) + _defercols = [ defer('created_at'), defer('updated_at'), @@ -81,6 +205,27 @@ def __init__(self, password=None, **kwargs): self.password = password super(User, self).__init__(**kwargs) + @hybrid_property + def name(self): + if self._name: + return self._name.name + + @name.setter + def name(self, value): + if not value: + self._name = None + else: + if self._name is not None: + self._name.name = value + else: + self._name = Name(name=value, owner=self, id=self.uuid) + + @name.expression + def name(cls): + return db.select([Name.name]).where(Name.user_id == cls.id).label('name') + + username = db.synonym('name') + @property def is_active(self): return self.status == USER_STATUS.ACTIVE @@ -103,36 +248,6 @@ def _set_password(self, password): #: Write-only property (passwords cannot be read back in plain text) password = property(fset=_set_password) - # FIXME: move this to an SQLAlchemy validator - - #: Username (may be null) - @hybrid_property - def username(self): - return self._username - - @username.setter - def username(self, value): - if not value: - self._username = None - elif self.is_valid_username(value): - self._username = value - - # Alias name to username - name = username - - def is_valid_username(self, value): - if not valid_username(value): - return False - existing = User.query.filter(db.or_( - User.username == value, - User.buid == value)).first() # Avoid User.get to skip status check - if existing and existing.id != self.id: - return False - existing = Organization.get(name=value) - if existing: - return False - return True - def password_has_expired(self): return self.pw_hash is not None and self.pw_expires_at is not None and self.pw_expires_at <= datetime.utcnow() @@ -288,8 +403,12 @@ def get(cls, username=None, buid=None, defercols=False): :param str buid: Buid to lookup :param bool defercols: Defer loading non-critical columns """ - param, value = require_one_of(True, username=username, buid=buid) - query = cls.query.filter_by(**{param: value}) + require_one_of(username=username, buid=buid) + + if username is not None: + query = cls.query.join(Name).filter(Name.name == username) + else: + query = cls.query.filter_by(buid=buid) if defercols: query = query.options(*cls._defercols) user = query.one_or_none() @@ -312,11 +431,11 @@ def all(cls, buids=None, userids=None, usernames=None, defercols=False): if userids and not buids: buids = userids if buids and usernames: - query = cls.query.filter(or_(cls.buid.in_(buids), cls.username.in_(usernames))) + query = cls.query.join(Name).filter(or_(cls.buid.in_(buids), Name.name.in_(usernames))) elif buids: query = cls.query.filter(cls.buid.in_(buids)) elif usernames: - query = cls.query.filter(cls.username.in_(usernames)) + query = cls.query.join(Name).filter(Name.name.in_(usernames)) else: raise Exception @@ -345,11 +464,11 @@ def autocomplete(cls, query): # and doesn't use an index on PostgreSQL (there's a functional index defined below). if not query: return [] - users = cls.query.filter(cls.status == USER_STATUS.ACTIVE, + users = cls.query.join(Name).filter(cls.status == USER_STATUS.ACTIVE, or_( # Match against buid (exact value only), fullname or username, case insensitive cls.buid == query[:-1], db.func.lower(cls.fullname).like(db.func.lower(query)), - db.func.lower(cls._username).like(db.func.lower(query)) + db.func.lower(Name.name).like(db.func.lower(query)) ) ).options(*cls._defercols).limit(100).all() # Limit to 100 results if query.startswith('@') and UserExternalId.__at_username_services__: @@ -367,16 +486,6 @@ def autocomplete(cls, query): return users -create_user_index = DDL( - 'CREATE INDEX ix_user_username_lower ON "user" (lower(username) varchar_pattern_ops); ' - 'CREATE INDEX ix_user_fullname_lower ON "user" (lower(fullname) varchar_pattern_ops);') -event.listen(User.__table__, 'after_create', - create_user_index.execute_if(dialect='postgresql')) - -event.listen(User.__table__, 'before_drop', - DDL('DROP INDEX ix_user_username_lower; DROP INDEX ix_user_fullname_lower;').execute_if(dialect='postgresql')) - - class UserOldId(UuidMixin, BaseMixin, db.Model): __tablename__ = 'useroldid' __uuid_primary_key__ = True @@ -409,8 +518,9 @@ def get(cls, uuid): ) -class Organization(UuidMixin, BaseMixin, db.Model): +class Organization(SharedNameMixin, UuidMixin, BaseMixin, db.Model): __tablename__ = 'organization' + __title_length__ = 80 # owners_id cannot be null, but must be declared with nullable=True since there is # a circular dependency. The post_update flag on the relationship tackles the circular # dependency within SQLAlchemy. @@ -422,8 +532,7 @@ class Organization(UuidMixin, BaseMixin, db.Model): use_alter=True, name='organization_members_id_fkey'), nullable=True) members = db.relationship('Team', primaryjoin='Organization.members_id == Team.id', uselist=False, cascade='all', post_update=True) # No delete-orphan cascade here - _name = db.Column('name', db.Unicode(80), unique=True, nullable=True) - title = db.Column(db.Unicode(80), default=u'', nullable=False) + title = db.Column(db.Unicode(__title_length__), default=u'', nullable=False) #: Deprecated, but column preserved for existing data until migration description = deferred(db.Column(db.UnicodeText, default=u'', nullable=False)) @@ -442,31 +551,30 @@ def __init__(self, *args, **kwargs): super(Organization, self).__init__(*args, **kwargs) self.make_teams() - def make_teams(self): - if self.owners is None: - self.owners = Team(title=_(u"Owners"), org=self) - if self.members is None: - self.members = Team(title=_(u"Members"), org=self) - @hybrid_property def name(self): - return self._name + if self._name: + return self._name.name @name.setter def name(self, value): - if self.valid_name(value): - self._name = value + if not value: + self._name = None + else: + if self._name is not None: + self._name.name = value + else: + self._name = Name(name=value, owner=self, id=self.uuid) - def valid_name(self, value): - if not valid_username(value): - return False - existing = Organization.get(name=value) - if existing and existing.id != self.id: - return False - existing = User.query.filter_by(username=value).first() # Avoid User.get to skip status check - if existing: - return False - return True + @name.expression + def name(cls): + return db.select([Name.name]).where(Name.org_id == cls.id).label('name') + + def make_teams(self): + if self.owners is None: + self.owners = Team(title=_(u"Owners"), org=self) + if self.members is None: + self.members = Team(title=_(u"Members"), org=self) def __repr__(self): return ''.format( @@ -522,8 +630,12 @@ def get(cls, name=None, buid=None, defercols=False): :param str buid: Buid of the organization :param bool defercols: Defer loading non-critical columns """ - param, value = require_one_of(True, name=name, buid=buid) - query = cls.query.filter_by(**{param: value}) + require_one_of(name=name, buid=buid) + + if name is not None: + query = cls.query.join(Name).filter(Name.name == name) + else: + query = cls.query.filter_by(buid=buid) if defercols: query = query.options(*cls._defercols) return query.one_or_none() @@ -537,7 +649,7 @@ def all(cls, buids=None, names=None, defercols=False): query = query.options(*cls._defercols) orgs.extend(query.all()) if names: - query = cls.query.filter(cls.name.in_(names)) + query = cls.query.join(Name).filter(Name.name.in_(names)) if defercols: query = query.options(*cls._defercols) orgs.extend(query.all()) @@ -546,8 +658,9 @@ def all(cls, buids=None, names=None, defercols=False): class Team(UuidMixin, BaseMixin, db.Model): __tablename__ = 'team' + __title_length__ = 250 #: Displayed name - title = db.Column(db.Unicode(250), nullable=False) + title = db.Column(db.Unicode(__title_length__), nullable=False) #: Organization org_id = db.Column(None, db.ForeignKey('organization.id'), nullable=False) org = db.relationship(Organization, primaryjoin=org_id == Organization.id, @@ -593,7 +706,7 @@ def get(cls, buid=None): return cls.query.filter_by(buid=buid).one_or_none() -# -- User/Org/Team email/phone and misc +# -- User email/phone and misc class UserEmail(BaseMixin, db.Model): __tablename__ = 'useremail' @@ -607,6 +720,11 @@ class UserEmail(BaseMixin, db.Model): private = db.Column(db.Boolean, nullable=False, default=False) type = db.Column(db.Unicode(30), nullable=True) + __table_args__ = ( + db.Index('ix_useremail_email_lower', db.func.lower(_email).label('email_lower'), + unique=True, postgresql_ops={'email_lower': 'varchar_pattern_ops'}), + ) + def __init__(self, email, **kwargs): super(UserEmail, self).__init__(**kwargs) self._email = email.lower() @@ -660,15 +778,6 @@ def get(cls, email=None, md5sum=None): return cls.query.filter_by(md5sum=md5sum).one_or_none() -create_useremail_index = DDL( - 'CREATE UNIQUE INDEX ix_useremail_email_lower ON useremail (lower(email) varchar_pattern_ops);') -event.listen(UserEmail.__table__, 'after_create', - create_useremail_index.execute_if(dialect='postgresql')) - -event.listen(UserEmail.__table__, 'before_drop', - DDL('DROP INDEX ix_useremail_email_lower').execute_if(dialect='postgresql')) - - class UserEmailClaim(BaseMixin, db.Model): __tablename__ = 'useremailclaim' user_id = db.Column(None, db.ForeignKey('user.id'), nullable=False) @@ -889,7 +998,11 @@ class UserExternalId(BaseMixin, db.Model): oauth_token_secret = db.Column(db.String(1000), nullable=True) oauth_token_type = db.Column(db.String(250), nullable=True) - __table_args__ = (db.UniqueConstraint('service', 'userid'), {}) + __table_args__ = ( + db.UniqueConstraint('service', 'userid'), + db.Index('ix_userexternalid_username_lower', db.func.lower(username).label('username_lower'), + postgresql_ops={'username_lower': 'varchar_pattern_ops'}) + ) def __repr__(self): return ''.format( @@ -921,11 +1034,3 @@ def permissions(self, user, inherited=None): add_primary_relationship(User, 'primary_email', UserEmail, 'user', 'user_id') add_primary_relationship(User, 'primary_phone', UserPhone, 'user', 'user_id') - -create_userexternalid_index = DDL( - 'CREATE INDEX ix_userexternalid_username_lower ON userexternalid (lower(username) varchar_pattern_ops);') -event.listen(UserExternalId.__table__, 'after_create', - create_userexternalid_index.execute_if(dialect='postgresql')) - -event.listen(UserExternalId.__table__, 'before_drop', - DDL('DROP INDEX ix_userexternalid_username_lower;').execute_if(dialect='postgresql')) diff --git a/lastuser_oauth/forms/profile.py b/lastuser_oauth/forms/profile.py index b71886a..b8b965b 100644 --- a/lastuser_oauth/forms/profile.py +++ b/lastuser_oauth/forms/profile.py @@ -1,11 +1,11 @@ # -*- coding: utf-8 -*- from flask import current_app -from coaster.utils import valid_username, sorted_timezones +from coaster.utils import sorted_timezones from baseframe import _, __ import baseframe.forms as forms -from lastuser_core.models import UserEmail, getuser +from lastuser_core.models import Name, User, UserEmail, getuser timezones = sorted_timezones() @@ -51,32 +51,35 @@ def validate_old_password(self, field): class ProfileForm(forms.Form): fullname = forms.StringField(__("Full name"), - validators=[forms.validators.DataRequired(), forms.validators.Length(max=80)]) + validators=[forms.validators.DataRequired(), forms.validators.Length(max=User.__title_length__)]) email = forms.EmailField(__("Email address"), validators=[forms.validators.DataRequired(), forms.ValidEmail()], widget_attrs={'autocorrect': 'none', 'autocapitalize': 'none'}) - username = forms.AnnotatedTextField(__("Username"), validators=[forms.validators.DataRequired()], + username = forms.AnnotatedTextField(__("Username"), + validators=[forms.validators.DataRequired(), forms.validators.Length(max=Name.__name_length__)], filters=[forms.filters.none_if_empty()], - prefix=u"https://hasgeek.com/…", + prefix=u"https://hasgeek.com/", widget_attrs={'autocorrect': 'none', 'autocapitalize': 'none'}) timezone = forms.SelectField(__("Timezone"), validators=[forms.validators.DataRequired()], choices=timezones) def validate_username(self, field): - # # Usernames are now mandatory. This should be commented out: - # if not field.data: - # field.data = None - # return - field.data = field.data.lower() # Usernames can only be lowercase - if not valid_username(field.data): + if field.data.lower() in current_app.config['RESERVED_USERNAMES']: + raise forms.ValidationError(_("This name is reserved")) # To be deprecated in favour of one below + + reason = self.edit_obj.validate_name_candidate(field.data) + if not reason: + return # Username is available + if reason == 'invalid': raise forms.ValidationError(_("Usernames can only have alphabets, numbers and dashes (except at the ends)")) - if field.data in current_app.config.get('RESERVED_USERNAMES', []): - raise forms.ValidationError(_("This name is reserved")) - if not self.edit_user.is_valid_username(field.data): - raise forms.ValidationError(_("This username is taken")) + elif reason == 'reserved': + raise forms.ValidationError(_("This username is reserved")) + elif reason in ('user', 'org'): + raise forms.ValidationError(_("This username has been taken")) + else: + raise forms.ValidationError(_("This username is not available")) # TODO: Move to function and place before ValidEmail() def validate_email(self, field): - field.data = field.data.lower() # Convert to lowercase existing = UserEmail.get(email=field.data) if existing is not None and existing.user != self.edit_obj: raise forms.ValidationError(_("This email address has been claimed by another user")) diff --git a/lastuser_oauth/views/account.py b/lastuser_oauth/views/account.py index d044b8c..cda05f1 100644 --- a/lastuser_oauth/views/account.py +++ b/lastuser_oauth/views/account.py @@ -131,7 +131,7 @@ def login_service_postcallback(service, userdata): user = register_internal(None, userdata.get('fullname'), None) extid.user = user if userdata.get('username'): - if valid_username(userdata['username']) and user.is_valid_username(userdata['username']): + if valid_username(userdata['username']) and user.is_valid_name(userdata['username']): # Set a username for this user if it's available user.username = userdata['username'] else: # We have an existing user account from extid or useremail diff --git a/lastuser_ui/forms/org.py b/lastuser_ui/forms/org.py index 17c998f..f2c2aaf 100644 --- a/lastuser_ui/forms/org.py +++ b/lastuser_ui/forms/org.py @@ -1,43 +1,54 @@ # -*- coding: utf-8 -*- from flask import current_app, Markup, url_for -from coaster.utils import valid_username from coaster.auth import current_auth from baseframe import _, __ import baseframe.forms as forms -from lastuser_core.models import User, Organization +from lastuser_core.models import Name, User, Organization, Team __all__ = ['OrganizationForm', 'TeamForm'] class OrganizationForm(forms.Form): - title = forms.StringField(__("Organization name"), validators=[forms.validators.DataRequired()]) - name = forms.AnnotatedTextField(__("Username"), validators=[forms.validators.DataRequired()], - prefix=u"https://hasgeek.com/…", + title = forms.StringField(__("Organization name"), + validators=[forms.validators.DataRequired(), forms.validators.Length(max=Organization.__title_length__)]) + name = forms.AnnotatedTextField(__("Username"), + validators=[forms.validators.DataRequired(), forms.validators.Length(max=Name.__name_length__)], + prefix=u"https://hasgeek.com/", widget_attrs={'autocorrect': 'none', 'autocapitalize': 'none'}) def validate_name(self, field): - if not valid_username(field.data): - raise forms.ValidationError(_("Invalid characters in name")) - if field.data in current_app.config['RESERVED_USERNAMES']: + if field.data.lower() in current_app.config['RESERVED_USERNAMES']: + raise forms.ValidationError(_("This name is reserved")) # To be deprecated in favour of one below + + if self.edit_obj: + reason = self.edit_obj.validate_name_candidate(field.data) + else: + reason = Name.validate_name_candidate(field.data) + if not reason: + return # Name is available + if reason == 'invalid': + raise forms.ValidationError(_("Names can only have alphabets, numbers and dashes (except at the ends)")) + elif reason == 'reserved': raise forms.ValidationError(_("This name is reserved")) - existing = User.get(username=field.data) - if existing is not None: - if existing == current_auth.user: + elif reason == 'user': + if field.data == current_auth.user.username: raise forms.ValidationError(Markup(_(u"This is your current username. " u'You must change it first from your account ' u"before you can assign it to an organization").format( account=url_for('account')))) else: - raise forms.ValidationError(_("This name is taken")) - existing = Organization.get(name=field.data) - if existing is not None and existing.id != self.edit_id: - raise forms.ValidationError(_("This name is taken")) + raise forms.ValidationError(_("This name has been taken by another user")) + elif reason == 'org': + raise forms.ValidationError(_("This name has been taken by another organization")) + else: + raise forms.ValidationError(_("This name is not available")) class TeamForm(forms.Form): - title = forms.StringField(__("Team name"), validators=[forms.validators.DataRequired()]) + title = forms.StringField(__("Team name"), + validators=[forms.validators.DataRequired(), forms.validators.Length(max=Team.__title_length__)]) users = forms.UserSelectMultiField(__("Users"), validators=[forms.validators.DataRequired()], description=__("Lookup a user by their username or email address"), lastuser=None, usermodel=User, diff --git a/migrations/versions/065146599a21_useremail_email_index_pattern.py b/migrations/versions/065146599a21_useremail_email_index_pattern.py new file mode 100644 index 0000000..a492674 --- /dev/null +++ b/migrations/versions/065146599a21_useremail_email_index_pattern.py @@ -0,0 +1,31 @@ +"""UserEmail email index pattern + +Revision ID: 065146599a21 +Revises: 518321b25909 +Create Date: 2018-10-01 11:41:56.346902 + +""" +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = '065146599a21' +down_revision = '518321b25909' +branch_labels = None +depends_on = None + + +# This revision fixes a bug introduced in revision 3b17b62bf8e4, +# where text_pattern_ops was used instead of varchar_pattern_ops + +def upgrade(): + op.drop_index('ix_useremail_email_lower') + op.execute(sa.DDL( + 'CREATE UNIQUE INDEX ix_useremail_email_lower ON useremail (lower(email) varchar_pattern_ops);')) + + +def downgrade(): + op.drop_index('ix_useremail_email_lower') + op.execute(sa.DDL( + 'CREATE UNIQUE INDEX ix_useremail_email_lower ON useremail (lower(email) text_pattern_ops);')) diff --git a/migrations/versions/518321b25909_username_model.py b/migrations/versions/518321b25909_username_model.py new file mode 100644 index 0000000..0cb292d --- /dev/null +++ b/migrations/versions/518321b25909_username_model.py @@ -0,0 +1,78 @@ +"""Username model + +Revision ID: 518321b25909 +Revises: c446b9bc0691 +Create Date: 2018-09-29 02:23:56.669605 + +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy_utils import UUIDType + + +# revision identifiers, used by Alembic. +revision = '518321b25909' +down_revision = 'c446b9bc0691' +branch_labels = None +depends_on = None + + +def upgrade(): + op.create_table('name', + sa.Column('created_at', sa.DateTime(), nullable=False), + sa.Column('updated_at', sa.DateTime(), nullable=False), + sa.Column('name', sa.Unicode(length=63), nullable=False), + sa.Column('user_id', sa.Integer(), nullable=True), + sa.Column('org_id', sa.Integer(), nullable=True), + sa.Column('reserved', sa.Boolean(), nullable=False), + sa.Column('id', UUIDType(binary=False), nullable=False), + sa.CheckConstraint(u'CASE WHEN (user_id IS NOT NULL) THEN 1 ELSE 0 END + CASE WHEN (org_id IS NOT NULL) THEN 1 ELSE 0 END + CASE WHEN (reserved = true) THEN 1 ELSE 0 END = 1', name='username_owner_check'), + sa.ForeignKeyConstraint(['org_id'], ['organization.id'], ondelete='CASCADE'), + sa.ForeignKeyConstraint(['user_id'], ['user.id'], ondelete='CASCADE'), + sa.PrimaryKeyConstraint('id'), + sa.UniqueConstraint('name'), + sa.UniqueConstraint('org_id'), + sa.UniqueConstraint('user_id') + ) + op.create_index(op.f('ix_name_reserved'), 'name', ['reserved'], unique=False) + + op.execute(sa.DDL(''' + INSERT INTO "name" (reserved, id, created_at, updated_at, name, user_id) + SELECT False, uuid, created_at, updated_at, username, id FROM "user" + WHERE username IS NOT null + ORDER BY created_at; + ''')) + op.execute(sa.DDL(''' + INSERT INTO "name" (reserved, id, created_at, updated_at, name, org_id) + SELECT False, uuid, created_at, updated_at, name, id FROM "organization" + WHERE name IS NOT null + ORDER BY created_at; + ''')) + + op.drop_index(op.f('ix_user_username_lower'), table_name='user') + op.execute(sa.DDL('''CREATE UNIQUE INDEX ix_name_name_lower ON "name" (lower(name) varchar_pattern_ops);''')) + + op.drop_constraint(u'organization_name_key', 'organization', type_='unique') + op.drop_column(u'organization', 'name') + op.drop_constraint(u'user_username_key', 'user', type_='unique') + op.drop_column(u'user', 'username') + + +def downgrade(): + op.add_column(u'user', sa.Column('username', sa.VARCHAR(length=80), autoincrement=False, nullable=True)) + op.create_unique_constraint(u'user_username_key', 'user', ['username']) + op.add_column(u'organization', sa.Column('name', sa.VARCHAR(length=80), autoincrement=False, nullable=True)) + op.create_unique_constraint(u'organization_name_key', 'organization', ['name']) + + op.drop_index(op.f('ix_name_name_lower'), table_name='name') + op.execute(sa.DDL('''CREATE INDEX ix_user_username_lower ON "user" (lower(username) varchar_pattern_ops);''')) + + op.execute(sa.DDL(''' + UPDATE "user" SET (username) = (SELECT name FROM "name" WHERE "name".user_id = "user".id) + ''')) + op.execute(sa.DDL(''' + UPDATE "organization" SET (name) = (SELECT name FROM "name" WHERE "name".org_id = "organization".id) + ''')) + + op.drop_index(op.f('ix_name_reserved'), table_name='name') + op.drop_table('name') diff --git a/tests/unit/lastuser_core/test_db.py b/tests/unit/lastuser_core/test_db.py index 5988277..84ff186 100644 --- a/tests/unit/lastuser_core/test_db.py +++ b/tests/unit/lastuser_core/test_db.py @@ -7,21 +7,23 @@ class TestDatabaseFixture(unittest.TestCase): @classmethod - def setUpClass(self): + def setUpClass(cls): """ Initialize a test DB and call to make fixtures. """ - self.app = app + cls.app = app db.create_all() - self.fixtures = Fixtures() - self.fixtures.make_fixtures() - self.fixtures.test_client = app.test_client() + cls.fixtures = Fixtures() + cls.fixtures.make_fixtures() + cls.fixtures.test_client = app.test_client() @classmethod - def tearDownClass(self): + def tearDownClass(cls): """ Remove test session and tables. """ - db.session.rollback() db.drop_all() db.session.remove() + + def tearDown(self): + db.session.rollback() diff --git a/tests/unit/lastuser_core/test_model_client_AuthToken.py b/tests/unit/lastuser_core/test_model_client_AuthToken.py index 5d8231d..0239f90 100644 --- a/tests/unit/lastuser_core/test_model_client_AuthToken.py +++ b/tests/unit/lastuser_core/test_model_client_AuthToken.py @@ -88,7 +88,7 @@ def test_AuthToken_all(self): alastor_token = models.AuthToken(client=client, user=alastor, scope=[u'id']) greyback = models.User(username=u'greyback', fullname=u'Fenrir Greyback') greyback_token = models.AuthToken(client=client, user=greyback, scope=[u'id']) - pottermania = models.Organization(name=u'Batdog', title=u'Batdog') + pottermania = models.Organization(name=u'pottermania', title=u'Pottermania') pottermania.owners.users.append(hermione) pottermania_members = [hermione, alastor, greyback, myrtle] for member in pottermania_members: diff --git a/tests/unit/lastuser_core/test_model_client_UserClientPermissions.py b/tests/unit/lastuser_core/test_model_client_UserClientPermissions.py index 6c8da77..cb1fb0d 100644 --- a/tests/unit/lastuser_core/test_model_client_UserClientPermissions.py +++ b/tests/unit/lastuser_core/test_model_client_UserClientPermissions.py @@ -26,7 +26,7 @@ def test_UserClientPermissions_migrate_user(self): """ # scenario 1: when *only* olduser has UserClientPermissions instance old_crusoe = self.fixtures.crusoe - new_crusoe = models.User(username=u'chef_crusoe') + new_crusoe = models.User(username=u'chef-crusoe') models.UserClientPermissions.migrate_user(old_crusoe, new_crusoe) for each in new_crusoe.client_permissions: self.assertIsInstance(each, models.UserClientPermissions) @@ -38,7 +38,7 @@ def test_UserClientPermissions_migrate_user(self): access_permissions_old_oakley = u'siteadmin' access_permissions_new_oakley = u'siteeditor' old_oakley_userclientperms = models.UserClientPermissions(user=old_oakley, client=client, access_permissions=access_permissions_old_oakley) - new_oakley = models.User(username=u'oakley_the_stud') + new_oakley = models.User(username=u'oakley-the-stud') new_oakley_userclientperms = models.UserClientPermissions(user=new_oakley, client=client, access_permissions=access_permissions_new_oakley) db.session.add(old_oakley_userclientperms) db.session.add(new_oakley_userclientperms) @@ -55,7 +55,7 @@ def test_userclientpermissions_pickername(self): """ Test for UserClientPermissions' pickername """ - finnick = models.User(username=u'finnick', fullname=u'Finnick Odair') + finnick = models.User(username=u'finnick', fullname=u"Finnick Odair") district4 = models.Client(title=u"District 4") access_permissions = u'siteadmin' result = models.UserClientPermissions(user=finnick, client=district4, access_permissions=access_permissions) @@ -65,7 +65,7 @@ def test_userclientpermissions_buid(self): """ Test for UserClientPermissions' buid """ - beetee = models.User(username=u'beetee', fullname=u'Beetee') + beetee = models.User(username=u'beetee', fullname=u"Beetee") district3 = models.Client(title=u'District 3') access_permissions = u'siteadmin' result = models.UserClientPermissions(user=beetee, client=district3, access_permissions=access_permissions) diff --git a/tests/unit/lastuser_core/test_model_user_Name.py b/tests/unit/lastuser_core/test_model_user_Name.py new file mode 100644 index 0000000..02c4959 --- /dev/null +++ b/tests/unit/lastuser_core/test_model_user_Name.py @@ -0,0 +1,105 @@ +# -*- coding: utf-8 -*- + +from sqlalchemy.exc import IntegrityError +from lastuserapp import db +import lastuser_core.models as models +from .test_db import TestDatabaseFixture + + +class TestName(TestDatabaseFixture): + + def test_is_available_name(self): + """ + Names are only available if valid and unused + """ + assert models.Name.is_available_name('invalid_name') is False + # 'piglet' is a name taken in the fixtures + assert models.Name.get('piglet') is not None + assert models.Name.is_available_name('piglet') is False + assert models.Name.is_available_name('peppa') is True + + def test_validate_name_candidate(self): + """ + The name validator returns error codes as expected + """ + assert models.Name.validate_name_candidate(None) == 'blank' + assert models.Name.validate_name_candidate('') == 'blank' + assert models.Name.validate_name_candidate('invalid_name') == 'invalid' + assert models.Name.validate_name_candidate('0123456789' * 7) == 'long' + assert models.Name.validate_name_candidate('0123456789' * 6) is None + assert models.Name.validate_name_candidate('test-reserved') is None + db.session.add(models.Name(name=u'test-reserved', reserved=True)) + db.session.commit() + assert models.Name.validate_name_candidate('test-reserved') == 'reserved' + assert models.Name.validate_name_candidate('piglet') == 'user' + assert models.Name.validate_name_candidate('batdog') == 'org' + + def test_reserved_name(self): + """ + Names can be reserved, with no user or organization + """ + reserved_name = models.Name(name=u'reserved-name', reserved=True) + db.session.add(reserved_name) + db.session.commit() + retrieved_name = models.Name.get(u'reserved-name') + assert retrieved_name is reserved_name + assert reserved_name.user is None + assert reserved_name.user_id is None + assert reserved_name.org is None + assert reserved_name.org_id is None + + def test_unassigned_name(self): + """ + Names must be assigned to a user or organization if not reserved + """ + unassigned_name = models.Name(name=u'unassigned') + db.session.add(unassigned_name) + with self.assertRaises(IntegrityError): + db.session.commit() + + def test_double_assigned_name(self): + """ + Names cannot be assigned to both a user and an organization simultaneously + """ + user = models.User(fullname=u"User") + org = models.Organization(title=u"Organization") + name = models.Name(name=u'double-assigned', user=user, org=org) + db.session.add_all([user, org, name]) + with self.assertRaises(IntegrityError): + db.session.commit() + + def test_user_two_names(self): + """ + A user cannot have two names + """ + assert self.fixtures.piglet.username == u'piglet' + peppa = models.Name(name=u'peppa', user=self.fixtures.piglet) + db.session.add(peppa) + with self.assertRaises(IntegrityError): + db.session.commit() + + def test_org_two_names(self): + """ + An organization cannot have two names + """ + assert self.fixtures.batdog.name == u'batdog' + bathound = models.Name(name=u'bathound', org=self.fixtures.batdog) + db.session.add(bathound) + with self.assertRaises(IntegrityError): + db.session.commit() + + def test_remove_name(self): + """ + Removing a name from a user or org also removes it from the Name table + """ + assert self.fixtures.oakley.username == u'oakley' + assert models.Name.get(u'oakley') is not None + self.fixtures.oakley.username = None + db.session.commit() + assert models.Name.get(u'oakley') is None + + assert self.fixtures.specialdachs.name == u'specialdachs' + assert models.Name.get(u'specialdachs') is not None + self.fixtures.specialdachs.name = None + db.session.commit() + assert models.Name.get(u'specialdachs') is None diff --git a/tests/unit/lastuser_core/test_model_user_Organization.py b/tests/unit/lastuser_core/test_model_user_Organization.py index 5e028b4..ab0a524 100644 --- a/tests/unit/lastuser_core/test_model_user_Organization.py +++ b/tests/unit/lastuser_core/test_model_user_Organization.py @@ -104,8 +104,8 @@ def test_organization_valid_name(self): Test for checking if given is a valid organization name """ hufflepuffs = models.Organization(name=u'hufflepuffs', title=u'Huffle Puffs') - self.assertFalse(hufflepuffs.valid_name(u'#$%#%___2836273untitled')) - self.assertTrue(hufflepuffs.valid_name(u'hufflepuffs')) + self.assertFalse(hufflepuffs.is_valid_name(u'#$%#%___2836273untitled')) + self.assertTrue(hufflepuffs.is_valid_name(u'hufflepuffs')) def test_organization_pickername(self): """ @@ -173,10 +173,10 @@ def test_organization_name(self): name is a setter method """ insurgent = models.Organization(title=u'Insurgent') - insurgent.name = u'35453496*%&^$%^' - self.assertIsNone(insurgent.name) - insurgent.name = u'Insurgent' - self.assertIsNone(insurgent.name) + with self.assertRaises(ValueError): + insurgent.name = u'35453496*%&^$%^' + with self.assertRaises(ValueError): + insurgent.name = u'Insurgent' insurgent.name = u'insurgent' self.assertEqual(insurgent.name, u'insurgent') diff --git a/tests/unit/lastuser_core/test_model_user_User.py b/tests/unit/lastuser_core/test_model_user_User.py index 258d881..4dfd6d2 100644 --- a/tests/unit/lastuser_core/test_model_user_User.py +++ b/tests/unit/lastuser_core/test_model_user_User.py @@ -13,29 +13,29 @@ def test_User(self): """ Test for creation of user object from User model """ - user = models.User(username=u"lena", fullname=u"Lena Audrey Dachshund") + user = models.User(username=u'lena', fullname=u"Lena Audrey Dachshund") db.session.add_all([user]) db.session.commit() - lena = models.User.query.filter_by(username=u"lena").first() + lena = models.User.get(username=u'lena') self.assertIsInstance(lena, models.User) - self.assertEqual(user.username, u"lena") + self.assertEqual(user.username, u'lena') self.assertEqual(user.fullname, u"Lena Audrey Dachshund") - def test_user_is_valid_username(self): + def test_user_is_valid_name(self): """ Test to check if given is a valid username associated with the user """ crusoe = self.fixtures.crusoe # scenario 1: not a valid username number_one = models.User(username=u'number1', fullname=u'Number One') - self.assertFalse(number_one.is_valid_username(u'Number1')) + self.assertFalse(number_one.is_valid_name(u'Number1')) # scenario 2: a valid username but not the username of instance passed - self.assertFalse(crusoe.is_valid_username(u"oakley")) + self.assertFalse(crusoe.is_valid_name(u"oakley")) # scenario 3: a existing username - self.assertTrue(crusoe.is_valid_username(u"crusoe")) + self.assertTrue(crusoe.is_valid_name(u"crusoe")) # scenario 4: a existing org batdog = self.fixtures.batdog - self.assertFalse(crusoe.is_valid_username(batdog.name)) + self.assertFalse(crusoe.is_valid_name(batdog.name)) def test_user_profileid(self): """ @@ -304,7 +304,7 @@ def test_user_is_active(self): """ crusoe = self.fixtures.crusoe self.assertEqual(crusoe.status, 0) - oakley = models.User.query.filter_by(username=u"oakley").first() + oakley = models.User.get(username=u'oakley') oakley.status = 1 self.assertEqual(oakley.status, 1) @@ -341,7 +341,7 @@ def test_user_merged_user(self): """ # ## Merge a user onto an older user ### crusoe = self.fixtures.crusoe - crusoe2 = models.User(username=u"batdog", fullname=u"Batdog") + crusoe2 = models.User(username=u"crusoe2", fullname=u"Crusoe2") db.session.add(crusoe2) db.session.commit() merged_user = models.merge_users(crusoe, crusoe2) diff --git a/tests/unit/lastuser_core/test_model_user_UserOldId.py b/tests/unit/lastuser_core/test_model_user_UserOldId.py index f612c77..0900c98 100644 --- a/tests/unit/lastuser_core/test_model_user_UserOldId.py +++ b/tests/unit/lastuser_core/test_model_user_UserOldId.py @@ -17,12 +17,12 @@ def test_UserOldId_get(self): Test for verifying creation and retrieval of UserOldId instance """ crusoe = self.fixtures.crusoe - batdog = models.User(username=u"batdog", fullname=u"Batdog") - db.session.add(batdog) + bathound = models.User(username=u"bathound", fullname=u"Bathound") + db.session.add(bathound) db.session.commit() - merged = models.merge_users(crusoe, batdog) + merged = models.merge_users(crusoe, bathound) if merged == crusoe: - other = batdog + other = bathound else: other = crusoe query_for_olduser = models.UserOldId.get(other.uuid) diff --git a/tests/unit/lastuser_core/test_models.py b/tests/unit/lastuser_core/test_models.py index e4385f5..9f57b4c 100644 --- a/tests/unit/lastuser_core/test_models.py +++ b/tests/unit/lastuser_core/test_models.py @@ -12,21 +12,21 @@ def test_merge_users(self): """ # Scenario 1: if first user's created_at date younger than second user's created_at crusoe = self.fixtures.crusoe - batdog = models.User(username=u"batdog", fullname=u"Batdog") - db.session.add(batdog) + bathound = models.User(username=u"bathound", fullname=u"Bathound") + db.session.add(bathound) db.session.commit() - merged = models.merge_users(crusoe, batdog) + merged = models.merge_users(crusoe, bathound) self.assertEqual(merged, crusoe) self.assertIsInstance(merged, models.User) # because the logic is to merge into older account self.assertEqual(crusoe.status, 0) - self.assertEqual(batdog.status, 2) + self.assertEqual(bathound.status, 2) # Scenario 1: if second user's created_at date older than first user 's created_at - tyrion = models.User(username=u'tyrion', fullname=u'Tyrion Lannister') + tyrion = models.User(username=u'tyrion', fullname=u"Tyrion Lannister") db.session.add(tyrion) db.session.commit() - subramanian = models.User(username=u'subramanian', fullname=u'Tyrion subramanian') + subramanian = models.User(username=u'subramanian', fullname=u"Tyrion Subramanian") db.session.add(subramanian) db.session.commit() merged = models.merge_users(subramanian, tyrion) @@ -54,7 +54,7 @@ def test_getuser(self): # scenario 2: with @ in name and not extid d_email = u'd@dothraki.vly' - daenerys = models.User(username=d_email, fullname=u'Daenerys Targaryen', email=d_email) + daenerys = models.User(username=u'daenerys', fullname=u"Daenerys Targaryen", email=d_email) daenerys_email = models.UserEmail(email=d_email, user=daenerys) db.session.add_all([daenerys, daenerys_email]) @@ -67,7 +67,7 @@ def test_getuser(self): # scenario 3: with no @ starting in name, check by UserEmailClaim j_email = u'jonsnow@nightswatch.co.uk' - jonsnow = models.User(username=u'jonsnow', fullname=u'Jon Snow') + jonsnow = models.User(username=u'jonsnow', fullname=u"Jon Snow") jonsnow_email_claimed = models.UserEmailClaim(email=j_email, user=jonsnow) db.session.add_all([jonsnow, jonsnow_email_claimed]) db.session.commit() @@ -76,14 +76,14 @@ def test_getuser(self): self.assertEqual(result4, jonsnow) # scenario 5: with no @ anywhere in name, fetch username - arya = models.User(username=u'arya', fullname=u'Arya Stark') + arya = models.User(username=u'arya', fullname=u"Arya Stark") db.session.add(arya) db.session.commit() result5 = models.getuser(u'arya') self.assertEqual(result5, arya) # scenario 6: with no starting with @ name and no UserEmailClaim or UserEmail - cersei = models.User(username=u'cersei', fullname=u'Cersei Lannister') + cersei = models.User(username=u'cersei', fullname=u"Cersei Lannister") db.session.add(cersei) db.session.commit() result6 = models.getuser(u'cersei@thelannisters.co.uk')