Page MenuHomePhabricator

Update GoogleLogin to use AuthManager
Closed, ResolvedPublic

Description

This should likely be redone as a PrimaryAuthenticationProvider, where it can now integrate with Special:UserLogin instead of having to do its own special page and such.

Details

Related Gerrit Patches:
mediawiki/extensions/GoogleLogin : REL1_27Rewrite GoogleLogin to be a PrimaryAuthenticationProvider
mediawiki/extensions/GoogleLogin : masterRewrite GoogleLogin to be a PrimaryAuthenticationProvider

Event Timeline

Tgr created this task.Aug 26 2015, 6:46 AM
Tgr raised the priority of this task from to Needs Triage.
Tgr updated the task description. (Show Details)
Tgr added subscribers: Aklapper, Tgr.
Florian set Security to None.
Anomie updated the task description. (Show Details)May 13 2016, 8:10 PM

Change 289099 had a related patch set uploaded (by Florianschmidtwelzow):
WIP: Rewrite GoogleLogin to be a PrimaryAuthenticationProvider

https://gerrit.wikimedia.org/r/289099

Florian claimed this task.May 23 2016, 7:44 PM
Florian triaged this task as Medium priority.

Change 289099 merged by jenkins-bot:
Rewrite GoogleLogin to be a PrimaryAuthenticationProvider

https://gerrit.wikimedia.org/r/289099

Florian closed this task as Resolved.Jun 18 2016, 5:01 AM

*yay* :)

Anomie added a subscriber: Anomie.Jun 20 2016, 2:35 PM

\o/

If you have any feedback on how AuthManager made things better or worse for an extension like this, I'd like to hear it. One of our goals with AuthManager was to make authentication against an external site reasonably easy to do, compared to the old system where it was necessary to do all sorts of weird hacks.

First of all: AuthManager is a lot easier to handle as AuthPlugin or (like GoogleLogin does) implementing anything from scratch. The interfaces provide nearly anything to integrate a login provider into the login/signup workflow. However, two points which we could do better:

  • Login -> Signup -> Login workflow should be implemented as it is reasonable, that an user, who authenticates with an external site, want to create a new account if there isn't one so far for this credentials, or at least an option to create a new account (I thought we already have a task for this but I couldn't find it :/)
  • An extension still has to handle a "connection" between the local wiki account and the identifier of the external site (GoogleLogin uses the user_google_user table for that). However, it would be cool to have a central table for it, and an interface that allows an extension to specify the type of the connection (e.g. google_auth) and the interface handles the connection between multiple external site account connections and the one local wiki account (the same what GoogleLogin does with the GoogleUser class and the user_google_user table, but more global and better thought). What do you think about that? It would make it easier for Linkproviders to create/manage links between accounts :)

Otherwise I can't think about big pain points in using AuthManager, probably some examples would be good (however, linking some extensions on the doc page which already implement providers for different use cases may be enough as examples :)).

  • An extension still has to handle a "connection" between the local wiki account and the identifier of the external site (GoogleLogin uses the user_google_user table for that). However, it would be cool to have a central table for it, and an interface that allows an extension to specify the type of the connection (e.g. google_auth) and the interface handles the connection between multiple external site account connections and the one local wiki account (the same what GoogleLogin does with the GoogleUser class and the user_google_user table, but more global and better thought). What do you think about that? It would make it easier for Linkproviders to create/manage links between accounts :)

That table would need to be something like

CREATE TABLE /*_*/user_account_links (
    ual_id int unsigned NOT NULL PRIMARY KEY AUTO_INCREMENT,
    ual_user int unsigned NOT NULL, -- user_id or an ID from CentralIdLookup, same for all types.
    ual_type int unsigned NOT NULL, -- FK to ualt_id
    ual_external_account_identifier ??? NOT NULL
);
CREATE INDEX /*i*/ual_user_type ON /*_*/user_account_link (ual_user,ual_type);
CREATE UNIQUE INDEX /*i*/ual_external_account ON /*_*/user_account_link (ual_type,ual_external_account_identifier);

CREATE TABLE /*_*/user_account_link_types (
    ualt_id int unsigned NOT NULL PRIMARY KEY AUTO_INCREMENT,
    ualt_type varchar(?) NOT NULL
);
CREATE UNIQUE INDEX /*i*/ualt_type ON /*_*/user_account_link_types (ualt_type);

The troublesome bit is that ual_external_account_identifier field. Is it a string (e.g. an email address), or an integer, or a GUID, or some JSON blob with multiple fields? How big does the field need to be? Can we sensibly apply the shown uniqueness constraint on it to help prevent multiple accounts linked to the same external account?

I think, that the ual_external_account_identifier should be a string, or maybe a text field (I'm not sure, how long an identifier can be). I think most of the external account ids will be an integer, a GUID or the e-mail address. Any other identifier sounds like a very very rare case (at least I haven't seen this so far, which doesn't mean, that it doesn't exist).

Can we sensibly apply the shown uniqueness constraint on it to help prevent multiple accounts linked to the same external account?

I can't think how we should handle, if one external account is linked to multiple "internal" users. If a user wants to login with the account, what wiki user should be logged in? Should the user has the possibility to select the correct user? There could be a reasonable use case, if an user has one admin account and one "normal" user account but want to login with one external account (and select which what account he want to login). So I'm not entirely sure, what we want to support and what not, depending on this, we can apply the unique index or not :/

  • Login -> Signup -> Login workflow should be implemented as it is reasonable, that an user, who authenticates with an external site, want to create a new account if there isn't one so far for this credentials, or at least an option to create a new account (I thought we already have a task for this but I couldn't find it :/)

-> T138678: AuthManager should support to create a new account for a Link PrimaryAuthenticationProvider

Change 299866 had a related patch set uploaded (by Gergő Tisza):
Rewrite GoogleLogin to be a PrimaryAuthenticationProvider

https://gerrit.wikimedia.org/r/299866

Change 299866 merged by jenkins-bot:
Rewrite GoogleLogin to be a PrimaryAuthenticationProvider

https://gerrit.wikimedia.org/r/299866