This module is broken for me, running D9 and PHP 8. This is due to incompatibility with the underlying dependencies as opposed to PHP 8-specific code issues. This incompatibility is related to what's noted in this comment, namely that "drupal/simple_oauth": "^5.0" is not compatible with league/oauth2-server": "^8.2". They mention a workaround that pegs to a lower version of oath2-server, but this is not an option when running PHP 8.

The first error that's encountered is addressed in https://www.drupal.org/project/simple_oauth/issues/3187358#comment-13931772. However, this leads to a cascade of bugs. Specifically, adding that patch fixes ArgumentCountError: Too few arguments to function Drupal\simple_oauth\Entities\AccessTokenEntity::convertToJWT(), 0 passed in /var/www/html/drupal/vendor/league/oauth2-server/src/Entities/Traits/AccessTokenTrait.php, but then leads to another error Error: Cannot instantiate interface Lcobucci\JWT\Builder in Drupal\simple_oauth\Entities\AccessTokenEntity->convertToJWT() (line 30 of /var/www/html/drupal/web/modules/contrib/simple_oauth/src/Entities/AccessTokenEntity.php).

That error led me to https://github.com/lcobucci/jwt/discussions/744, which points out that this is an issue that's likely to surface as a result of migrating from lcobucci/jwt v3 to v4. That specific error is due to the Builder class now being an Interface instead. There are general upgrade steps outlined in https://lcobucci-jwt.readthedocs.io/en/stable/upgrading/. Deprecation notices seem to be being addressed in https://www.drupal.org/project/simple_oauth/issues/3194635.

I'd imagine the best way forward would be to peg simple_oauth to jwt v4 and make updates accordingly.

CommentFileSizeAuthor
#16 3230707-16.patch3.92 KBtaran2l
#14 3230707-14.diff3.87 KBtaran2l
#11 3230707-11.diff3.26 KBtaran2l
#5 3230707--jwt-4-compatibility--5.patch2.68 KBmrweiner
#4 3230707--jwt-4-compatibility--4.patch3.08 KBmrweiner
#3 3230707--jwt-4-compatibility--3.patch2.55 KBmrweiner

Issue fork simple_oauth-3230707

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mrweiner created an issue. See original summary.

mrweiner’s picture

mrweiner commented
Issue summary: View changes
mrweiner’s picture

mrweiner commented
Status: Active » Needs review
StatusFileSize
new 3230707--jwt-4-compatibility--3.patch2.55 KB

Attaching a patch that updates the syntax used in src/Entities/AccessTokenEntity.php to be compatible with JWT v4 per https://lcobucci-jwt.readthedocs.io/en/stable/upgrading/. Credit to @bucefal91 and @NickLediet as well for trying to address parts of this on their own, probably based on versioning that was relevant at the time.

This patch does not update composer.json requirements as I'm not sure exactly what they need to be pegged to, but I'll note thay my own composer.lock is showing oauth2-server@8.3.2 and lcobucci/jwt@4.1.4.

mrweiner’s picture

mrweiner commented
StatusFileSize
new 3230707--jwt-4-compatibility--4.patch3.08 KB

Looks like $this->getExpiryDateTime() already gives us a DateTimeImmutable, so no reason to create a new one in expiresAt(). Doing this is causing a weird unexpected character exception randomly.

mrweiner’s picture

mrweiner commented
StatusFileSize
new 3230707--jwt-4-compatibility--5.patch2.68 KB

Whoops, last one includes a composer.json change that's causing issues.

Status: Needs review » Needs work

The last submitted patch, 5: 3230707--jwt-4-compatibility--5.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mrweiner’s picture

mrweiner commented

Don't really understand why #3 passed and #5 failed when it just simplifies the expiresAt method param. Maybe somebody else has an idea.

mattjones86’s picture

mattjones86
Location 🇬🇧 GMT+0
commented

I can confirm this patch works for me on PHP8.0. Thanks!

EDIT: Also seems to have passed tests when requesting re-test.

jwwj’s picture

jwwj commented

The patch works for me as well. Though in order for all the parts to fit together while using a composer based setup I had to do the following:

composer require league/oauth2-server:"8.3.3 as 8.1.1"
composer require lcobucci/jwt:"4.1.5 as 3.4.6"
composer require steverhoades/oauth2-openid-connect-server:"2.4.0 as 1.3.0"
composer require drupal/simple_oauth:5.0.5

This gave me the latest dependency versions that work on PHP8 even though simple_oauth currently requires oauth2-server < 8.2. After that I could apply the patch in #5, and everything seems to work so far :)

mrweiner’s picture

mrweiner commented

@jwwj interesting -- I actually didn't need to manually require anything non-drupal. I do have "drupal/simple_oauth": "^5.0" and "drupal/jwt": "^1.0@RC" tho.

taran2l’s picture

