[CreateAccount job] Never set an empty name when creating an account
ClosedPublic

Authored by wbauer on Feb 18 2020, 11:38 AM.

Details

Summary

At least when creating a google account, signon-ui isn't able to extract the username from the login page anymore since a couple of months (and newer versions ported to QtWebEngine don't even extract it at all anymore because it was considered to be too fragile, which apparently proved to be true...).

This looks kind of broken, and actually made kio-gdrive useless as that doesn't even list accounts with an empty name.

To fix it, set the name to some arbitrary string in the case that info.userName() is empty.

BUG: 414219
FIXED-IN: 19.12.3

Test Plan

Open systemsettings5->Online Accounts and add a "Google" account.
It displays a name for the new account now:

(see the screenshot in the bug report for how it looked before)

Note that you may need https://cgit.kde.org/kaccounts-integration.git/commit/?id=2420b04e3d01cd02c26e238221a7603614830d15 in kaccounts-integration, as the "Online Accounts" is more or less broken in 19.12, see https://bugs.kde.org/415267 (although creating an account should actually work).

Open "Network"->"Google Drive" in dolphin (you need to have kio-gdrive installed of course), the new account(s) is/are shown now and can be accessed:

Also tried with a patched signon-ui that returns some fixed username, it used that one then instead of the fallback, as intended.

Diff Detail

Repository
R155 KAccounts Integration
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
wbauer requested review of this revision.Feb 18 2020, 11:38 AM
wbauer created this revision.

As the new "Online Accounts" KCM in 19.12 actually shows the account type (i.e. info.caption()) in brackets after the name, we could also use a more generic name if preferred, e.g. something like "Account1" (but that should probably be translated as well so is not really suitable for the 19.12 branch I suppose).
I'm open to suggestions here, that's why I added VDG as subscriber.

Btw, it may make sense to allow the user to rename an account in the KCM in the future, but that's of course unrelated to this fix.

wbauer edited the test plan for this revision. (Show Details)Feb 18 2020, 12:07 PM

Hm, ideally I'd say we can use https://www.googleapis.com/auth/userinfo.email to retrieve username instead of adding ad-hoc stuff? At this point we do already have a auth data, so we can just make a data request using auth data and get userinfo...

Hm, ideally I'd say we can use https://www.googleapis.com/auth/userinfo.email to retrieve username instead of adding ad-hoc stuff? At this point we do already have a auth data, so we can just make a data request using auth data and get userinfo...

Maybe, I have no idea if that's feasible to do here in this place.
But wouldn't that only work for google accounts? AIUI, this code here should in principle also support other providers (if supported by signon).

Or maybe Signon::IdentityInfo::userName() should do that (or something else) and again return the actual username instead of an empty string... (not sure if that is possible, but I mentioned it in the signon-ui bug report too, see https://gitlab.com/accounts-sso/signon-ui/issues/2)

I somehow have the feeling that it may be good to have a fallback here in any case though.

leinir accepted this revision.Feb 19 2020, 10:01 AM

i think that i'm with @wbauer here - but also Bhushan. This is most definitely a good fallback that i think we'll want in (also, good commenting), but a second patch to pull up more friendly names for accounts where that's possible/make sense/whatnot would also be pretty great. So... accept this one, and suggest further work i think (wow, it's almost like being at uni ;) )

This revision is now accepted and ready to land.Feb 19 2020, 10:01 AM
wbauer added a comment.EditedFeb 19 2020, 7:14 PM

i think that i'm with @wbauer here - but also Bhushan. This is most definitely a good fallback that i think we'll want in (also, good commenting), but a second patch to pull up more friendly names for accounts where that's possible/make sense/whatnot would also be pretty great. So... accept this one, and suggest further work i think (wow, it's almost like being at uni ;) )

