Very cool!! thank you!! The ⚠️ ones are security issues, the rest of them are mostly nitpicks but I still would like to address them before deploying this code.
⚠️ Both insert_email_update_row and update_email MUST ensure that the new_email is not an email address being used by another user. They should error or otherwise indicate failure in that case. ⚠️
⚠️email_change_confirmation in auth.py should have @account_required. ⚠️
For simplicity, I don't think we should put the new email into the URL of the email_change_confirmation endpoint. It can just be /change_email/<string:token>. It should use the flask session to get the original email address (session['account']).
I think check_email_change_token and emails_contained_in_row should be merged into one function.
then inside check_email_change_token it will run the query
select new_email from email_updates where token = %s and current_email = $s
if re.fullmatch('^[a-z0-9]+[\._]?[a-z0-9]+[@]\w+[.]\w{2,3}$', new_email):
We already have a way of validating email addresses that we use in login in auth.py:
if not email:
errors.append("email is required")
elif len(email.strip()) < 6 or email.count('@') != 1 or email.count('.') == 0:
errors.append("enter a valid email address")
Validating emails with regex is very difficult and almost always someone will show up with a valid email that fails the regex. I like doing it like this because it's guaranteed never to reject a valid email address.
If you want we could move the email validation into a dedicated function in shared.py
The POST handler can just return a redirect to /account on success so the code that renders the page doesn't have to be duplicated.
print("[DEBUG] Email change message: " + message)
This should use current_app.logger.info(...)
BTW for check_email_change_token or any database model method which only runs SELECT queries, aka its read-only, no UPDATE, INSERT, or DELETE, you don't have to call self.connection.commit()
Very cool!! thank you!! The ⚠️ ones are security issues, the rest of them are mostly nitpicks but I still would like to address them before deploying this code.
----------------
⚠️ Both `insert_email_update_row` and `update_email` MUST ensure that the `new_email` is not an email address being used by another user. They should error or otherwise indicate failure in that case. ⚠️
----------------
⚠️ `email_change_confirmation` in `auth.py` should have `@account_required`. ⚠️
For simplicity, I don't think we should put the new email into the URL of the `email_change_confirmation` endpoint. It can just be `/change_email/<string:token>`. It should use the flask session to get the original email address (`session['account']`).
I think `check_email_change_token` and `emails_contained_in_row` should be merged into one function.
`email_change_confirmation` will call
`new_email = get_model().check_email_change_token(token, session['account'])`
then inside `check_email_change_token` it will run the query
`select new_email from email_updates where token = %s and current_email = $s`
----------------
> ` if re.fullmatch('^[a-z0-9]+[\._]?[a-z0-9]+[@]\w+[.]\w{2,3}$', new_email):`
We already have a way of validating email addresses that we use in `login` in [`auth.py`](https://git.cyberia.club/cyberia/capsul-flask/src/branch/main/capsulflask/auth.py#L56-L59):
```
if not email:
errors.append("email is required")
elif len(email.strip()) < 6 or email.count('@') != 1 or email.count('.') == 0:
errors.append("enter a valid email address")
```
Validating emails with regex is very difficult and almost always someone will show up with a valid email that fails the regex. I like doing it like this because it's guaranteed never to reject a valid email address.
If you want we could move the email validation into a dedicated function in `shared.py`
----------------
> ```
> @bp.route("/account", methods=("GET", "POST"))
> def account():
> ```
For code clarity I think it would be better to split this out into 2 functions like so:
```
@bp.route("/account", methods=("GET",))
def get_account():
```
and
```
@bp.route("/account", methods=("POST",))
def post_account():
```
The `POST` handler can just return a redirect to `/account` on success so the code that renders the page doesn't have to be duplicated.
----------------
> `print("[DEBUG] Email change message: " + message)`
This should use `current_app.logger.info(...)`
----------------
BTW for `check_email_change_token` or any database model method which only runs `SELECT` queries, aka its read-only, no `UPDATE`, `INSERT`, or `DELETE`, you don't have to call `self.connection.commit()`
Allows users to change their own email addresses without needing help from support.
Very cool!! thank you!! The ⚠️ ones are security issues, the rest of them are mostly nitpicks but I still would like to address them before deploying this code.
⚠️ Both
insert_email_update_rowandupdate_emailMUST ensure that thenew_emailis not an email address being used by another user. They should error or otherwise indicate failure in that case. ⚠️⚠️
email_change_confirmationinauth.pyshould have@account_required. ⚠️For simplicity, I don't think we should put the new email into the URL of the
email_change_confirmationendpoint. It can just be/change_email/<string:token>. It should use the flask session to get the original email address (session['account']).I think
check_email_change_tokenandemails_contained_in_rowshould be merged into one function.email_change_confirmationwill callnew_email = get_model().check_email_change_token(token, session['account'])then inside
check_email_change_tokenit will run the queryselect new_email from email_updates where token = %s and current_email = $sWe already have a way of validating email addresses that we use in
logininauth.py:Validating emails with regex is very difficult and almost always someone will show up with a valid email that fails the regex. I like doing it like this because it's guaranteed never to reject a valid email address.
If you want we could move the email validation into a dedicated function in
shared.pyFor code clarity I think it would be better to split this out into 2 functions like so:
and
The
POSThandler can just return a redirect to/accounton success so the code that renders the page doesn't have to be duplicated.This should use
current_app.logger.info(...)BTW for
check_email_change_tokenor any database model method which only runsSELECTqueries, aka its read-only, noUPDATE,INSERT, orDELETE, you don't have to callself.connection.commit()