Closed Bug 1308038 Opened 5 years ago Closed 5 years ago

Clicking Manage Account unverifies Firefox Account and disables sync

Categories

(Cloud Services :: Server: Firefox Accounts, defect)

defect
Not set
normal

Tracking

(firefox55 fixed)

RESOLVED FIXED
Tracking Status
firefox55 --- fixed

People

(Reporter: stef, Assigned: stomlinson)

References

Details

(Whiteboard: [fxa-waffle-ignore])

Attachments

(1 file, 4 obsolete files)

I wanted to check my fxa details in 50.0b4 but instead of getting something useful like https://accounts.firefox.com/settings my fxa got unverified and sync was disabled after clicking "Manage Account" link in preferences.

After that, there was an e-mail from some random box on the Internet with verification link that sort of restored previous state and gave me an option to open sync setting in browser preferences.

1. Mozilla shouldn't send e-mail messages (important at least) from random unverifiable servers.
2. Clicking Manage Account (or other similar action) shouldn't disable configured services and unverify Firefox Account.
3. Clicking Manage Account should open https://accounts.firefox.com/settings or something similar (possibly after additional verification).
Yikes, not sure what's going on here

> there was an e-mail from some random box on the Internet

Could you please say more about "random box on the Internet"?  Did the email appear to come from somewhere other than accounts.firefox.com?
Flags: needinfo?(splewako)
This is almost certainly something funky happening in FxA serviers, so moving the bug to that component.
Component: Sync → Server: Firefox Accounts
Product: Firefox → Cloud Services
Version: 50 Branch → other
> my fxa got unverified