Yes, I do agree with Bhushan too, I'm just not sure that this is the right place to do it.
As signon is used as an abstraction (isn't it?), it somehow feels wrong having to deal with specifics for some provider (or even authentification method) to me.

Also, retrieving the username that way may fail as well as it is a network operation after all. (probably unlikely as the original authentification/login goes via the network too, but still...)

Btw, I'm already looking into implementing that, but I have to admit that sofar I don't really know how all fits together/works though.
One problem I see already is that this slot is called twice actually, once already *before* the authentification window shows up...

If I do find a (good) way to restore getting the username for Google accounts in kaccounts, I'll certainly upload a followup patch too. ;-)

Anyway, as this is just a fallback (that ideally should not be used), I think that using the proposed naming scheme is fine too, and as this patch has been accepted now (and it does fix the problem at hand) I'm going to push it as it is. (I'll still wait a couple of days though, for possible further comments)

wbauer planned changes to this revision.Feb 20 2020, 12:56 PM

One problem I see already is that this slot is called twice actually, once already *before* the authentification window shows up...

Hm, after thinking a bit more about it, I suppose that means that this patch is probably not a good idea after all.
It would set the name the first time already, while the username may only be available when it's being called the second time (and as the name is not empty anymore, it would not set it again).

I guess I'll have to come up with something else...

wbauer requested review of this revision.Mar 2 2020, 10:33 AM

Sorry for the delay.

One problem I see already is that this slot is called twice actually, once already *before* the authentification window shows up...

Hm, after thinking a bit more about it, I suppose that means that this patch is probably not a good idea after all.
It would set the name the first time already, while the username may only be available when it's being called the second time (and as the name is not empty anymore, it would not set it again).

Ok, further testing revealed that this part is actually *not* called twice because of the if (!m_done) { return; } right before it.

I also tried with a patched signon-ui that always returns a fixed username, and in that case the returned username was actually taken instead of the fallback.

So I suppose I just push this as-is to have creating google accounts fixed in 19.12.3, unless there still are objections in the next couple of hours... (tagging is at 23:59 UTC today)

I'll try implementing getting the real username as suggested, but that's certainly too late for 19.12.3 now.

This revision is now accepted and ready to land.Mar 2 2020, 10:33 AM
wbauer edited the test plan for this revision. (Show Details)Mar 2 2020, 10:35 AM

Sorry for the delay.

One problem I see already is that this slot is called twice actually, once already *before* the authentification window shows up...

Hm, after thinking a bit more about it, I suppose that means that this patch is probably not a good idea after all.
It would set the name the first time already, while the username may only be available when it's being called the second time (and as the name is not empty anymore, it would not set it again).

Ok, further testing revealed that this part is actually *not* called twice because of the if (!m_done) { return; } right before it.

i believe this is the point at which i point at an early return and go "could we please not?" ;)

I also tried with a patched signon-ui that always returns a fixed username, and in that case the returned username was actually taken instead of the fallback.

So I suppose I just push this as-is to have creating google accounts fixed in 19.12.3, unless there still are objections in the next couple of hours... (tagging is at 23:59 UTC today)

I'll try implementing getting the real username as suggested, but that's certainly too late for 19.12.3 now.

Incremental fixes are also good :)

This revision was automatically updated to reflect the committed changes.
Chipy added a subscriber: Chipy.Dec 13 2020, 9:03 AM

Hi there,
I also want to add my Google Drive account to Dolphin, but running into exactly the same problem.
I don't understand exactly, how I can fix it and which files I have to change.

It'd be very nice, if someone could help me, please.

Thank you in advance!
Chipy

wbauer added a comment.EditedDec 14 2020, 1:06 PM

I also want to add my Google Drive account to Dolphin, but running into exactly the same problem.
I don't understand exactly, how I can fix it and which files I have to change.

It'd be very nice, if someone could help me, please.

Well, this is a code review, and not the place to ask support questions...

If you really have the *same* problem that is fixed by this, you should probably ask your distribution to backport this patch or update their package, as this change is included since 19.12.3.

If you compile yourself, just download this patch and apply it using the "patch" command, or better just update your source checkout or download a newer version.