Closed Bug 1016783 Opened 10 years ago Closed 8 years ago

Get rid of prefs hackery introduced in bug 976109 for MLP

Categories

(Hello (Loop) :: Client, defect, P3)

defect
Points:
2

Tracking

(Not tracked)

RESOLVED INCOMPLETE
backlog backlog-

People

(Reporter: dmosedale, Unassigned)

References

Details

(Whiteboard: [investigation])

This was a condition of getting r+ on the patches in bug 976109.  For the anonymous case, we'll probably want to look into using storage.  For the non-anonymous case, will we be using the Firefox Accounts session token anyway?
Priority: -- → P1
Target Milestone: --- → mozilla33
we need research.  crossing priveledged/non-priviledged boundaries.  don't have a plan of attack yet and need to determine that to break down.
Whiteboard: [p=2, investigation]
Target Milestone: mozilla33 → mozilla34
Could we just store these in password manager?
(In reply to Mark Banner (:standard8) from comment #2)
> Could we just store these in password manager?

That makes sense to me!
Whiteboard: [p=2, investigation] → [p=2, investigation][loop-uplift]
Target Milestone: mozilla34 → mozilla35
Hi Adam,

talked to maire and we were looking to postpone this to Fx36 clean up, unless you had strong objections for needing it before Fx36.
Flags: needinfo?(adam)
(In reply to sescalante from comment #4)
> talked to maire and we were looking to postpone this to Fx36 clean up,
> unless you had strong objections for needing it before Fx36.

I think that's perfectly fine, as long as we haven't identified any concrete issues (other than simple hygiene) that arise from our current setup.
Flags: needinfo?(adam)
(In reply to Mark Hammond [:markh] from comment #3)
> (In reply to Mark Banner (:standard8) from comment #2)
> > Could we just store these in password manager?
> 
> That makes sense to me!

I there a way to do that without having them appear in the user-visible password manager UI? Can we also make sure they aren't copied among browsers via Sync? I think that kind of replication would have properties we're not equipped to handle.

It seems to me that the cleanest way to handle this would be to fix Bug 1033587, and then use localstorage for storing hawk tokens.
backlog: --- → Fx36+
backlog: Fx36+ → backlog
Whiteboard: [p=2, investigation][loop-uplift] → [p=2, investigation]
Target Milestone: mozilla35 → ---
backlog: backlog+ → backlog-
type of things that may get messed up as we integrated with FxA, Hello, Sync - but not yet.
Status: NEW → RESOLVED
Points: --- → 2
Closed: 9 years ago
Resolution: --- → WONTFIX
Whiteboard: [p=2, investigation] → [investigation]
(In reply to sescalante from comment #7)
> type of things that may get messed up as we integrated with FxA, Hello, Sync
> - but not yet.

Shell -- was this closed by mistake? The way we left things with :MarkH, by my recollection, was that we promised to fix this when we had cycles to do so (cf. Bug 976109 comment 88). I don't believe we can close this as "wontfix" without first conferring with him to make sure he's okay with leaving things as-is.
Flags: needinfo?(sescalante)
(In reply to Adam Roach [:abr] from comment #8)
> (In reply to sescalante from comment #7)
> > type of things that may get messed up as we integrated with FxA, Hello, Sync
> > - but not yet.
> 
> Shell -- was this closed by mistake? The way we left things with :MarkH, by
> my recollection, was that we promised to fix this when we had cycles to do
> so (cf. Bug 976109 comment 88). I don't believe we can close this as
> "wontfix" without first conferring with him to make sure he's okay with
> leaving things as-is.

I was at the meeting, so I think I can explain this.

I think most of the issue is there's no clear statement anywhere as to *why* the current storage of hawk session token and FxA tokens in prefs is bad, nor any clear way forward as to where they should be stored (again with why).

That makes it very hard to reason that fixing this will provide us with some additional value that we haven't got today, especially as we've been released for a few cycles.

If we can get a clearer explanation here, then we can reconsider it.
Flags: needinfo?(sescalante)
Flags: needinfo?(mhammond)
Flags: needinfo?(adam)
My overarching concern here is that we square things away with MarkH, since we had left him with the impression that this approach was temporary. If he signs off on it, I'm okay.

OTOH, we're going to need to store substantially more data locally as part of the context design, so it would make *sense* to move the tokens into the same datastore as the context info (and I *do* start to get queasy at the prospect of storing all the context-related data in prefs).
Flags: needinfo?(adam)
As a quoted comment in https://github.com/adamroach/gecko-dev/pull/26 says:
> Do we really need to set this pref? I can't see where it is read, and we are already
> copping grief for storing sync hawk credentials in a way that isn't protected by the
> master-password, and this pref would have the same concern.

And Bug 976109 comment 86
> This saves hawk tokens into a pref for later use. Note that it currently gets a 
> new token on every startup. Re-using existing tokens will happen in a future bug.

Also, bug 1139743 is adding a way to cache OAuth tokens for reading-list and storing/managing then with the FxA user storage, and finally:

(In reply to Adam Roach [:abr] from comment #10)
> OTOH, we're going to need to store substantially more data locally as part
> of the context design, so it would make *sense* to move the tokens into the
> same datastore as the context info (and I *do* start to get queasy at the
> prospect of storing all the context-related data in prefs).

All make me think we should still implement this.  However, my concern is more from the FxA side of the world than from the browser side of the world, so I'll defer the NI to ckarlof and rfkelly as the most recent and current FxA owners.
Flags: needinfo?(rfkelly)
Flags: needinfo?(mhammond)
Flags: needinfo?(ckarlof)
Ok, reopening whilst we keep the discussion going.

Storing the FxA tokens in a central cache as per that bug does sound a lot nicer. Feels like it'd fix a couple of our bugs as well.

The separate hawk tokens that we store for guest sessions is probably still up for debate, like Adam says, we might want to consider this in some other store as we do the context work - which I'm looking into at the moment.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
I would recommend that Hello continue to manage the storage of its own tokens in whatever way the Hello team determines to be best. Bug 1139743 is slamming in some token storage APIs in 38 for RL (which is essentially an extension of Sync), but I want these to bake a little longer before we start broadening their use. These token management APIs were designed to be used more broadly, but they are a bit coupled to the notion that a user is logged into Sync, which we're not requiring for a user to use Loop. 

Storing tokens in the prefs is not the worse thing, but I'd prefer some disk storage in the user's profile.
Flags: needinfo?(ckarlof)
I don't have anything to add on top of :ckarlof's comment, clearning ni? for myself
Flags: needinfo?(rfkelly)
Rank: 38
Flags: firefox-backlog+
Priority: P1 → P3
Support for Hello/Loop has been discontinued.

https://support.mozilla.org/kb/hello-status

Hence closing the old bugs. Thank you for your support.
Status: REOPENED → RESOLVED
Closed: 9 years ago8 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.