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

go SDK userInfo response does not support extra claims #1048

Closed
fredbi opened this issue Sep 24, 2018 · 2 comments
Closed

go SDK userInfo response does not support extra claims #1048

fredbi opened this issue Sep 24, 2018 · 2 comments
Milestone

Comments

@fredbi
Copy link

fredbi commented Sep 24, 2018

Do you want to request a feature or report a bug?
bug

What is the current behavior?
userinfo response declared in hydra go SDK swagger only declares standard OIDC claims, and does not support extra claims (e.g. a map[string]interface{} field).
This contradicts the documentation of the Userinfo() endpoint in the SDK.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem.
use a go client with hydra SDK to inquire userinfo with an access token

What is the expected behavior?
userinfo is described in https://openid.net/specs/openid-connect-core-1_0.html#UserInfo
as a general JSON object, including custom claims. It is not limited to standard OIDC claims.
Expect an extra field in type UserinfoResponse, like IdTokenExtra map[string]interface{].

Which version of the software is affected?
1.0.0 beta 9

@fredbi fredbi changed the title userInfo reponse does not support extra claims go SDK userInfo reponse does not support extra claims Sep 24, 2018
@fredbi fredbi changed the title go SDK userInfo reponse does not support extra claims go SDK userInfo response does not support extra claims Sep 25, 2018
@aeneasr
Copy link
Member

aeneasr commented Sep 25, 2018

I think it would make generally sense to remove the public endpoints, such as userinfo, oauth2/token, oauth2/auth, oauth2/revoke from the SDK and only keep the administrative ones here. Reason being that a lot of people think that those methods actually help you implement oauth2 or oidc specific flows. they do not. userinfo is one such example where it's also difficult to properly set up authorization with access tokens (especially refreshing them).

@aeneasr aeneasr added the docs label Oct 25, 2018
@aeneasr aeneasr added this to the v1.0.0-rc.1 milestone Oct 25, 2018
@aeneasr
Copy link
Member

aeneasr commented Nov 5, 2018

I revisited this and the problem is that we can't express this with the go type system at the moment. We really want to have a userinfo payload that represents the common values as specified by the openid foundation, which leads to this layout:

	// Subject - Identifier for the End-User at the IssuerURL.
	Subject string `json:"sub"`

	// End-User's full name in displayable form including all name parts, possibly including titles and suffixes, ordered according to the End-User's locale and preferences.
	Name string `json:"name,omitempty"`

	// Given name(s) or first name(s) of the End-User. Note that in some cultures, people can have multiple given names; all can be present, with the names being separated by space characters.
	GivenName string `json:"given_name,omitempty"`

//...
}

Because of that, we can't add an extra field as the extra field is embedded (using maps in the userinfo handler) directly in there. In go, we can't express that. It's not possible to have a mixed struct with defined fields and "free-for-all" fields as structured by maps. The downside of using a map[string]interface{} here to represent additional fields is not worth it IMO as the userinfo endpoint is really easy to consume.

Unfortunately, this is a nofix for the time being. If you have other ideas feel free to comment/reopen.

@aeneasr aeneasr closed this as completed Nov 5, 2018
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

No branches or pull requests

2 participants