Closed Bug 1239701 Opened 7 years ago Closed 7 years ago

Datastore should allow us to define stores accessible just by certified apps.

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX
2.6 S6 - 1/29

People

(Reporter: arcturus, Unassigned)

Details

Attachments

(3 files, 1 obsolete file)

In bug 1181329 we allowed homescreen apps to access datastores.

This means we potentially allow homescreen apps to delete your in case of emergency contacts or steal your history.

We should be able to define datastores that could be accessed just by certified apps. And fix the gaia DS that will relay on DS being certified.
Assignee: nobody → ferjmoreno
Really, we just want access-control for datastores, like we have it for GPS and other permissions. If we could, in system, get a signal 'App <manifestURL> is requesting access to <datastore name> owned by <manifestURL>', we could present that to the user (and indeed, block off access to things like the password store by default).

I don't think we should complicate how datastores are declared, I think we should give the user more control and allow datastores to be used by all privileged apps (and not just homescreens, which, really, is a completely arbitrary decision, born of necessity rather than desire).
There have been several attempts to open this API to privileged apps. The last one was tracked at [1].

TBH, at this point, given where we are with FxOS, I don't think DataStore will ever be exposed to privileged apps. IMHO we should instead solve the DataStore use case (share data across origins) with a different tool that can be properly exposed to the web (foreign fetch?, shared IDB?).

In any case, this bug is about fixing the privacy issue introduced by bug 1181329. As Francisco mentioned in the description, we shouldn't be exposing information such as emergency contacts or history to privileged homescreens.

Ehsan, Andrea, what do you think?

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=942641
Flags: needinfo?(ehsan)
Flags: needinfo?(amarchesini)
Blocks: fxos-sync
Target Milestone: --- → 2.6 S6 - 1/29
I think making DataStores certified-only is fine.  It really isn't worth anyone's time doing something more sophisticated.  DataStore should be considered as dead technology.  :-)
Flags: needinfo?(ehsan)
(In reply to :Ehsan Akhgari from comment #3)
> I think making DataStores certified-only is fine.  It really isn't worth
> anyone's time doing something more sophisticated.  DataStore should be
> considered as dead technology.  :-)

Unless someone does the work to port bookmarks and history to a different storage mechanism that allows sharing, this isn't an option. 3rd-party Home screens need to be able to display pinned sites and pages.
Attached patch Untested patch (obsolete) — Splinter Review
(In reply to Chris Lord [:cwiiis] from comment #4)
> (In reply to :Ehsan Akhgari from comment #3)
> > I think making DataStores certified-only is fine.  It really isn't worth
> > anyone's time doing something more sophisticated.  DataStore should be
> > considered as dead technology.  :-)
> 
> Unless someone does the work to port bookmarks and history to a different
> storage mechanism that allows sharing, this isn't an option. 3rd-party Home
> screens need to be able to display pinned sites and pages.

Sorry, that should have read "Making *some* DataStores certified-only".
Attached patch v1Splinter Review
Attachment #8708469 - Attachment is obsolete: true
Attachment #8708974 - Flags: review?(amarchesini)
Attached patch TestsSplinter Review
This also adds a test for bug 1181329
Comment on attachment 8708974 [details] [diff] [review]
v1

Fabrice, could you take a look at the Webapps.jsm part, please? Thanks!
Attachment #8708974 - Flags: review?(fabrice)
Attachment #8708975 - Flags: review?(amarchesini)
Attachment #8708977 - Flags: review?(fabrice)
Status: NEW → ASSIGNED
Comment on attachment 8708974 [details] [diff] [review]
v1

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

Just revert all the changes in nsIDataStore.idl and DataStoreImpl.js. Except if we want to expose/use _certifiedOnly

::: dom/datastore/DataStoreImpl.js
@@ +87,5 @@
>      this._window = aWindow;
>      this._name = aName;
>      this._owner = aOwner;
>      this._readOnly = aReadOnly;
> +    this._certifiedOnly = aCertifiedOnly;

This value is not exposed/used in anyway here. Why do we care to have it?

::: dom/datastore/DataStoreService.cpp
@@ +83,3 @@
>                  bool aEnabled)
>    {
> +    Init(aName, aOriginURL, aManifestURL, aReadOnly, aCertifiedOnly, aEnabled);

80chars.

@@ +1054,5 @@
>        continue;
>      }
>  
> +    uint16_t status;
> +    if (NS_FAILED(aPrincipal->GetAppStatus(&status))) {

We don't need to retrieve the status in case mCertifiedOnly is false.
Do:

if (appInfo->mCertifiedOnly) {
  GetStatus();
  ErrorHandling...
  if (status != ..
}

::: dom/datastore/nsIDataStore.idl
@@ +15,5 @@
>    void init(in nsIDOMWindow window,
>              in DOMString name,
>              in DOMString manifestURL,
> +            in boolean readOnly,
> +            in boolean certifiedOnly);

This seems not needed.
Attachment #8708974 - Flags: review?(amarchesini) → review+
Attachment #8708975 - Flags: review?(amarchesini) → review+
No longer blocks: 877648
Attachment #8708977 - Flags: review?(fabrice) → review+
Attachment #8708974 - Flags: review?(fabrice) → review+
No longer blocks: fxos-sync
Assignee: ferjmoreno → nobody
Status: ASSIGNED → NEW
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.