Skip to content
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

Create and receive mozmail relay alias #993

Merged
merged 38 commits into from
Aug 24, 2021

Conversation

say-yawn
Copy link
Contributor

@say-yawn say-yawn commented Aug 5, 2021

About this PR

Since we own mozmail.com we want to make sure that Relay can create Relay aliases and get emails relayed with the new domain.

Acceptance Criteria

  • Add logic to create Relay alias with mozmail.com as the domain
  • When TEST_MOZMAIL environment variable is set to True users Relay alias is created with mozmail.com domain INSTEAD of relay.firefox.com
  • Even when TEST_MOZMAIL is False users with mozilla.com email address can use mozmail.com
  • Both free and premium users can generate random aliases with mozmail.com
  • Send alias with mozmail domain to the real user's email
  • Increment forward/block on the relay alias

Related to

MPP-712 and MPP-714

Test

Test that relay address works as is

  1. Send email with non-English characters and attachment to the old relay address (relay.firefox.com equivalent)
  2. Copy the address by clicking on the copy icon and check that the correct relay address is copied on clipboard
  3. Check that email is properly received and the alias on the dashboard (/accounts/profile/) is incremented

Test that relay address is generated with the mozmail.com when the TEST_MOZMAIL environment variable is set to True, MOZMAIL_DOMAIN, and RELAY_FIREFOX_DOMAIN is set

  1. Set the environment variable for TEST_MOZMAIL to True and MOZMAIL_DOMAIN, and RELAY_FIREFOX_DOMAIN is set to the respective mozmail domain and restart the service
  2. Go the the user dashboard (/accounts/profile/)
  3. Check that existing Relay Address have the old domain (relay.firefox.com equivalent)
  4. Click "Generate New Alias" and check that alias is created with a new domain (mozmail.com equivalent)
  5. Copy the address by clicking on the copy icon and check that the correct relay address is copied on clipboard
  6. Check that aliases previously created with old domain still has its' old domain and the dash board looks like:

Screen Shot 2021-08-05 at 10 30 58

Test that emails are relayed to both alias generated with mozmail.com and existing alias generated with relay.firefox.com

  1. Send email to alias generated with old domain (relay.firefox.com equivalent). Send non-English characters and attachments with email size BELOW 150KB.
  2. Check that you receive an email and the old domain is reflected properly like:

Screen Shot 2021-08-05 at 10 49 12

3. Send email to alias generated with new domain (mozmail.com equivalent). Send non-English characters and attachments with email size below 150KB. 4. Check that you receive an email and the new domain is reflected properly like:

Screen Shot 2021-08-05 at 10 49 20

  1. Check that for both aliases, the number of forward/block is incremented
  2. Do steps 1-5 again with the alias set to blocked and check that emails are blocked and incremented properly.

Test email generation on add-on works properly

  1. Open the add-on and generate new alias
  2. Check that alias generated and filled on the website it was generated on has the mozmail domain
  3. Check that on /accounts/profile/ the alias was generated with the mozmail domain

Test that users without the mozilla.com email address can use the Relay as is

  1. Remove values for the environment variables for TEST_MOZMAIL, MOZMAIL_DOMAIN, and RELAY_FIREFOX_DOMAIN
  2. Generate email on dashboard and check that the alias is generated on a relay.firefox.com equivalent domain
  3. Send an email with international characters and attachment with size smaller than 150KB
  4. Check that email is received in the correct domain (non-mozmail.com)
  5. Check that existing aliases also can receive emails with the right address

Test that users without the mozilla.com email address can use the Relay as is

  1. Remove values for the environment variables for TEST_MOZMAIL, MOZMAIL_DOMAIN, and RELAY_FIREFOX_DOMAIN
  2. Generate email on dashboard and check that the alias is generated on mozmail.com equivalent domain
  3. Send an email with international characters and attachment with size smaller than 150KB
  4. Check that email is received in the correct domain (mozmail.com)
  5. Check that existing aliases also can receive emails with the right address

@say-yawn say-yawn temporarily deployed to fx-private-relay August 5, 2021 19:46 Inactive
@say-yawn say-yawn temporarily deployed to fx-private-relay August 12, 2021 01:55 Inactive
When the TEST_MOZMAIL and ADDITIONAL_DOMAINS are not set in settings return the domain based on the ON_HEROKU setting
When the TEST_MOZMAIL and ADDITIONAL_DOMAINS are set return the first domain from ADDITIONAL_DOMAINS
Hash should be the address and domain included
@say-yawn say-yawn temporarily deployed to fx-private-relay August 12, 2021 03:48 Inactive
@say-yawn
Copy link
Contributor Author

