Created attachment 181477 [details] [review]
Patch

Btw, if using polkit from jhbuild you need to do something like this

 # chown root:root /opt/gnome-shell/install/libexec/polkit-agent-helper-1 && chmod 4755 /opt/gnome-shell/install/libexec/polkit-agent-helper-1

(Btw, it's completely wrong to build polkit from jhbuild - you really should just require that people get it from their distro vendor instead etc etc </rant>)

I made some youtube videos (for an upcoming blog entry) showcasing this feature

 http://www.youtube.com/watch?v=AaCxqhHpqFk
 http://www.youtube.com/watch?v=1IvDf0YSQVk
 http://www.youtube.com/watch?v=pBX5mrReHYQ

Enjoy!

(In reply to comment #0)
> > 3. However, even with the above mentioned hack, I think there's a bug somewhere
> in the shell because we often fail acquire the grab. This can usually be fixed
> by pressing SUPER twice (showing the Activities overlay and then back) and
> trying again. I don't know enough about the shell internals to diagnose
> further.

Talked to Owen and he asked me to try the patch in bug 642188 comment 8 - that seems to fix the problem!

Review of attachment 181477 [details] [review]:

You need more content in your commit message - should describe briefly what an authentication agent is and mention the native/JS split.

Hard for me to review the UI logic without doing a lot more work to understand how PolicyKit works, so the comments below are a little superficial, but most of the UI code did seem to make sense.

Worked in a quick test except for mispositioning of the cursor within the password entry when the dialog first shown, which is going to be a StEntry bug.

::: configure.ac
@@ +72,3 @@
 LIBEDATASERVERUI3_REQUIRED=2.91.6
 TELEPATHY_GLIB_MIN_VERSION=0.13.12
+POLKIT_MIN_REQUIRED=0.100

OK, it sucks that we have MIN_VERSION for everything but eds and _REQUIRED for eds, but stick to one or the other style, don't mix them! :-)

::: js/ui/main.js
@@ +215,3 @@
+
+    // Become a PolicyKit authentication anget
+    PolkitAuthenticationAgent.init()

I guess this is another reason we really need to get the shell initialized and past here before gnome-session continues (we already have problems with not owning the notification name soon enough) - getting polkit prompts at login is definitely bad, but definitely will happen for someone.

Seems a bit oddly grouped - I'd move it up next to EndSessionDialog.init()

::: js/ui/polkitAuthenticationAgent.js
@@ +54,3 @@
+                                 y_fill: true });
+
+        let icon = new St.Icon({ icon_name:   'dialog-password-symbolic' });

stray spaces

@@ +93,3 @@
+        let userRealName = this._user.get_real_name()
+        this._userLoadedId = this._user.connect('notify::is_loaded',
+                                                Lang.bind(this, this._updateContent));

This callback is too generically named unless you foresee it doing something other than handling the icon in the future

@@ +100,3 @@
+        if (userName == 'root') {
+            userRealName = _('Administrator');
+        }

Stray {}

@@ +105,3 @@
+        if (userRealName.length == 0) {
+            userRealName = userName;
+        }

Stray {}

@@ +108,3 @@
+
+        let userLayout = new St.BoxLayout({ style_class: 'polkit-dialog-user-layout',
+                                            vertical: false });

Our convention for this is userBox not userLayout (looks like the endSessionDialog and modalDialog classes use the Layout convention, but that just got missed in review)

@@ +176,3 @@
+        this._updateContent();
+
+        this._done_emitted = false;

fixVariableStyle

@@ +178,3 @@
+        this._done_emitted = false;
+
+        this._identity_to_auth = Polkit.UnixUser.new_for_name(userName);

andThis

