ENH Add extension for new signing key and upgrade to CMS 5 #31
Conversation
- 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
|
@blueo would appreciate if I could have your feedback on this when you have time. |
|
Hey @blueo, any chance you could take a look at this PR? 🙂 |
blueo
left a comment
blueo
left a comment
There was a problem hiding this comment.
I'm by no means an expert on this module but I've left a couple of questions on some implementation details
| return $this->authorizationValidator; | ||
| } | ||
|
|
||
| public function setAuthorizationValidator(?AuthorizationValidatorInterface $value): void |
There was a problem hiding this comment.
is there a use case for setting this to null? If not then you could probably remove ? from the type
There was a problem hiding this comment.
Yes. If null is passed, the ResourceServer will use BearerTokenValidator (tied up to RSA) by default.
| 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]); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Can't recall unfortunately, probably a failing test. I'll have a look. Thanks.
| * @param mixed $controller | ||
| */ | ||
| public static function getMember($controller): ?Member | ||
| public function getMember($controller): ?Member |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I'll double check this one too. But thanks. I'll make sure to update the doc, it's mentioned in there.
| ### Generate a private/public key pair using Ed25519 algorithm | ||
|
|
||
| ```shell | ||
| openssl genpkey -algorithm Ed25519 -out private.key |
There was a problem hiding this comment.
looks like we should probably put this as the recommended way on the main Readme.md? it is a better default than RSA
There was a problem hiding this comment.
A recommentdation yes, but probably leave RSA as default since not most application use the new algorithm (more so support it)
| $token = null; | ||
|
|
||
| // Get token from extension (in case of different implementation than the default) | ||
| $this->extend('updateJWT', $token); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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
left a comment
blueo
left a comment
There was a problem hiding this comment.
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
| $token = null; | ||
|
|
||
| // Get token from extension (in case of different implementation than the default) | ||
| $this->extend('updateJWT', $token); |
There was a problem hiding this comment.
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?
3 participants
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.