_index_POST in email/views.py returns the wrong domain address for Relay. Needs to be fixed so the correct Relay Address and domain is returned. Otherwise, the add-on will be broken when Mozmail is turned on.

Copy link
Member

@groovecoder groovecoder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good, but this seems like we're conflating the purpose of the SITE_ORIGIN and ADDITIONAL_DOMAINS settings when we maybe don't need to? Could we simplify everything and just have the 2 separate settings serve the 2 separate purposes, even if the domain value is duplicated in them?

So, SITE_ORIGIN always means "the origin of the app website" and maybe EMAIL_DOMAINS always means "the email domains for which the app works". In the settings that might look like:

For local apps:

SITE_ORIGIN=http://127.0.0.1:8000
EMAIL_DOMAINS=127.0.0.1

For dev:

SITE_ORIGIN=https://dev.fxprivaterelay.nonprod.cloudops.mozgcp.net/
EMAIL_DOMAINS=mail.dev.fxprivaterelay.nonprod.cloudops.mozgcp.net

For stage:

SITE_ORIGIN=https://stage.fxprivaterelay.nonprod.cloudops.mozgcp.net/
EMAIL_DOMAINS=stage.fxprivaterelay.nonprod.cloudops.mozgcp.net

For prod:

SITE_ORIGIN=https://relay.firefox.com
EMAIL_DOMAINS=relay.firefox.com,mozmail.com

Use integer to describe relay.firefox.com or mozmail.com addresses so we can change the values of these based on different environment. Updated tests accordingly.
@say-yawn say-yawn temporarily deployed to fx-private-relay August 18, 2021 19:30 Inactive
@say-yawn say-yawn temporarily deployed to fx-private-relay August 18, 2021 19:50 Inactive
@say-yawn say-yawn temporarily deployed to fx-private-relay August 18, 2021 22:32 Inactive
@say-yawn say-yawn temporarily deployed to fx-private-relay August 19, 2021 15:41 Inactive
@say-yawn say-yawn temporarily deployed to fx-private-relay August 19, 2021 17:30 Inactive
@groovecoder groovecoder temporarily deployed to fx-private-relay August 19, 2021 18:29 Inactive
@say-yawn say-yawn temporarily deployed to fx-private-relay August 23, 2021 14:54 Inactive
@groovecoder
Copy link
Member

While testing, found some things ...

  1. On http://127.0.0.1:8000/accounts/profile/, I clicked "Generate New Alias" and I got a JSON response in the browser?
  2. In the add-on, I clicked "Generate New Alias" in the button pop-up, and the email alias that was put into the form had the OLD domain? (But back on http://127.0.0.1:8000/accounts/profile/ it showed correctly with the NEW domain)
  3. After I removed the env vars, all new aliases were still created at the mozmail.com domain.

Comment on lines +233 to +234
RELAY_FIREFOX_DOMAIN = config('RELAY_FIREFOX_DOMAIN', 'relay.firefox.com', cast=str)
MOZMAIL_DOMAIN = config('MOZMAIL_DOMAIN', 'mozmail.com', cast=str)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I set RELAY_FIREFOX_DOMAIN to unfck.email and MOZMAIL_DOMAIN to mozmail.unfck.email)

The fact that these have default values seems to have messed up my aliases when I removed the env vars. My aliases were created with the domain 2 value, which became MOZMAIL_DOMAIN, and then all my aliases changed to @mozmail.com instead of going back to the unfck.email domain that they actually work on.

I think we're conflating env vars and DB values too much here. We may need to atone for the Relay Original Sin of NOT storing the domain values in the address records themselves.

@say-yawn
Copy link
Contributor Author

say-yawn commented Aug 24, 2021

