Page MenuHomePhabricator

Migrate OAuth extension back from wikimedia/oauth2-server fork to upstream
Open, Stalled, HighPublic

Description

In 2020, we in the Core Platform Team forked the thephpleague/oauth2-server package as wikimedia/oauth2-server to incorporate changes while waiting for upstream. They don't seem likely to necessarily include these changes (and one has been declined already), so we need to decide on a longer term solution.

Relevant PR:

See also: T255034: Wikimedia API Gateway Long-term Use

Related Objects

Event Timeline

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

The other option is to fully fork it. Change the name, update the readme, register it in packagist.

The other option is to fully fork it. Change the name, update the readme, register it in packagist.

+1, even as a temporary solution that would be an improvement IMO. The current situation is confusing, this is the only mediawiki/vendor package that uses commit pinning, the syntax for which isn't really intuitive - that just caused an outage (T321160).

There is apparently an OAuth-specific JWT spec now, which matches envoy's expectations. I left a comment in #1137.

(Also, as pointed out in that discussion, we could just override the behavior in a subclass.)

Aklapper renamed this task from Migrate away from wikimedia/oauth2-server fork to upstream to [API Gateway] Migrate away from wikimedia/oauth2-server fork to upstream.Apr 1 2024, 8:10 AM
Aklapper removed a subscriber: Pchelolo.

As API Gateway is nowadays owned by ServiceOps new, adding the ServiceOps new project tag to open API Gateway tasks tagged with the deprecated/archived "Platform Team Initiatives (API Gateway)" tag at https://phabricator.wikimedia.org/project/profile/4321/, as part of Phabricator Housekeeping.

Krinkle renamed this task from [API Gateway] Migrate away from wikimedia/oauth2-server fork to upstream to Migrate OAuth extension back from wikimedia/oauth2-server fork to upstream.Apr 15 2024, 3:24 PM
Krinkle updated the task description. (Show Details)
Krinkle updated the task description. (Show Details)

I'm trying to make sense of the exact fork situation, without much success. The OAuth extension's composer.json file pins 61d770 (since rEOAUaf26a29b05e2: composer.json: Pin league/oauth2-server to commit, which seems like a step backwards; before that, it was using the fork's v9.0.0-alpha branch). Github says that commit is on the v9.0.0-alpha branch, but it doesn't actually show up in the branch history. Pull request #2 says 61d770 was merged into v9.0.0-alpha but I can't find it there. (It doesn't help that our own commits aren't on the top of the branch history like one would expect, but interleaved with upstream in what looks like a chronological order, going back hundreds of commits.) Also it looks like at some point v9.0.0-alpha got merged into master, and then master got merged back into v9.0.0-alpha? The whole thing is very confusing. Here's the first few lines of git log --graph, just to give an idea of how messed-up it is:

*   commit 77b9a8fe727e9132a9961f12114b3b7ba2545c1c (HEAD -> v9.0.0-alpha, origin/v9.0.0-alpha, origin/HEAD)
|\  Merge: a68eb58 fd1790c
| | Author: Sam Reed <reedy@wikimedia.org>
| | Date:   2022-08-02
| | 
| |     Merge pull request #5 from wikimedia/master
| |   
| *   commit fd1790cf94df98a7d2e4b8a39f03b8cf05b3a69f (origin/master)
| |\  Merge: 9c1b8e9 a68eb58
| |/  Author: Sam Reed <reedy@wikimedia.org>
|/|   Date:   2022-08-02
| |   
| |       Merge branch 'v9.0.0-alpha' into master
| |   
* |   commit a68eb585ce4ce095032503afe78057ae26e3b892
|\ \  Merge: 58fdace 0c2f32c
| | | Author: Sam Reed <sam@reedyboy.net>
| | | Date:   2022-04-17
| | | 
| | |     Merge branch 'thephpleague:master' into v9.0.0-alpha
| | |   
* | |   commit 58fdace1b2b33b0d6c40f426658a898a7566bd25
|\ \ \  Merge: 61d770d a3b4782
| | | | Author: Sam Reed <reedy@wikimedia.org>
| | | | Date:   2022-02-08
| | | | 
| | | |     Merge pull request #3 from wikimedia/update2_v9.0.0-alpha
| | | |     
| | | |     Update v9.0.0 alpha to thephpleague/oauth2-server master@85bb8de
| | | |   
| * | |   commit a3b47824653494811f1f002ae96f4a6ccd621ea9
| |\ \ \  Merge: efc8ad2 f5698a3
| | | | | Author: Reedy <reedy@wikimedia.org>
| | | | | Date:   2022-02-08
| | | | | 
| | | | |     Merge tag '8.3.3' into update2_v9.0.0-alpha
| | | | | 
| * | | | commit efc8ad29189f42ee30b10e55528ec76f76e99a6a
|/| | | | Merge: 61d770d 85bb8de
| | | | | Author: Reedy <reedy@wikimedia.org>
| | | | | Date:   2021-10-02
| | | | | 
| | | | |     Merge remote-tracking branch 'upstream/master' into v9.0.0-alpha
| | | | |   
* | | | |   commit 61d770dc284898ea2905d66e12f8f7e5f6664092
|\ \ \ \ \  Merge: a00cc3c f16a81a
| | | | | | Author: Clara <candrewwani@gmail.com>
| | | | | | Date:   2021-03-04
| | | | | | 
| | | | | |     Merge pull request #2 from wikimedia/update_v9.0.0-alpha
| | | | | |     
| | | | | |     Update v9.0.0 alpha
| | | | | | 
| * | | | | commit f16a81a5f7d7b878009f0790ce55e3244b934839
|/| | | | | Merge: a00cc3c 15abf4a
| | | | | | Author: Clara Andrew-Wani <candrewwani@gmail.com>
| | | | | | Date:   2021-01-13
| | | | | | 
| | | | | |     Merge remote-tracking branch 'upstream/master' into update_v9.0.0-alpha
| | | | | |     
| | | | | |     # Conflicts:
| | | | | |     #       src/Entities/Traits/AccessTokenTrait.php
| | | | | |   
* | | | | |   commit a00cc3c27428034e21b64e5264cd5fcaf2ed6072
|\ \ \ \ \ \  Merge: 2119d56 6d7ad64
| | | | | | | Author: Clara <candrewwani@gmail.com>
| | | | | | | Date:   2020-08-31
| | | | | | | 
| | | | | | |     Merge pull request #1 from wikimedia/issuer
| | | | | | |     
| | | | | | |     Add support for specifying iss claim on access token JWT
| | | | | | | 
| * | | | | | commit 6d7ad649b2d92c38aaa909d5f838d6d49576399e (origin/dev-issuer)
|/ / / / / /  Author: Petr Pchelko <ppchelko@wikimedia.org>
| | | | | |   Date:   2020-08-27
| | | | | |   
| | | | | |       Add support for specifying iss claim on access token JWT
| | | | | |   
* | | | | |   commit 2119d56f6f4376567645b96d71ff623d170b894f
|\ \ \ \ \ \  Merge: 340d1ab d240052
| | | | | | | Author: Clara Andrew-Wani <candrewwani@gmail.com>
| | | | | | | Date:   2020-08-11
| | | | | | | 
| | | | | | |     Merging changes from https://github.com/thephpleague/oauth2-server/pull/1122
| | | | | | | 
| * | | | | | commit d2400529527105a5a08aeb1980aba25829858aef
| | | | | | | Author: Sebastian Kroczek <me@xbug.de>
| | | | | | | Date:   2020-07-27
| | | | | | | 
| | | | | | |     Changes copyright
| | | | | | | 
| * | | | | | commit 40fed670c31f00d0b70c3c465b3449b099eb34f4
| | | | | | | Author: Andrew Millington <andrew@noexceptions.io>
| | | | | | | Date:   2020-07-27
| | | | | | | 
| | | | | | |     Revert formatting change
| | | | | | | 
| * | | | | | commit 74a934e7921f83053925df40ff443f70888c0f55
| | | | | | | Author: Andrew Millington <andrew@noexceptions.io>
| | | | | | | Date:   2020-07-27

I think the actual difference from upstream is the commit Add support for specifying iss claim on access token JWT and some version of the private claims pull request (which one though? it got updates multiple times in the last few years), but it would be nice if someone who was involved could help decipher it.

Wrt the PRs in the task description:

https://github.com/thephpleague/oauth2-server/pull/1122 - open

No movement from upstream.

https://github.com/thephpleague/oauth2-server/pull/1138 - declined...

It seems like upstream is planning to do some version of this but hasn't yet.

IMO the next step would be to make it clear how the fork differs from upstream, write instructions on how to recreate it (as a FORK.md file or something), and switch from periodically merging upstream into the fork to starting with the upstream version for each release and then applying our patches (and squashing the private claims patches into one to make it more manageable). And switch back OAuth's composer JSON from pinning a commit to using a branch.

Yeah, from memory, the short version is that there's 2 or 3 patches ontop, which it would look like you have deciphered.

I believe (it's over 3 years ago) that after bringing in upstream changes, the head of the branch actually didn't work for us anymore, so it got pinned against an older version before it was pulled in.

But the immediate issue here is not v9.0.0-alpha being outdated, it's that currently our composer.json (both vendor and the extension) pin it to a specific commit that was the state of v9.0.0-alpha circa 2021 and is way behind current v9.0.0-alpha. Using v9.0.0-alpha without commit-level pinning would be fine.

And as I said above...

It's pinned to the exact hash in wikimedia/oauth2-server (rEOAUaf26a29b05e2: composer.json: Pin league/oauth2-server to commit) because when I tried to update it after a rebase, it failed with some errors that looking at the vendor patch, I tagged in T302757: OAuth test failures. https://github.com/thephpleague/oauth2-server/issues/1266 as the upstream which was closed without anything really being resolved, even months later...

Removing the hash level pinning might fix this exact issue, but it's not going to pass CI due to the aforementioned issues.

The failure being T302757: OAuth test failures. And then trying to (unsuccessfully) report/fix this upstream https://github.com/thephpleague/oauth2-server/issues/1266 and https://github.com/thephpleague/oauth2-server/pull/1138

I think we should fix this by the time we switch to PHP 8.3. It would be nice to move to library versions which were tested on that version. At least for lcobucci/jwt (which is pinned to an old version because the old version of oauth-server requires that) that's not the case today. (We can fix that without unforking, by just merging in some upstream changes, but I'm not sure it would be less effort, and we'd just be rolling the ball forward.)

With T399243: Support JWT generation for session tokens in MediaWiki core adding JWT generation to core, core and wikimedia/oauth2-server now both depend on lcobucci/jwt and will eventually get into a version conflict, so that's one more reason to clean this up.

Reedy triaged this task as High priority.Dec 31 2025, 4:39 PM

2026 is basically upon us...

This has been blocking stuff for a while, never mind this task being open for coming up 5.5 years. And it looks like it's starting to get in the way of numerous other things too.

IMO the next step would be to make it clear how the fork differs from upstream, write instructions on how to recreate it (as a FORK.md file or something), and switch from periodically merging upstream into the fork to starting with the upstream version for each release and then applying our patches (and squashing the private claims patches into one to make it more manageable). And switch back OAuth's composer JSON from pinning a commit to using a branch.

I believe most of the changes are either in the originally referenced PR.

https://github.com/thephpleague/oauth2-server/pull/1122 is a stack of 40 commits (ugh), whereas https://github.com/thephpleague/oauth2-server/pull/1138 is "just" one.

Your comment at https://github.com/thephpleague/oauth2-server/issues/1137#issuecomment-1474717945 seems to have been ignored (for 2.5 years), which pointed out one of the changes we wanted was in the spec now...

Which has been forked in https://github.com/thephpleague/oauth2-server/issues/1434...


As of writing, https://github.com/thephpleague/oauth2-server/releases/tag/9.3.0 is the latest release.

Would taking that, and trying to re-apply the two PR on it be a good way forward?

I've created a currently empty https://github.com/wikimedia/oauth2-server/tree/9.3.0-WMF based on https://github.com/thephpleague/oauth2-server/releases/tag/9.3.0, with currently no changes.

I've updated/rebased/whatever (mostly fixed so their CI passes) https://github.com/thephpleague/oauth2-server/pull/1138 into https://github.com/wikimedia/oauth2-server/pull/19 as a PR against https://github.com/wikimedia/oauth2-server/tree/9.3.0-WMF

Based on https://patch-diff.githubusercontent.com/raw/thephpleague/oauth2-server/pull/1138.patch

I also did some CR at https://github.com/thephpleague/oauth2-server/pull/1138/changes as there were a few broken changes (some in examples, so not a big deal)...

Also, phpstan is so annoying as a static analyser, it seems unable to look at a guarding conditional... Ignore it is!

 ------ ----------------------------------------------------------------- 
 Line   src/Entities/Traits/AccessTokenTrait.php (in context of class    
        LeagueTests\Stubs\AccessTokenEntity)                             
------ ----------------------------------------------------------------- 
 76     Parameter #1 $issuer of method Lcobucci\JWT\Builder::issuedBy()  
        expects non-empty-string, string given.                          
------ -----------------------------------------------------------------
------ ----------------------------------------------------------------- 
 Line   src/Entities/Traits/AccessTokenTrait.php (in context of class    
        LeagueTests\Stubs\AccessTokenEntity)                             
------ ----------------------------------------------------------------- 
 76     Casting to string something that's already string.               
 76     Parameter #1 $issuer of method Lcobucci\JWT\Builder::issuedBy()  
        expects non-empty-string, string given.                          
------ -----------------------------------------------------------------

https://github.com/wikimedia/oauth2-server/pull/19 is passing their CI with minimal disabling/exclusions in place.

https://github.com/thephpleague/oauth2-server/pull/1122 redone in https://github.com/wikimedia/oauth2-server/pull/20

I think both end up conflicting with each other...

So lets see if I can get both to merge in one go...

Change #1222274 had a related patch set uploaded (by Reedy; author: Reedy):

[mediawiki/vendor@master] Update league/oauth2-server and dependancies

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

Change #1222275 had a related patch set uploaded (by Reedy; author: Reedy):

[mediawiki/extensions/OAuth@master] Update league/oauth2-server

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

Change #1222278 had a related patch set uploaded (by Reedy; author: Reedy):

[mediawiki/core@master] Upgrade psr/http-message and lcobucci/jwt

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

Change #1222278 abandoned by Reedy:

[mediawiki/core@master] Upgrade psr/http-message and lcobucci/jwt

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

Change #1222279 had a related patch set uploaded (by Reedy; author: Reedy):

[mediawiki/core@master] Upgrade psr/http-message and lcobucci/jwt

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

Change #1222278 restored by Reedy:

[mediawiki/core@master] Upgrade psr/http-message and lcobucci/jwt

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

Change #1222278 abandoned by Reedy:

[mediawiki/core@master] Upgrade psr/http-message and lcobucci/jwt

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

I note that the work I've done on this doesn't actually fix the state goal of moving away from the fork... But it does move us forward (~3-3.5 years) on our version of the oauth2 server, unblocking T397068: Upgrade psr/http-message to 2.0 or later among others.

Ok, patches are up, probably need some commit summary improvements, and some RELEASE-NOTES where appropriate.

Vendor fails because of changes needed in core... Core will pass with it depending on a Wikibase patch. Another Wikibase patch depending on core (and the first Wikibase patch) prove that tests pass, it's just a dependancy cycle. OAuth passing depending on Core and down.

Vendor will need force merging, so will the Wikibase patch, but then the core patch should merge fine, and so will any other related ones...

Change #1229647 had a related patch set uploaded (by Reedy; author: Reedy):

[mediawiki/vendor@master] Update league/oauth2-server and dependancies

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

Change #1222274 abandoned by Reedy:

[mediawiki/vendor@master] Update league/oauth2-server and dependancies

Reason:

New change id - https://gerrit.wikimedia.org/r/1229647

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

Change #1229647 merged by Gergő Tisza:

[mediawiki/vendor@master] Update league/oauth2-server and dependancies

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

Change #1222279 merged by jenkins-bot:

[mediawiki/core@master] Upgrade psr/http-message and lcobucci/jwt

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

Change #1222275 merged by jenkins-bot:

[mediawiki/extensions/OAuth@master] Update league/oauth2-server and dependancies

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

Change #1239176 had a related patch set uploaded (by Reedy; author: Reedy):

[mediawiki/vendor@REL1_43] Upgrading psr/http-message (1.1 => 2.0)

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

Change #1239177 had a related patch set uploaded (by Reedy; author: Reedy):

[mediawiki/vendor@REL1_44] Upgrading psr/http-message (1.1 => 2.0)

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

Change #1239178 had a related patch set uploaded (by Reedy; author: Reedy):

[mediawiki/vendor@REL1_45] Upgrading psr/http-message (1.1 => 2.0)

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

Change #1239182 had a related patch set uploaded (by Reedy; author: Reedy):

[mediawiki/core@REL1_45] Upgrade psr/http-message and lcobucci/jwt

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

Change #1239185 had a related patch set uploaded (by Reedy; author: Reedy):

[mediawiki/core@REL1_44] Upgrade psr/http-message

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

Change #1239187 had a related patch set uploaded (by Reedy; author: Reedy):

[mediawiki/core@REL1_43] Upgrade psr/http-message

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

matmarex subscribed.

Reedy's work made it much clearer what exactly we need from the fork, and I ended up reading a lot of oauth2-server code while debugging T417839, so I feel like I know enough to give this a try.

Change #1240644 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/OAuth@master] Migrate from wikimedia/oauth2-server fork to league/oauth2-server

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

Change #1240651 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/vendor@master] Migrate from wikimedia/oauth2-server fork to league/oauth2-server

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

Change #1240651 merged by jenkins-bot:

[mediawiki/vendor@master] Migrate from wikimedia/oauth2-server fork to league/oauth2-server

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

Change #1240644 merged by jenkins-bot:

[mediawiki/extensions/OAuth@master] Migrate from wikimedia/oauth2-server fork to league/oauth2-server

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