Did it ask you to login again, or did it take you straight to a page saying "we've sent a verification email to..."?
(In reply to Ryan Kelly [:rfkelly] from comment #1)
> Could you please say more about "random box on the Internet"?  Did the email
> appear to come from somewhere other than accounts.firefox.com?

It didn't exactly come because some random box didn't offer certificate and mail server rejected communication. After some digging it was probably a27-125.smtp-out.us-west-2.amazonses.com [54.240.27.125]

(In reply to Ryan Kelly [:rfkelly] from comment #3)
> Did it ask you to login again, or did it take you straight to a page saying
> "we've sent a verification email to..."?

Honestly I'm not sure - for the first time I think I needed to signin on the page I was directed to and then verification email info appeared and things got bad, second time it was probably straight to the page.
Flags: needinfo?(splewako)
> [the verification email] didn't exactly come because some random box didn't offer certificate 
> and mail server rejected communication. After some digging it was probably
> a27-125.smtp-out.us-west-2.amazonses.com [54.240.27.125]

It's possible this was an Amazon issue, as I believe we have most of the necessary email-related identity/security features in place.  :jrgm does the above sound familiar to you?

Stefan, if you're able to include any diagnostic logging from your mailserver as to why the email was rejected, that would also be helpful.
Flags: needinfo?(jrgm)
(In reply to Ryan Kelly [:rfkelly] from comment #6)
> Stefan, if you're able to include any diagnostic logging from your
> mailserver as to why the email was rejected, that would also be helpful.

> Oct 06 10:46:11 omron5.splewako.com postfix/postscreen[1763]: CONNECT from [54.240.27.132]:56632 to [178.62.218.98]:25
> Oct 06 10:46:17 omron5.splewako.com postfix/postscreen[1763]: PASS NEW [54.240.27.132]:56632
> Oct 06 10:46:18 omron5.splewako.com postfix/smtpd[1767]: connect from a27-132.smtp-out.us-west-2.amazonses.com[54.240.27.132]
> Oct 06 10:46:19 omron5.splewako.com postfix/smtpd[1767]: NOQUEUE: abort: TLS from a27-132.smtp-out.us-west-2.amazonses.com[54.240.27.132]: No client certificate presented
> Oct 06 10:46:19 omron5.splewako.com postfix/smtpd[1767]: disconnect from a27-132.smtp-out.us-west-2.amazonses.com[54.240.27.132]
Am I wrong that even on mailservers allowing connections from random boxes, Firefox Account mail (from "accounts@firefox.com") should be rejected (or land directly in spam at least) by Mozilla very own SPF rules (firefox.com "v=spf1 mx a include:amazonses.com -all", amazonses.com "v=spf1 ip4:199.255.192.0/22 ip4:199.127.232.0/22 ip4:54.240.0.0/18 -all").
I'm no SPF expert, but this looks OK to me at first glance - the connecting IP was "54.240.27.132" which is covered by "ip4:54.240.0.0/18" in the SPF record for amazonses.com.
Right, somehow I assumed it is /22 not /18 so at least it is some box saying it is the designated one.
I'm (slowly) trying to get my head around all the pieces here, so apologies if this is obvious to everyone else.  Googling this specific error message:

> TLS from <...>: No client certificate presented

Suggests that your mail server was expecting SES to present a valid *client* certificate to identify itself.  Posts like the following:

  http://serverfault.com/questions/790121/openssl-no-client-certificate-presented-smtp-postfix

Suggest that this may not be common behaviour, and posts like the following:

  http://stackoverflow.com/questions/23122064/postfix-smtp-relay-client-does-not-offer-tls-client-certificate-to-the-server

Suggest that even if configured, email-sending clients may not provide such certificates by default.  Stefan, do you have any unusually strict config options set on your mailserver, and if so which ones?  That could help us track down exactly what's going on here.
Flags: needinfo?(jrgm) → needinfo?(splewako)
Of course it may be simply that SES does not support TLS client certificates, and that's why this is failing.  But at the very least it'll be good for us to know that explicitly, and maybe we can harass them about it :-)
Yes, this particular server requires connecting server to identify itself - quite strict but shouldn't be that surprising and "email-sending clients" is rather misleading term for smtp between servers.
Flags: needinfo?(splewako)
The next step here is probably for us to ask Amazon about TLS client certificates in SES...:jbuck is this the sort of thing we can escalate through our support contract?
That would be great, especially that this shouldn't be hard on tech level and possibly could become standard for SES.
By the way, I don't think I mentioned it here, but this part:

> 2. Clicking Manage Account (or other similar action) shouldn't disable configured services and unverify Firefox Account.
> 3. Clicking Manage Account should open https://accounts.firefox.com/settings or something similar (possibly after additional verification).

We were able reproduce independently, and are tracking in github at:

  https://github.com/mozilla/fxa-content-server/issues/4252
Whiteboard: [fxa-waffle-ignore]
Depends on: 1323853
Attached patch fxa-handshake.diff (obsolete) — Splinter Review
Here is the beginnings of a patch to do the "FxA Handshake" we discussed in Hawaii. The handshake allows Firefox to be the canonical source of truth when it comes to which user is currently signed into FxA, which has the benefit of allowing users who have cleared their localStorage to gracefully recover.

I have not yet handled private browsing mode, nor have I added any tests. I'm looking for initial feedback to see if this approach is reasonable.

Goes along with https://github.com/mozilla/fxa-content-server/pull/4695
Attachment #8832820 - Flags: feedback?(rfkelly)
Attachment #8832820 - Flags: feedback?(markh)
Comment on attachment 8832820 [details] [diff] [review]
fxa-handshake.diff

Review of attachment 8832820 [details] [diff] [review]:
-----------------------------------------------------------------

Makes perfect send to me!

::: services/fxaccounts/FxAccountsWebChannel.jsm
@@ +33,5 @@
>  const COMMAND_LOGOUT               = "fxaccounts:logout";
>  const COMMAND_DELETE               = "fxaccounts:delete";
>  const COMMAND_SYNC_PREFERENCES     = "fxaccounts:sync_preferences";
>  const COMMAND_CHANGE_PASSWORD      = "fxaccounts:change_password";
> +const COMMAND_HANDSHAKE            = "fxaccounts:handshake";

I think something like "account_status" or similar might be a better name than "handshake"

@@ +173,5 @@
>          this._helpers.changePassword(data).catch(error =>
>            this._sendError(error, message, sendingContext));
>          break;
> +      case COMMAND_HANDSHAKE:
> +        log.debug("handshake received");

I don't think we need this .debug call - the caller of this function appears to already log it?

@@ +174,5 @@
>            this._sendError(error, message, sendingContext));
>          break;
> +      case COMMAND_HANDSHAKE:
> +        log.debug("handshake received");
> +         this._helpers.getSignedInUser().then((userData) => {

getSignedInUser will return null when there's no signed in user, so I think you need to handle that in this block.

@@ +183,5 @@
> +              signedInUser: {
> +                sessionToken: userData.sessionToken,
> +                email: userData.email,
> +                uid: userData.uid,
> +                verified: userData.verified

sending "verified" here seems strange as that's state which is owned by the server.

@@ +190,5 @@
> +          };
> +
> +          log.debug("FxAccountsWebChannel response", response);
> +          this._channel.send(response, sendingContext);
> +        }, (err) => {

instead of an errhandler here, I think we should use a trailing .catch, so errors in the main handler are also caught - and that .catch handler should just call |this._sendError| as some of the other handlers do.

I'd also be happy for this to make _receiveMessage assume a returned promise and have the caller to this._sendError() if the promise fails (whereas currently it just does error handling if the function itself fails) - OTOH, I totally understand if you'd prefer to leave that to someone else in the future :)
Attachment #8832820 - Flags: feedback?(markh) → feedback+
FWIW I like the approach here, seems nice and simple.  I'll try to get Bug 1323853 to a sensible resolution so we can keep moving this forward.

> > +const COMMAND_HANDSHAKE = "fxaccounts:handshake";
> I think something like "account_status" or similar might be a better name than "handshake"

+1; in my head this is something like "probe login status" but that's probably not a very good name in code :-)
Also, I think it's probably worth sending back some basic context info with the response, e.g. what auth-broker to use if one hasn't been specified explicitly.  We've found ourselves wanting to know this recently when doing the connect-a-second-device flow.
> it's probably worth sending back some basic context info with the response

To clarify, by this I mean, tell the page whether you're "fx_desktop_v4" or "fx_ios_v2" or something else, basically pass back the value of context=foo that you *would* have sent into the page if it'd been opened through a link from preferences.
(In reply to Ryan Kelly [:rfkelly] from comment #20)
> FWIW I like the approach here, seems nice and simple.  I'll try to get Bug
> 1323853 to a sensible resolution so we can keep moving this forward.
> 
> > > +const COMMAND_HANDSHAKE = "fxaccounts:handshake";
> > I think something like "account_status" or similar might be a better name than "handshake"
> 
> +1; in my head this is something like "probe login status" but that's
> probably not a very good name in code :-)


I went with "fxaccounts:handshake" because as rfkelly points out in the very next comment, the data being passed *is* going to grow to be more than account/login status. The original idea for the handshake was to also be able to pass "capabilities"/behavior overrides that the browser expects, instead of the current scheme of using query parameters.
(In reply to Shane Tomlinson [:stomlinson] from comment #23)
> I went with "fxaccounts:handshake" because as rfkelly points out in the very
> next comment, the data being passed *is* going to grow to be more than
> account/login status. The original idea for the handshake was to also be
> able to pass "capabilities"/behavior overrides that the browser expects,
> instead of the current scheme of using query parameters.

I still think it's a bad name ;) I could live with login-handshake or similar, but think maybe "fxa-status" is more descriptive and accurate (and login/account status seems reasonably a subset of that)
(In reply to Mark Hammond [:markh] from comment #24)
> I still think it's a bad name ;) I could live with login-handshake or
> similar, but think maybe "fxa-status" is more descriptive and accurate (and
> login/account status seems reasonably a subset of that)

+1 - `fxa-status` sounds like a good, descriptive name. I'll update to that when I get the chance. Thanks :markh and :rfkelly!
Attachment #8832820 - Flags: feedback?(rfkelly) → feedback+
Just to confirm: per the outcome of Bug 1323853, we need to avoid leaking login state to private-browsing windows.  In practice I expect this will mean that we still respond to the `fxa-status` probe, but if it comes from a private-browsing window, we act as though no-one is logged in.

Detecting that the message came from a private-browsing window is left as an exercise to the reader...
(In reply to Ryan Kelly [:rfkelly] from comment #26)
> Just to confirm: per the outcome of Bug 1323853, we need to avoid leaking
> login state to private-browsing windows.  In practice I expect this will
> mean that we still respond to the `fxa-status` probe, but if it comes from a
> private-browsing window, we act as though no-one is logged in.
> 
> Detecting that the message came from a private-browsing window is left as an
> exercise to the reader...


The resolution in Bug 1323853 is for FxA to only request "fxa_status" IFF service=sync. That
allows for the "Manage Account" button to Just Work, even in PB mode, but does *not* leak any 
information if the user is signing into an OAuth relier or casually browses to https://accounts.firefox.com/.

With that info. I'm updating my patch and asking for a review. The corresponding content-server PR is
at https://github.com/mozilla/fxa-content-server/pull/4695. Even if r+'d, this patch should not land until
*after* the content server patch, or else we are going to bust Nightly users.
Attachment #8832820 - Attachment is obsolete: true
Attachment #8843356 - Flags: review?(rfkelly)
Attachment #8843356 - Flags: review?(markh)
Comment on attachment 8843356 [details] [diff] [review]
Add an fxaccounts:fxa_status WebChannel message

Review of attachment 8843356 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/fxaccounts/tests/xpcshell/test_web_channel.js
@@ +257,5 @@
> +      }
> +    }
> +  });
> +
> +  channel._channel = {

:markh - I wasn't sure of the best way to go about mocking out the channel, nobody has done it before. I could pass in a channel mock, and if it's passed in, avoid calling _setupChannel.
Comment on attachment 8843356 [details] [diff] [review]
Add an fxaccounts:fxa_status WebChannel message

Review of attachment 8843356 [details] [diff] [review]:
-----------------------------------------------------------------

The code looks good from my PoV, although I'll lean on :markh for checking the details.

> for FxA to only request "fxa_status" IFF service=sync

I slightly worry that some future refactor of content-server will forget about this restriction, or accidentally break it.  I wonder if there's any way we can have the browser help enforce this?  For example, we could have web content send its value of "service" as part of the handshake, and have the browser only respond when it is in fact equal to "sync".

We'll also need to test some of the edge-cases here, such as the following:

* Open a private browsing window
* Open "manage account" in said window, and get logged in because of the handshake
* In that same tab, navigate away from accounts.firefox.com and over to https://addons.mozilla.org
* Try to login to addons.mozilla.org

Does that use the data we got from the handshake to process the login, potentially breaking out contract with private browsing mode?
Attachment #8843356 - Flags: review?(rfkelly) → review+
Comment on attachment 8843356 [details] [diff] [review]
Add an fxaccounts:fxa_status WebChannel message

Review of attachment 8843356 [details] [diff] [review]:
-----------------------------------------------------------------

I don't have any problems with this patch, and the test looks fine (and it would also be fine if you did what you suggested ;) I would r+ it, although Ryan's comments imply some further changes are probably necessary.

Comment 22 from Ryan implies some additional context was going to be sent back in the handshake - is that no longer necessary?

Ryan's most recent idea also sounds worthwhile - you send in the message the value of "service", and we only respond if it is "sync".

The other comment from Ryan is worth considering too - I didn't realize that the result of this message will end up back in localStorage, so I think the comment about about:addons is going to be correct - that page will indeed find the local storage in place (and I don't think it will be necessary to use the same tab - IIUC a new tab in an existing PB window would also reproduce it). Ditto containers (ie, bug 1340948) where although we seem to agree FxA itself should look logged into the browser itself, oauth is still a little unclear.
Attachment #8843356 - Flags: review?(markh)
Assignee: nobody → stomlinson
Leaving a note that I will get back to this after returning from PTO.

I agree with :rfk's suggestion to send the `service` query parameter to the browser with the `fxaccounts:fxa_status` request. The additional bit of information will allow the browser to do the right thing w.r.t. PB mode.

:markh - I had a stab at trying to determine whether the tab in which the `fxaccounts:fxa_status` is in PB mode, but made no (forward) progress. Any hints here? :rfkelly suggested I query origin attributes. All the search hints reference docShell, which I haven't successfully gotten a reference to. Another option I was looking at is `usePrivateBrowsing` [2], but again, almost all of the results reference docShell.


[1] - https://dxr.mozilla.org/mozilla-central/search?q=getOriginAttributes
[2] - https://dxr.mozilla.org/mozilla-central/search?q=usePrivateBrowsing
Flags: needinfo?(markh)
Status: NEW → ASSIGNED
The |sendingContext| param has a |browser| attribute, which I think will have a .docShell attribute.
Flags: needinfo?(markh)
This goes along with https://github.com/mozilla/fxa-content-server/pull/4695 and takes a slightly different approach.

When sending `fxaccounts:fxa_status`, we send along a `service` to decide whether to send user data in PB mode. If in non-PB mode, always send the user data, returning `null` if no user is signed in. In PB mode, only send user data if `service=sync`.

On the content server side, we send the `fxaccounts:fxa_status` request if the user is on Firefox Desktop >= 55. We can update the gating logic to ensure the message is only sent to users with supporting browsers once this patch lands.

If the browser supports fxaccounts:fxa_status and the user is signing into Sync, user data is *not written to localStorage*, instead the browser's view of the world is always used. If the user is signing into an OAuth relier and user data is returned from the browser and no user data exists in localStorage, the user data will be used to seed the `signin` screen.
Attachment #8843356 - Attachment is obsolete: true
Attachment #8867784 - Flags: review?(rfkelly)
Attachment #8867784 - Flags: review?(markh)
Comment on attachment 8867784 [details] [diff] [review]
Add an fxaccounts:fxa_status WebChannel message

Review of attachment 8867784 [details] [diff] [review]:
-----------------------------------------------------------------

This look solid to me, thanks Shane!  I left a couple of nits but I don't see any blockers.

Setting f+ rather than r+ because I don't really consider myself qualified to review-for-landing in this code.

::: services/fxaccounts/FxAccountsWebChannel.jsm
@@ +33,5 @@
>  const COMMAND_LOGOUT               = "fxaccounts:logout";
>  const COMMAND_DELETE               = "fxaccounts:delete";
>  const COMMAND_SYNC_PREFERENCES     = "fxaccounts:sync_preferences";
>  const COMMAND_CHANGE_PASSWORD      = "fxaccounts:change_password";
> +const COMMAND_FXA_STATUS           = "fxaccounts:fxa_status";

The nitpicker in me wonders if the "fxa_" prefix on the name here is redundant, but I know we discussed this name already so I'm happy to just leave it be :-)

@@ +174,5 @@
>          this._helpers.changePassword(data).catch(error =>
>            this._sendError(error, message, sendingContext));
>          break;
> +      case COMMAND_FXA_STATUS:
> +        log.debug("fxa_status received");

Stylistically, I wonder if this code should be moved into this._helpers like its peers above, rather than done inline in the `switch` statement.

@@ +177,5 @@
> +      case COMMAND_FXA_STATUS:
> +        log.debug("fxa_status received");
> +
> +        function isPrivateBrowsingMode () {
> +          const isPrivateBrowsing = !! sendingContext.browser.docShell.usePrivateBrowsing;

Is there any chance that one of the properties in this chain does not exist, causing an "undefined has no property foo" error?

@@ +205,5 @@
> +        return new Promise((resolve, reject) => {
> +          if (shouldGetSignedInUser(data)) {
> +            resolve(this._helpers.getSignedInUser());
> +          }
> +          resolve(null)

IIUC this will call `resolve` twice when `shouldGetSignedInUser` returns true.
Attachment #8867784 - Flags: review?(rfkelly) → feedback+
Duplicate of this bug: 1362190
Comment on attachment 8867784 [details] [diff] [review]
Add an fxaccounts:fxa_status WebChannel message

Review of attachment 8867784 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/fxaccounts/FxAccountsWebChannel.jsm
@@ +175,5 @@
>            this._sendError(error, message, sendingContext));
>          break;
> +      case COMMAND_FXA_STATUS:
> +        log.debug("fxa_status received");
> +

Yeah, it should be in helpers. You could also consider making the function in helpers an "async" function (which should make the flow easier to grok), and here just call it as if it is a promise function (ie, this._helpers.whatever().catch(...))

@@ +198,5 @@
> +          // Sync is viewed as an integral part of the browser, interacting
> +          // with FxA as part of a Sync flow should work all the time. If
> +          // Sync is broken in PB mode, users will think Firefox is broken.
> +          log.debug('service', data.service);
> +          return ! isPrivateBrowsingMode() || data.service === "sync";

nit: remove the space after "!" (I expect eslint will complain about that, but I'm not sure)
Attachment #8867784 - Flags: review?(markh) → feedback+
The major differences to the last patch:

* Extract the bulk of the fxaStatus code into the "helpers" object.
* Make "fxaStatus" and async function, which vastly simplifies the flow control logic. Thanks for the tip :markh!
Attachment #8867784 - Attachment is obsolete: true
Attachment #8869366 - Flags: review?(rfkelly)
Attachment #8869366 - Flags: review?(markh)
Comment on attachment 8869366 [details] [diff] [review]
Add an fxaccounts:fxa_status WebChannel message

Review of attachment 8869366 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/fxaccounts/FxAccountsWebChannel.jsm
@@ +330,5 @@
> +    const isPrivateBrowsing = !!(sendingContext &&
> +                                 sendingContext.browser &&
> +                                 sendingContext.browser.docShell &&
> +                                 sendingContext.browser.docShell.usePrivateBrowsing);
> +    log.debug("is private browsing", isPrivateBrowsing);

I should have mentioned this before, but given we are testing with mocks, I've a vague concern that should some implementation detail of webchannels, or how PB is treated by docshells change, this will silently break.

So could you please arrange for this to log.error() if the chain above doesn't work correctly? I'm just thinking something like:

if (!sendingContext ||
    !sendingContext.browser ||
    !sendingContext.browser.docShell ||
    sendingContext.browser.docShell.usePrivateBrowsing === undefined) {
  log.error(blah);
  return true;

(TBH I'm not too clear whether that should return true of false - true seems a safer option at the cost of breaking non-private windows should we ever hit this state)

I don't think I need to look at this again with that change, but obviously feel free to ask me to look anyway ;)
Attachment #8869366 - Flags: review?(markh) → review+
Comment on attachment 8869366 [details] [diff] [review]
Add an fxaccounts:fxa_status WebChannel message

Review of attachment 8869366 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great to me; as before the f+ rather than r+ here is only because I don't consider myself qualified to give the latter :-)

::: services/fxaccounts/FxAccountsWebChannel.jsm
@@ +351,5 @@
> +    // The difference in behaviour is to try to match user
> +    // expectations as to what is and what isn't part of the browser.
> +    // Sync is viewed as an integral part of the browser, interacting
> +    // with FxA as part of a Sync flow should work all the time. If
> +    // Sync is broken in PB mode, users will think Firefox is broken.

Not sure if this sort of thing is done much in the Firefox codebase, but it may be worth linking explicitly to https://bugzilla.mozilla.org/show_bug.cgi?id=1323853 where we hammered out the details of the behaviour here.  (esp. since IIUC that's a different bug to the one that will get mentioned in the commit message here)
Attachment #8869366 - Flags: review?(rfkelly) → feedback+
(In reply to Ryan Kelly [:rfkelly] from comment #40)
> Not sure if this sort of thing is done much in the Firefox codebase, but it
> may be worth linking explicitly to
> https://bugzilla.mozilla.org/show_bug.cgi?id=1323853

:thumbsup:
The difference between this patch and the last is when checking isPrivateBrowsingMode, check for the existence of sendingContext, browser, docShell and usePrivateBrowsing.
Attachment #8869366 - Attachment is obsolete: true
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/67f2a626ede4
Add handshake capability to share state with FxA. r=markh, r=rkelly
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/67f2a626ede4
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Depends on: 1378766
You need to log in before you can comment on or make changes to this bug.