This is probably because the ORIGIN of the request and the SITE_ORIGIN being checked on is not the same, hence it's returning the JSON response as seen in this code snippet. I think it's because you are using ngrok? Try again with the SITE_ORIGIN being the ngrok endpoint and it should work as expected. I did not get this problem on the dev server.

  • In the add-on, I clicked "Generate New Alias" in the button pop-up, and the email alias that was put into the form had the OLD domain? (But back on http://127.0.0.1:8000/accounts/profile/ it showed correctly with the NEW domain)

Oh, I think this is a problem on the add-on. Since the add-on is appending the domain, as deduced by the JSON response here only having the address field I'm guessing the add-on is appending the domain. I can add a field to the JSON response for the domain, or change the address field to include the entire address (local and domain) but seems like we need an add-on change with this. Good catch.

  • After I removed the env vars, all new aliases were still created at the mozmail.com domain.

Yup, this is a problem though I didn't think we would be removing the env vars for any reason and if that were to happen that's why I added the default value on the settings config.

As for conflating the env vars and DB values, the reason for separating the two was so we can make sure the value in the DB is static even though the environment may be different, hence leaving the value of the domain be an env variable. So instead of the table for RelayAddress looking like:

address | domain
xyz123   | relay.firefox.com
abc123   | mozmail.com

the table will look like

address | domain
xyz123   | 1
abc123   | 2

where the human readable value of 1 and 2 will be the domains associated with the environment. I can definitely see this be a pain in the future but given that I've already set the standard of driving the domain from the util get_domains_from_settings and the readable value with property in the model domain_value I think we'll be able to use the numerical values without too much confusion.

@groovecoder
Copy link
Member

This is probably because the ORIGIN of the request and the SITE_ORIGIN being checked on is not the same, hence it's returning the JSON response as seen in this code snippet. I think it's because you are using ngrok? Try again with the SITE_ORIGIN being the ngrok endpoint and it should work as expected. I did not get this problem on the dev server.

That was it!

Oh, I think this is a problem on the add-on. Since the add-on is appending the domain, as deduced by the JSON response here only having the address field I'm guessing the add-on is appending the domain. I can add a field to the JSON response for the domain, or change the address field to include the entire address (local and domain) but seems like we need an add-on change with this. Good catch.

Hmm ... when I inspect the add-on network request, it gets back a response like this:

{"id": 186, "address": "[email protected]:8000"}

So it looks like we can control the address on the server side, via this block at the end of _index_POST, which calls this relay_from_domain function, which grabs the domain from settings.RELAY_FROM_ADDRESS.

We should be able to update & fix that to pull the domain from the RelayAddress or DomainAddress object itself.

I don't see any immediate problem with storing the domains as static values in the DB with numeric identifiers, other than some mental complexity in understanding how/why it works that way. But I'd like to consider some potential future scenarios (e.g., adding MORE domains) and how this implementation could affect those future scenarios.

emails/views.py Outdated
@@ -123,7 +123,8 @@ def _index_POST(request):
)
return JsonResponse({
'id': relay_address.id,
'address': address_string
'address': address_string,
'domain': relay_addres.domain_value
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to add the domain property to the response that's good, but this doesn't fix the issue, because the address_string is still '%s@%s where the 2nd string substitution is based on the relay_from_domain. So the add-on will need to:

  1. get the localpart from the address value by splitting it at the @ character
  2. append the domain part from the domain value

AND we will need to coordinate the release of the add-on with the additional parsing code with the release of the server code.

A better fix that de-couples the code-bases, but still extends the response in a way the add-on can use later:

diff --git a/emails/models.py b/emails/models.py
index 621589b..fb62ff0 100644
--- a/emails/models.py
+++ b/emails/models.py
@@ -272,6 +272,10 @@ class RelayAddress(models.Model):
     def domain_value(self):
         return DOMAINS.get(self.get_domain_display())
 
+    @property
+    def full_address(self):
+        return '%s@%s' % (self.address, self.domain_value)
+
 
 class DeletedAddress(models.Model):
     address_hash = models.CharField(max_length=64, db_index=True)
diff --git a/emails/views.py b/emails/views.py
index ec587a8..790c529 100644
--- a/emails/views.py
+++ b/emails/views.py
@@ -115,12 +115,10 @@ def _index_POST(request):
             return redirect('profile')
 
     if settings.SITE_ORIGIN not in request.headers.get('Origin', ''):
-        address_string = '%s@%s' % (
-            relay_address.address, relay_from_domain(request)['RELAY_DOMAIN']
-        )
         return JsonResponse({
             'id': relay_address.id,
-            'address': address_string
+            'address': relay_address.full_address,
+            'domain': relay_address.domain_value
         }, status=201)
 
     return redirect('profile')

@groovecoder groovecoder merged commit f417382 into main Aug 24, 2021
@groovecoder groovecoder deleted the create-mozmail-relay-alias-mpp-712 branch August 24, 2021 21:18
@say-yawn say-yawn mentioned this pull request Sep 23, 2021
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants