Prevent registering recently-deleted usernames #8723
Conversation
seanh
force-pushed
the
prevent-reusing-recently-deleted-usernames
branch
from
78a7693
to
4f84a9e
Compare
| @@ -72,7 +74,6 @@ def test_it_is_valid_when_authorized_users_email( | |||
| schemas.unique_email(dummy_node, "elliot@bar.com") | |||
|
|
|||
|
|
|||
| @pytest.mark.usefixtures("user_model") | |||
There was a problem hiding this comment.
Bad tests: mocking models.User. We shouldn't mock models. In this instance the presence of this mock means that once I added an SQL query of the User model to the schema code all the tests started failing because SQLAlchemy crashes if you pass a mock object to a query instead of a real model object.
The reason the tests are mocking the model is that the model has methods on it like get_by_username(), get_by_email(), etc. Models shouldn't have methods like this, those belong on services.
| schema = schemas.RegisterSchema().bind(request=pyramid_request) | ||
| user_model.get_by_username.return_value = None |
There was a problem hiding this comment.
In most cases the mocking was actually unnecessary: the real get_by_username() will return None anyway. The test for what happens when user_models.get_*() does return something is actually missing and there's a nocover comment!
| raise colander.Invalid(node, msg) | ||
| raise exc | ||
|
|
||
| if request.db.scalars( |
There was a problem hiding this comment.
We could move this to the user service and maybe even combine it with the usage of get_by_username in something like "is_username_avaiable".
There was a problem hiding this comment.
Could to. I'm actually not sure there's much benefit in moving this code to a service though, I think it'll only be needed in this one place in the registration schema.
| @@ -19,3 +19,7 @@ def split_user(userid): | |||
| if match: | |||
| return {"username": match.groups()[0], "domain": match.groups()[1]} | |||
| raise InvalidUserId(userid) | |||
|
|
|||
|
|
|||
| def format_userid(username, authority): | |||
seanh
force-pushed
the
record-user-deletions
branch
from
7ab0ba2
to
b875bfd
Compare
seanh
force-pushed
the
prevent-reusing-recently-deleted-usernames
branch
from
4f84a9e
to
32babe1
Compare
Testing
Log in to http://localhost:5000/login as
devdata_admin, go to http://localhost:5000/admin/users?username=devdata_user&authority=localhost and deletedevdata_user. Now go to http://localhost:5000/signup and try to register an account with the usernamedevdata_user. Onmainthis would succeed. On this branch you should see this: