Page MenuHomePhabricator

Update Auth_remoteuser to use AuthManager
Closed, ResolvedPublic

Description

This should be redone as a SessionProvider. On the plus side, you'll no longer need to worry about faking up a LoginForm or auto-creating the user yourself.

Event Timeline

Tgr created this task.Aug 26 2015, 6:45 AM
Tgr updated the task description. (Show Details)
Tgr raised the priority of this task from to Needs Triage.
Tgr added subscribers: Aklapper, Tgr.
Liuxinyu970226 added a subscriber: Liuxinyu970226.
Anomie updated the task description. (Show Details)May 13 2016, 7:19 PM
Xdaveyx added a subscriber: Xdaveyx.Jul 7 2016, 9:07 AM

We've written a new remote authentication extension: https://github.com/tentwentyfour/remoteauth-ng

It's rather a new extension than a rewrite so we were wondering whether it'd make sense to push this to the same repo or not.

Related question, is the procedure for developer access still through https://www.mediawiki.org/wiki/Developer_access or has the process changed since Phabricator has been put in place?

Anomie added a subscriber: Anomie.Aug 30 2016, 4:05 PM

We've written a new remote authentication extension: https://github.com/tentwentyfour/remoteauth-ng

It's rather a new extension than a rewrite so we were wondering whether it'd make sense to push this to the same repo or not.

That's up to you and the maintainers of the existing extension.

Related question, is the procedure for developer access still through https://www.mediawiki.org/wiki/Developer_access or has the process changed since Phabricator has been put in place?

That is still the current procedure, as far as I know.

That's up to you and the maintainers of the existing extension.

All right. We already contacted them, we'll wait a few days for their answer. (But the latest non-i18n commit on the repo is from 2012, so our hopes are not too high)

Related question, is the procedure for developer access still through https://www.mediawiki.org/wiki/Developer_access or has the process changed since Phabricator has been put in place?

That is still the current procedure, as far as I know.

Great, thanks!

It needs a rewrite anyways. I'll try and take a look this weekend.

It will probably need a number of configuration options given how flexible the old one is.

gmr added a subscriber: gmr.Aug 31 2016, 6:57 AM

Took a brief look and I noticed it is licensed under Apache 2.0. I like that license, but it isn't compatible with GPL 2.0 which MediaWiki is licensed under. Any rewrite to replace this extension will need to be licensed under GPL 2.0.

Took a brief look and I noticed it is licensed under Apache 2.0. I like that license, but it isn't compatible with GPL 2.0 which MediaWiki is licensed under. Any rewrite to replace this extension will need to be licensed under GPL 2.0.

Just to clarify, MediaWiki is GPL-2.0 *or later*, and Apache 2.0 is compatible with GPL 3.0, so it's fine.

kwisatz added a comment.EditedAug 31 2016, 7:09 AM

Hmm... From what I found on extension licenses, it only needs to be a GPLv2 compatible license. The wiki page explicitly lists Apache 2.0 among those.

Not that we have a very strong feeling about this, but just to clarify why we picked Apache 2.0

Thanks for the followup. Apache 2.0 is compatible with GPL 3.0 and MediaWiki can be distributed under GPL 3.0, but not everything GPL 2.0 can be distributed under GPL 3.0.

An extension to replace the current extension needs to be licensed the same as MediaWiki.

Licensing and customization are the important pieces I'm looking for. Some systems send the users real name and email address, some use headers instead of environment variables, some places the username is the email and just needs a domain appended (or removed), etc.

This extension gets used a lot of different ways, so making it easy to configure is important.

An extension to replace the current extension needs to be licensed the same as MediaWiki.

Could you please go into more detail why that is the case?
I based my assumption that Apache 2.0 was fine on the statement on this page:
https://www.mediawiki.org/wiki/Manual:Developing_extensions#License

This extension gets used a lot of different ways, so making it easy to configure is important.

Oh, and yes, we're looking into that.

