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

Use JWT gem and some refactor #7

Merged
merged 37 commits into from
Sep 13, 2019
Merged

Use JWT gem and some refactor #7

merged 37 commits into from
Sep 13, 2019

Conversation

fjaeger
Copy link
Contributor

@fjaeger fjaeger commented Jun 18, 2019

The jwt gem is way more mature and more widely used than json-jwt.

@fjaeger
Copy link
Contributor Author

fjaeger commented Jun 19, 2019

What do you think (besides the 😄)?

@merefield
Copy link

merefield commented Aug 5, 2019

This PR appears to solve the issue with the lack of name and email? (which is critical to adoption for us)

@fjaeger
Copy link
Contributor Author

fjaeger commented Aug 5, 2019

This PR appears to solve the issue with the lack of name and email?

No, unfortunately not. Apple needs to provide a REST API endpoint to retrieve name/email as they currently only pass this information upon successful authentication once and not on subsequent authentications.

→ It's on Apple now to get their homework done

@merefield
Copy link

as they currently only pass this information upon successful authentication once and not on subsequent authentications

Let me check, I think that's probably good enough for our use case (Authentication and account setup in Discourse).

@fjaeger
Copy link
Contributor Author

fjaeger commented Aug 5, 2019

as they currently only pass this information upon successful authentication once and not on subsequent authentications

Let me check, I think that's probably good enough for our use case (Authentication and account setup in Discourse).

Could be, but imagine the following: User signs up via Apple, then deletes the account. Next time during "Sign in with Apple", the authentication would be successful, but would not include email/name anymore as it was provided on first auth

@dzunk
Copy link
Contributor

dzunk commented Aug 15, 2019

Derp, of course there's already a PR open... fixes #10.

@fjaeger
Copy link
Contributor Author

fjaeger commented Aug 31, 2019

By the way, this PR also handles #9

@dzunk
Copy link
Contributor

dzunk commented Sep 7, 2019

@nhosoya with iOS 13 and the "Sign in with Apple" requirement coming soon, it's important that we get the omniauth integration working well. My team at work is focused on this right now, and it would be very valuable to have an up-to-date version of this gem on Rubygems. Do you think you'll have time to take a look at this and merge the PR soon, or possibly allow another maintainer to take over?

dzunk and others added 2 commits September 9, 2019 14:58
Move raw_info into extra hash
Move raw_info into extra hash
@nhosoya
Copy link
Owner

nhosoya commented Sep 13, 2019

@fjaeger @dzunk
Thank you for your great contributions, and sorry for the delay...

Copy link
Owner

@nhosoya nhosoya left a comment

Choose a reason for hiding this comment

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

LGTM!

@nhosoya nhosoya merged commit 8c1fba1 into nhosoya:master Sep 13, 2019
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.

4 participants