Update PluggableAuth to use AuthManager
Closed, ResolvedPublic

Description

See parent task and T110414#1578206.

There are a very large number of changes, so older changes are hidden. Show Older Changes

@Tgr, the fundamental problem here, as far as I understand it, is that both MediaWiki and SimpleSAMLphp use PHP sessions. I could make them co-exist in previous versions of MediaWiki. Is there no way to make them co-exist in the new MediaWiki session implementation? If not, I will accept as a constraint that SimpleSAMLphp must be configured to use something other than PHP sessions for storage.

@MarkAHershberger, my approach would be to use non-MediaWiki memcache in the SimpleSAMLphp configuration. The issue is not sharing information between MediaWiki and SimpleSAMLphp, but rather to let each do its own thing without stepping on each other.

Tgr added a comment.Aug 22 2016, 9:31 PM

MediaWiki only uses PHP sessions for B/C; you can disable that via $wgPHPSessionHandling. In any case, not sure how those two could conflict, unless you try to use them in the same request? Which again would be a questionable design decision.

The MediaWiki extension in question initiates a SimpleSAMLphp authentication request in the context of a MediaWiki special page by invoking a function in the SimpleSAMLphp library (https://simplesamlphp.org/). Before initiating the request, the SimpleSAMLphp code stores state information. When a response is received, the SimpleSAMLphp code attempts to retrieve the stored state information. This works when running MediaWiki 1.26 and lower using PHP sessions, the default mechanism in SimpleSAMLphp, but not in MediaWiki 1.27. There is nothing questionable in this design.

While it would be good to get this working using PHP sessions, the default mechanism in SimpleSAMLphp, I was able to switch to SQL for session management (section 2.2 at https://simplesamlphp.org/docs/stable/simplesamlphp-maintenance#section_2), and it worked flawlessly.

Tgr added a comment.Aug 23 2016, 2:12 AM

I doubt it works with 1.26 if not the default session configuration is used (e.g. with $wgSessionsInObjectCache enabled - the MediaWiki side would write to the object cache and the SAML side would try to read from files or whatever is configured as the native PHP session handler).

In 1.27 you have a few ways of making it work, all pretty bad:

  • on the MediaWiki side, temporarily disable MediaWiki's session handling (session_set_save_handler( new SessionHandler )) and store the data via PHP's normal session handling mechanism, then hope nothing else in the request needs session handling
  • try to get a functional SessionManager instance on the SAML side. There is nothing conceptually wrong with this one, and it would be nice if MediaWiki would allow you to do it easily but it really doesn't. You'll have to load LocalSettings, then Setup, which will fire some extension hooks, etc. Depending on what session backend MediaWiki is configured to use, DB handling (with LBFactory and all that) or whatever else might get pulled in.
  • force $wgPHPSessionHandling to disabled, in which case MediaWiki session handling and PHP session handling will be completely detached and you can use session_start and $_SESSION in MediaWiki to store SAML data.

But the only good way is to just not use sessions. They are not meant for data sharing between two different frameworks which do not share session handling logic.

I doubt it works with 1.26 if not the default session configuration is used (e.g. with $wgSessionsInObjectCache enabled - the MediaWiki side would write to the object cache and the SAML side would try to read from files or whatever is configured as the native PHP session handler).

Fair enough. It works in the default configuration, but it has not been tested in other configurations that I know of.

In 1.27 you have a few ways of making it work, all pretty bad:

It does not sound like it is worthwhile investing in getting it to work in MW 1.27. I will note in the extension page that SimpleSAMLphp will have to be configured to use a storage mechanism other that PHP sessions.

Change 311146 had a related patch set uploaded (by Cicalese):
Added SessionForRequest hook.

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

Linking earlier discussion about PluggableAuth on the AuthManager RFC: https://www.mediawiki.org/wiki/Topic:Sclzvdfqnjufkoar.

Also note the discussion on the patch at https://gerrit.wikimedia.org/r/#/c/311146/.

I have several specific questions about the new authentication framework that relate to the initiation of the authentication workflow. Note that there are two distinct use cases (manual login and auto login) that must both be supported, but both would share the actual authentication/authorization code. The only difference is in how the workflow gets initiated.

  1. Is it possible to begin authentication when a user clicks on the "Log in" link or directly visits Special:UserLogin (manual login) WITHOUT presenting a login form to the user? This would be necessary when no user input is required from the user or when all user input will be handled by an external authentication manager.
  1. Is it possible to trigger authentication automatically when it is detected that no user is logged in (i.e. auto login)? This would have to happen after a session is established, so it could not be done in a SessionProvider. This is the purpose of the core hook I proposed, but perhaps there is another way that already exists to detect this situation and trigger authentication. Again, no login form would be presented to the user by the wiki.
  1. When the wiki is configured for auto login, the "Log in" and "Log out" links must not appear. Is there a standard way to do so?

There are additional questions of authentication/authorization sequencing and local user creation that would depend upon answers to the questions above.

While I do not want to write code that bypasses the core authentication framework, the framework appears to make several assumptions that are at odds with our required workflow. I detailed our workflow in feedback to the original RFC, linked above.

If it is possible to extend the new authentication framework by making some changes to core to support our workflow, making PluggableAuth unnecessary, that would be great. Or perhaps it would at least take care of workflow initiation, leaving the mechanics of authentication/authorization handoff to PluggableAuth. I am, as always, happy to adapt this code in any way that makes sense to have it work in collaboration with core. But, if that is not possible, it will need to continue creatively to bypass some core functionality.

Tgr added a comment.Sep 17 2016, 10:06 PM
  1. Is it possible to begin authentication when a user clicks on the "Log in" link or directly visits Special:UserLogin (manual login) WITHOUT presenting a login form to the user? This would be necessary when no user input is required from the user or when all user input will be handled by an external authentication manager.

That is T141474. Right now the closest you can get is a login page that consists of a single button. Adding functionality for bypassing the form when it only consists of a single button is probably not much work.

  1. Is it possible to trigger authentication automatically when it is detected that no user is logged in (i.e. auto login)? This would have to happen after a session is established, so it could not be done in a SessionProvider. This is the purpose of the core hook I proposed, but perhaps there is another way that already exists to detect this situation and trigger authentication. Again, no login form would be presented to the user by the wiki.

By "authentication", you really mean a redirect, right?

One way to do it would be to allow the SessionProvider to return something that tells SessionManager that the browser needs to be redirected; the other is to add a hook for early redirect, probably somewhere early in MediaWiki::main().

https://gerrit.wikimedia.org/r/#/c/311146 is not useful for that because it gets invoked whenever someone calls WebRequest::getSession() (ie. hard to predict when exactly, possibly multiple times per request, possibly not for the actual user request since sessions can be exported and imported), and you don't have any way to break the normal control flow after setting a redirect.

  1. When the wiki is configured for auto login, the "Log in" and "Log out" links must not appear. Is there a standard way to do so?

They disappear when the active session provider says (via SessionProvider::canChangeUser()) that it cannot attach an arbitrary user to the session. That's not so much "autologin" as "no login needed at all" (OAuth, client-side certs etc) but maybe we are just using mismatching vocabularies.

That is T141474. Right now the closest you can get is a login page that consists of a single button. Adding functionality for bypassing the form when it only consists of a single button is probably not much work.

That would be great. Is that something I should start trying to figure out, or would somebody familiar with that code be able to look into it sometime soon?

By "authentication", you really mean a redirect, right?

Yes - or more specifically, calling the code that would invoke the appropriate authentication library function that would cause a redirect.

One way to do it would be to allow the SessionProvider to return something that tells SessionManager that the browser needs to be redirected; the other is to add a hook for early redirect, probably somewhere early in MediaWiki::main().

I'm not sure how the former would work. The two authentication libraries I have the most experience with (OpenID Connect and SimpleSAMLphp) have a function that you call that does the setup and redirect. They do not return back to the calling code, so it's not like they return an address that the calling code redirects to.

If I understand your latter suggestion, it is very similar to the hook I added, but you would prefer a different location for that hook. Is that correct? I will look at MediaWiki::main() to see if there is a good location for that hook to be inserted, but any suggestions are always welcome.

Or, maybe in line with your first suggestion, could the SessionProvider return something that tells SessionManager redirect to Special:UserLogin? Could this cause a PluggableAuthPrimaryAuthenticationProvider to take over? That would eliminate the current approach of calling a PluggableAuth:login() function, allowing the flow to be consistent with the new authentication framework regardless of whether login is initiated automatically or by clicking a Log in link.

https://gerrit.wikimedia.org/r/#/c/311146 is not useful for that because it gets invoked whenever someone calls WebRequest::getSession() (ie. hard to predict when exactly, possibly multiple times per request, possibly not for the actual user request since sessions can be exported and imported)

Yes, I was concerned about that and was hoping that feedback on that patch would indicate whether or not that was a good location for it.

and you don't have any way to break the normal control flow after setting a redirect.

I'm not sure I completely understand your point here, but I do agree that redirects play havoc with the control flow.

They disappear when the active session provider says (via SessionProvider::canChangeUser()) that it cannot attach an arbitrary user to the session. That's not so much "autologin" as "no login needed at all" (OAuth, client-side certs etc) but maybe we are just using mismatching vocabularies.

Good to know. So I would need to create a session provider that, if auto login is selected, does enough to be chosen as the active session provider and returns false for canChangeUser()? I suppose I could add a subclass of CookieSessionProvider for this, setting it at maximum priority and overriding canChangeUser(). But, I'm not sure if this is an elegant solution or a hack. Or is there a way to configure CookieSessionProvider to make this happen?

So, it sounds like I need:

  1. a core patch that prevents the login form from being displayed if it contains nothing but a single button
  1. a different location in core for a hook that could be used to trigger login if the wiki is configured for auto login or a way for the SessionProvider to tell the SessionManger to redirect to Special:UserLogin
  1. a PluggableAuthSessionProvider or a way to configure CookieSessionProvider so that canChangeUser() returns false
  1. a PluggableAuthPrimaryAuthenticationProvider that will call the appropriate authentication library through the authentication plugin and, if the user is authenticated, the appropriate optional authorization plugin

Does this make sense?

Thanks for your help with this. I want to get this deployed as soon as possible so we can upgrade to MW 1.27, but I do want to make sure that the solution is the best possible and is working in concert with the new authentication framework.

That is T141474. Right now the closest you can get is a login page that consists of a single button. Adding functionality for bypassing the form when it only consists of a single button is probably not much work.

That would be great. Is that something I should start trying to figure out, or would somebody familiar with that code be able to look into it sometime soon?

The general outline would be to detect that there is only one PRIMARY_REQUIRED AuthenticationRequest, the only user-interactable control in that request is a button, and there are no other requests providing interactable controls. If so, automatically "click" that button. Chances are the code for this would go in LoginSignupSpecialPage::execute() before the call to $this->trySubmit(), or possibly override trySubmit() itself, to call $this->handleFormSubmit() directly with appropriate values in $data.

This may be complicated by the fact that AuthManager itself wants a RememberMeAuthenticationRequest, which is probably going to want to include a checkbox, unless you have your custom SessionProvider always be the one used and have it return 0 for ->getRememberUserDuration().

the other is to add a hook for early redirect, probably somewhere early in MediaWiki::main().

$wgExtensionFunctions, maybe? It's not exactly a hook, but it serves the same purpose and is called early. You'd likely have it make a check like

if ( !defined( 'MW_NO_SESSION' ) && !$wgCommandLineMode ) {
    $sessionUser = MediaWiki\Session\SessionManager::getGlobalSession()->getUser();
    if ( $sessionUser->getId() === 0 && !User::isValidUserName( $sessionUser->getName() ) ) {
        // Do redirect (and don't return)
    }
}

Or add a hook to the end of Setup.php like in Gerrit change 268231 in which case you could check $wgUser->isAnon() directly instead of the two $sessionUser tests.

Either way that might be a little better than a hook in MediaWiki::main() since it'll cover api.php, img_auth.php, and so on too.

Or, maybe in line with your first suggestion, could the SessionProvider return something that tells SessionManager redirect to Special:UserLogin?

IMO it would make more sense to have an option to have OutputPage::showPermissionsErrorPage() redirect directly to Special:UserLogin instead of displaying the "Please log in to view other pages" message, rather than having SessionManager be trying to signal for a redirect.

Good to know. So I would need to create a session provider that, if auto login is selected, does enough to be chosen as the active session provider and returns false for canChangeUser()? I suppose I could add a subclass of CookieSessionProvider for this, setting it at maximum priority and overriding canChangeUser(). But, I'm not sure if this is an elegant solution or a hack. Or is there a way to configure CookieSessionProvider to make this happen?

You might want to subclass ImmutableSessionProviderWithCookie instead of CookieSessionProvider, and possibly have your constructor fill in a default sessionCookieName parameter. That would avoid potential confusion where it might accidentally fall back to code that tries to use UserID and Token cookies.

There are a few ways this could end up working:

  • Have provideSessionInfo() ask the plugin for the name of the logged-in user for the passed $request, and maybe also the session ID. Note this determination shouldn't depend on $_COOKIE or other globals. You might use the 'forceUse' SessionInfo parameter to avoid trouble in case the saved session is for a different user than the plugin returns.[1]
  • Do like BotPasswordSessionProvider::newSessionForRequest() and manually create a Session and persist it upon login, which will then be used by future calls to provideSessionInfo().

[1]: Consider the case where the provider returns UserA with session ID "XYZ123", but session ID "XYZ123" already exists for UserB or for an anonymous user. By default SessionManager ignores the returned UserA+XYZ123 session, but with 'forceUse' it will instead delete the existing UserB+XYZ123 session data so it can recreate session XYZ123 for UserA.

Tgr added a comment.Sep 20 2016, 2:37 AM

That would be great. Is [T141474] something I should start trying to figure out, or would somebody familiar with that code be able to look into it sometime soon?

I want to eventually, but it's not high on my priority list. Left some thoughts in T141474#2651191.


Calling a library that sets headers and then just dies seems like a very ugly thing to do inside a framework. There are all kinds of teardown operations MediaWiki might want to do - if the library does not return, cookies might not get set, logs might not get flushed to disk, implicit transactions might not get committed etc. If you can't use a better library, maybe the best thing to do is to expose it as its own endpoint and then redirect to it (and let it redirect further). Or you can redirect to the login page first, and have that redirect to the new endpoint, so that other providers get a chance to interact. E.g. we have a provider that prevents blocked users from logging in when the wiki is configured to do that. Per-request authentication (via a SessionProvider) circumvents checks like that, which might be unintuitive for something that's not actually per-request.

Calling a library that sets headers and then just dies seems like a very ugly thing to do inside a framework.

Agreed. In a perfect world, one would never do that.

If you can't use a better library . . .

Sadly, both libraries I have been using follow that model. And, since the extension is by intent "pluggable", it is likely that others who want to extend it will have the same issue.

maybe the best thing to do is to expose it as its own endpoint and then redirect to it (and let it redirect further). Or you can redirect to the login page first, and have that redirect to the new endpoint, so that other providers get a chance to interact.

But, won't both of those options cause the same problem that you describe above? It's still a redirect. If the code is in the middle of executing an authentication workflow (i.e. in the middle of a PrimaryAuthenticationProvider OR SessionProvider action), how would it break out of that workflow to get to the redirect without bypassing the teardown code? One way or the other, it will be in the middle of trying to answer the question of whether the user can be authenticated and will need to redirect to accomplish that.

E.g. we have a provider that prevents blocked users from logging in when the wiki is configured to do that.

When the external authentication service redirects back to the wiki, the other providers will have an opportunity to run. Or at least that's what I would hope would happen.

Per-request authentication (via a SessionProvider) circumvents checks like that, which might be unintuitive for something that's not actually per-request.

I'm not sure that I understand what you are saying in that sentence at least as it relates to this discussion.

The general outline would be to detect . . .

Thanks for the suggestion. I will look into that.

This may be complicated by the fact that AuthManager itself wants a RememberMeAuthenticationRequest, which is probably going to want to include a checkbox, unless you have your custom SessionProvider always be the one used and have it return 0 for ->getRememberUserDuration().

Good to know.

$wgExtensionFunctions, maybe? ... Or add a hook to the end of Setup.php ... Either way that might be a little better than a hook in MediaWiki::main() since it'll cover api.php, img_auth.php, and so on too.

I'll try those. I definitely would like this to work for api.php and img_auth.php.

IMO it would make more sense to have an option to have OutputPage::showPermissionsErrorPage() redirect directly to Special:UserLogin instead of displaying the "Please log in to view other pages" message, rather than having SessionManager be trying to signal for a redirect.

But, auto login should occur (when configured) regardless of whether there is a permissions error on a page. Every page should be viewed from the perspective of a logged in user. Or, if an authentication/authorization error occurs, an error page should be displayed.

You might want to subclass ImmutableSessionProviderWithCookie instead of CookieSessionProvider,

Thank you for the suggestion.

[1]: Consider the case where the provider returns UserA with session ID "XYZ123", but session ID "XYZ123" already exists for UserB or for an anonymous user. By default SessionManager ignores the returned UserA+XYZ123 session, but with 'forceUse' it will instead delete the existing UserB+XYZ123 session data so it can recreate session XYZ123 for UserA.

I'm going to have to study this case to fully understand it. Thanks for the warning.

But, won't both of those options cause the same problem that you describe above? It's still a redirect. If the code is in the middle of executing an authentication workflow (i.e. in the middle of a PrimaryAuthenticationProvider OR SessionProvider action), how would it break out of that workflow to get to the redirect without bypassing the teardown code? One way or the other, it will be in the middle of trying to answer the question of whether the user can be authenticated and will need to redirect to accomplish that.

A PrimaryAuthenticationProvider should return a REDIRECT AuthenticationResponse rather than doing the redirect itself. Of course, that depends on your library not trying to insist on doing the redirect itself.

Per-request authentication (via a SessionProvider) circumvents checks like that, which might be unintuitive for something that's not actually per-request.

I'm not sure that I understand what you are saying in that sentence at least as it relates to this discussion.

He's saying that authenticating via a SessionProvider bypasses checks that only happen on login, such as "you're not allowed to log in if you're blocked". That specific case is easy enough for the SessionProvider to check for itself, but others may not be.

cicalese added a comment.EditedSep 20 2016, 5:47 PM

But, won't both of those options cause the same problem that you describe above? It's still a redirect.

A PrimaryAuthenticationProvider should return a REDIRECT AuthenticationResponse rather than doing the redirect itself. Of course, that depends on your library not trying to insist on doing the redirect itself.

Oh, got it. So, my PluggableAuthPrimaryAuthenticationProvider would return a REDIRECT response, which would cause a disciplined redirect to the endpoint I have created, and that endpoint would then call the unfriendly authentication library function which redirects and exits without returning from the function. I see how that could work.

He's saying that authenticating via a SessionProvider bypasses checks that only happen on login, such as "you're not allowed to log in if you're blocked". That specific case is easy enough for the SessionProvider to check for itself, but others may not be.

The only reason I would need to implement a SessionProvider, following from the discussion above, would be to hide the login/logout links if the wiki is configured for auto login. No login would be done from a SessionProvider. If the wiki is configured for manual login, login would be initiated by the user clicking on the login link or navigating directly to Special:UserLogin, and would be handled by a PluggableAuthPrimaryAuthenticationProvider. If the wiki is configured for auto login, login would be accomplished if no user is already logged in by a redirect to Special:UserLogin, which would then proceed as with manual login above, running all of the appropriate checks. That redirect to Special:UserLogin would happen in $wgExtensionFunctions or a hook (although I'm not certain how that could be done ensuring that everything gets torn down appropriately). In the case of auto login when a user is already logged in, nothing special needs to happen.

Tgr added a comment.Sep 20 2016, 8:32 PM

SessionProviders are really meant for authentication methods that can individually authenticate every request (cookies, OAuth, a fixed IP -> user mapping etc). They are not ideal for handling autologin, which is a traditional login-type workflow (except without user interaction) which relies on normal CookieSessionProvider behavior for per-request authentication. What happens if your wiki has multiple autologin-based SSO providers? You can have multiple auth providers which each do their check one after another until one succeeds, but you can only have one session provider per request. You want to disable traditional login (hide the link) when you use automatically initiated login, but you don't want to disable logout. Etc.

I feel autologin should be added to the PrimaryAuthenticationProvider interface somehow (with the framework taking care of redirect and login initiation if needed).


Re: redirects, some frameworks use exception handling for that: you throw new MWRedirectException( $url ), the framework catches it, does the teardown and then redirects. That is an option for MediaWiki as well, although I think a dedicated hook is a better fit (and using auth providers, as above, an even better one).


As I said in the other task, an automatically initiated login has some warts. Imagine what happens if the session expires while the user is spending a lot of time on a complicated edit; the work would be lost and the user would have no idea what is happening. CentralAuth uses JS requests for logging in, which is a lot nicer (although it depends on the browser supporting JS and not having paranoid privacy settings, but it seems acceptable to just demand users not meeting that to click on the login link manually). I am not sure if that can be meaningfully generalized or that core is a good place for it even if it can be, but it's worth considering at least.

cicalese added a comment.EditedSep 20 2016, 8:48 PM

Agreed that SessionProviders don't seem to be the place for auto login. The recommendation above was only to use a SessionProvider to remove the login/logout links, not for auto login - although that solution does seem to be rather heavyweight for that purpose. But, that was the suggestion in order to replace my previous code that eliminated the links. I'm just trying to accommodate your recommendations.

I will look at the possibility of having JavaScript initiate the auto login.

Do you have an example you can point me to for returning a REDIRECT response from a PrimaryAuthenticationProvider? I'm having some trouble figuring out what is necessary for that.

I will tackle manual login first. I will concurrently try to determine if auto login is strictly necessary. It was initially included as a convenience, but I actually do not need it for the wikis I developed the extension for. I know it is being used by the PluggableSSO extension (not on mediawiki.org yet), but I'm not sure if it is strictly necessary.

Tgr added a comment.Sep 20 2016, 9:30 PM

The recommendation above was only to use a SessionProvider to remove the login/logout links, not for auto login - although that solution does seem to be rather heavyweight for that purpose. But, that was the suggestion in order to replace my previous code that eliminated the links.

Yeah, that's the best you can do currently; I am just saying we should maybe fix that. Login links are displayed based on AuthManager::canAuthenticateNow() which currently just checks the session provider but could easily do other things.

Do you have an example you can point me to for returning a REDIRECT response from a PrimaryAuthenticationProvider? I'm having some trouble figuring out what is necessary for that.

GoogleLogin is the best example currently.

I will look at the possibility of having JavaScript initiate the auto login.

CentralAuth does that via centralautologin.js and Special:CentralAutoLogin (which is also used for a bunch of other things and is in general not the easiest code to follow...)

Change 311146 abandoned by Cicalese:
Added SessionForRequest hook.

Reason:
Working on an alternative approach.

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

Change 299107 merged by jenkins-bot:
Update for MW 1.27

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

Change 312703 had a related patch set uploaded (by Cicalese):
Update for MW 1.27

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

cicalese added a comment.EditedSep 25 2016, 6:24 PM

Patch https://gerrit.wikimedia.org/r/#/c/312703/4 is ready for comments/testing. It is still missing the features below.

PluggableAuth To Do:
Are there any other functions in PluggableAuthPrimaryAuthenticationProvider that must be implemented?

Core To Do:
Allow disabling Remember Me when unnecessary
Do not show login page if there are no fields

I just discovered that in order to auto create a wiki account when a user logs in the first time, the following must be set:

$wgGroupPermissions['*']['autocreateaccount'] = true;

However, I don't see the autocreateaccount right documented at https://www.mediawiki.org/wiki/Manual:User_rights.

I just discovered that in order to auto create a wiki account when a user logs in the first time, the following must be set:

$wgGroupPermissions['*']['autocreateaccount'] = true;

Technically it's only needed if you override the default $wgGroupPermissions['*']['createaccount'] = true;.

However, I don't see the autocreateaccount right documented at https://www.mediawiki.org/wiki/Manual:User_rights.

It's a wiki! (:

I added it for you.

Thank you!!

Mostly I wanted confirmation that I hadn't misinterpreted what I was seeing before adding it, but having you add it was even better :-)

The general outline would be to detect that there is only one PRIMARY_REQUIRED AuthenticationRequest, the only user-interactable control in that request is a button, and there are no other requests providing interactable controls. If so, automatically "click" that button. Chances are the code for this would go in LoginSignupSpecialPage::execute() before the call to $this->trySubmit(), or possibly override trySubmit() itself, to call $this->handleFormSubmit() directly with appropriate values in $data.

This may be complicated by the fact that AuthManager itself wants a RememberMeAuthenticationRequest, which is probably going to want to include a checkbox, unless you have your custom SessionProvider always be the one used and have it return 0 for ->getRememberUserDuration().

Unless you find anything else that is missing from the current patch at https://gerrit.wikimedia.org/r/#/c/312703/4, this is the last remaining bit that I need to have PluggableAuth working as desired under MW 1.27: not displaying the login form if it is unnecessary. The PluggableAuth code in the above referenced patch currently works without this change to core, but the unnecessary form is undesireable.

I've looked through the code referenced above, and I have found the relevant bits, but it will still take a bit of figuring out. I have a few initial questions:

  1. I see that the RememberMeAuthenticationRequest is unconditionally added by AuthManager. How would you suggest preventing it from being added? Or, since the plan is to bypass the login form, would we just add code to ignore the field created by that AuthenticationRequest when making the decision whether to bypass the form?
  1. Do you have any additional thoughts on the approach to bypassing the form before I dive in?

I also have one lingering doubt about the PluggableAuth implementation. I created PluggableAuthBeginAuthenticationRequest to trigger the PluggableAuth code, but I found that the request was being filtered out since it did not contribute any fields. So I added the dummy hidden pluggableauth field in it to force it not to be filtered. Is there some other approach that would be preferable?

And, I'm having issues figure out the correct way to include my authentication provider. I added

"AuthManagerConfig": {
  "primaryauth": {
    "PluggableAuthPrimaryAuthenticationProvider": {
      "class": "PluggableAuthPrimaryAuthenticationProvider",
      "authoritative": true,
      "sort": 0
    }
  }

to my extension.json, but it is ignored. I tried adding a $wgExtensionFunctions[] function to set the primaryauth value, but that code never was executed. I finally resorted to:

wfLoadExtension( 'PluggableAuth' );
$wgAuthManagerAutoConfig['primaryauth'] = [
  PluggableAuthPrimaryAuthenticationProvider::class => [
    'class' => PluggableAuthPrimaryAuthenticationProvider::class,
    'sort' => 0
  ]
];

but I do not want to require that admins have to include that additional code block. What is the best approach for this? Is the fact that the information in extension.json is ignored a bug, or is there an error in my configuration?

Finally, just a confession that I didn't 'completely' create my own endpoint for the redirect. I'm using an UnlistedSpecialPage, since I do still need the MediaWiki context to be able to autoload the code for my plugins. But, it seems that it might be at least somewhat preferable to redirect from the special page execute() than from the middle of the authentication workflow.

Thanks again for all of your help as I try to get this code updated.

Change 312865 had a related patch set uploaded (by Cicalese):
Bypass login page if no user input is required.

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

Tgr added a comment.Sep 26 2016, 7:42 PM
  1. I see that the RememberMeAuthenticationRequest is unconditionally added by AuthManager. How would you suggest preventing it from being added? Or, since the plan is to bypass the login form, would we just add code to ignore the field created by that AuthenticationRequest when making the decision whether to bypass the form?

I think ignoring it makes more sense than not adding it at all. What you basically want is to pretend the form was POSTed on certain GET requests (see AuthManagerSpecialPage::setRequest; it's already used to handle the return after an REDIRECT authentication response), and that way someone can still decide what the default value for the remember flag should be (e.g. MobileFrontend sets it to true), Also you won't trip up extensions which just assume it will be there and don't check.

  1. Do you have any additional thoughts on the approach to bypassing the form before I dive in?

I also have one lingering doubt about the PluggableAuth implementation. I created PluggableAuthBeginAuthenticationRequest to trigger the PluggableAuth code, but I found that the request was being filtered out since it did not contribute any fields. So I added the dummy hidden pluggableauth field in it to force it not to be filtered. Is there some other approach that would be preferable?

Override loadFromSubmission to always return true. But it doesn't seem like you need that request at all.

And, I'm having issues figure out the correct way to include my authentication provider.

The key is AuthManagerAutoConfig (extension.json has a whitelist for globals).

I tried adding a $wgExtensionFunctions[] function to set the primaryauth value, but that code never was executed.

The configuration might already be loaded at that point.

Finally, just a confession that I didn't 'completely' create my own endpoint for the redirect. I'm using an UnlistedSpecialPage, since I do still need the MediaWiki context to be able to autoload the code for my plugins. But, it seems that it might be at least somewhat preferable to redirect from the special page execute() than from the middle of the authentication workflow.

Definitely preferable.

Apart from needing the autoloader, a new endpoint would probably force wiki admins to update URL rewrite rules, which sucks. I don't see a good solution for that - we could add a dedicated hook, but it's hard to find a time where it's late enough that stuff you might need (e.g. session handling) is available but nothing depends on a proper teardown yet. OTOH breaking out from a special page that did not do anything before the breakout probably won't cause much too trouble.

cicalese added a comment.EditedSep 26 2016, 9:06 PM

Override loadFromSubmission to always return true. But it doesn't seem like you need that request at all.

The only reason I'm using that request is to cause my beginPrimaryAuthentication function to be called, which triggers the redirect to the unlisted special page. Is there another way to cause that function to be called?

The key is AuthManagerAutoConfig (extension.json has a whitelist for globals).

I could swear I tried that and was still getting the default providers, giving me the username/password fields. But, now it is working . . .

Finally, just a confession that I didn't 'completely' create my own endpoint for the redirect.

Definitely preferable.

Great! Thanks!

Is there a way to detect that the authentication flow caused a new user to be created? In the providerAllowsAuthenticationDataChange() function, I need to save the real name and email address of the user unconditionally when the user is first created. But, if the user already existed previously, I only save them under certain circumstances.

Tgr added a comment.Sep 26 2016, 10:42 PM

The only reason I'm using that request is to cause my beginPrimaryAuthentication function to be called, which triggers the redirect to the unlisted special page. Is there another way to cause that function to be called?

beginPrimaryAuthentication is always when authorization begins (unless some other primary provider was called first and did not abstain). Authentication requests are just a mechanism to expose/describe data to the API and to persist it through multi-step authentication; they don't have any side effects. Most providers define their own authentication request type and abstain if it is not present, but that's just a convention.

The workflow for all login/signup/link is basically

  1. iterate through all providers, call their test* method, bail if one fails
  2. iterate through all primary providers, call their begin* method
    1. while the return status is UI or REDIRECT call the provider's continue* method
    2. if the return status is PASS, jump to 3 (or bail out if user creation or autocreation is needed and it fails)
    3. if the return status is FAIL, bail out
    4. if the return status is ABSTAIN, continue with the next primary provider
  3. iterate through all secondary providers, call their begin* method
    1. while the return status is UI or REDIRECT call the provider's continue* method
    2. if the return status is FAIL (this is only allowed on login), bail out
    3. if the return status is PASS/ABSTAIN, continue with the next secondary provider

(Or maybe we special-cased getAuthenticationRequests() returning an empty array and I forgot about it?)

Is there a way to detect that the authentication flow caused a new user to be created? In the providerAllowsAuthenticationDataChange() function, I need to save the real name and email address of the user unconditionally when the user is first created. But, if the user already existed previously, I only save them under certain circumstances.

You can implement autoCreatedAccount or the LocalUserCreated hook. If you mean "is there a way to tell just from a User object that the user was created right now", I don't think so. providerAllowsAuthenticationDataChange will only be called when someone tries to tell a provider to do a data change, though (e.g. by using the password change page). It's not a User.php hook or anything like that. Also, real name / email are not authentication data; they are handled via providerAllowsPropertyChange, but again, that's not a hook and will not be triggered automatically when code creates/updates a User object. When providers mess with the user object directly in PrimaryAuthenticationProvider::finishAccountCreation or a similar callback, they are assumed to know what they are doing.

beginPrimaryAuthentication is always when authorization begins (unless some other primary provider was called first and did not abstain). Authentication requests are just a mechanism to expose/describe data to the API and to persist it through multi-step authentication; they don't have any side effects. Most providers define their own authentication request type and abstain if it is not present, but that's just a convention.

The workflow for all login/signup/link is basically ...

Thanks! That's very helpful! That workflow would be a great addition to https://www.mediawiki.org/wiki/Manual:SessionManager_and_AuthManager#As_a_provider.

As it turns out, I do need an authentication request to get the return to address. I could rely on one of the automatically created requests, but I figure it is safer to ensure that one exists by continuing to use PluggableAuthBeginAuthenticationRequest.

(Or maybe we special-cased getAuthenticationRequests() returning an empty array and I forgot about it?)

Nope, it seems to work OK, when an empty array is returned. I did notice, however, that when I did so, my begin function received a PasswordAuthenticationRequest in addition to the previously existent RememberMeAuthenticationRequest, even though there was no password field. Interesting.

Is there a way to detect that the authentication flow caused a new user to be created? In the providerAllowsAuthenticationDataChange() function,

That was incorrect. The code is actually in the postAuthentication() function.

I need to save the real name and email address of the user unconditionally when the user is first created. But, if the user already existed previously, I only save them under certain circumstances.

You can implement autoCreatedAccount or the LocalUserCreated hook.

I'll look into those.

If you mean "is there a way to tell just from a User object that the user was created right now", I don't think so.

Agreed. I was looking for something passed to the postAuthentication() function. But, now, I'm wondering if that's even the right function to be putting the code in. I want something after the user has been created, but while the session information is still available. I realized yesterday that the values I am trying to retrieve in postAuthentication() using $this->manager->getAuthenticationSessionData() are all blank. I was trying to add a new user flag to the session data, until I realized that none of my other values were available at that point.

Also, real name / email are not authentication data

Agreed. But, they are often provided by the external authentication service, in which case, as long as the user is not managing them on the wik (editmyprivateinfo right)i, they are saved locally by PluggableAuth.

; they are handled via providerAllowsPropertyChange, but again, that's not a hook and will not be triggered automatically when code creates/updates a User object. When providers mess with the user object directly in PrimaryAuthenticationProvider::finishAccountCreation or a similar callback, they are assumed to know what they are doing.

Oh! finshAccountCreation() looks like the function I need to save the real name and email address.

Oh! finshAccountCreation() looks like the function I need to save the real name and email address.

Nope. It is only called for account creation, not auto creation.

Tgr added a comment.Sep 27 2016, 5:26 PM

That was incorrect. The code is actually in the postAuthentication() function.

That shares the same session as the rest of the loginflow (beginAuthentication etc) so you can just use AuthManager::setAuthenticationSessionData.

Agreed. I was looking for something passed to the postAuthentication() function. But, now, I'm wondering if that's even the right function to be putting the code in. I want something after the user has been created, but while the session information is still available. I realized yesterday that the values I am trying to retrieve in postAuthentication() using $this->manager->getAuthenticationSessionData() are all blank. I was trying to add a new user flag to the session data, until I realized that none of my other values were available at that point.

If so, that's a bug. Session data should be removed right after postAuthentication is called.

If so, that's a bug. Session data should be removed right after postAuthentication is called.

The session data was definitely not available in postAuthentication(), but I was able to work around that by accessing it earlier in continuePrimaryAuthentication() and autoCreatedAccount(). That also solved my problem of knowing whether the user had just been created. So, I no longer need postAuthentication(), but you may want to investigate whether that is indeed a bug. I was wondering if the call to setSessionDataForUser() might somehow be interfering.

I've been unable to reproduce data set with setAuthenticationSessionData() being unavailable during postAuthentication(). Is your code somehow changing the session?

I went back and added in some test data and found that it is now available in postAuthentication(). I must have fixed whatever error was causing that behavior when I refactored my code. Sorry for the false alarm.

So, I have two remaining issues.

The first is finishing up the patch for bypassing the login form.

The second issue is related. When I get an authentication error, I am redirected back to Special:UserLogin and the error is displayed. That's good. But, I'm also getting username and password fields, I think because I'm getting a PasswordAuthenticationRequest in the request array. This is a problem, because having those fields present is confusing to the user when they are expecting to be able to login without a username and password. I can live with the Remember Me checkbox, but the other fields are a problem. A few days ago, I was only getting the Remember Me request/checkbox. Something that I have changed has caused the PasswordAuthenticationRequest to show up now as well. But, I can't find the code that instantiates it. How is it getting created, and how can I prevent this from happening? Thanks!

Check what's in $wgAuthManagerConfig or $wgAuthManagerAutoConfig, see if you've got some other provider in there. Then check to make sure you didn't somehow change your code to extend AbstractPasswordPrimaryAuthenticationProvider instead of AbstractPrimaryAuthenticationProvider or something like that.

Tgr added a comment.Sep 27 2016, 9:16 PM

Something that I have changed has caused the PasswordAuthenticationRequest to show up now as well.

If you want to debug that, it probably gets added in AuthManager::getAuthenticationRequestsInternal when it iterates through the providers, so you can check which one does it.

Check what's in $wgAuthManagerConfig or $wgAuthManagerAutoConfig, see if you've got some other provider in there. Then check to make sure you didn't somehow change your code to extend AbstractPasswordPrimaryAuthenticationProvider instead of AbstractPrimaryAuthenticationProvider or something like that.

Bingo! I used to have the following in my LocalSettings.php:

$wgAuthManagerAutoConfig['primaryauth'] = [
  PluggableAuthPrimaryAuthenticationProvider::class => [
    'class' => PluggableAuthPrimaryAuthenticationProvider::class,
    'sort' => 0
  ]
];

I also have the following in my extension.json:

"AuthManagerAutoConfig": {
  "primaryauth": {
    "PluggableAuthPrimaryAuthenticationProvider": {
      "class": "PluggableAuthPrimaryAuthenticationProvider",
      "authoritative": true,
      "sort": 0
    }
  }
},

Clearly I don' t need both, but when I remove the code in LocalSettings.php, the PasswordAuthenticationRequest appears. I'm guessing there's some modification I should make to the code in extension.json to make it go away?

I should add that I don't have any other providers included in either place.

If you want to debug that, it probably gets added in AuthManager::getAuthenticationRequestsInternal when it iterates through the providers, so you can check which one does it.

I had checked there before and didn't see it getting added in any obvious way, but perhaps it calls code elsewhere that adds it. I will liberally dose that function with debugging statements.

Thanks!

Tgr added a comment.Sep 27 2016, 9:52 PM

AuthManagerAutoConfig is an associative array so adding it once or twice should not make any difference (apart from the presence of the authoritative key and that does not do anything - it's for AbstractPasswordPrimaryAuthenticationProvider only and even for that it's in the wrong location). More likely you have some third piece of setup code somewhere which overrides the settings for PluggableAuthPrimaryAuthenticationProvider (and then is in turn overridden by LocalSettings).

OK, so DefaultSettings.php initializes $wgAuthManagerAutoConfig. My LocalSettings.php code replaced that initialization with only a single entry pointing to my PluggableAuthPrimaryAuthenticationProvider. The configuration in extension.json, on the other hand, just adds another entry to the 'primaryauth' array, keeping all of the other providers. So, is there any way to disable the password provider other than brute force in LocalSettings.php?

That is, I would like to be able optionally to completely suppress password-based login if I have another provider that can provide login. We do not want local password-based login to be a fallback.

Tgr added a comment.Sep 28 2016, 12:56 AM

Not really. You can probably use some early hook like SetupAfterCache, or you can call AuthManager::forcePrimaryAuthenticationProviders, but neither is intended to be used that way. The intention was that disabling providers should be done by the site admin, to avoid problems from many providers trying to modify each other's configuration in various ways.

I figured it out! Instead of setting AuthManagerAutoConfig at the top level in my extension.json, I needed to set AuthManagerConfig (no Auto) in the "config" part. And, I needed to include empty sections for preauth and secondaryauth. Now the username and password fields do not show up, because I'm not getting a LocalPasswordPrimaryAuthenticationProvider.

@Tgr, I just saw your comment. The downside of setting AuthManagerConfig in my extension.json is, of course, that it does to some degree take the power away from the site admin. But, they could always override that setting in their LocalSettings.php. Having it in the PluggableAuth extension.json file makes the extension work out of the box with no configuration, but setting up multiple providers at a given site would take additional configuration - which it likely would anyway.

@Tgr and @Anomie, thank you for your continued feedback. I now have tested patches for PluggableAuth (https://gerrit.wikimedia.org/r/#/c/312703/) and core (https://gerrit.wikimedia.org/r/#/c/312865) that I am satisfied with. I believe that I have addressed all major issues. Is there anything that I am forgetting? Any reasons not to merge? If the core patch is acceptable, would it be possible to backport it to MW 1.27?

Anomie added a subscriber: Legoktm.Sep 28 2016, 3:38 PM

I figured it out! Instead of setting AuthManagerAutoConfig at the top level in my extension.json, I needed to set AuthManagerConfig (no Auto) in the "config" part. And, I needed to include empty sections for preauth and secondaryauth.

If you do that, you're breaking every other authentication extension, and core features such as legacy authentication hooks, throttling, and $wgBlockDisablesLogin, and possibly also the user's potential customizations in LocalSettings.php.

Maybe @Legoktm knows another way, but I don't think in general that extension.json is designed to remove other settings in a configuration array like this.

OK, understood - other suggestions? The root problem is the need to disable the password authentication provider - and even the remember me authentication provider, if possible. It may be that some installations want local password-based authentication as a fallback with PluggableAuth, but others will expressly want to forbid local password-based authentication. Is there a flag that I'm missing to disable this? If not, can we add one? I think the problem is not so much the additive nature of the extension.json handling as the fact that the default behavior of the authentication framework cannot be controlled.

The default behavior of the authentication framework can be controlled, by the site admin.

Your best bet to screw around with the defaults is probably to use a registration callback to unset the specific providers you don't want in $wgAuthManagerAutoConfig.

Thanks! That did the trick. New patch coming.

I am working on an update to https://gerrit.wikimedia.org/r/312703 that addresses the comments on patch 11, including making PluggableAuthBeginAuthenticationRequest a ButtonAuthenticationRequest.

I'm having a problem that I'm hoping there is a simple fix to. When I have both PluggableAuth and GoogleLogin loaded, I get the login dialog with both buttons. Great! However, the beginPrimaryAuthentication() function in both PrimaryAuthenticationProviders are passed the full array of all active requests, with no indication of which button triggered the authentication flow. Both extensions use ButtonAuthenticationRequest::getRequestByName(), but that function does not help, if it is provided the full list of requests. Consequently, whichever request is listed first in the array of requests wins.

Also, I noticed that GoogleLogin always manages to put its button at the bottom of the form. If the username and password fields are present, the button for PluggableAuth is ending up below those fields, but above the default login button, which is confusing for the user. I'm sure there's some code in GoogleLogin that positions the button correctly, but I have not been able to find it yet. If you happen to know off hand, I'd appreciate a nudge in the correct direction.

Thanks!!

Tgr added a comment.Sep 30 2016, 8:21 PM

I'm having a problem that I'm hoping there is a simple fix to. When I have both PluggableAuth and GoogleLogin loaded, I get the login dialog with both buttons. Great! However, the beginPrimaryAuthentication() function in both PrimaryAuthenticationProviders are passed the full array of all active requests, with no indication of which button triggered the authentication flow. Both extensions use ButtonAuthenticationRequest::getRequestByName(), but that function does not help, if it is provided the full list of requests. Consequently, whichever request is listed first in the array of requests wins.

That would be a bug. Can you add a failing test case to ButtonAuthenticationRequestTest::testGetRequestByName?

Also, I noticed that GoogleLogin always manages to put its button at the bottom of the form.

That's done via the AuthChangeFormFields hook.

That would be a bug. Can you add a failing test case to ButtonAuthenticationRequestTest::testGetRequestByName?

I think the bug would actually be in AuthManager, not ButtonAuthenticationRequest. Either beginPrimaryAuthentication() - and consequently getRequestByName() - should only be passed the requests that are relevant to the button being pressed, or those functions needs to be provided information to distinguish which requests are relevant and which are not. I can't think of how to formulate a test for that in ButtonAuthentiationRequestTest. Perhaps I could in AuthManagerTest, but I'd have to figure out how to simulate a button press. But, before I do that, how is the filtering of the requests by button press supposed to happen?

That's done via the AuthChangeFormFields hook.

Thanks.

Tgr added a comment.Oct 1 2016, 1:09 AM

I think the bug would actually be in AuthManager, not ButtonAuthenticationRequest. Either beginPrimaryAuthentication() - and consequently getRequestByName() - should only be passed the requests that are relevant to the button being pressed, or those functions needs to be provided information to distinguish which requests are relevant and which are not. I can't think of how to formulate a test for that in ButtonAuthentiationRequestTest. Perhaps I could in AuthManagerTest, but I'd have to figure out how to simulate a button press. But, before I do that, how is the filtering of the requests by button press supposed to happen?

beginPrimaryAuthentication should know what requests are relevant to it and select it from the array. ButtonAuthenticationRequest::getRequestByName is a helper function to do that (just like AuthenticationRequest::getRequestByClass). So e.g. [https://github.com/wikimedia/mediawiki-extensions-GoogleLogin/blob/master/includes/auth/GooglePrimaryAuthenticationProvider.php#L300-L303 this GoogleLogin code] tells you whether the respective button was pressed (it relies on the fact that by default no request object will be created if all fields of the given request type have been left empty, or in the case of a button, unpressed). If that does not work right for multiple buttons, capture what data is being used in AuthenticationRequest::loadRequestsFromSubmission, and then create a test case with something like

$reqs = AuthenticationRequest::loadRequestsFromSubmission( $data );
$req = ButtonAuthenticationRequest::getRequestByName( $reqs, $name );
...assert $req...

Thanks! That helped a lot! The problem was in PluggableAuthBeginAuthenticationRequest. When I converted it to a ButtonAuthenticationRequest, I did not remove its loadFromSubmission() function, which was always returning true. That was needed before the button was added, because it had no other fields, so it was always getting filtered out. It's all starting to make sense now. So, there's no problem in core.

I found a second bug, this time in my code to bypass the login page. I was not setting the name of the button in the request if the login page was bypassed, so the button-triggered authentication providers did not know to handle the authentication request. I just uploaded a new version of my core patch to fix that. I will also soon be uploading a new version of my PluggableAuth patch that has the button added. I'm really glad you had me add that button, since it surfaced this issue. And, thanks for all of your help getting to this point.

Tgr added a comment.Oct 4 2016, 1:05 AM

Do you want to speed up the review by testing the backports? I don't have 1.27 set up. These scenarios should cover everything, I think:

  • login with PluggableAuth as the only provider
  • login with PluggableAuth and GoogleLogin
  • login with password change (set $wgInvalidPasswordReset to true and $wgPasswordPolicy['policies']['user']['MinimalPasswordLength'] = 1000; or something) - should have a "skip" and "continue" button
  • password change via Special:Preferences

Sure thing! I've already tested the first two cases as well as various combinations with LocalPassword. I'll check the last two now.

  • I got the skip and continue buttons when I tried to log in with a shorter than 1000 password
  • I tried to change the password via Special:Preferences, and it correctly complained that it wasn't 1000 characters - was I looking for something else here?

Any more comments on the PluggableAuth patch before I merge? I have appreciated all of the feedback, and it is much better code for that review. The code is tested in a variety of configurations, and the number/complexity of the comments seems to have abated. If there are no further comments, I will go ahead and merge.

It would be great to see the core patches merged soon, too. Although PluggableAuth will work without them, having the buttons work correctly and the login screen bypass functional are necessary before any production deployments on my end.

Thanks!

Tgr set Security to Software security bug.Oct 5 2016, 6:58 PM
Tgr added a project: Security.
Tgr changed the visibility from "Public (No Login Required)" to "Custom Policy".
This comment was removed by Tgr.
Tgr changed the visibility from "Custom Policy" to "Public (No Login Required)".Oct 5 2016, 6:59 PM
Tgr changed Security from Software security bug to None.

Argh, sorry, clicked the wrong tab.

Oh, that's good. because I was totally confused by your comment! :-)

Change 312703 merged by jenkins-bot:
Update for MW 1.27

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

I'm waiting on code review and merge for https://gerrit.wikimedia.org/r/#/c/312865/, then a backport of that patch to REL1_27. Once that is done, I will be able to close this task. Any help in getting that patch merged would be much appreciated.

Change 317152 had a related patch set uploaded (by Cicalese):
Update for MW 1.27

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

Change 317152 merged by jenkins-bot:
Update for MW 1.27

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

Manilal added a subscriber: Manilal.EditedNov 22 2016, 11:27 AM

I'm not sure whether this is the right place to ask this question. We are running a MediaWiki-1.27 instance with PluggableAuth e and SimpleSAMLphp extensions. Everything works as expected from a technical perspective, but several users have reported usability issues in the login functionality.

Currently, when the user clicks on the Log In link, it takes the user to Special:UserLogin page which has the buttons as shown below


Normally, most of the first-time users click on the Log In button and nothing happens. The buttons are confusing and most of the user doesn't have a clue on which one to click. They are also completely unfamiliar with the word PluggableAuth (majority of our users are non-technical people).

It would be nice if you could implement one of the following features:

  1. If $wgPluggableAuth_EnableLocalLogin is false and SimpleSAMLphp is the sole authentication plugin, then mediawiki should take the user directly to the SimpleSAMLphp login page instead of the intermediate page with the buttons.
  2. If the above is not possible, then there should be some config item which will help administrators to set a custom label for the PluggableAuth Button. This way we could change the button label to Login to XYZ Single Sign-On, instead of the generic label.

I would prefer the first one as it would be much more easier for end-users. I'm new to MediaWiki API, but I can also help you to add this feature if you can provide guidance.

Thank you for your feedback. I'm glad that the extension is working well for you from a technical perspective.

If $wgPluggableAuth_EnableLocalLogin is false and SimpleSAMLphp is the sole authentication plugin, then mediawiki should take the user directly to the SimpleSAMLphp login page instead of the intermediate page with the buttons.

I agree completely. That is the purpose of the following patch to MediaWiki core: https://gerrit.wikimedia.org/r/#/c/312865/. I have been waiting for security review of that patch so that it can be merged. I need that patch to be merged before I'm able to upgrade our wiki farms to MediaWiki 1.27, so it is critical to me.

If the above is not possible, then there should be some config item which will help administrators to set a custom label for the PluggableAuth Button. This way we could change the button label to Login to XYZ Single Sign-On, instead of the generic label.

That button text can be overridden in the wiki by putting the desired text on page MediaWiki:Pluggableauth-loginbutton-label. I have been considering adding a configuration variable to override that message text, which would be useful on wiki farms so that page would not need to be edited on every wiki. I agree that the default text isn't very useful, but it is unclear that there is a better default, since the text would depend very much upon the situation in which it would be used.

Tgr added a comment.Nov 22 2016, 7:19 PM

I agree completely. That is the purpose of the following patch to MediaWiki core: https://gerrit.wikimedia.org/r/#/c/312865/. I have been waiting for security review of that patch so that it can be merged. I need that patch to be merged before I'm able to upgrade our wiki farms to MediaWiki 1.27, so it is critical to me.

I apologize for the delay. The review was scheduled for the week before last but then things happened. I'll see if things are back to normal by now.

@cicalese Thanks for your quick response. If the first suggestion is implemented, then I won't bother about changing the button label. I will test it again as soon as the code review is completed and merged to upstream.

Change 312865 merged by jenkins-bot:
Bypass login page if no user input is required.

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

Change 323644 had a related patch set uploaded (by Cicalese):
Bypass login page if no user input is required.

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

@Tgr , thank you for merging https://gerrit.wikimedia.org/r/312865. I tried to cherry pick the change to REL1_27 in gerrit, but it reported a merge conflict. I believe it is because of some additional comment text in AuthenticationRequest.php. Before I try to generate a patch manually, is there an easier way? I was able to cherry pick the change in gerrit to REL1_28, so https://gerrit.wikimedia.org/r/#/c/323644/ is now awaiting review. I'm assuming that is necessary, since, although 1.28 has not yet be released, the release branch has been created. Is that so?

Thanks!

Cindy

Tgr added a comment.Nov 26 2016, 3:27 AM

I tried to cherry pick the change to REL1_27 in gerrit, but it reported a merge conflict. I believe it is because of some additional comment text in AuthenticationRequest.php. Before I try to generate a patch manually, is there an easier way?

Check out REL1_27, run git review -x 312865 (which is a normal cherry-pick, just with nicer parameters), resolve the conflict, run git review.

Or you can just backport the patch which added the comments - probably not easier, but might be less effort in the long run if there will be future backports to the the same lines.

I was able to cherry pick the change in gerrit to REL1_28, so https://gerrit.wikimedia.org/r/#/c/323644/ is now awaiting review. I'm assuming that is necessary, since, although 1.28 has not yet be released, the release branch has been created. Is that so?

Yes.

Change 323644 merged by jenkins-bot:
Bypass login page if no user input is required.

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

Change 323646 had a related patch set uploaded (by Cicalese):
Bypass login page if no user input is required.

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

OK, the patch for REL1_27 is https://gerrit.wikimedia.org/r/#/c/323646! Once that is merged, I think this can be closed!

Change 323646 merged by jenkins-bot:
Bypass login page if no user input is required.

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

cicalese closed this task as "Resolved".Nov 26 2016, 6:12 PM

Thanks everybody with your help with this - especially @Tgr.