Skip to content

Update integration authentication documentation for usage with lcobucci/jwt ^4 #1017

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

Merged
merged 2 commits into from
Jul 11, 2021

Conversation

glaubinix
Copy link
Contributor

The security docs were mentioning lcobucci/jwt:^3.4 which doesn't support php 8. Updated the security docs to reflect all necessary changes to work with lcobucci/jwt:^4.1

Passing ChainedFormatter::withUnixTimestampDates() to the builder method is necessary because otherwise all dates will be format via $date->format('U.u') as microseconds. GitHub expects unix timestamps and will return a 401 response with 'Expiration time' claim ('exp') must be a numeric value representing the future time at which the assertion expires.

@acrobat acrobat mentioned this pull request Jun 8, 2021
@acrobat acrobat self-requested a review June 8, 2021 10:18
@raghavendra89
Copy link

raghavendra89 commented Jun 8, 2021

Thanks! I just wanted to submit a pull request for this. But saw yours.

There is one more minor issue at line 48:
$github = new Github\Client($builder, 'machine-man-preview');

Here $builder variable is referenced, but it is not initiated. We can add this line before line 48:

$builder = new Builder(new GuzzleHttp\Client());

and include: use Github\HttpClient\Builder;

@acrobat
Copy link
Collaborator

acrobat commented Jun 19, 2021

@glaubinix can you apply the extra comments that @raghavendra89 made above? Thanks!

@glaubinix
Copy link
Contributor Author

@acrobat done

@acrobat acrobat merged commit de96379 into KnpLabs:master Jul 11, 2021
@acrobat
Copy link
Collaborator

acrobat commented Jul 11, 2021

Thank you @glaubinix

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

Successfully merging this pull request may close these issues.

4 participants