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 sub claim if oid is missing #4141

Merged
merged 4 commits into from
May 22, 2023
Merged

Use sub claim if oid is missing #4141

merged 4 commits into from
May 22, 2023

Conversation

bgavrilMS
Copy link
Member

Fixes #4140

@bgavrilMS bgavrilMS marked this pull request as ready for review May 18, 2023 11:10
Copy link
Contributor

@jmprieur jmprieur left a comment

Choose a reason for hiding this comment

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

LGTM
Was GetUniqueId() already defined?

@bgavrilMS
Copy link
Member Author

LGTM Was GetUniqueId() already defined?

Yeah, it's used to compute AuthenticationResult.UniqueId which is public API. I changed the comments on it.

@bgavrilMS bgavrilMS requested review from trwalke and pmaytak May 18, 2023 16:05
@gladjohn
Copy link
Contributor

public string GetUniqueId()
{
    return ObjectId ?? Subject;
}

what does subject return?

@bgavrilMS
Copy link
Member Author

public string GetUniqueId()
{
    return ObjectId ?? Subject;
}

what does subject return?

sub claim. I added comments everywhere on this. Seems to be mandatory as per OIDC spec. Should be an ID for app + user, but can theoretically be different for app2 + user. Stable enough for token caching for sure.

@bgavrilMS bgavrilMS merged commit 0d98f32 into main May 22, 2023
@bgavrilMS bgavrilMS deleted the bogavril/4140 branch May 22, 2023 08:42
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] SerializeAdalV3 returns null when Id Token has no oid claim
7 participants