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

Add ID token verification endpoint #253

Merged
merged 4 commits into from
May 30, 2022
Merged

Conversation

yskkin
Copy link
Contributor

@yskkin yskkin commented May 24, 2022

This is implementation for https://developers.line.biz/en/reference/line-login/#verify-id-token.

We need this for liff.getIDToken().

@yskkin
Copy link
Contributor Author

yskkin commented May 24, 2022

We may also need verify_access_token and get_profile_by_access_token for a LIFF app which does not have openid scope.
https://developers.line.biz/en/docs/liff/using-user-profile/#use-user-info-on-server

By the way, there is no way to check/change LIFF app's scope 😕

@yskkin yskkin marked this pull request as draft May 24, 2022 03:17
Copy link
Member

@zenizh zenizh left a comment

Choose a reason for hiding this comment

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

Thanks for sending in your pull request! I have commented on a few of them and would appreciate your checking.

# @param options [Hash] Optional request body
#
# @return [Net::HTTPResponse]
def verify_id_token(id_token, options = {})
Copy link
Member

Choose a reason for hiding this comment

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

Passing options as a hash may result in unintended parameters being passed to the endpoint. It may be too much to worry about, but it is better to be safe than sorry.

I think it would be better to pass only concrete parameters, i.e. nonce and user_id, like other methods such as narrowcast.

def narrowcast(messages, recipient: nil, filter: nil, limit: nil, headers: {})

Suggested change
def verify_id_token(id_token, options = {})
def verify_id_token(id_token, nonce: nil, user_id: nil)


it 'verifies ID token' do
uri_template = Addressable::Template.new Line::Bot::API::DEFAULT_OAUTH_ENDPOINT + '/oauth2/v2.1/verify'
stub_request(:post, uri_template).to_return { |request| {body: '', status: 200} }
Copy link
Member

Choose a reason for hiding this comment

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

In the test, the body of the response is empty, but it actually returns the contents of the ID token. I think it would be better to add content validation for some values, referring to the "Example response" in the documentation.
https://developers.line.biz/en/reference/line-login/#verify-id-token-response

@yskkin
Copy link
Contributor Author

yskkin commented May 24, 2022

addressed @zenizh's reveiw and add implementation for verify_access_token and get_profile_by_access_token

@yskkin yskkin marked this pull request as ready for review May 24, 2022 15:03
Copy link
Member

@zenizh zenizh left a comment

Choose a reason for hiding this comment

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

Thank you for your response. I think it is very nice. I made one minor comment, please check it when you have a chance.

lib/line/bot/client.rb Show resolved Hide resolved
# @param access_token [String] access token
#
# @return [Net::HTTPResponse]
def verify_access_token(access_token)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://developers.line.biz/en/reference/line-login-v2/#verify-access-token
There is v2 version of this API.
Should I rename this to verify_access_token_v2_1?
I named this way since I thought v2.1 is superset of v2.

Same for get_profile_by_access_token.

Copy link
Member

Choose a reason for hiding this comment

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

I think verify_access_token is fine. Because LINE Login v2.0 is deprecated. Also, the documentation comment says "Verify access token v2.1", so it seems that users will not have to worry about it.

Copy link
Member

@zenizh zenizh left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for your contribution!

@zenizh
Copy link
Member

zenizh commented May 27, 2022

@yskkin I'll be releasing a version next week that includes this change. Sorry for the wait!

@zenizh zenizh merged commit 8b5c015 into line:master May 30, 2022
@yskkin yskkin deleted the id-token-verify branch May 30, 2022 05:43
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