Tranfer aliases to a new mailbox when deleting mailboxes (#1534)

* Set up npm clean install instead of npm install in order to keep the version of npm packages 🎨

* Add option to transfer the alias to a new mailbox when a mailbox is deleted

* Moved alias transfer to job

* Lint

* Update forms

* Revert dockerfile change

Co-authored-by: ewen <ewen.coppens@a1.digital>
Co-authored-by: Adrià Casajús <adria.casajus@proton.ch>

.gitignore

@@ -15,3 +15,4 @@ venv/
 .coverage
 htmlcov
 adhoc
+.env.*

app/api/views/mailbox.py

@@ -78,12 +78,15 @@ def delete_mailbox(mailbox_id):
     Delete mailbox
     Input:
         mailbox_id: in url
+        (optional) transfer_aliases_to: in body. Id of the new mailbox for the aliases.
+                                        If omitted or the value is set to -1,
+                                        the aliases of the mailbox will be deleted too.
     Output:
         200 if deleted successfully
 
     """
     user = g.user
-    mailbox = Mailbox.get(mailbox_id)
+    mailbox = Mailbox.get(id=mailbox_id)
 
     if not mailbox or mailbox.user_id != user.id:
         return jsonify(error="Forbidden"), 403
@@ -91,11 +94,36 @@ def delete_mailbox(mailbox_id):
     if mailbox.id == user.default_mailbox_id:
         return jsonify(error="You cannot delete the default mailbox"), 400
 
+    data = request.get_json() or {}
+    transfer_mailbox_id = data.get("transfer_aliases_to")
+    if transfer_mailbox_id and int(transfer_mailbox_id) >= 0:
+        transfer_mailbox = Mailbox.get(transfer_mailbox_id)
+
+        if not transfer_mailbox or transfer_mailbox.user_id != user.id:
+            return (
+                jsonify(error="You must transfer the aliases to a mailbox you own."),
+                403,
+            )
+
+        if transfer_mailbox_id == mailbox_id:
+            return (
+                jsonify(
+                    error="You can not transfer the aliases to the mailbox you want to delete."
+                ),
+                400,
+            )
+
+        if not transfer_mailbox.verified:
+            return jsonify(error="Your new mailbox is not verified"), 400
+
     # Schedule delete account job
     LOG.w("schedule delete mailbox job for %s", mailbox)
     Job.create(
         name=JOB_DELETE_MAILBOX,
-        payload={"mailbox_id": mailbox.id},
+        payload={
+            "mailbox_id": mailbox.id,
+            "transfer_mailbox_id": transfer_mailbox_id,
+        },
         run_at=arrow.now(),
         commit=True,
     )

app/dashboard/views/mailbox.py

@@ -3,7 +3,7 @@ from flask import render_template, request, redirect, url_for, flash
 from flask_login import login_required, current_user
 from flask_wtf import FlaskForm
 from itsdangerous import TimestampSigner
-from wtforms import validators
+from wtforms import validators, IntegerField
 from wtforms.fields.html5 import EmailField
 
 from app import parallel_limiter
@@ -28,6 +28,13 @@ class NewMailboxForm(FlaskForm):
     )
 
 
+class DeleteMailboxForm(FlaskForm):
+    mailbox_id = IntegerField(
+        validators=[validators.DataRequired()],
+    )
+    transfer_mailbox_id = IntegerField()
+
+
 @dashboard_bp.route("/mailbox", methods=["GET", "POST"])
 @login_required
 @parallel_limiter.lock(only_when=lambda: request.method == "POST")
@@ -40,28 +47,53 @@ def mailbox_route():
 
     new_mailbox_form = NewMailboxForm()
     csrf_form = CSRFValidationForm()
+    delete_mailbox_form = DeleteMailboxForm()
 
     if request.method == "POST":
-        if not csrf_form.validate():
-            flash("Invalid request", "warning")
-            return redirect(request.url)
         if request.form.get("form-name") == "delete":
-            mailbox_id = request.form.get("mailbox-id")
+            if not delete_mailbox_form.validate():
-            mailbox = Mailbox.get(mailbox_id)
+                flash("Invalid request", "warning")
+                return redirect(request.url)
+            mailbox = Mailbox.get(delete_mailbox_form.mailbox_id.data)
 
             if not mailbox or mailbox.user_id != current_user.id:
-                flash("Unknown error. Refresh the page", "warning")
+                flash("Invalid mailbox. Refresh the page", "warning")
                 return redirect(url_for("dashboard.mailbox_route"))
 
             if mailbox.id == current_user.default_mailbox_id:
                 flash("You cannot delete default mailbox", "error")
                 return redirect(url_for("dashboard.mailbox_route"))
 
+            transfer_mailbox_id = delete_mailbox_form.transfer_mailbox_id.data
+            if transfer_mailbox_id and transfer_mailbox_id > 0:
+                transfer_mailbox = Mailbox.get(transfer_mailbox_id)
+
+                if not transfer_mailbox or transfer_mailbox.user_id != current_user.id:
+                    flash("You must transfer the aliases to a mailbox you own.")
+                    return redirect(url_for("dashboard.mailbox_route"))
+
+                if transfer_mailbox.id == mailbox.id:
+                    flash(
+                        "You can not transfer the aliases to the mailbox you want to delete."
+                    )
+                    return redirect(url_for("dashboard.mailbox_route"))
+
+                if not transfer_mailbox.verified:
+                    flash("Your new mailbox is not verified")
+                    return redirect(url_for("dashboard.mailbox_route"))
+
             # Schedule delete account job
-            LOG.w("schedule delete mailbox job for %s", mailbox)
+            LOG.w(
+                f"schedule delete mailbox job for {mailbox.id} with transfer to mailbox {transfer_mailbox_id}"
+            )
             Job.create(
                 name=JOB_DELETE_MAILBOX,
-                payload={"mailbox_id": mailbox.id},
+                payload={
+                    "mailbox_id": mailbox.id,
+                    "transfer_mailbox_id": transfer_mailbox_id
+                    if transfer_mailbox_id > 0
+                    else None,
+                },
                 run_at=arrow.now(),
                 commit=True,
             )
@@ -74,7 +106,10 @@ def mailbox_route():
 
             return redirect(url_for("dashboard.mailbox_route"))
         if request.form.get("form-name") == "set-default":
-            mailbox_id = request.form.get("mailbox-id")
+            if not csrf_form.validate():
+                flash("Invalid request", "warning")
+                return redirect(request.url)
+            mailbox_id = request.form.get("mailbox_id")
             mailbox = Mailbox.get(mailbox_id)
 
             if not mailbox or mailbox.user_id != current_user.id:
@@ -112,12 +147,12 @@ def mailbox_route():
                 elif not email_can_be_used_as_mailbox(mailbox_email):
                     flash(f"You cannot use {mailbox_email}.", "error")
                 else:
-                    new_mailbox = Mailbox.create(
+                    transfer_mailbox = Mailbox.create(
                         email=mailbox_email, user_id=current_user.id
                     )
                     Session.commit()
 
-                    send_verification_email(current_user, new_mailbox)
+                    send_verification_email(current_user, transfer_mailbox)
 
                     flash(
                         f"You are going to receive an email to confirm {mailbox_email}.",
@@ -126,7 +161,8 @@ def mailbox_route():
 
                     return redirect(
                         url_for(
-                            "dashboard.mailbox_detail_route", mailbox_id=new_mailbox.id
+                            "dashboard.mailbox_detail_route",
+                            mailbox_id=transfer_mailbox.id,
                         )
                     )
 
@@ -134,36 +170,11 @@ def mailbox_route():
         "dashboard/mailbox.html",
         mailboxes=mailboxes,
         new_mailbox_form=new_mailbox_form,
+        delete_mailbox_form=delete_mailbox_form,
         csrf_form=csrf_form,
     )
 
 
-def delete_mailbox(mailbox_id: int):
-    from server import create_light_app
-
-    with create_light_app().app_context():
-        mailbox = Mailbox.get(mailbox_id)
-        if not mailbox:
-            return
-
-        mailbox_email = mailbox.email
-        user = mailbox.user
-
-        Mailbox.delete(mailbox_id)
-        Session.commit()
-        LOG.d("Mailbox %s %s deleted", mailbox_id, mailbox_email)
-
-        send_email(
-            user.email,
-            f"Your mailbox {mailbox_email} has been deleted",
-            f"""Mailbox {mailbox_email} along with its aliases are deleted successfully.
-
-Regards,
-SimpleLogin team.
-        """,
-        )
-
-
 def send_verification_email(user, mailbox):
     s = TimestampSigner(MAILBOX_SECRET)
     mailbox_id_signed = s.sign(str(mailbox.id)).decode()

docs/api.md

@@ -764,6 +764,7 @@ Input:
 
 - `Authentication` header that contains the api key
 - `mailbox_id`: in url
+- (optional) `transfer_aliases_to`: in body as json. id of the new mailbox for the aliases. If omitted or set to -1, the aliases will be delete with the mailbox.
 
 Output:
 

job_runner.py

@@ -124,6 +124,58 @@ def welcome_proton(user):
     )
 
 
+def delete_mailbox_job(job: Job):
+    mailbox_id = job.payload.get("mailbox_id")
+    mailbox = Mailbox.get(mailbox_id)
+    if not mailbox:
+        return
+
+    transfer_mailbox_id = job.payload.get("transfer_mailbox_id")
+    alias_transferred_to = None
+    if transfer_mailbox_id:
+        transfer_mailbox = Mailbox.get(transfer_mailbox_id)
+        if transfer_mailbox:
+            alias_transferred_to = transfer_mailbox.email
+
+            for alias in mailbox.aliases:
+                if alias.mailbox_id == mailbox.id:
+                    alias.mailbox_id = transfer_mailbox.id
+                    if transfer_mailbox in alias._mailboxes:
+                        alias._mailboxes.remove(transfer_mailbox)
+                else:
+                    alias._mailboxes.remove(mailbox)
+                    if transfer_mailbox not in alias._mailboxes:
+                        alias._mailboxes.append(transfer_mailbox)
+                Session.commit()
+
+    mailbox_email = mailbox.email
+    user = mailbox.user
+    Mailbox.delete(mailbox_id)
+    Session.commit()
+    LOG.d("Mailbox %s %s deleted", mailbox_id, mailbox_email)
+
+    if alias_transferred_to:
+        send_email(
+            user.email,
+            f"Your mailbox {mailbox_email} has been deleted",
+            f"""Mailbox {mailbox_email} and its alias have been transferred to {alias_transferred_to}.
+        Regards,
+        SimpleLogin team.
+        """,
+            retries=3,
+        )
+    else:
+        send_email(
+            user.email,
+            f"Your mailbox {mailbox_email} has been deleted",
+            f"""Mailbox {mailbox_email} along with its aliases have been deleted successfully.
+                Regards,
+                SimpleLogin team.
+                """,
+            retries=3,
+        )
+
+
 def process_job(job: Job):
     if job.name == config.JOB_ONBOARDING_1:
         user_id = job.payload.get("user_id")
@@ -178,27 +230,7 @@ def process_job(job: Job):
             retries=3,
         )
     elif job.name == config.JOB_DELETE_MAILBOX:
-        mailbox_id = job.payload.get("mailbox_id")
+        delete_mailbox_job(job)
-        mailbox = Mailbox.get(mailbox_id)
-        if not mailbox:
-            return
-
-        mailbox_email = mailbox.email
-        user = mailbox.user
-
-        Mailbox.delete(mailbox_id)
-        Session.commit()
-        LOG.d("Mailbox %s %s deleted", mailbox_id, mailbox_email)
-
-        send_email(
-            user.email,
-            f"Your mailbox {mailbox_email} has been deleted",
-            f"""Mailbox {mailbox_email} along with its aliases are deleted successfully.
-Regards,
-SimpleLogin team.
-""",
-            retries=3,
-        )
 
     elif job.name == config.JOB_DELETE_DOMAIN:
         custom_domain_id = job.payload.get("custom_domain_id")

templates/dashboard/mailbox.html

@@ -25,17 +25,18 @@
       <div class="alert alert-primary collapse {% if mailboxes|length == 1 %} show{% endif %}"
            id="howtouse"
            role="alert">
-        A <em>mailbox</em> is just another personal email address. When creating a new alias, you could choose the
+        A <em>mailbox</em> is just another personal email address. When creating a new alias, you could choose
+        the
         mailbox that <em>owns</em> this alias, i.e:
-        <br />
+        <br/>
         - all emails sent to this alias will be forwarded to this mailbox
-        <br />
+        <br/>
         - from this mailbox, you can reply/send emails from the alias.
-        <br />
+        <br/>
-        <br />
+        <br/>
         When you signed up, a mailbox is automatically created with your email <b>{{ current_user.email }}</b>
-        <br />
+        <br/>
-        <br />
+        <br/>
         The mailbox doesn't have to be your email: it can be your friend's email
         if you want to create aliases for your buddy.
       </div>
@@ -74,11 +75,12 @@
                 </h5>
                 <h6 class="card-subtitle mb-2 text-muted">
                   Created {{ mailbox.created_at | dt }}
-                  <br />
+                  <br/>
                   <span class="font-weight-bold">{{ mailbox.nb_alias() }}</span> aliases.
-                  <br />
+                  <br/>
                 </h6>
-                <a href="{{ url_for('dashboard.mailbox_detail_route', mailbox_id=mailbox.id) }}">Edit ➡</a>
+                <a href="{{ url_for('dashboard.mailbox_detail_route', mailbox_id=mailbox.id) }}">Edit
+                ➡</a>
               </div>
               <div class="card-footer p-0">
                 <div class="row">
@@ -89,7 +91,7 @@
                         {{ csrf_form.csrf_token }}
                         <input type="hidden" name="form-name" value="set-default">
                         <input type="hidden" class="mailbox" value="{{ mailbox.email }}">
-                        <input type="hidden" name="mailbox-id" value="{{ mailbox.id }}">
+                        <input type="hidden" name="mailbox_id" value="{{ mailbox.id }}">
                         <button class="card-link btn btn-link {% if mailbox.id == current_user.default_mailbox_id %} disabled{% endif %}">
                           Set As Default Mailbox
                         </button>
@@ -98,10 +100,24 @@
                   {% endif %}
                   <div class="col">
                     <form method="post">
-                      {{ csrf_form.csrf_token }}
+                      {{ delete_mailbox_form.csrf_token }}
                       <input type="hidden" name="form-name" value="delete">
                       <input type="hidden" class="mailbox" value="{{ mailbox.email }}">
-                      <input type="hidden" name="mailbox-id" value="{{ mailbox.id }}">
+                      <input type="hidden" name="mailbox_id" value="{{ mailbox.id }}">
+                      <select hidden name="transfer_mailbox_id" value="">
+                        <option value="-1">
+                          Delete my aliases
+                        </option>
+                        {% for mailbox_opt in mailboxes %}
+
+                          {% if mailbox_opt.verified and mailbox_opt.id != mailbox.id %}
+
+                            <option value="{{ mailbox_opt.id }}">
+                              {{ mailbox_opt.email }}
+                            </option>
+                          {% endif %}
+                        {% endfor %}
+                      </select>
                       <span class="card-link btn btn-link text-danger float-right delete-mailbox {% if mailbox.id == current_user.default_mailbox_id %} disabled{% endif %}">
                         Delete
                       </span>
@@ -128,31 +144,39 @@
 {% block script %}
 
   <script>
-    $(".delete-mailbox").on("click", function (e) {
+        $(".delete-mailbox").on("click", function (e) {
-      let mailbox = $(this).parent().find(".mailbox").val();
+            let mailbox = $(this).parent().find(".mailbox").val();
 
-      let that = $(this);
+            let new_mailboxes = $(this).parent().find("select[name='transfer_mailbox_id']").find("option")
-      let message = `All aliases owned by this mailbox <b>${mailbox}</b> will be also deleted, ` +
+            let inputOptions = new_mailboxes.map((index, option) => { return {["value"]: option.value, ["text"]: option.text}}).toArray()
-        " please confirm.";
 
-      bootbox.confirm({
+            let that = $(this);
-        message: message,
+            let message = `All aliases owned by the mailbox <b>${mailbox}</b> will be also deleted.<br>` +
-        buttons: {
+                "You can choose to transfer them to a different mailbox:<br><br>";
-          confirm: {
+
-            label: 'Yes, delete it',
+            bootbox.prompt({
-            className: 'btn-danger'
+                title: '<b>Delete Mailbox</b>',
-          },
+                message: message,
-          cancel: {
+                value: ["-1"],
-            label: 'Cancel',
+                inputType: 'select',
-            className: 'btn-outline-primary'
+                inputOptions: inputOptions,
-          }
+                buttons: {
-        },
+                    confirm: {
-        callback: function (result) {
+                        label: 'Yes, delete it',
-          if (result) {
+                        className: 'btn-danger'
-            that.closest("form").submit();
+                    },
-          }
+                    cancel: {
-        }
+                        label: 'Cancel',
-      })
+                        className: 'btn-outline-primary mr-auto'
-    });
+                    }
+                },
+                callback: function (result) {
+                    if (result) {
+                        that.closest("form").find("select[name='transfer_mailbox_id']").val(result)
+                        that.closest("form").submit();
+                    }
+                }
+            })
+        });
   </script>
 {% endblock %}

tests/jobs/test_delete_mailbox_job.py

@@ -0,0 +1,90 @@
+from sqlalchemy_utils.types.arrow import arrow
+
+from app.config import JOB_DELETE_MAILBOX
+from app.db import Session
+from app.mail_sender import mail_sender
+from app.models import Alias, Mailbox, Job, AliasMailbox
+from job_runner import delete_mailbox_job
+from tests.utils import create_new_user, random_email
+
+
+@mail_sender.store_emails_test_decorator
+def test_delete_mailbox_transfer_mailbox_primary(flask_client):
+    user = create_new_user()
+    m1 = Mailbox.create(
+        user_id=user.id, email=random_email(), verified=True, flush=True
+    )
+    m2 = Mailbox.create(
+        user_id=user.id, email=random_email(), verified=True, flush=True
+    )
+
+    alias_id = Alias.create_new(user, "prefix", mailbox_id=m1.id).id
+    AliasMailbox.create(alias_id=alias_id, mailbox_id=m2.id)
+    job = Job.create(
+        name=JOB_DELETE_MAILBOX,
+        payload={"mailbox_id": m1.id, "transfer_mailbox_id": m2.id},
+        run_at=arrow.now(),
+        commit=True,
+    )
+    Session.commit()
+    delete_mailbox_job(job)
+    alias = Alias.get(alias_id)
+    assert alias.mailbox_id == m2.id
+    assert len(alias._mailboxes) == 0
+    mails_sent = mail_sender.get_stored_emails()
+    assert len(mails_sent) == 1
+    assert str(mails_sent[0].msg).find("alias have been transferred") > -1
+
+
+@mail_sender.store_emails_test_decorator
+def test_delete_mailbox_transfer_mailbox_in_list(flask_client):
+    user = create_new_user()
+    m1 = Mailbox.create(
+        user_id=user.id, email=random_email(), verified=True, flush=True
+    )
+    m2 = Mailbox.create(
+        user_id=user.id, email=random_email(), verified=True, flush=True
+    )
+    m3 = Mailbox.create(
+        user_id=user.id, email=random_email(), verified=True, flush=True
+    )
+
+    alias_id = Alias.create_new(user, "prefix", mailbox_id=m1.id).id
+    AliasMailbox.create(alias_id=alias_id, mailbox_id=m2.id)
+    job = Job.create(
+        name=JOB_DELETE_MAILBOX,
+        payload={"mailbox_id": m2.id, "transfer_mailbox_id": m3.id},
+        run_at=arrow.now(),
+        commit=True,
+    )
+    Session.commit()
+    delete_mailbox_job(job)
+    alias = Alias.get(alias_id)
+    assert alias.mailbox_id == m1.id
+    assert len(alias._mailboxes) == 1
+    assert alias._mailboxes[0].id == m3.id
+    mails_sent = mail_sender.get_stored_emails()
+    assert len(mails_sent) == 1
+    assert str(mails_sent[0].msg).find("alias have been transferred") > -1
+
+
+@mail_sender.store_emails_test_decorator
+def test_delete_mailbox_no_transfer(flask_client):
+    user = create_new_user()
+    m1 = Mailbox.create(
+        user_id=user.id, email=random_email(), verified=True, flush=True
+    )
+
+    alias_id = Alias.create_new(user, "prefix", mailbox_id=m1.id).id
+    job = Job.create(
+        name=JOB_DELETE_MAILBOX,
+        payload={"mailbox_id": m1.id},
+        run_at=arrow.now(),
+        commit=True,
+    )
+    Session.commit()
+    delete_mailbox_job(job)
+    assert Alias.get(alias_id) is None
+    mails_sent = mail_sender.get_stored_emails()
+    assert len(mails_sent) == 1
+    assert str(mails_sent[0].msg).find("along with its aliases have been deleted") > -1