Created attachment 321333 [details] [review]
location: Better name for a field

_proxy -> _managerProxy.

Created attachment 321334 [details] [review]
location: Add dialog to ask for location data access

Add a dialog that is used in a following patch, to ask user if they want
a requesting application to gain access to their location.

Created attachment 321335 [details] [review]
location: Add accuracyLevelToString()

Add a utility function to convert accuracy level enum to strings.

Created attachment 321336 [details] [review]
location: Add AppAuthorizer class

This class will be responsible for authorizing applications that try to
access location information. Since this is mainly targetted for xdg-app
applications, we make use of xdg-app's D-Bus API to store
per-application authorization.

Created attachment 321337 [details] [review]
location: Ask user to authorize applications

While we could have implemented this already a while ago, this would
have been a false security mechanism completely since we had no way of
reliably identifying applications. Now with xdg-app, we can at least
reliably identify bundles applications, let's give users a choice of
which applications in particular they are OK with giving location data
too.

While we still can't reliably identify system (non-xdg-app) applications,
it seems extremely unlikely we'll ever be able to do that (at least not
in the near future) so we'll have to trust them to not lie about their
IDs.

Created attachment 321419 [details] [review]
location: Add AppAuthorizer class

This class will be responsible for authorizing applications that try to
access location information. Since this is mainly targetted for xdg-app
applications, we make use of xdg-app's D-Bus API to store
per-application authorization.

Created attachment 321420 [details] [review]
location: Ask user to authorize applications

While we could have implemented this already a while ago, this would
have been a false security mechanism completely since we had no way of
reliably identifying applications. Now with xdg-app, we can at least
reliably identify bundles applications, let's give users a choice of
which applications in particular they are OK with giving location data
too.

While we still can't reliably identify system (non-xdg-app) applications,
it seems extremely unlikely we'll ever be able to do that (at least not
in the near future) so we'll have to trust them to not lie about their
IDs.

Review of attachment 321333 [details] [review]:

Sure

Review of attachment 321335 [details] [review]:

It's not quite clear why this is a separate patch, better to squash it

Review of attachment 321419 [details] [review]:

