users: add user creation endpoint #146
Conversation
egabancho
commented
Oct 8, 2024
egabancho
commented
- addresses Backend: Users: Support create/update of users and groups #23
egabancho
force-pushed
the
allow-user-creation
branch
from
a252e87 to
724ad8a
Compare
| raise ValidationError(errors) | ||
| # Create User | ||
| account_user = current_datastore.create_user(**data) | ||
| current_datastore.commit() # Commit to save the user to the database |
There was a problem hiding this comment.
Question: Should we Consider adding a rollback or transaction management for unexpected failures?
There was a problem hiding this comment.
🤔 Wouldn't the transaction roll back once the exception is raised? i.e., line 241
There was a problem hiding this comment.
i think only if the function is wrapped by the uow otherwise not. assuming you are referring to https://github.com/inveniosoftware/invenio-db/blob/305b5b179c1b9441f770c64bba2332436a79593e/invenio_db/uow.py#L171
There was a problem hiding this comment.
This was on an oversite! We did have RecordCommitOp at some point and removed it to check what errors we would get after saving, i.e. duplicate emails/username. Then we added the control code for that and forgot to put back the RecordCommitOp ... 😇
| @@ -100,7 +101,7 @@ class UserSchema(BaseRecordSchema, FieldPermissionsMixin): | |||
| visibility = fields.Str(dump_only=True) | |||
| is_current_user = fields.Method("is_self", dump_only=True) | |||
|
|
|||
| email = fields.String() | |||
| email = fields.Email() | |||
There was a problem hiding this comment.
While this is correct, it introduces stricter validation. Existing users with invalid email formats might fail schema validations. To prevent disruptions, a migration strategy should be planned to handle or correct existing invalid email entries.
There was a problem hiding this comment.
That's an excellent point. I'm unsure how you'd get wrongly formatted emails on the db, though.
There are plenty of other places where wrong emails might break stuff. Off the top of my head, some of the domain-related stuff, probably https://github.com/inveniosoftware/invenio-accounts/blob/master/invenio_accounts/utils.py#L264
I don't know what the best path forward is. If we decide to do a "migration", what do we do with wrongly formatted emails? (email is a required column on the DB).
Let's get someone else's opinion on this.
| user.activate() | ||
| user.verify() |
There was a problem hiding this comment.
Question:
The _create_user_as_admin method automatically activates and verifies the user without additional confirmation steps. Should we consider verification steps, such as sending a confirmation email or letting the user log in and self-activate their account as it might be necessary to ensure the user's email is valid and belongs to them?
There was a problem hiding this comment.
We thought/discussed this internally and concluded that, because an email to change the password has already been sent, the email would be verified and correct once the new user changed their password. Does it make sense?
There was a problem hiding this comment.
Let's defer to the CERN team on this decision. However, I think it's important to consider that if an admin enters an incorrect email address "admins are still humans :) ", it could result in an active and verified account that isn't linked to the intended user.
Implementing a verification step during the user's first login would ensure that the email is valid and belongs to the correct individual, thereby enhancing account security.
| for key in [ | ||
| "id", | ||
| "domain", | ||
| "preferences", | ||
| "profile", | ||
| "identities", | ||
| "domaininfo", | ||
| ]: | ||
| if key in data: | ||
| data.pop(key) |
There was a problem hiding this comment.
Here you remove specific keys from the data dictionary to pass schema validation. However, it does not handle unexpected or additional fields that might be present, maybe a more robust data sanitization or validation mechanism could prevent potential issues from unanticipated input fields?
There was a problem hiding this comment.
That's a good point; we should probably do it the other way around!
|
Thanks a lot for this, LGTM! |
egabancho
force-pushed
the
allow-user-creation
branch
2 times, most recently
from
3f64d0e to
8b88f02
Compare
