Added self-service email changes #46

Closed

Ghost wants to merge 1 commit from (deleted):main into main

Ghost commented

2023-03-12 04:00:49 +00:00

First-time contributor

Allows users to change their own email addresses without needing help from support.

Ghost added 1 commit 2023-03-12 04:00:51 +00:00

Added self-service email changes 69e2b1a024

forest commented

2023-03-12 05:46:10 +00:00

Owner

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:

        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()

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()`