GNOME Bugzilla – Bug 658961
Be able to ask for IM account password
Last modified: 2017-04-06 22:16:46 UTC
Created attachment 196405 [details] screenshot After installing updates after an install and rebooting I got the prompt in the attached shot. This is really weird. We don't want to be randomly prompting the user out of context for his/her password. Entering the password for the gnome online accounts should be done from the gnome online accounts tool only. If that's what this is.
That's because of bug #658785 (the account shouldn't connect automatically) and bug #652546 (we don't, and can't, implement SSO for Google tak atm). *** This bug has been marked as a duplicate of bug 658785 ***
Actually there is a 3rd bug: the Shell should ask itself for the password. - Be sure to *not* have Emapthy running - Try to connect a newly created GOA account (or another account not having its passwords saved) - empathy-auth-client popups a dialog asking for the password Expected behaviour: the Shell asks for this password in a less intrusive way. I guess we could use a tray notification allowing user to enter the password directly.
Created attachment 197058 [details] screenshot Here is how it looks like atm. Two problems so far: - bug #659575 makes it look weird - User needs to be able to ask for the password to be saved (if not he will have to enter it each time the account connect). How would you do that from an UI pov?
(In reply to comment #3) > Here is how it looks like atm. Two problems so far: > > - bug #659575 makes it look weird > - User needs to be able to ask for the password to be saved (if not he will > have to enter it each time the account connect). How would you do that from an > UI pov? My two cents: - in the ideal use case, you should never ask passwords for GOA accounts, as they already have been granted an authorization from the online accounts panel. GOA should already cover the case when an account needs to renew its authorization credentials by itself (as far as I understand, this is still not possible due to bug 652546 at the moment, right?) - for accounts which don't have the password saved (i.e. non-GOA accounts created manually from Empathy itself), I believe we could either have a switch in the notification itself, or just delegate that to empathy-accounts (e.g. always save the password typed there, and trigger the notification only in case no password has been entered)...I think I prefer the latter.
(In reply to comment #4) > - in the ideal use case, you should never ask passwords for GOA accounts, as > they already have been granted an authorization from the online accounts panel. > GOA should already cover the case when an account needs to renew its > authorization credentials by itself (as far as I understand, this is still not > possible due to bug 652546 at the moment, right?) Yes; that's not possible atm. > - for accounts which don't have the password saved (i.e. non-GOA accounts > created manually from Empathy itself), I believe we could either have a switch > in the notification itself, or just delegate that to empathy-accounts (e.g. > always save the password typed there, and trigger the notification only in case > no password has been entered)...I think I prefer the latter. I'm not sure to understand what you mean. Atm if empathy is running, it will generate a notification having a 'Provide' button. Hitting this button will open empathy-auth-client's password (cf first screenshot). I guess we could do that in the Shell as well so that will work even if empathy isn't running but that seems a bit weird to me tbh. Makes much sense to allow user to enter his password directly into the notification.
(In reply to comment #5) > > - for accounts which don't have the password saved (i.e. non-GOA accounts > > created manually from Empathy itself), I believe we could either have a switch > > in the notification itself, or just delegate that to empathy-accounts (e.g. > > always save the password typed there, and trigger the notification only in case > > no password has been entered)...I think I prefer the latter. > > I'm not sure to understand what you mean. Atm if empathy is running, it will > generate a notification having a 'Provide' button. Hitting this button will > open empathy-auth-client's password (cf first screenshot). I guess we could do > that in the Shell as well so that will work even if empathy isn't running but > that seems a bit weird to me tbh. Makes much sense to allow user to enter his > password directly into the notification. I was referring only to the question "how do I toggle remember password if we use a notification?". What I mean is - having the "remember password" option for GOA accounts is not interesting, as we want to fix bug 652546 and never prompt in that case. (I believe as a workaround in the meantime the password could be always saved for GOA accounts so we spawn the entry only once) - for other custom accounts I was just proposing not to worry about this in the notification, as empathy-accounts' UI is the place where you create the account and decide whether to save a password or not.
Agreed. So we should only consider adding automatic saving of GOA accounts if we didn't fix bug #652546 for 3.4. So that means my branch can be reviewed as it for now. :)
Created attachment 197865 [details] [review] Be able to ask for IM account password
Review of attachment 197865 [details] [review]: Just some drive-by comments ::: js/ui/telepathyClient.js @@ +486,3 @@ + _approveServerAuth: function(account, conn, channel, dispatchOp, context) { + let source = new ApproverSource(dispatchOp, _("Password needed"), + Shell.util_icon_from_string('gtk-dialog-authentication')); Isn't that the GTK stock icon name? I think you should use 'dialog-password' here instead. @@ +1586,3 @@ + // Claim the channel as we are going to provide the password + this._dispatchOp.claim_with_async(this._tpClient, + Lang.bind(this, function(dispatchOp, result) { I think it's clearer to indent it like this: this._dispatchOp.claim_with_async(this._tpClient, Lang.bind(this, function(dispatchOp, result) { })); ::: src/shell-tp-client.c @@ +507,3 @@ + } + else if (status == TP_SASL_STATUS_SUCCEEDED || + status == TP_SASL_STATUS_SERVER_FAILED || AFAICS it's wrong for this to be in an "else if()" block, as the channel won't be unreffed when the status is STATUS_SUCCEEDED.
Please handle only channels with X-TELEPATHY-PASSWORD auth mechanism, soon gtalk will use GOA's access-token and xmpp-messenger does not support password at all. See related bug: https://bugs.freedesktop.org/show_bug.cgi?id=41388 And similar fix in empathy: http://git.gnome.org/browse/empathy/commit/?id=c7fe998d565bd37ed066e1fee0e7281c7f6117ce Also in the case keyring has the password, won't this show the notification area and close it quickly?
(In reply to comment #9) > ::: js/ui/telepathyClient.js > @@ +486,3 @@ > + _approveServerAuth: function(account, conn, channel, dispatchOp, context) > { > + let source = new ApproverSource(dispatchOp, _("Password needed"), > + > Shell.util_icon_from_string('gtk-dialog-authentication')); > > Isn't that the GTK stock icon name? I think you should use 'dialog-password' > here instead. changed. > @@ +1586,3 @@ > + // Claim the channel as we are going to provide the password > + this._dispatchOp.claim_with_async(this._tpClient, > + Lang.bind(this, function(dispatchOp, > result) { > > I think it's clearer to indent it like this: > > this._dispatchOp.claim_with_async(this._tpClient, Lang.bind(this, > function(dispatchOp, result) { > > })); changed. > ::: src/shell-tp-client.c > @@ +507,3 @@ > + } > + else if (status == TP_SASL_STATUS_SUCCEEDED || > + status == TP_SASL_STATUS_SERVER_FAILED || > > AFAICS it's wrong for this to be in an "else if()" block, as the channel won't > be unreffed when the status is STATUS_SUCCEEDED. You're right; good catch. fixed. (In reply to comment #10) > Please handle only channels with X-TELEPATHY-PASSWORD auth mechanism, soon > gtalk will use GOA's access-token and xmpp-messenger does not support password > at all. done. > Also in the case keyring has the password, won't this show the notification > area and close it quickly? Nope, empathy-auth-client acts as a pre-approver (Observer).
Created attachment 198606 [details] [review] Be able to ask for IM account password
Created attachment 198624 [details] [review] Be able to ask for IM account password
Review of attachment 198624 [details] [review]: ::: js/ui/telepathyClient.js @@ +495,3 @@ + + let source = new ApproverSource(dispatchOp, _("Password needed"), + Shell.util_icon_from_string('dialog-password')); Shell.util_icon_from_string has removed, Gio.icon_new_for_string should be used instead.
Created attachment 199906 [details] [review] Be able to ask for IM account password
(In reply to comment #15) > Be able to ask for IM account password Could the last patch please get a review by the maintainers?
Review of attachment 199906 [details] [review]: Mostly fine, minor nits. ::: js/ui/telepathyClient.js @@ +490,3 @@ + // Only prompt user if channel supports password + if (available_mechanisms.indexOf('X-TELEPATHY-PASSWORD') == -1) { + Shell.decline_dispatch_op(context, 'Does not support X-TELEPATHY-PASSWORD'); Four space indents. @@ +1590,3 @@ + + this._entry = new St.Entry({ can_focus: true }); + this._entry.clutter_text.set_password_char('\u25cf'); // ● U+25CF BLACK CIRCLE In order to embed Unicode chars inline, we'd need to put an "encoding" property in the modeline for tools to detect. I'd just remove the comment. @@ +1606,3 @@ + Shell.provide_password (this._channel, text); + } catch (err) { + throw new Error('Failed to Claim channel: ' + err); Put the closing brace to the try on its own line.
[Bumping GNOME Target field as this has not happened for 3.2]
Actually I'm not 100% happy of this approach. empathy-auth-client gained more logic in 3.4 to deal better with wrong password being entered, etc. Duplicating this logic into the Shell would be a bit sad from a code pov. Maybe we should have something more complex like gnome-keyring's unlock UIs which can be provided by different components? I'd be in favor of not merging this patch for 3.4; empathy-auth-client still does the job even if it's not perfect from an UI pov.
(In reply to comment #19 by Guillaume Desmottes) > I'd be in favor of not merging this patch for 3.4; empathy-auth-client still > does the job even if it's not perfect from an UI pov. Has anything changed with regard to 3.6, or is this postponed to a "mysterious future"?
Nothing changed and the current situation is not that bad tbh so it's mostly a postpone to some future.
Note that this is fixed in Ubuntu, the password is asked by their control center. When GOA will have done the same work, it will be responsible of asking passwords.
Removing GNOME 3.4 target
Chat integration has been reduced to showing notifications for active one-on-one channels. That is, gnome-shell never touches the telepathy presence, either automatically or in response to user action. So if there's a presence change, it has been initiated in an application, and I don't think it makes sense to delegate any authentication handling to the shell.