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

fix: no openid scope crash #48

Merged
merged 3 commits into from
Mar 4, 2024
Merged

fix: no openid scope crash #48

merged 3 commits into from
Mar 4, 2024

Conversation

DanielRivers
Copy link
Contributor

Explain your changes

When no openid scope is supplied, the storage token storage fails and errors.

This resolves: kinde-oss/kinde-sveltekit-sdk#21

Checklist

🛟 If you need help, consider asking for advice over in the Kinde community.

@coel
Copy link
Member

coel commented Mar 4, 2024

@DanielRivers, looking at the OpenID Connect spec, the openid scope is required.

If the openid scope value is not present, the behavior is entirely unspecified.

I guess we can handle it as just OAuth2, but as far as I understand we're assuming OpenID Connect, which is probably why it is assuming the id token is there.

The developer experience doesn't sound good though. Perhaps we should throw an explicit error that openid is required, or possibly we always include it for them?

@DanielRivers
Copy link
Contributor Author

@DanielRivers, looking at the OpenID Connect spec, the openid scope is required.

If the openid scope value is not present, the behavior is entirely unspecified.

I guess we can handle it as just OAuth2, but as far as I understand we're assuming OpenID Connect, which is probably why it is assuming the id token is there.

The developer experience doesn't sound good though. Perhaps we should throw an explicit error that openid is required, or possibly we always include it for them?

I went for the approch of ensuring that openid is passed as its required as part of the spec

@DanielRivers DanielRivers requested a review from coel March 4, 2024 15:57
@coel coel merged commit 6bec968 into main Mar 4, 2024
2 checks passed
@coel coel deleted the fix/openid-missing-crash branch March 4, 2024 23:24
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.

Bug: missing openid scope causes crash on login
2 participants