Closed Bug 1173688 Opened 9 years ago Closed 8 years ago

Password manager sync promo appears when signing in/up for Sync from an iframe

Categories

(Toolkit :: Password Manager, defect)

40 Branch
defect
Not set
major
Points:
3

Tracking

()

RESOLVED FIXED
mozilla41
Iteration:
41.3 - Jun 29
Tracking Status
firefox40 --- fixed
firefox41 --- fixed

People

(Reporter: stomlinson, Assigned: MattN)

References

()

Details

Attachments

(4 files)

Ref [1], [2]

The Firefox password manager offers to save passwords for Firefox Accounts/Sync when FxA is embedded in an iframe.

The Firefox password manager ignores FxA when embedded in Firefox Accounts, but not when FxA is used for Sync and embedded in an iframe.

The new firstrun flow for Fx40 will embed FxA into an iframe, it would be ideal to fix this bug before the Fx40 release.

[1] - https://bugzilla.mozilla.org/show_bug.cgi?id=1146904
[2] - https://github.com/mozilla/fxa-content-server/issues/2373
[3] - https://dxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/LoginManagerContent.jsm#659
I saw this in a custom Fennec build, tried to verify elsewhere and didn't see it again -- presumably because I wasn't hitting the iframe flow.  There is a discussion of this particular on dev-fxacct and shane's [3] is the about:accounts specific work-around.
:bmaggs - do you believe this needs to be done for Fx40?
Flags: needinfo?(wmaggs)
I can't reproduce. I know from talking to Chris what it is supposed to look like; Shane, could you send me a screenshot? 

My impression is if it is a quick fix (tiny patch) we should do it now, given the potential to put new Sync users in a signup loop.
Flags: needinfo?(wmaggs) → needinfo?(stomlinson)
Flags: needinfo?(stomlinson)
(In reply to Bill Maggs from comment #3)
> I can't reproduce. I know from talking to Chris what it is supposed to look
> like; Shane, could you send me a screenshot? 
> 
> My impression is if it is a quick fix (tiny patch) we should do it now,
> given the potential to put new Sync users in a signup loop.

The problem only manifests when signing in to Sync from within an iframe, not the normal about:accounts based flow. No iframe based "sign in to Sync" sites exist yet, the first such site (the firstrun flow) is scheduled to be released with Fx 40. 

The attached screenshot was created on my local development environment that was used to simulate the firstrun flow when I developed the Fx patch for https://bugzilla.mozilla.org/show_bug.cgi?id=1146904.
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Iteration: --- → 41.3 - Jun 29
Points: --- → 3
I added an "origin" property to PopupNotification options since it's part of Shorlander's new doorhanger designs anyways and the information is useful for this hack.

See https://www.lucidchart.com/documents/edit/87ab1cc8-e708-49d3-8b91-6e2e6da346fb/1 for an example. I'm passing the whole origin but the front-end can then remove the scheme if it wishes. It's easy to go from a URL/origin to a hostname but not the other way around so that's why I chose passing the origin.
Btw. we use the Target Milestone to indicate the version a fix first lands in, not a future target. The "status-firefoxXY: affected" flags are used for indicating which versions are affected.
Target Milestone: mozilla40 → ---
(In reply to Matthew N. [:MattN] from comment #8)
> Btw. we use the Target Milestone to indicate the version a fix first lands
> in, not a future target. The "status-firefoxXY: affected" flags are used for
> indicating which versions are affected.

Thanks Matthew, I'm still learning Bugzilla etiquette.
I just realized that my fix reverts a toolkit change and replaces it with a desktop Firefox one. If there is a similar promo on Android it will also need to be suppressed. Nick, are you saying this is a promo on Fennec? If so, can we get a separate bug for that?
Summary: Password manager offers to save passwords for Firefox Accounts when used to sign in/up for Sync from an iframe → Password manager sync promo appears when signing in/up for Sync from an iframe
https://reviewboard.mozilla.org/r/11433/#review9803

::: browser/base/content/urlbarBindings.xml:3074
(Diff revision 1)
> +              notification.options.origin == "https://accounts.firefox.com") {

It would be ideal if we can avoid the hard-coded URL and instead check against one of the fxa prefs. Sadly the state of those prefs isn't great - extracting the origin from identity.fxaccounts.remote.signup.uri is probably ok.

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:941
(Diff revision 1)
> +        origin: login.hostname,

it probably wouldn't hurt to add a comment indicating that nsILoginInfo.hostname is actually an origin
Comment on attachment 8622735 [details]
MozReview Request: Bug 1173688 - Don't show the Sync promo for https://accounts.firefox.com password doorhangers. r=markh

https://reviewboard.mozilla.org/r/11435/#review9809

Ship It!
Attachment #8622735 - Flags: review?(markh) → review+
https://hg.mozilla.org/mozilla-central/rev/1edffee3ccb1
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Comment on attachment 8622735 [details]
MozReview Request: Bug 1173688 - Don't show the Sync promo for https://accounts.firefox.com password doorhangers. r=markh

Approval Request Comment
[Feature/regressing bug #]: Some FxA login attempts won't be from about:accounts anymore in 40
[User impact if declined]: The user may end up in a sync setup loop as a sync promo will push people into setting up sync while they're in the middle of it.
[Describe test coverage new/current, TreeHerder]: Manual testing by FxA developers
[Risks and why]: Low risk chance of breaking doorhangers but no automated tests failed.
[String/UUID change made/needed]: None
Attachment #8622735 - Flags: approval-mozilla-aurora?
Comment on attachment 8622735 [details]
MozReview Request: Bug 1173688 - Don't show the Sync promo for https://accounts.firefox.com password doorhangers. r=markh

New feature, we want that polished.
Attachment #8622735 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1175300
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This seems to have regressed, I am again being asked to save my FxA password from about:accounts and the firstrun page.
Note that you shouldn't re-open fixed Fx bugs, especially more than a day or two after landing as we use the bug fields to track the version a fix occurred in. Fixing a bug multiple times in one BMO bug makes it hard to understand the history of fixes. The usual process is to file a new bug blocking the original fix.

(In reply to Shane Tomlinson [:stomlinson] from comment #19)
> This seems to have regressed, I am again being asked to save my FxA password
> from about:accounts and the firstrun page.

Nope, see the bug summary. We decided it's fine to offer to save the password but simply not show the "sync promo" promoting sync while you're logging into sync. You're screenshot doesn't show the footer of the panel where the promo would be so I can't tell if the promo was there.
Status: REOPENED → RESOLVED
Closed: 9 years ago8 years ago
Resolution: --- → FIXED
(In reply to Matthew N. [:MattN] from comment #21)
> Note that you shouldn't re-open fixed Fx bugs, especially more than a day or
> two after landing as we use the bug fields to track the version a fix
> occurred in. Fixing a bug multiple times in one BMO bug makes it hard to
> understand the history of fixes. The usual process is to file a new bug
> blocking the original fix.

Thanks for the etiquette info, I did not know.

> 
> (In reply to Shane Tomlinson [:stomlinson] from comment #19)
> > This seems to have regressed, I am again being asked to save my FxA password
> > from about:accounts and the firstrun page.
> 
> Nope, see the bug summary. We decided it's fine to offer to save the
> password but simply not show the "sync promo" promoting sync while you're
> logging into sync. You're screenshot doesn't show the footer of the panel
> where the promo would be so I can't tell if the promo was there.


ckarlof gave us the full history last night, everything makes sense now. The dropdown did not show
the Sync promo.

Thanks Matt!
You need to log in before you can comment on or make changes to this bug.