-
Notifications
You must be signed in to change notification settings - Fork 183
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
Conversation
Set possible options for domain fields and set default domain
If the domain is the not the default domain
Append changes the existing list
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
|
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.
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.
and change default domain based on TEST_MOZMAIL
While testing, found some things ...
|
RELAY_FIREFOX_DOMAIN = config('RELAY_FIREFOX_DOMAIN', 'relay.firefox.com', cast=str) | ||
MOZMAIL_DOMAIN = config('MOZMAIL_DOMAIN', 'mozmail.com', cast=str) |
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 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.
This is probably because the
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
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:
the table will look like
where the human readable value of |
That was it!
Hmm ... when I inspect the add-on network request, it gets back a response like this:
So it looks like we can control the address on the server side, via this block at the end of We should be able to update & fix that to pull the domain from the 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 |
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.
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:
- get the localpart from the
address
value by splitting it at the@
character - 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')
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
TEST_MOZMAIL
environment variable is set toTrue
users Relay alias is created with mozmail.com domain INSTEAD of relay.firefox.comTEST_MOZMAIL
isFalse
users withmozilla.com
email address can use mozmail.comRelated to
MPP-712 and MPP-714
Test
Test that relay address works as is
Test that relay address is generated with the mozmail.com when the
TEST_MOZMAIL
environment variable is set toTrue
,MOZMAIL_DOMAIN
, andRELAY_FIREFOX_DOMAIN
is setTEST_MOZMAIL
toTrue
andMOZMAIL_DOMAIN
, andRELAY_FIREFOX_DOMAIN
is set to the respective mozmail domain and restart the serviceTest that emails are relayed to both alias generated with mozmail.com and existing alias generated with relay.firefox.com
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:
Test email generation on add-on works properly
/accounts/profile/
the alias was generated with the mozmail domainTest that users without the mozilla.com email address can use the Relay as is
TEST_MOZMAIL
,MOZMAIL_DOMAIN
, andRELAY_FIREFOX_DOMAIN
Test that users without the mozilla.com email address can use the Relay as is
TEST_MOZMAIL
,MOZMAIL_DOMAIN
, andRELAY_FIREFOX_DOMAIN