ENH Add extension for new signing key and upgrade to CMS 5 by mikenuguid · Pull Request #31 · IanSimpson/ss-oauth2-server

mikenuguid · 2025-06-04T03:24:06Z

Adds ability to set own signing key via AuthorizationValidatorInterface . This is also a refactor of the previous approach allowing any user to customise a signing key on a project. A default validator is used by default making this backwards compatible.
Upgrade to work with CMS 5
Revert modified unit tests
Add new unit tests for extension points
Add example docs for EdDSA

- Adding a value to dynamic properties is getting flagged by newer PHP versions e.g. 8.3, add this properties to the class itself since they are cannot be inherited from parent.

- use dependency injection to add new token validator for new algorithm - add extension points for generating token - remove unnecessary code from package

- add docs for configuring EdDSA/Ed25519 access token - revert previously updated unit tests - add unit tests for new API

mikenuguid · 2025-06-23T02:28:39Z

@blueo would appreciate if I could have your feedback on this when you have time.

mikenuguid · 2025-08-31T20:56:19Z

Hey @blueo, any chance you could take a look at this PR? 🙂

blueo

I'm by no means an expert on this module but I've left a couple of questions on some implementation details

blueo · 2025-08-31T21:24:11Z

code/OauthServerController.php

+        return $this->authorizationValidator;
+    }
+
+    public function setAuthorizationValidator(?AuthorizationValidatorInterface $value): void


is there a use case for setting this to null? If not then you could probably remove ? from the type

Yes. If null is passed, the ResourceServer will use BearerTokenValidator (tied up to RSA) by default.

blueo · 2025-08-31T21:27:50Z

tests/OauthServerControllerTest.php

-        OauthServerController::config()->merge('grant_expiry_interval', 'PT1H');
-        $this->assertSame('PT1H', $oauthController::getGrantTypeExpiryInterval());
+        OauthServerController::config()->merge('grant_expiry_interval', ['PT1H']);
+        $this->assertSame('PT1H', $oauthController::getGrantTypeExpiryInterval()[0]);


what's the rationale for loosening the type on getGrantTypeExpiryInterval and allowing an array? Seems a string is what you're using here anyway and its changing the API

Can't recall unfortunately, probably a failing test. I'll have a look. Thanks.

blueo · 2025-08-31T21:29:51Z

code/OauthServerController.php

     * @param mixed $controller
     */
-    public static function getMember($controller): ?Member
+    public function getMember($controller): ?Member


