Improve ActingAsKeycloakUser trait #114
Conversation
|
@robsontenorio , is it possible that we can make a progress here? I have noticed the PR is open for over a week but there is no response. I'd be happy to make necessary adjustments if you have any concern regarding merging the PR. To be clear, due to the lack of customizing the token payload the library becomes unusable in certain projects where the exact token payload is being asserted. |
|
Please , include tests and update the README if needed. @tuytoosh could you please review it? |
|
@robsontenorio , done. @tuytoosh , please note I have also made some change to use the configured db column instead of hard-coded |
| @@ -16,7 +16,8 @@ | |||
| "prefer-stable": true, | |||
| "require": { | |||
| "firebase/php-jwt": "^6.3", | |||
| "php": "^8.0" | |||
| "php": "^8.0", | |||
| "ext-openssl": "*" | |||
There was a problem hiding this comment.
Are you sure is this needed ?
if so , please, use a specific version.
There was a problem hiding this comment.
Yes. The library depends on openssl extension and it is good to specify the requirement so consumers would be aware of it. I cannot be sure of the minimum required version of the extension for this particular library, but even existence of this line should be enough to force consumers installing one of the modern versions.
There was a problem hiding this comment.
@robsontenorio , I am not sure if this is the holding part for the PR, but I can remove the line if it bothers you to speed up the release. We are going to release a feature depending on this repository in our project, but cannot make tests pass unless the library allows configuring the test token payload. Therefore, your efforts in merging this PR quickly will be greatly appreciated.
Also I am not sure if it is you or @tuytoosh the reviewer as you are the one assigned for this PR.
elnurvl
force-pushed
the
token-payload
branch
4 times, most recently
from
ebd395d to
e6d164a
Compare
|
Sorry for the late feedback. From my point of view, it is quite OK to be merged. |
README.md
Outdated
| @@ -331,7 +331,7 @@ Auth::hasAnyScope(['scope-f', 'scope-k']) // false | |||
|
|
|||
| # Acting as a Keycloak user in tests | |||
There was a problem hiding this comment.
Since this is not the main title, we can change it to H2.
There was a problem hiding this comment.
Updated the README. Do you think this can be merged followed by release of a new version into packagist this week? I assume it is possible since there is not much change request around the PR, but not sure about your release schedules.
There was a problem hiding this comment.
I don't know about release schedule but since it's not a major version, it can be released ASAP.
|
Thanks guys ! |
The aim of the change is to allow passing custom claims to the test token as well as adding widely accepted claims by default.