::: js/ui/status/location.js
@@ +267,3 @@
+    },
+
+    Authorize: function(onAuthDone) {

Style nit: method names should use camelCase (except when proxying a remote method)

@@ +270,3 @@
+        this._onAuthDone = onAuthDone;
+
+        this._authorizeFromPermissionStore();

The split here is a bit odd - why not move _authorizeFromPermissionStore() inline and split out the LookupRemote() callback instead?

@@ +288,3 @@
+                    this._accuracyLevel = this.reqAccuracyLevel;
+                    this._permStoreProxy = null;
+                    this._completeAuth();

This side-steps the check for a valid .desktop ID that's currently in place, is that on purpose?

@@ +303,3 @@
+
+            let data;
+            [this._permissions, data] = result;

data is unused, so you could simply use:
[this._permissions] = result;

@@ +306,3 @@
+            let permission = this._permissions[this.desktopId];
+            if (permission == null || permission.length < 2) {
+                this._userAuthorizeApp();

Could get rid of the special casing:

let [levelStr, timesStr] = permission || [];
this._accuracyLevel = GeoclueAccuracyLevel[levelStr] ||
                      GeoclueAccuracyLevel.NONE;
this._timesAllowed = Number(timesStr) || 0;

@@ +325,3 @@
+    _userAuthorizeApp: function() {
+        var appSystem = Shell.AppSystem.get_default();
+        var app = appSystem.lookup_app(this.desktopId + ".desktop");

Unless we don't want this check for some reason when xdg-app isn't available, this should be the first thing in authorize() - no need to call a remote method if we throw away the result anyway.
It may also make sense to preserve at least parts of the comment removed with the corresponding code in the next commit here?
Lastly a style nit: use 'let' to declare local variables

@@ +344,3 @@
+            this._dialog = new GeolocationDialog(name, reason);
+        else
+            this._dialog.update(name, reason);

Does this ever happen? AppAuthorizer isn't reused, so the dialog isn't either, no?

@@ +349,3 @@
+                                            Lang.bind(this, function() {
+            this._dialog.disconnect(closedId);
+            if (this._dialog.allowed)

Getting a result back via a public property after the dialog is closed is a bit ugly - better make the dialog emit a signal and pass the result as parameter (something like "this.emit('response', result);")

@@ +356,3 @@
+        }));
+
+        this._dialog.open(global.get_current_time ());

That's already the default, so could just use open(). Otherwise no space after method name

Review of attachment 321420 [details] [review]:

Some bits in the commit message read funny:

"this would have been a false security mechanism completely" - you probably mean "this would have been a completely false security mechanism"?

The second sentence - maybe:
"Now with xdg-app that we can at least reliably identify bundled applications, allow users to choose which applications they are OK with giving location data to"

::: js/ui/status/location.js
@@ +136,3 @@
+            let ret = (accuracyLevel != GeoclueAccuracyLevel.NONE);
+            invocation.return_value(GLib.Variant.new('(bu)',
+                                    [ret, accuracyLevel]));

Misleading indentation, the array is the 2nd parameter of variant_new(), not return_value()

Created attachment 321450 [details] [review]
location: Bring GeolocationDialog closer to mockups

(In reply to Zeeshan Ali (Khattak) from comment #0)
> The dialog shown is a crude form of:
> 
> https://dl.dropboxusercontent.com/u/5031519/location-access-dialog.png
> 
> Something I'm hoping Florian can help me transform to look like the mockup.

Here you are, plus the "review" of attachment 321334 [details] [review] - feel free to squash.

Created attachment 321451 [details] [review]
location: Style geolocation dialog

... and the gnome-shell-sass submodule change used to generate the CSS in the last patch.

(In reply to Florian Müllner from comment #12)
> Created attachment 321450 [details] [review] [review]
> location: Bring GeolocationDialog closer to mockups
> 
> (In reply to Zeeshan Ali (Khattak) from comment #0)
> > The dialog shown is a crude form of:
> > 
> > https://dl.dropboxusercontent.com/u/5031519/location-access-dialog.png
> > 
> > Something I'm hoping Florian can help me transform to look like the mockup.
> 
> Here you are, plus the "review" of attachment 321334 [details] [review] [review] -
> feel free to squash.

Cool! thanks so much. I'll fix..

Created attachment 321476 [details] [review]
location: Add dialog to ask for location data access

Add a dialog that is used in a following patch, to ask user if they want
a requesting application to gain access to their location.

Created attachment 321477 [details] [review]
location: Add AppAuthorizer class

This class will be responsible for authorizing applications that try to
access location information. Since this is mainly targetted for xdg-app
applications, we make use of xdg-app's D-Bus API to store
per-application authorization.

Created attachment 321478 [details] [review]
location: Ask user to authorize applications

While we could have implemented this already a while ago, this would
have been a completely false security mechanism since we had no way of
reliably identifying applications. Since now with xdg-app, we can at least
reliably identify bundled applications, let's give users a choice of
which applications in particular they are OK with giving location data
too.

While we still can't reliably identify system (non-xdg-app) applications,
it seems extremely unlikely we'll ever be able to do that (at least not
in the near future) so we'll have to trust them to not lie about their
IDs.

Next release of geoclue will take the ID of bundled application directly
from corresponding xdg-app metadata so bundled applications can't simply
lie about their IDs.

Created attachment 321480 [details] [review]
location: Add dialog to ask for location data access

Add a dialog that is used in a following patch, to ask user if they want
a requesting application to gain access to their location.

Co-author: Florian Müllner <fmuellner@gnome.org>

Created attachment 321481 [details] [review]
location: Add AppAuthorizer class

This class will be responsible for authorizing applications that try to
access location information. Since this is mainly targetted for xdg-app
applications, we make use of xdg-app's D-Bus API to store
per-application authorization.

Created attachment 321482 [details] [review]
location: Ask user to authorize applications

While we could have implemented this already a while ago, this would
have been a completely false security mechanism since we had no way of
reliably identifying applications. Since now with xdg-app, we can at least
reliably identify bundled applications, let's give users a choice of
which applications in particular they are OK with giving location data
too.

While we still can't reliably identify system (non-xdg-app) applications,
it seems extremely unlikely we'll ever be able to do that (at least not
in the near future) so we'll have to trust them to not lie about their
IDs.

Next release of geoclue will take the ID of bundled application directly
from corresponding xdg-app metadata so bundled applications can't simply
lie about their IDs.

Comment on attachment 321333 [details] [review]
location: Better name for a field

Attachment 321333 [details] pushed as ccfd5e3 - location: Better name for a field

Review of attachment 321480 [details] [review]:

Looks mostly good to me.

This one is a bit tricky to land btw - before pushing, the submodule update for the style changes should be squashed in.

::: js/ui/status/location.js
@@ +229,3 @@
+    _init: function(name, reason, reqAccuracyLevel) {
+        this.parent({ styleClass: 'geolocation-dialog',
+                      destroyOnClose: false });

So I'm pretty sure that this is currently leaking; you should either remove that property and always create a new dialog, or make sure to manually destroy the dialog when the corresponding authorizer is disposed, or make the dialog a (file-)global instead of an AppAuthorizer property.

As mentioned in the previous review, I don't see how the dialog is actually reused currently, so the 2nd option looks like a lot of work for nothing - either the 1st or 3rd option should be fine, no particular preference on my part ...

Review of attachment 321481 [details] [review]:

::: js/ui/status/location.js
@@ +36,3 @@
+    }
+
+    return "NONE";

Style nit: Our convention is to use single quotes for non-translatable strings

@@ +309,3 @@
+        let permission = this._permissions[this.desktopId];
+
+        let [levelStr, timeStr] = permission || ['NONE', '0'];

Careful - the fallback takes care of permission being null/undefined ...

@@ +312,3 @@
+        this._accuracyLevel = GeoclueAccuracyLevel[levelStr] ||
+                              GeoclueAccuracyLevel.NONE;
+        this._timesAllowed = Number(timeStr);

... but not of it being an empty array or containing just the accuracy level. _timesAllowed will be NaN in those cases, which is neither smaller nor greater nor equal than 3 ...

@@ +315,3 @@
+
+        if (this._timesAllowed < 3)
+            this._userAuthorizeApp();

... so you never ask the user here

@@ +336,3 @@
+                                              Lang.bind(this,
+                                                        function(dialog, level) {
+            this._dialog.disconnect(responseId);

The indentation here is a bit wild - this is clearer IMHO:

let responseId = this._dialog.connect('response', Lang.bind(this,       
    function(dialog, level) {                                           
        this._dialog.disconnect(responseId);
        ...
    }));

Review of attachment 321482 [details] [review]:

In the commit message:
"giving location data too" => "giving location data to"

Otherwise LGTM

Review of attachment 321481 [details] [review]:

::: js/ui/status/location.js
@@ +309,3 @@
+        let permission = this._permissions[this.desktopId];
+
+        let [levelStr, timeStr] = permission || ['NONE', '0'];

I'm not sure I understood. Are you just complementing the code for being cautious?

@@ +312,3 @@
+        this._accuracyLevel = GeoclueAccuracyLevel[levelStr] ||
+                              GeoclueAccuracyLevel.NONE;
+        this._timesAllowed = Number(timeStr);

Why would it be empty of only contain accuracy level if we always save non-empty array and ensure timestamp is present?

(In reply to Zeeshan Ali (Khattak) from comment #25)

> Why would it be empty of only contain accuracy level if we always save
> non-empty array and ensure timestamp is present?

The previous patch checked for:

  (permission == null || permission.length < 2)

The fallback replaces the first condition, but not the second one - so unless that condition wasn't actually needed, you end up with the aforementioned problem.

(In reply to Florian Müllner from comment #26)
> (In reply to Zeeshan Ali (Khattak) from comment #25)
> 
> > Why would it be empty of only contain accuracy level if we always save
> > non-empty array and ensure timestamp is present?
> 
> The previous patch checked for:
> 
>   (permission == null || permission.length < 2)
> 
> The fallback replaces the first condition, but not the second one - so
> unless that condition wasn't actually needed, you end up with the
> aforementioned problem.

Ah no, that condition was only needed on my machine after I had saved a value before adding the timestamp. :)

Comment on attachment 321481 [details] [review]
location: Add AppAuthorizer class

(In reply to Zeeshan Ali (Khattak) from comment #27)
> Ah no, that condition was only needed on my machine after I had saved a
> value before adding the timestamp. :)

OK, then the fallback will work in the expected cases. I still think using separate fallbacks to defend against unexpected values (users messing around with the store, ...) is good practice, but not essential.

(In reply to Florian Müllner from comment #28)
> Comment on attachment 321481 [details] [review] [review]
> location: Add AppAuthorizer class
> 
> (In reply to Zeeshan Ali (Khattak) from comment #27)
> > Ah no, that condition was only needed on my machine after I had saved a
> > value before adding the timestamp. :)
> 
> OK, then the fallback will work in the expected cases. I still think using
> separate fallbacks to defend against unexpected values (users messing around
> with the store, ...) is good practice, but not essential.

if we have only 1 item in array, we'll get an error earlier already:

gjs> let a = ['hi']
gjs> let [b, c] = a
typein:2: strict warning: reference to undefined property a[1]

Created attachment 321505 [details] [review]
location: Add dialog to ask for location data access

Add a dialog that is used in a following patch, to ask user if they want
a requesting application to gain access to their location.

Co-author: Florian Müllner <fmuellner@gnome.org>

Created attachment 321506 [details] [review]
location: Add AppAuthorizer class

This class will be responsible for authorizing applications that try to
access location information. Since this is mainly targetted for xdg-app
applications, we make use of xdg-app's D-Bus API to store
per-application authorization.

Created attachment 321507 [details] [review]
location: Ask user to authorize applications

While we could have implemented this already a while ago, this would
have been a completely false security mechanism since we had no way of
reliably identifying applications. Since now with xdg-app, we can at least
reliably identify bundled applications, let's give users a choice of
which applications in particular they are OK with giving location data
to.

While we still can't reliably identify system (non-xdg-app) applications,
it seems extremely unlikely we'll ever be able to do that (at least not
in the near future) so we'll have to trust them to not lie about their
IDs.

Next release of geoclue will take the ID of bundled application directly
from corresponding xdg-app metadata so bundled applications can't simply
lie about their IDs.

(In reply to Zeeshan Ali (Khattak) from comment #29)
> if we have only 1 item in array, we'll get an error earlier already:
> 
> gjs> let a = ['hi']
> gjs> let [b, c] = a
> typein:2: strict warning: reference to undefined property a[1]

That's in gjs-console, not gnome-shell - the above should work without warning in the shell (and all follow-ups should work fine as well: Number(undefined) == NaN, NaN < 3 == false, NaN.toString() == "NaN", Number("NaN") == NaN, ...)

Review of attachment 321505 [details] [review]:

OK

Review of attachment 321506 [details] [review]:

::: js/ui/status/location.js
@@ +311,3 @@
+        let [levelStr, timeStr] = permission || ['NONE', '0'];
+        this._accuracyLevel = GeoclueAccuracyLevel[levelStr] ||
+                              GeoclueAccuracyLevel.NONE;

Btw, if we don't need another fallback for 'times', why is there one here?

Review of attachment 321507 [details] [review]:

OK

Created attachment 321525 [details] [review]
location: Add AppAuthorizer class

This class will be responsible for authorizing applications that try to
access location information. Since this is mainly targetted for xdg-app
applications, we make use of xdg-app's D-Bus API to store
per-application authorization.

Created attachment 321526 [details] [review]
location: Ask user to authorize applications

While we could have implemented this already a while ago, this would
have been a completely false security mechanism since we had no way of
reliably identifying applications. Since now with xdg-app, we can at least
reliably identify bundled applications, let's give users a choice of
which applications in particular they are OK with giving location data
to.

While we still can't reliably identify system (non-xdg-app) applications,
it seems extremely unlikely we'll ever be able to do that (at least not
in the near future) so we'll have to trust them to not lie about their
IDs.

Next release of geoclue will take the ID of bundled application directly
from corresponding xdg-app metadata so bundled applications can't simply
lie about their IDs.

Review of attachment 321525 [details] [review]:

::: js/ui/status/location.js
@@ +312,3 @@
+        this._accuracyLevel = GeoclueAccuracyLevel[levelStr] ||
+                              GeoclueAccuracyLevel.NONE;
+        this._timesAllowed = Number(timeStr): 0;

Syntax error, should be || 0;

Created attachment 321588 [details] [review]
location: Add AppAuthorizer class

This class will be responsible for authorizing applications that try to
access location information. Since this is mainly targetted for xdg-app
applications, we make use of xdg-app's D-Bus API to store
per-application authorization.

Created attachment 321589 [details] [review]
location: Ask user to authorize applications

While we could have implemented this already a while ago, this would
have been a completely false security mechanism since we had no way of
reliably identifying applications. Since now with xdg-app, we can at least
reliably identify bundled applications, let's give users a choice of
which applications in particular they are OK with giving location data
to.

While we still can't reliably identify system (non-xdg-app) applications,
it seems extremely unlikely we'll ever be able to do that (at least not
in the near future) so we'll have to trust them to not lie about their
IDs.

Next release of geoclue will take the ID of bundled application directly
from corresponding xdg-app metadata so bundled applications can't simply
lie about their IDs.

Review of attachment 321588 [details] [review]:

OK

Review of attachment 321589 [details] [review]:

OK

Attachment 321505 [details] pushed as e98a434 - location: Add dialog to ask for location data access
Attachment 321588 [details] pushed as 34fc454 - location: Add AppAuthorizer class
Attachment 321589 [details] pushed as a1e8c79 - location: Ask user to authorize applications