Skip to content

Update security.md #1014

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

Closed
wants to merge 1 commit into from
Closed

Update security.md #1014

wants to merge 1 commit into from

Conversation

kwn
Copy link

@kwn kwn commented May 28, 2021

TLDR

Small fix for the authentication docs. The example from the docs currently doesn't work.

Details

In the latest version of the Lcobucci\JWT package getToken() method returns Lcobucci\JWT\Token\Plain object. authenticate() method expects first parameter to be a string. Plain class doesn't have __toString() method implemented, so passing an instance of the Plain class to the authenticate() method throws an exception.

Apparently Lcobucci introduced a new method called payload() to the Plain class, so in order to get a string representation of the JWT token we need to call it explicitly.

TLDR:
Small fix for the authentication docs. The example from the docs currently doesn't work.

Details:
`getToken()` method currently returns `Lcobucci\JWT\Token\Plain` object. `authenticate()` expects string as a first parameter. `Plain` class doesn't have `__toString()` method implemented, so passing an instance of the `Plain` class to the `authenticate()` method throws an exception.
@acrobat
Copy link
Collaborator

acrobat commented May 30, 2021

Hi @kwn, thanks for the PR! If I read the lcobucci/jwt code correctly the payload method does return something different than the __toString method

Deprecated __toString method:

https://github.com/lcobucci/jwt/blob/511629a54465e89a31d3d7e4cf0935feab8b14c1/src/Token.php#L410-L429

Payload method:

https://github.com/lcobucci/jwt/blob/511629a54465e89a31d3d7e4cf0935feab8b14c1/src/Token.php#L394-L402

So it would actually be better to update the example to call $token->toString() so that the expected result is the same.

@glaubinix
Copy link
Contributor

I can confirm that calling $token->toString() is the correct method to call to receive the token to authenticate against GitHub

@acrobat
Copy link
Collaborator

acrobat commented Jun 8, 2021

Closing this in favor of #1017

@acrobat acrobat closed this Jun 8, 2021
@kwn
Copy link
Author

kwn commented Jun 10, 2021

Thanks for sorting it out and sorry for a late reply 👍

@kwn kwn deleted the patch-1 branch June 10, 2021 16:19
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.

3 participants