From 5121d1f3a11bc3b03d8ddb9a6b23c71f95bce571 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20Dre=C3=9Fler?= Date: Sun, 23 Jun 2019 11:40:42 +0200 Subject: [PATCH 1/9] polkitAgent: Fix a typo of a signal name https://gitlab.gnome.org/GNOME/gnome-shell/merge_requests/590 --- js/ui/components/polkitAgent.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/js/ui/components/polkitAgent.js b/js/ui/components/polkitAgent.js index b93e6700bf..aba29dd21e 100644 --- a/js/ui/components/polkitAgent.js +++ b/js/ui/components/polkitAgent.js @@ -52,7 +52,7 @@ var AuthenticationDialog = GObject.registerClass({ this._user = AccountsService.UserManager.get_default().get_user(userName); let userRealName = this._user.get_real_name(); - this._userLoadedId = this._user.connect('notify::is_loaded', + this._userLoadedId = this._user.connect('notify::is-loaded', this._onUserChanged.bind(this)); this._userChangedId = this._user.connect('changed', this._onUserChanged.bind(this)); -- GitLab From 5cb30d9e0d6447e023187c415301f3eaf2ba48e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20Dre=C3=9Fler?= Date: Sun, 23 Jun 2019 11:52:54 +0200 Subject: [PATCH 2/9] polkitAgent: Update user name on user changes Right now we only update the user avatar on the user-changed signal, but since we also display the users real name we should also update that if the user changes. https://gitlab.gnome.org/GNOME/gnome-shell/merge_requests/590 --- js/ui/components/polkitAgent.js | 73 ++++++++++++++++----------------- 1 file changed, 36 insertions(+), 37 deletions(-) diff --git a/js/ui/components/polkitAgent.js b/js/ui/components/polkitAgent.js index aba29dd21e..9dc9bc8234 100644 --- a/js/ui/components/polkitAgent.js +++ b/js/ui/components/polkitAgent.js @@ -51,46 +51,37 @@ var AuthenticationDialog = GObject.registerClass({ userName = userNames[0]; this._user = AccountsService.UserManager.get_default().get_user(userName); - let userRealName = this._user.get_real_name(); + + let userBox = new St.BoxLayout({ style_class: 'polkit-dialog-user-layout', + vertical: false }); + content.messageBox.add(userBox); + + this._userAvatar = new UserWidget.Avatar(this._user, + { iconSize: DIALOG_ICON_SIZE, + styleClass: 'polkit-dialog-user-icon' }); + this._userAvatar.hide(); + userBox.add(this._userAvatar, + { x_fill: true, + y_fill: false, + x_align: St.Align.END, + y_align: St.Align.START }); + + if (userName == 'root') + this._userLabel = new St.Label({ style_class: 'polkit-dialog-user-root-label', + text: _("Administrator") }); + else + this._userLabel = new St.Label({ style_class: 'polkit-dialog-user-label' }); + + userBox.add(this._userLabel, + { x_fill: true, + y_fill: false, + x_align: St.Align.END, + y_align: St.Align.MIDDLE }); + this._userLoadedId = this._user.connect('notify::is-loaded', this._onUserChanged.bind(this)); this._userChangedId = this._user.connect('changed', this._onUserChanged.bind(this)); - - // Special case 'root' - let userIsRoot = false; - if (userName == 'root') { - userIsRoot = true; - userRealName = _("Administrator"); - } - - if (userIsRoot) { - let userLabel = new St.Label(({ style_class: 'polkit-dialog-user-root-label', - text: userRealName })); - content.messageBox.add(userLabel, { x_fill: false, - x_align: St.Align.START }); - } else { - let userBox = new St.BoxLayout({ style_class: 'polkit-dialog-user-layout', - vertical: false }); - content.messageBox.add(userBox); - this._userAvatar = new UserWidget.Avatar(this._user, - { iconSize: DIALOG_ICON_SIZE, - styleClass: 'polkit-dialog-user-icon' }); - this._userAvatar.hide(); - userBox.add(this._userAvatar, - { x_fill: true, - y_fill: false, - x_align: St.Align.END, - y_align: St.Align.START }); - let userLabel = new St.Label(({ style_class: 'polkit-dialog-user-label', - text: userRealName })); - userBox.add(userLabel, - { x_fill: true, - y_fill: false, - x_align: St.Align.END, - y_align: St.Align.MIDDLE }); - } - this._onUserChanged(); this._passwordBox = new St.BoxLayout({ vertical: false, style_class: 'prompt-dialog-password-box' }); @@ -303,7 +294,15 @@ var AuthenticationDialog = GObject.registerClass({ } _onUserChanged() { - if (this._user.is_loaded && this._userAvatar) { + if (!this._user.is_loaded) + return; + + let userName = this._user.get_user_name(); + let realName = this._user.get_real_name(); + + if (userName != 'root') { + this._userLabel.set_text(realName); + this._userAvatar.update(); this._userAvatar.show(); } -- GitLab From 3ce472b139305149f157b8702fbb81f0fa9b8b29 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20Dre=C3=9Fler?= Date: Sun, 23 Jun 2019 12:15:12 +0200 Subject: [PATCH 3/9] polkitAgent: Also show user avatar for root user Show the user avatar for all users, including the root user. The root user will always have the generic avatar, but it looks more consistent than showing no avatar at all. This way we also don't have to worry about the spacing introduced by the polkit-dialog-user-layout CSS class, which would give the "Administrator" label a small offset to the left. https://gitlab.gnome.org/GNOME/gnome-shell/merge_requests/590 --- js/ui/components/polkitAgent.js | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/js/ui/components/polkitAgent.js b/js/ui/components/polkitAgent.js index 9dc9bc8234..4cebdedeb1 100644 --- a/js/ui/components/polkitAgent.js +++ b/js/ui/components/polkitAgent.js @@ -59,7 +59,6 @@ var AuthenticationDialog = GObject.registerClass({ this._userAvatar = new UserWidget.Avatar(this._user, { iconSize: DIALOG_ICON_SIZE, styleClass: 'polkit-dialog-user-icon' }); - this._userAvatar.hide(); userBox.add(this._userAvatar, { x_fill: true, y_fill: false, @@ -303,9 +302,7 @@ var AuthenticationDialog = GObject.registerClass({ if (userName != 'root') { this._userLabel.set_text(realName); - this._userAvatar.update(); - this._userAvatar.show(); - } + this._userAvatar.update(); } cancel() { -- GitLab From b0f4b5ab9f1932da40c5ce8634c070c06c2d80b5 Mon Sep 17 00:00:00 2001 From: Joaquim Rocha Date: Sun, 23 Jun 2019 12:32:35 +0200 Subject: [PATCH 4/9] polkitAgent: Use dialog as confirmation when the user has no password When a user has no password and a polkit authentication is started, instead of blindly initiating the admin session, the regular "Authentication Requested" dialog is shown (but without the password entry). This means that the user's admin session is only effectively started after the user chooses to proceed with the authentication which provides an extra confirmation step that can be vital for critical tasks. Ideally we should use a different wording than "authentication" when the user has no password set, and use "confirmation" instead. However polkit already sends the requests with such messages (e.g. "Authentication is required to configure software repositories"), and it's important to show those to the user, so this patch keeps the regular wording. Also start the authentication process in _onUserChanged() right after the dialog was created instead of calling performAuthentication() from _onInitiate(). The bug mentioned in _onInitiate() is no longer an issue since we show the dialog in all cases now anyway. https://gitlab.gnome.org/GNOME/gnome-shell/merge_requests/590 --- js/ui/components/polkitAgent.js | 53 ++++++++++++++++++++------------- 1 file changed, 32 insertions(+), 21 deletions(-) diff --git a/js/ui/components/polkitAgent.js b/js/ui/components/polkitAgent.js index 4cebdedeb1..5e8e6f508f 100644 --- a/js/ui/components/polkitAgent.js +++ b/js/ui/components/polkitAgent.js @@ -11,6 +11,11 @@ const ModalDialog = imports.ui.modalDialog; const ShellEntry = imports.ui.shellEntry; const UserWidget = imports.ui.userWidget; +const DialogMode = { + AUTH: 0, + CONFIRM: 1 +}; + var DIALOG_ICON_SIZE = 48; var WORK_SPINNER_ICON_SIZE = 16; @@ -77,12 +82,6 @@ var AuthenticationDialog = GObject.registerClass({ x_align: St.Align.END, y_align: St.Align.MIDDLE }); - this._userLoadedId = this._user.connect('notify::is-loaded', - this._onUserChanged.bind(this)); - this._userChangedId = this._user.connect('changed', - this._onUserChanged.bind(this)); - this._onUserChanged(); - this._passwordBox = new St.BoxLayout({ vertical: false, style_class: 'prompt-dialog-password-box' }); content.messageBox.add(this._passwordBox); this._passwordLabel = new St.Label(({ style_class: 'prompt-dialog-password-label' })); @@ -134,8 +133,16 @@ var AuthenticationDialog = GObject.registerClass({ this._doneEmitted = false; + this._mode = null; + this._identityToAuth = Polkit.UnixUser.new_for_name(userName); this._cookie = cookie; + + this._userLoadedId = this._user.connect('notify::is-loaded', + this._onUserChanged.bind(this)); + this._userChangedId = this._user.connect('changed', + this._onUserChanged.bind(this)); + this._onUserChanged(); } _setWorking(working) { @@ -145,7 +152,7 @@ var AuthenticationDialog = GObject.registerClass({ this._workSpinner.stop(); } - performAuthentication() { + _initiateSession() { this._destroySession(); this._session = new PolkitAgent.Session({ identity: this._identityToAuth, cookie: this._cookie }); @@ -206,7 +213,10 @@ var AuthenticationDialog = GObject.registerClass({ } _onAuthenticateButtonPressed() { - this._onEntryActivate(); + if (this._mode == DialogMode.CONFIRM) + this._initiateSession(); + else + this._onEntryActivate(); } _onSessionCompleted(session, gainedAuthorization) { @@ -237,7 +247,7 @@ var AuthenticationDialog = GObject.registerClass({ } /* Try and authenticate again */ - this.performAuthentication(); + this._initiateSession(); } } @@ -303,6 +313,19 @@ var AuthenticationDialog = GObject.registerClass({ this._userLabel.set_text(realName); this._userAvatar.update(); + + if (this._user.get_password_mode() == AccountsService.UserPasswordMode.NONE && this._mode != DialogMode.CONFIRM) { + this._mode = DialogMode.CONFIRM; + this._destroySession(); + + this._cancelButton.grab_key_focus(); + this._passwordBox.hide(); + + this._ensureOpen(); + } else if (this._mode != DialogMode.AUTH) { + this._mode = DialogMode.AUTH; + this._initiateSession(); + } } cancel() { @@ -365,19 +388,7 @@ var AuthenticationAgent = class { } this._currentDialog = new AuthenticationDialog(actionId, message, cookie, userNames); - - // We actually don't want to open the dialog until we know for - // sure that we're going to interact with the user. For - // example, if the password for the identity to auth is blank - // (which it will be on a live CD) then there will be no - // conversation at all... of course, we don't *know* that - // until we actually try it. - // - // See https://bugzilla.gnome.org/show_bug.cgi?id=643062 for more - // discussion. - this._currentDialog.connect('done', this._onDialogDone.bind(this)); - this._currentDialog.performAuthentication(); } _onCancel(_nativeAgent) { -- GitLab From 9b9d55cf8b15f2b9d7a1a24ff8c7afa9c3a23a65 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20Dre=C3=9Fler?= Date: Mon, 24 Jun 2019 10:57:24 +0200 Subject: [PATCH 5/9] polkitAgent: Set key focus to password entry after opening dialog We're only opening the dialog after we got a request from polkit, so set the key focus to the password field after we got a request and opened the dialog instead of using setInitialKeyFocus. This way we don't try to focus the password field by default if we aren't showing it (eg. in case the user has no password or is using fingerprint login). https://gitlab.gnome.org/GNOME/gnome-shell/merge_requests/590 --- js/ui/components/polkitAgent.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/js/ui/components/polkitAgent.js b/js/ui/components/polkitAgent.js index 5e8e6f508f..5552977755 100644 --- a/js/ui/components/polkitAgent.js +++ b/js/ui/components/polkitAgent.js @@ -97,7 +97,6 @@ var AuthenticationDialog = GObject.registerClass({ this._workSpinner = new Animation.Spinner(WORK_SPINNER_ICON_SIZE, true); this._passwordBox.add(this._workSpinner); - this.setInitialKeyFocus(this._passwordEntry); this._passwordBox.hide(); this._errorMessageLabel = new St.Label({ style_class: 'prompt-dialog-error-label' }); @@ -265,9 +264,12 @@ var AuthenticationDialog = GObject.registerClass({ this._passwordBox.show(); this._passwordEntry.set_text(''); - this._passwordEntry.grab_key_focus(); - this._updateSensitivity(true); + this._updatePasswordEntrySensitivity(true); + this._updateOkButtonSensitivity(false); + this._setWorking(false); + this._ensureOpen(); + this._passwordEntry.grab_key_focus(); } _onSessionShowError(session, text) { -- GitLab From eec3002da20b9a4408f8511f9edcab25a62dc536 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20Dre=C3=9Fler?= Date: Mon, 24 Jun 2019 12:36:47 +0200 Subject: [PATCH 6/9] polkitAgent: Make authenticate button insensitive if password is empty According to the mockups, make the polkit dialogs "Authenticate" button insensitive and don't respond to pressing the enter key if no password is supplied. https://gitlab.gnome.org/GNOME/gnome-shell/merge_requests/590 --- js/ui/components/polkitAgent.js | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/js/ui/components/polkitAgent.js b/js/ui/components/polkitAgent.js index 5552977755..ae389b69b0 100644 --- a/js/ui/components/polkitAgent.js +++ b/js/ui/components/polkitAgent.js @@ -94,6 +94,13 @@ var AuthenticationDialog = GObject.registerClass({ this._passwordBox.add(this._passwordEntry, { expand: true }); + this._passwordEntry.clutter_text.connect('text-changed', (text) => { + if (text.get_text().length === 0) + this._updateOkButtonSensitivity(false); + else + this._updateOkButtonSensitivity(true); + }); + this._workSpinner = new Animation.Spinner(WORK_SPINNER_ICON_SIZE, true); this._passwordBox.add(this._workSpinner); @@ -127,8 +134,8 @@ var AuthenticationDialog = GObject.registerClass({ action: this.cancel.bind(this), key: Clutter.Escape }); this._okButton = this.addButton({ label: _("Authenticate"), - action: this._onAuthenticateButtonPressed.bind(this), - default: true }); + action: this._onAuthenticateButtonPressed.bind(this) }); + this._updateOkButtonSensitivity(false); this._doneEmitted = false; @@ -191,18 +198,25 @@ var AuthenticationDialog = GObject.registerClass({ } } - _updateSensitivity(sensitive) { + _updatePasswordEntrySensitivity(sensitive) { this._passwordEntry.reactive = sensitive; this._passwordEntry.clutter_text.editable = sensitive; + } + _updateOkButtonSensitivity(sensitive) { this._okButton.can_focus = sensitive; this._okButton.reactive = sensitive; - this._setWorking(!sensitive); } _onEntryActivate() { let response = this._passwordEntry.get_text(); - this._updateSensitivity(false); + if (response.length === 0) + return; + + this._updatePasswordEntrySensitivity(false); + this._updateOkButtonSensitivity(false); + this._setWorking(true); + this._session.response(response); // When the user responds, dismiss already shown info and // error texts (if any) -- GitLab From 2f053df18d76cdf215132e5179ddda6efb4b5ecb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20Dre=C3=9Fler?= Date: Sun, 23 Jun 2019 13:11:58 +0200 Subject: [PATCH 7/9] polkitAgent: Cancel session after disconnecting signal handlers When cancelling the PolkitAgent session before disconnecting the signal handlers, we receive a 'completed' signal which will (if the authentication was not successful) show an error message. This means we'll get an error message as soon as we call _destroySession to cancel a session. In practice this wasn't really relevant so far since we only called _destroySession when we wanted to see an error message anyway. Now we also call _destroySession when the user changes and no longer has a password set, in this case we just want to hide the password input without showing an error message. To fix this, disconnect the signal handlers before cancelling the session, which makes sure we don't receive the last 'completed' signal. This also makes this._completed obsolete. https://gitlab.gnome.org/GNOME/gnome-shell/merge_requests/590 --- js/ui/components/polkitAgent.js | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/js/ui/components/polkitAgent.js b/js/ui/components/polkitAgent.js index ae389b69b0..5afa11df86 100644 --- a/js/ui/components/polkitAgent.js +++ b/js/ui/components/polkitAgent.js @@ -233,11 +233,9 @@ var AuthenticationDialog = GObject.registerClass({ } _onSessionCompleted(session, gainedAuthorization) { - if (this._completed || this._doneEmitted) + if (this._doneEmitted) return; - this._completed = true; - /* Yay, all done */ if (gainedAuthorization) { this._emitDone(false); @@ -306,14 +304,13 @@ var AuthenticationDialog = GObject.registerClass({ _destroySession() { if (this._session) { - if (!this._completed) - this._session.cancel(); - this._completed = false; - this._session.disconnect(this._sessionCompletedId); this._session.disconnect(this._sessionRequestId); this._session.disconnect(this._sessionShowErrorId); this._session.disconnect(this._sessionShowInfoId); + + this._session.cancel(); + this._session = null; } } -- GitLab From 66433e7fd6b6640fd5edbdb354b36ea2882cb3c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20Dre=C3=9Fler?= Date: Sun, 23 Jun 2019 16:33:20 +0200 Subject: [PATCH 8/9] polkitAgent: Reset dialog to defaults after cancelling polkit session Since we don't know if polkit/PAM will request a password (emitting the "request" signal) or use another authentication method like a fingerprint, hide the password field and make the "Authenticate" button insensitive after cancelling the session, just like we do when creating the dialog. https://gitlab.gnome.org/GNOME/gnome-shell/merge_requests/590 --- js/ui/components/polkitAgent.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/js/ui/components/polkitAgent.js b/js/ui/components/polkitAgent.js index 5afa11df86..f9293325c0 100644 --- a/js/ui/components/polkitAgent.js +++ b/js/ui/components/polkitAgent.js @@ -313,6 +313,10 @@ var AuthenticationDialog = GObject.registerClass({ this._session = null; } + + this._passwordBox.hide(); + this._cancelButton.grab_key_focus(); + this._updateOkButtonSensitivity(false); } _onUserChanged() { @@ -331,8 +335,7 @@ var AuthenticationDialog = GObject.registerClass({ this._mode = DialogMode.CONFIRM; this._destroySession(); - this._cancelButton.grab_key_focus(); - this._passwordBox.hide(); + this._updateOkButtonSensitivity(true); this._ensureOpen(); } else if (this._mode != DialogMode.AUTH) { -- GitLab From c58bdaa1ce1c0d8565bdbd7a26b132d2fafe4e03 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20Dre=C3=9Fler?= Date: Sun, 23 Jun 2019 16:32:29 +0200 Subject: [PATCH 9/9] polkitAgent: Use a timeout for resetting the dialog Since polkit takes a few milliseconds from initializing the session to emitting the 'request' signal, don't introduce visual distraction by hiding the password field and showing it again a few ms later. https://gitlab.gnome.org/GNOME/gnome-shell/merge_requests/590 --- js/ui/components/polkitAgent.js | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/js/ui/components/polkitAgent.js b/js/ui/components/polkitAgent.js index f9293325c0..05c049ed08 100644 --- a/js/ui/components/polkitAgent.js +++ b/js/ui/components/polkitAgent.js @@ -160,6 +160,9 @@ var AuthenticationDialog = GObject.registerClass({ _initiateSession() { this._destroySession(); + + this._sessionRequestHappened = false; + this._session = new PolkitAgent.Session({ identity: this._identityToAuth, cookie: this._cookie }); this._sessionCompletedId = this._session.connect('completed', this._onSessionCompleted.bind(this)); @@ -263,6 +266,8 @@ var AuthenticationDialog = GObject.registerClass({ } _onSessionRequest(session, request, echoOn) { + this._sessionRequestHappened = true; + // Cheap localization trick if (request == 'Password:' || request == 'Password: ') this._passwordLabel.set_text(_("Password:")); @@ -314,9 +319,20 @@ var AuthenticationDialog = GObject.registerClass({ this._session = null; } - this._passwordBox.hide(); - this._cancelButton.grab_key_focus(); - this._updateOkButtonSensitivity(false); + if (this._sessionRequestTimeoutId) + GLib.source_remove(this._sessionRequestTimeoutId); + + this._sessionRequestTimeoutId = GLib.timeout_add(GLib.PRIORITY_DEFAULT, 200, () => { + if (!this._sessionRequestHappened) { + this._passwordBox.hide(); + this._cancelButton.grab_key_focus(); + this._updateOkButtonSensitivity(false); + } + + this._sessionRequestTimeoutId = 0; + return GLib.SOURCE_REMOVE; + }); + GLib.Source.set_name_by_id(this._sessionRequestTimeoutId, '[gnome-shell] this._sessionRequestTimeoutId'); } _onUserChanged() { @@ -335,6 +351,9 @@ var AuthenticationDialog = GObject.registerClass({ this._mode = DialogMode.CONFIRM; this._destroySession(); + this._sessionRequestHappened = true; + this._passwordBox.hide(); + this._cancelButton.grab_key_focus(); this._updateOkButtonSensitivity(true); this._ensureOpen(); @@ -362,6 +381,9 @@ var AuthenticationDialog = GObject.registerClass({ } this._destroySession(); + + if (this._sessionRequestTimeoutId) + GLib.source_remove(this._sessionRequestTimeoutId); } }); -- GitLab