@@ +221,3 @@
+        // Cheap localization trick
+        if (request == 'Password:') {
+            this._passwordLabel.set_text(_('Password:'));

Stray {}

@@ +239,3 @@
+        this._passwordEntry.set_text('');
+        this._errorMessage.set_text(text);
+        this._errorLayout.show();

if show-error/show-info can be shown in circumstances other than the end of the dialog, do you need to hide these elements at some point?

@@ +248,3 @@
+    },
+
+    destroy: function() {

obscurely named. destroy name should generally be reserved for a method that does { this.actor.destroy(); }

@@ +288,3 @@
+        this._native.connect('cancel', Lang.bind(this, this._onCancel));
+        this._currentDialog = null;
+        this._retryGrabTimerId = 0;

Since 8a22ea948f11 fixes the immediate problem you were having, let's skip this - if we want to add a retry facility it should be somewhere more generic

@@ +350,3 @@
+
+    _reallyCompleteRequest: function(wasCancelled) {
+        this._currentDialog.close();

what if another auth was initiated before this was called? seems problematical

@@ +359,3 @@
+        if (!wasCancelled) {
+            this._native.complete()
+        }

Stray {}

@@ +365,3 @@
+        if (this._currentDialog == null) {
+            log('polkitAuthenticationAgent: Attempting to complete request ' +
+                'but no dialog is showing');

reading this code I have no idea if this is a) normal in some corner cases (so shodn't be logged), or b) an "assertion failure"

::: src/shell-polkit-authentication-agent.c
@@ +77,3 @@
+  if (subject == NULL)
+    {
+      g_warning ("Error get session for the process we are in: %s (%s %d)",

s/get/getting/

@@ +96,3 @@
+                 error->message,
+                 g_quark_to_string (error->domain),
+                 error->code);

You know more about polkit error messages, so if you want to do this, it's fine, but as a genreal GError thing, the message is suppposed to contain all human readable information, and the domain/code are just in case the program needs to handle some errors differently.

@@ +101,3 @@
+    }
+
+  /* TODO: could save handle and unregister later */

why would we want to do this? if it doesn't make sense for the shell but might make sense if this code was reused, then I wouldn't mark the comment as a TODO, I'd mark it as:;

 /* We don't need to unregister, so skip saving the handle */

(or whatever)

@@ +111,3 @@
+shell_polkit_authentication_agent_finalize (GObject *object)
+{
+  //ShellPolkitAuthenticationAgent *agent = SHELL_POLKIT_AUTHENTICATION_AGENT (object);

Avoid C++ comments (search through file to find all)

@@ +215,3 @@
+          if (getpwuid_r (uid, &pwd, buf, sizeof (buf), &ppwd) == 0)
+            {
+              g_ptr_array_add (p, g_strdup (pwd.pw_name));

Maybe g_utf8_validate() here?

@@ +220,3 @@
+            {
+              g_warning ("Error looking up user name for uid %d", uid);
+            }

Generally don't use {} when not needed on either branch of an if

@@ +274,3 @@
+  else
+    {
+      //g_debug ("COMPLETING SCHEDULED %s cookie %s", request->action_id, request->cookie);

How could this happen? auth_request_complete() gets called only with agent->current_request

@@ +299,3 @@
+    }
+
+  //auth_request_complete (request);

Can't leave this here and commented out without an explanation of why - looks just stray to me

I just released polkit 0.100, see

 http://lists.freedesktop.org/archives/polkit-devel/2011-February/000343.html

If you are using Fedora there are already packages built for f15 and f16.

Owen: thanks for the review - I'll follow up tomorrow.

Created attachment 181605 [details] [review]
Take 2

Hey,

Here's a new version. I ran into a GLib problem (see bug 642968) while stress-testing the code a bit more (specifically the test triggering that included cancelling the auth attempt by nuking pkexec while the authentication was under way). So the code has been slightly rearranged.

(In reply to comment #5)
> Review of attachment 181477 [details] [review]:
> 
> You need more content in your commit message - should describe briefly what an
> authentication agent is and mention the native/JS split.

OK, done.

> Hard for me to review the UI logic without doing a lot more work to understand
> how PolicyKit works, so the comments below are a little superficial, but most
> of the UI code did seem to make sense.
> 
> Worked in a quick test except for mispositioning of the cursor within the
> password entry when the dialog first shown, which is going to be a StEntry bug.

Yes. I also think there's another bug with St.Label and/or ModalDialog when the label is wrapped:

 http://people.freedesktop.org/~david/StLabel-wrap-problem.png

You can easily reproduce this by running

 $ /usr/bin/pkexec /usr/bin/pk-example-frobnicate

since that produces a message that is wrapped...

> ::: configure.ac
> @@ +72,3 @@
>  LIBEDATASERVERUI3_REQUIRED=2.91.6
>  TELEPATHY_GLIB_MIN_VERSION=0.13.12
> +POLKIT_MIN_REQUIRED=0.100
> 
> OK, it sucks that we have MIN_VERSION for everything but eds and _REQUIRED for
> eds, but stick to one or the other style, don't mix them! :-)

Ok, MIN_VERSION it is.

> ::: js/ui/main.js
> @@ +215,3 @@
> +
> +    // Become a PolicyKit authentication anget
> +    PolkitAuthenticationAgent.init()
> 
> I guess this is another reason we really need to get the shell initialized and
> past here before gnome-session continues (we already have problems with not
> owning the notification name soon enough) - getting polkit prompts at login is
> definitely bad, but definitely will happen for someone.

Yeah.

> Seems a bit oddly grouped - I'd move it up next to EndSessionDialog.init()

Moved it there.

> ::: js/ui/polkitAuthenticationAgent.js
> @@ +54,3 @@
> +                                 y_fill: true });
> +
> +        let icon = new St.Icon({ icon_name:   'dialog-password-symbolic' });
> 
> stray spaces

Fixed.

> @@ +93,3 @@
> +        let userRealName = this._user.get_real_name()
> +        this._userLoadedId = this._user.connect('notify::is_loaded',
> +                                                Lang.bind(this,
> this._updateContent));
> 
> This callback is too generically named unless you foresee it doing something
> other than handling the icon in the future

Renamed to _onUserChanged.

> @@ +100,3 @@
> +        if (userName == 'root') {
> +            userRealName = _('Administrator');
> +        }
> 
> Stray {}

Fixed.

> @@ +105,3 @@
> +        if (userRealName.length == 0) {
> +            userRealName = userName;
> +        }
> 
> Stray {}

Fixed.

> @@ +108,3 @@
> +
> +        let userLayout = new St.BoxLayout({ style_class:
> 'polkit-dialog-user-layout',
> +                                            vertical: false });
> 
> Our convention for this is userBox not userLayout (looks like the
> endSessionDialog and modalDialog classes use the Layout convention, but that
> just got missed in review)

OK, fixed.

> @@ +176,3 @@
> +        this._updateContent();
> +
> +        this._done_emitted = false;
> 
> fixVariableStyle

Fixed.

> @@ +178,3 @@
> +        this._done_emitted = false;
> +
> +        this._identity_to_auth = Polkit.UnixUser.new_for_name(userName);
> 
> andThis

Fixed.

> @@ +221,3 @@
> +        // Cheap localization trick
> +        if (request == 'Password:') {
> +            this._passwordLabel.set_text(_('Password:'));
> 
> Stray {}

Fixed.

> @@ +239,3 @@
> +        this._passwordEntry.set_text('');
> +        this._errorMessage.set_text(text);
> +        this._errorLayout.show();
> 
> if show-error/show-info can be shown in circumstances other than the end of the
> dialog, do you need to hide these elements at some point?

They are hidden whenever the user responds to a request (see _onEntryActivate).

> @@ +248,3 @@
> +    },
> +
> +    destroy: function() {
> 
> obscurely named. destroy name should generally be reserved for a method that
> does { this.actor.destroy(); }

OK, it's now called destroySession().

> @@ +288,3 @@
> +        this._native.connect('cancel', Lang.bind(this, this._onCancel));
> +        this._currentDialog = null;
> +        this._retryGrabTimerId = 0;
> 
> Since 8a22ea948f11 fixes the immediate problem you were having, let's skip this
> - if we want to add a retry facility it should be somewhere more generic

OK, nuked.

> @@ +350,3 @@
> +
> +    _reallyCompleteRequest: function(wasCancelled) {
> +        this._currentDialog.close();
> 
> what if another auth was initiated before this was called? seems problematical

Can't happen - ::initiate is not emitted while a request is in flight. So as long as we are being careful not calling complete() (to finish a request) we're fine.

> 
> @@ +359,3 @@
> +        if (!wasCancelled) {
> +            this._native.complete()
> +        }
> 
> Stray {}

I nuked this code so no longer a problem.

> @@ +365,3 @@
> +        if (this._currentDialog == null) {
> +            log('polkitAuthenticationAgent: Attempting to complete request ' +
> +                'but no dialog is showing');
> 
> reading this code I have no idea if this is a) normal in some corner cases (so
> shodn't be logged), or b) an "assertion failure"

It's b) so I nuked it.

> ::: src/shell-polkit-authentication-agent.c
> @@ +77,3 @@
> +  if (subject == NULL)
> +    {
> +      g_warning ("Error get session for the process we are in: %s (%s %d)",
> 
> s/get/getting/

Fixed.

> @@ +96,3 @@
> +                 error->message,
> +                 g_quark_to_string (error->domain),
> +                 error->code);
> 
> You know more about polkit error messages, so if you want to do this, it's
> fine, but as a genreal GError thing, the message is suppposed to contain all
> human readable information, and the domain/code are just in case the program
> needs to handle some errors differently.

It's used in a g_warning() which is only used for diagnostic purposes - the more information we can get, the better.

> @@ +101,3 @@
> +    }
> +
> +  /* TODO: could save handle and unregister later */
> 
> why would we want to do this? if it doesn't make sense for the shell but might
> make sense if this code was reused, then I wouldn't mark the comment as a TODO,
> I'd mark it as:;
> 
>  /* We don't need to unregister, so skip saving the handle */
> 
> (or whatever)

Done.

> @@ +111,3 @@
> +shell_polkit_authentication_agent_finalize (GObject *object)
> +{
> +  //ShellPolkitAuthenticationAgent *agent = SHELL_POLKIT_AUTHENTICATION_AGENT
> (object);
> 
> Avoid C++ comments (search through file to find all)

OK.

> @@ +215,3 @@
> +          if (getpwuid_r (uid, &pwd, buf, sizeof (buf), &ppwd) == 0)
> +            {
> +              g_ptr_array_add (p, g_strdup (pwd.pw_name));
> 
> Maybe g_utf8_validate() here?

Good idea.

> @@ +220,3 @@
> +            {
> +              g_warning ("Error looking up user name for uid %d", uid);
> +            }
> 
> Generally don't use {} when not needed on either branch of an if

The other branch now has more than one statement...

> @@ +274,3 @@
> +  else
> +    {
> +      //g_debug ("COMPLETING SCHEDULED %s cookie %s", request->action_id,
> request->cookie);
> 
> How could this happen? auth_request_complete() gets called only with
> agent->current_request

It actually can happen (we queue up requests and a request in the middle of the queue can easily be canceled) but the code was actualy buggy. Now fixed.

> @@ +299,3 @@
> +    }
> +
> +  //auth_request_complete (request);
> 
> Can't leave this here and commented out without an explanation of why - looks
> just stray to me

It's stray, sorry. Nuked.

Review of attachment 181605 [details] [review]:

Looks very close

::: js/ui/polkitAuthenticationAgent.js
@@ +285,3 @@
+        this._native.connect('cancel', Lang.bind(this, this._onCancel));
+        this._currentDialog = null;
+        this._retryGrabTimerId = 0;

Leftover

@@ +290,3 @@
+
+    _onInitiate: function(nativeAgent, actionId, message, iconName, cookie, userNames) {
+        if (this._currentDialog != null) {

So you say this is OK because this can't happen before reallyCompleteRequest runs, so there's no danger that reallyCompleteRequest will complete the wrong dialog. So, then, why is it necessary? It doesn't seem to me to be possible for this to be both correct and necessary.

Created attachment 181622 [details] [review]
Take 3

(In reply to comment #8)
> Review of attachment 181605 [details] [review]:
> 
> Looks very close
> 
> ::: js/ui/polkitAuthenticationAgent.js
> @@ +285,3 @@
> +        this._native.connect('cancel', Lang.bind(this, this._onCancel));
> +        this._currentDialog = null;
> +        this._retryGrabTimerId = 0;
> 
> Leftover

Nuked.

> @@ +290,3 @@
> +
> +    _onInitiate: function(nativeAgent, actionId, message, iconName, cookie,
> userNames) {
> +        if (this._currentDialog != null) {
> 
> So you say this is OK because this can't happen before reallyCompleteRequest
> runs, so there's no danger that reallyCompleteRequest will complete the wrong
> dialog. So, then, why is it necessary? It doesn't seem to me to be possible for
> this to be both correct and necessary.

That is correct. Nuked.

Review of attachment 181622 [details] [review]:

OK, let's land this!

Comment on attachment 181622 [details] [review]
Take 3

Committed as

http://git.gnome.org/browse/gnome-shell/commit/?id=86b925a294aaaf9f5f3936e060294273cafbb40e

Thanks for the review!