Hello.
Pretty much on the same day like kwisatz i finished a rewrite of Auth_remoteuser to use the SessionProvider API.
You can find it here: https://github.com/noris-network/mediawiki-extensions-sessionprovider-remoteuser
I hope i managed to keep it feature compatible with the old extension.

Cheers,
Andi.

Thanks Andreasfink! You could not have provided this at a better time. I was struggling with this yesterday and your code solves my problem today.

@kwisatz @Andreasfink Thanks for the contributions. I'm working on this today through Monday (long weekend). I hope to get a review up in Gerrit to receive comments during this time.

@kwisatz In the end it is my preference for this extension, but as I explained above Apache 2.0 is not compatible with GPL 2.0 (only 3.0) and the current extension is GPL 2.0+ licensed.

I didn't get as far as I wanted, but I have posted my progress so far.

https://gerrit.wikimedia.org/r/#/c/308683/

I expect there are bugs in there as I have only started testing it out, but please do leave feedback on any issues you see.

Also, it might be a few days before I get back to this, but I have some wikis I need to update (so I won't forget).

Hi.
Any updates on this? I read through the comments on gerrit but i am afraid i have to admit that this goes much too deep into MediaWiki internals.
I never worked with it before and i do not think i can help you out on the open remarks.

Change 327891 had a related patch set uploaded (by Jrchamp):
Bug T110292 - Update Auth_remoteuser to use AuthManager

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

Code has been submitted, reviewed and rewritten. What's the next step to get this moving along?

jrchamp assigned this task to Enst80.Mar 7 2017, 3:42 PM
jrchamp added a subscriber: jrchamp.

Stefan has done an amazing job with this. If anyone knows how we can get this change merged, please respond. Thank you.

Thanks everyone. I haven't had a chance to look yet, but I will make time
this weekend.

Enst80 added a comment.Mar 7 2017, 6:01 PM

Thank you @GICodeWarrior for responding to my email and @jrchamp for still reviewing my current patch fixes.

I submitted Patch Set 10 (fixed another code example bug in the README.md) and thinking of it now as ready to be released as version 2.0.0 (when @GICodeWarrior has reviewed it).

Hi Stefan,

Thanks again for your work on this extension.

I have a few questions/requests before merging:

I don't see code to disable the edit options on properties such as real name or email address in the user interface. I see some code that will override user changes, but that's different. It creates a confusing user experience if you can make changes but they are overridden, and it causes an unnecessary DB write on every request. Being able to keep data up-to-date is useful in itself, but it shouldn't run on every request. Can you fix these issues?

I see you've created a new hook Auth_remoteuser_processEnvVar. I like that idea, but I'm a little confused by the name. It appears to only process the username field and not any other environment variables. Maybe it should be renamed to _filterUserName, or _normalizeUserName, or similar?

A couple of the other configuration options seem confusing. $wgAuth_remoteuser_EnvVarName should be _UserNameEnvVar or something like that. I think $wgAuth_remoteuser_ChangeUser should be _AllowAlternateAuth, or maybe _AllowLocalAuth (not sure if it allows just local or arbitrary fallback). $wgAuth_remoteuser_FacetUserName should be something like _UserNameFilters or similar. If you have other ideas to make these a bit more clear, let me know.

In addition to supporting named environment variables, it would be great if we supported setting the username directly, maybe similar to how you accept properties directly. Some Apache modules for example will supply the username as a header instead of an environment variable. This is a new feature, but since you're creating an entirely new configuration format I think it's important to support this use case.

In order to make upgrades as easy as possible, we need to include some sort of configuration shim that allows the old-style configuration to be used. Maybe stick it in a separate file to include/enable it? Or add a config flag to support the old config options?

One item I was trying to add is the ability to redirect the logout action to another URL. This allows admins to send users to a single-sign-out or other similar page on logout. However, this is a new feature and won't affect the configuration format (only add to it). So this feature can wait until a later time.

Please let me know if you have any questions. I am generally busy during weekdays, but I will do my best to respond in a timely fashion.

Thanks,
Rusty.

Thanks @GICodeWarrior for valueable feedback. I am busy this week, but didn't had the time to finish the implementation of your worthwhile remarks. I'll upload a new Patch Set next week.

Sounds great, thanks.

Enst80 added a comment.EditedMar 24 2017, 5:54 PM

Okay, that was a rather big overhaul of a single PatchSet. I still have some points i wanted to be in before release (merge). But on the other hand i want your feedback of the things which are in already. So i uploaded the current state of my rework (PatchSet 12). So lot's of stuff to review for you at least. ;-)

What's in now:

  • Full support of legacy parameters!
  • Renamed configuration parameters to comply with legacy parameters.
  • Renamed nearly everything to become more clear what gets done by the function/variable.
  • User properties (preference settings) do support closures now to defer their evaluation till it is really required.
  • Users preference settings reflect the forced properties (disabled in the user interface) now
  • Support for arbitrary remote user name sources/providers (as you wished @GICodeWarrior ) . This let's to an extension which can be really useful for other extensions do inherit from Auth_remoteuser (maybe NTLM or Ldap Auth etc.) ;-)

What's still missing:

  • Support for different 'logout' and 'password change' links. I startet on this already, but didn't got it in time before the weekend ^^
  • Composer support? (eventually)
  • Handling of 'AutoCreateUser' when global rights are set to forbid this ($wgGroupPermissions['*']['createaccount'] = false;) --- Thanks to C.J.M.N. who pointed me on that issue (by writing me an email - thank you).

Thanks @Enst80. Lots of great updates. :-D

I left a couple comments on the patchset. If you have any questions, just let me know.

I wouldn't worry about the logout redirect for this change, it will be easier to stick that in as a separate change anyways.

Enst80 added a comment.EditedApr 9 2017, 11:06 PM

Phew, long time for an update ;-) But here it is: latest PatchSet - Ready to be released (after review and bugfixing)

