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

feat: expose id token #68

Merged
merged 6 commits into from
Sep 14, 2023
Merged

feat: expose id token #68

merged 6 commits into from
Sep 14, 2023

Conversation

kwajiehao
Copy link
Contributor

@kwajiehao kwajiehao commented Sep 11, 2023

Overview

This PR does 2 things:

  1. Expose the id token as part of the callback() method
  2. Perform validation on the id token in a manner consistent across SDKs

1. Expose the ID token as part of the callback() method

Currently, we only expose the sub and access token after performing all the requisite checks on the ID token. But the id token is useful because it acts as proof that the user has been authenticated by sgID. Some relying parties want to store
the ID token as part of their audit trail. This is why we're exposing the ID token here.

2. Perform validation on the id token in a manner consistent across SDKs

Validation is done according to the SDK implementation strategy here: https://www.notion.so/opengov/SDK-implementation-requirements-1f9b7cbd2bd4406b85d6645fe3e365dd

Note that in this case, the openid-client library does validation on the ID token for us, so behavior deviates from the Python SDK.

Tests

  • Tested locally that the idToken is received with sgid-demo-frontend-spa and the Express example in this repo.
  • Added unit tests

This commit exposes the id token in the callback method.

The id token is useful because it acts as proof that the user has
been authenticated by sgID. Some relying parties want to store
the id token as part of their audit trail. This is why we're exposing
the id token here.
This commit modifies the implementation for the check on whether
the id token exists, or is valid, so that it is consistent with our
implementation strategy detailed here:
https://www.notion.so/opengov/SDK-implementation-requirements-1f9b7cbd2bd4406b85d6645fe3e365dd

Mainly, it checks that the ID token is a non-empty string, and ensures
that the error message thrown is the same.
@kwajiehao kwajiehao changed the title (WIP) feat: expose id token feat: expose id token Sep 14, 2023
Copy link
Contributor

@PrawiraGenestonlia PrawiraGenestonlia left a comment

Choose a reason for hiding this comment

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

lgtm!

Copy link
Contributor

@PrawiraGenestonlia PrawiraGenestonlia left a comment

Choose a reason for hiding this comment

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

lgtm!

@kwajiehao kwajiehao merged commit 7cd9da0 into develop Sep 14, 2023
@kwajiehao kwajiehao deleted the feat/expose-id-token branch September 14, 2023 14:12
kwajiehao added a commit that referenced this pull request Oct 11, 2023
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.

2 participants