taran2l
Primary language Ukrainian
Location Lviv
commented
Status: Needs work » Needs review
StatusFileSize
new 3230707-11.diff3.26 KB
  • a few more deprecations
  • bump dependency on league/oauth2-server
  • remove unneeded direct dependecy on lcobucci/jwt
taran2l’s picture

taran2l
Primary language Ukrainian
Location Lviv
commented

Status: Needs review » Needs work

The last submitted patch, 11: 3230707-11.diff, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

taran2l’s picture

taran2l
Primary language Ukrainian
Location Lviv
commented
Status: Needs work » Needs review
StatusFileSize
new 3230707-14.diff3.87 KB

Ok, new version of the league/oauth2-serverlibrary has slightly changed what errors are being thrown, see https://github.com/thephpleague/oauth2-server/commit/96b76efc5c70cdccf97...

Updated patch is here (also fixes a few code style errors)

Status: Needs review » Needs work

The last submitted patch, 14: 3230707-14.diff, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

taran2l’s picture

taran2l
Primary language Ukrainian
Location Lviv
commented
Status: Needs work » Needs review
StatusFileSize
new 3230707-16.patch3.92 KB

Response code too ...

bradjones1’s picture

bradjones1
Primary language English
commented

Applied #16 and ran the test suite locally against PHP 7.4; got a bunch of test-related deprecation notices (#3253976: Address test deprecation notices & get CI working again) but no failures. Running DrupalCI against latest Drupal core for PHP 7.4 and 8.0.

bradjones1’s picture

bradjones1
Primary language English
commented 
+++ b/composer.json
@@ -3,9 +3,8 @@
-        "league/oauth2-server": "^8.0 < 8.2",
-        "lcobucci/jwt": "^3.4",
-        "steverhoades/oauth2-openid-connect-server": "^1.1",
+        "league/oauth2-server": "^8.3",
+        "steverhoades/oauth2-openid-connect-server": "^1.3",

I think we may still want to express our own direct dependency on lcobucci/jwt since we directly implement it, but otherwise this LGTM.

bradjones1’s picture

bradjones1
Primary language English
commented
Status: Needs review » Needs work

Need to address PHP 8: https://www.drupal.org/pift-ci-job/2262963

Looks like 2.x of the OIDC library isn't really much of a BC break.

The league library is compatible with the jwt library 3.x or 4.x.

bradjones1 opened merge request !17

bradjones1’s picture

bradjones1
Primary language English
commented
Status: Needs work » Needs review
bradjones1’s picture

bradjones1
Primary language English
commented

Hrm... well now I need to figure out how to get ext-sodium installed on DrupalCI. I've pinged on Drupal Slack for an example to customize the runner.

bradjones1’s picture

bradjones1
Primary language English
commented
Status: Needs review » Postponed

PP on DrupalCI

bradjones1’s picture

bradjones1
Primary language English
commented

Note to anyone also using Commerce Authnet: #3254044: Bump lcobucci/jwt to ^4

Raised the question re: ext-sodium on Drupal Slack; doesn't look like there's any straightforward answer?

bradjones1’s picture

bradjones1
Primary language English
commented

If DrupalCI can't handle this, I will test locally and upload artifacts here; this will limit testing for the whole module so perhaps also we might need to switch to GitLab CI, which is now a thing (though I think, opt-in.)

bradjones1’s picture

bradjones1
Primary language English
commented
Status: Postponed » Needs review

Definitive: DrupalCI is going to choke on this. I think *maybe* we could disable checking platform requirements on composer install, but I kind of don't want to do that.

So I'll have to figure out something different, e.g. mirroring this to gitlab.com until Drupal's GitLab implementation catches up.

See https://drupal.slack.com/archives/C223PR743/p1639594406185700?thread_ts=...

simonbaese’s picture

simonbaese
Location Berlin
commented

Just a short feedback. Have been running #5 and later #16 with multiple different configurations during the past months without any problems. Also, the implementation looks good to go for me.

bradjones1’s picture

bradjones1
Primary language English
commented

Thanks. There are a few deprecations I'm addressing from local testing against 8.x but I should be wrapped up with that today.

bradjones1 credited bucefal91.

bradjones1’s picture

bradjones1
Primary language English
commented

bradjones1 credited e0ipso.

bradjones1’s picture

bradjones1
Primary language English
commented
Status: Needs review » Fixed

  • bradjones1 committed ca09275 on 5.x 
    Issue #3230707 by bradjones1, mrweiner, Taran2L, e0ipso, bucefal91: 5.x...
bradjones1’s picture

bradjones1
Primary language English
commented
bradjones1’s picture

bradjones1
Primary language English
commented

For anyone following this issue, please test https://www.drupal.org/project/simple_oauth/releases/5.1.0-beta1 and open new issues if there are any oversights.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.