Auth_remoteuser v2.0.0:

  • README with full description
  • finished refactoring and finalized naming scheme
  • descriptive parameter names
  • full legacy parameter support
  • handling of MediaWikis global permissions for account creation
  • arbitrary remote user name sources
  • useful/powerful default user name filters with regex support
  • flexible user preference settings
  • nearly all parameters support closures to defer their evaluation
  • varying logout urls
  • allows user switching in contrast to versions prior 2.0.0
  • message logging for debugging purposes

Change 327891 merged by jenkins-bot:
[mediawiki/extensions/Auth_remoteuser@master] Bug T110292 - Update Auth_remoteuser to use AuthManager

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

Hooray! Thank you both for making this milestone possible.

As far as possible next steps:

  • If Mediawiki 1.27 and newer require AuthManager implementations, does it make sense to replace the REL1_27 and REL1_28 branches with this code?
  • Maybe update the documentation and version information: https://www.mediawiki.org/wiki/Extension:Auth_remoteuser
  • Mark this task resolved
  • Celebrate! 🍰
GICodeWarrior closed this task as Resolved.

Thanks @Enst80 and @jrchamp for making this happen.

It would be useful to hear about some successful (or unsuccessful) deployments of the new code before backporting, but I don't have a strong opinion on the order of operations.

I've added subtasks for those items.

Change 349159 had a related patch set uploaded (by Enst80):
[mediawiki/extensions/Auth_remoteuser@REL1_27] Bug T110292 - Update Auth_remoteuser to use AuthManager

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

Change 349273 had a related patch set uploaded (by Enst80):
[mediawiki/extensions/Auth_remoteuser@REL1_28] Bug T110292 - Update Auth_remoteuser to use AuthManager

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

Change 349273 merged by jenkins-bot:
[mediawiki/extensions/Auth_remoteuser@REL1_28] Bug T110292 - Update Auth_remoteuser to use AuthManager

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

Change 349159 merged by jenkins-bot:
[mediawiki/extensions/Auth_remoteuser@REL1_27] Bug T110292 - Update Auth_remoteuser to use AuthManager

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