this is a breaking change (which is fine so long as you're planning to publish a new version). But its also referenced on the main readme.md as how to use the module so it'd be good to update it there too

I'll double check this one too. But thanks. I'll make sure to update the doc, it's mentioned in there.

blueo · 2025-08-31T21:30:49Z

docs/eddsa.md

+### Generate a private/public key pair using Ed25519 algorithm 
+
+```shell
+openssl genpkey -algorithm Ed25519 -out private.key


looks like we should probably put this as the recommended way on the main Readme.md? it is a better default than RSA

A recommentdation yes, but probably leave RSA as default since not most application use the new algorithm (more so support it)

blueo · 2025-08-31T22:08:36Z

code/Entities/AccessTokenEntity.php

+        $token = null;
+
+        // Get token from extension (in case of different implementation than the default)
+        $this->extend('updateJWT', $token);


i'm wondering about this change. Seems that most of the parts of convertToJWT are able to be changed in other public methods eg getScopes or you can override the public initJwtConfiguration and use the setters on https://lcobucci-jwt.readthedocs.io/en/stable/configuration/ ? Do you have an example of an extension? I'm thinking eddsa is probably a better default it could be good to include that change but allow updating it with injector or similar

https://github.com/silverstripeltd/westpac-2020/blob/master/app/src/DAM/OauthServer/AccessTokenEntityExtension.php#L21

yeah looking at that implementation it seems it would be better to override the public initJwtConfiguration rather than the private convertToJWT as that detail may change in future non breaking releases given its a private method. It also looks like that implementation could be part of the module? It could be configured using yaml instead of the default sha256 version as an option?

https://github.com/thephpleague/oauth2-server/blob/8.5.x/src/Entities/Traits/AccessTokenTrait.php#L44

It also looks like that implementation could be part of the module?

@blueo Double checking, Are you thinking about adding the eddsa config but still use RSA as default?

Given the amount of changes requested, I might need to ask for more time (budget) than we initially estimated this for so I might revisit it again some time in the future. Appreciate the feedback!

yeah I'd suggest making eddsa the default and bringing that implementation from the internal module in here as rsa getting a bit old now AFAIK. but could also leave it as default with just docs on how to change. Yeah can leave the implementation to when you can get to it :)

blueo

looking pretty good but I think the extension is probably on the wrong method and ideally we could include the Eddsa version of the configuration in the module too

blueo · 2025-09-04T21:47:42Z

code/Entities/AccessTokenEntity.php

+        $token = null;
+
+        // Get token from extension (in case of different implementation than the default)
+        $this->extend('updateJWT', $token);


yeah looking at that implementation it seems it would be better to override the public initJwtConfiguration rather than the private convertToJWT as that detail may change in future non breaking releases given its a private method. It also looks like that implementation could be part of the module? It could be configured using yaml instead of the default sha256 version as an option?

https://github.com/thephpleague/oauth2-server/blob/8.5.x/src/Entities/Traits/AccessTokenTrait.php#L44

- This should validate the grant type set on the CMS and the request. Previously, we set a different grant type on the CMS from the request. - Note: Grant Type value on the CMS should be updated otherwise this change will invalidate the (access token) request if the grant types are not identical.

- The access token expiry is added by default when issuing a new token so adding up from config can be removed.

- revert some previous "fixes" since they are not necessary anymore after running phpunit on barebone silverstripe installation

Jianbinzhu and others added 9 commits May 12, 2025 15:57

CMS 5 upgrade

c609c99

ENH Update to EDDSA(ED25519) signing key

a85f862

ENH Refactor code and ReadMe

c10571c

Update monolog/monolog to v3

bbf6886

Update Sever controller

e8683f0

Add private properties to avoid deprecation notice

00a500a

- Adding a value to dynamic properties is getting flagged by newer PHP versions e.g. 8.3, add this properties to the class itself since they are cannot be inherited from parent.

FIX unit tests

1fa13bd

Refactor code

d2dad78

- use dependency injection to add new token validator for new algorithm - add extension points for generating token - remove unnecessary code from package

Add docs and revert unit tests

ac4f62f

- add docs for configuring EdDSA/Ed25519 access token - revert previously updated unit tests - add unit tests for new API

mikenuguid mentioned this pull request Jun 4, 2025

ENH Update to EdDSA signing key #30

Closed

blueo suggested changes Aug 31, 2025

View reviewed changes

blueo suggested changes Sep 4, 2025

View reviewed changes

mikenuguid added 2 commits March 11, 2026 13:50

FIX Remove additional expiry time when issuing new access token

5f127e8

- The access token expiry is added by default when issuing a new token so adding up from config can be removed.

mikenuguid force-pushed the feature/use-secure-eddsa-cms5 branch 2 times, most recently from ae65efd to 02fcc48 Compare March 23, 2026 21:42

FIX unit tests

0be55fa

- revert some previous "fixes" since they are not necessary anymore after running phpunit on barebone silverstripe installation

mikenuguid force-pushed the feature/use-secure-eddsa-cms5 branch from 02fcc48 to 0be55fa Compare March 23, 2026 22:03

Conversation

mikenuguid commented Jun 4, 2025

Uh oh!

mikenuguid commented Jun 23, 2025

Uh oh!

mikenuguid commented Aug 31, 2025

Uh oh!

blueo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikenuguid Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blueo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mikenuguid Sep 4, 2025 •

edited

Loading