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

[SDK-3172] Add ID Token validation to device-code and passwordless #553

Merged
merged 1 commit into from
Mar 4, 2022

Conversation

frederikprijck
Copy link
Member

@frederikprijck frederikprijck commented Mar 3, 2022

Our SDK validates ID Tokens for all but 3 method calls:

  • Device Code
  • Passwordless with Email
  • Passwordless with SMS

We should align the validation here and ensure the above methods also validate the ID Token.

Note that this is technically a breaking change ... Anyone retrieving an Invalid ID Token will be able to use the methods today, while after this change they suddenly get an exception when calling one of these methods.

However, I would argue this should be fine to release in a minor version, as anyone that we break here is made aware of the incorrect ID Token usage. When needed, they can still rollback to a version prior to this change if they need a fix ASAP and then circle back on fixing their incorrect ID Tokens. But regardless I would argue we want to push them into correct ID Token usage sooner rather than later.

Apart from the above, this should be backwards compatible and not require any change for anyone using valid ID Tokens.

What do you think @stevehobbsdev ?

@frederikprijck frederikprijck requested a review from a team as a code owner March 3, 2022 09:51
@frederikprijck frederikprijck changed the title Add ID Token validation to device-code and passwordless [SDK-3172] Add ID Token validation to device-code and passwordless Mar 3, 2022
Copy link
Contributor

@stevehobbsdev stevehobbsdev left a comment

Choose a reason for hiding this comment

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

Agree, hope that nobody would be broken by this but let's acknowledge that possibility in the release notes/changelog when the release goes out 👍🏻

@frederikprijck frederikprijck merged commit 838c17f into master Mar 4, 2022
@frederikprijck frederikprijck deleted the feat/3172 branch March 4, 2022 11:06
@frederikprijck frederikprijck added this to the 7.14.1 milestone Mar 4, 2022
@frederikprijck frederikprijck mentioned this pull request Mar 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants