-
Notifications
You must be signed in to change notification settings - Fork 315
💻Allows email addresses as usernames when student account is cre… #6392
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still just reviewing as a contributor, but I think I spotted a security issue with this PR
if not user: | ||
user = self.db.user_by_username(body["username"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe adding this code alone will cause a bit of a authorization/security issue, unless we also change update_profile
in profile.py. The update_profile
code will also have to check if a user exists with user_by_username
when the user updates their email, in order to prevent conflicts.
The problematic scenario is:
- A Teacher creates two student accounts with emails as usernames. E.g. username: [email protected], and username: [email protected]
- I login as a student with my username, [email protected], then I go to my profile settings.
- My email is initially blank. I update my email to [email protected]
- Now [email protected] cannot login because I have taken their email address, and their account still only has a username.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brianpeiris Good catch! The point of this PR is to allow emails however, so maybe the following could solve this:
Use utils.valid_email
to check if someone's username is a valid email, and if it is, also set that accounts email equal to its username. This should solve the following situations:
- A teacher creating an account with username
floris@
(this account will not have its email set) - A teacher creating an account with username
[email protected]
while another account already has this as its email (the new account will be rejected as its email is already in use) - A teacher creating accounts with the usernames
[email protected]
andbrian
, then brian setting their email (which is currently blank) to[email protected]
. (Brians new email will not be allowed, as its already in use)
Would this still satisfy the original requirements and solve the security issues @jpelay ?
Hello, @brianpeiris! Can't thank you enough for taking care of reviewing this PR, I think all your suggestions are on point, specially the last one about the security issue. @FBastiaan04 please take a look into those whenever you can. |
Co-authored-by: Brian Peiris <[email protected]>
Co-authored-by: Brian Peiris <[email protected]>
for more information, see https://pre-commit.ci
Allows email addresses as usernames when student account is created by teacher.
The colon character (:) is still disallowed. The original reason for disallowing this was not found, and it was decided that there was not enough demand for allowing it to warrent the effort.
Fixes #5888
How to test
Follow these steps to verify this PR works as intended:
Checklist
Done? Check if you have it all in place using this list: (mark with x if done)
If you're unsure about any of these, don't hesitate to ask. We're here to help!