Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to set a TokenGenerator to override default implementation. #94

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

iamluc
Copy link

@iamluc iamluc commented Dec 29, 2016

It eases the use of JWT or other token types.

Could also fix #86

It eases the use of JWT or other token types.
@lavasov
Copy link

lavasov commented Jul 11, 2017

@GuilhemN Hi, is there any chance to get this merged in the nearest time?

@dinamic
Copy link

dinamic commented Jan 19, 2018

ping @GuilhemN

@GuilhemN
Copy link
Member

Well this needs to be reviewed first but I don't know this library well enough to do it.
If there were a few people reviewing it I would feel more comfortable and would be able to merge this.

Copy link

@cored0wn cored0wn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good implementation and very useful.
Especially for people who has to implement oauth extensions like openid connect (such as me).

*/
public function setTokenGenerator(TokenGeneratorInterface $tokenGenerator)
{
$this->tokenGenerator = $tokenGenerator;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The DI container should inject the specific generator so the value is set.
If no specific generator is available, then the default generator should be used.

@@ -1413,6 +1445,8 @@ private function createAuthCode(IOAuth2Client $client, $data, $redirectUri, $sco
*
* @ingroup oauth2_section_4
* @see OAuth2::genAuthCode()
*
* @deprecated since 1.3, will be removed in 2.0. Use a TokenGenerator instead.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method should be call the default generator method internally.


class RandomTokenGenerator implements TokenGeneratorInterface
{
public function genAccessToken(IOAuth2Client $client, $data, $scope = null, $access_token_lifetime = null, $issue_refresh_token = true, $refresh_token_lifetime = null)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This parameters should be extraced as class members and set over an constructor or setter methods.

@Valantir007
Copy link

Are there any plans to merge it?

@dinamic
Copy link

dinamic commented Feb 11, 2019

@Valantir007 I guess @iamluc would have to make an effort and fix the comments added before merging is on the table. The PR has been sitting like this since it was created and I have my doubts whether the OP is keen on implementing the changes.

@GuilhemN
Copy link
Member

@Valantir007 If you'd like to submit a new PR fixing the comments that would definitely help :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use Symfony’s SecureRandom instead of home-grown crypto code
6 participants