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

Consider generate ID Token for Resource Owner Password Flow #650

Closed
5 of 6 tasks
hueryang opened this issue Feb 7, 2022 · 5 comments
Closed
5 of 6 tasks

Consider generate ID Token for Resource Owner Password Flow #650

hueryang opened this issue Feb 7, 2022 · 5 comments
Labels
feat New feature or request.

Comments

@hueryang
Copy link

hueryang commented Feb 7, 2022

Preflight checklist

Describe your problem

We're using the fosite to provide the OAuth2 service. It will be great if fosite support to generate id token for resource owner password flow.

I checked, like okta, auth0 or azure, id token can be generated for resource owner password flow.
okta
Any OAuth flow can give you an access token, but not all support ID tokens.

Grant Type Access Token ID Token
Authorization Code
Authorization Code with PKCE
Implicit
Resource Owner Password
Client Credentials
SAML 2.0 Assertion

Link here - https://developer.okta.com/docs/concepts/oauth-openid/#does-your-application-need-an-id-token

auth0
Link here - https://auth0.com/docs/get-started/authentication-and-authorization-flow/call-your-api-using-resource-owner-password-flow#response

azure
Link here - https://docs.microsoft.com/en-us/azure/active-directory/develop/v2-oauth-ropc#successful-authentication-response

In the sample response of auth0 or azure's document, id_token is returned.

Describe your ideal solution

I suggest we can add a handler implements the TokenEndpointHandler interface, and generate id token in PopulateTokenEndpointResponse method for resource owner password flow.

Workarounds or alternatives

N/A

Version

v0.34.1

Additional Context

I'll be happy to work on this feature provided we agree on this. Thank you!

@hueryang hueryang added the feat New feature or request. label Feb 7, 2022
@vivshankar
Copy link
Contributor

I did raise this in the Slack channel some time back and was told that there wasn't interest in adding something like this because, technically, ROPC is described as an OAuth 2.0 flow and not used for Open ID Connect. I worked around this by adding my own handler that wrapped the ROPC handler in Fosite.

Happy to contribute this right away if there's interest, but here's the code.

package handler

import (
	"context"

	"github.com/ory/fosite"
	"github.com/ory/fosite/handler/openid"
	"github.com/ory/x/errorsx"
)

// ResourceOwnerPasswordCredentialsGrantHandler is the handler for ROPC that also supports
// scope=openid
type ResourceOwnerPasswordCredentialsGrantHandler struct {
	*openid.IDTokenHandleHelper
}

// HandleTokenEndpointRequest implements https://tools.ietf.org/html/rfc6749#section-4.3.2
func (c *ResourceOwnerPasswordCredentialsGrantHandler) HandleTokenEndpointRequest(ctx context.Context, request fosite.AccessRequester) error {
	return nil
}

// PopulateTokenEndpointResponse implements https://tools.ietf.org/html/rfc6749#section-4.3.3
func (c *ResourceOwnerPasswordCredentialsGrantHandler) PopulateTokenEndpointResponse(ctx context.Context, requester fosite.AccessRequester, responder fosite.AccessResponder) error {
	if !c.CanHandleTokenEndpointRequest(requester) {
		return errorsx.WithStack(fosite.ErrUnknownRequest)
	}

	if !requester.GetRequestedScopes().Has("openid") {
		return nil
	}

	sess, ok := requester.GetSession().(openid.Session)
	if !ok {
		return errorsx.WithStack(fosite.ErrServerError.WithDebug("Failed to generate id token because session must be of type fosite/handler/openid.Session."))
	}

	claims := sess.IDTokenClaims()
	if claims.Subject == "" {
		return errorsx.WithStack(fosite.ErrServerError.WithDebug("Failed to generate id token because subject is an empty string."))
	}

	requester.GrantScope("openid")
	responder.SetScopes(requester.GetGrantedScopes())
	return c.IssueExplicitIDToken(ctx, requester, responder)
}

// CanSkipClientAuth indicates if the handler can skip client auth
func (c *ResourceOwnerPasswordCredentialsGrantHandler) CanSkipClientAuth(requester fosite.AccessRequester) bool {
	return false
}

// CanHandleTokenEndpointRequest indicates the grant type supported
func (c *ResourceOwnerPasswordCredentialsGrantHandler) CanHandleTokenEndpointRequest(requester fosite.AccessRequester) bool {
	// grant_type REQUIRED.
	// Value MUST be set to "password".
	return requester.GetGrantTypes().ExactOne("password")
}

@hueryang
Copy link
Author

hueryang commented Apr 2, 2022

@vivshankar Thank you so much for your reply! I understand it now.

@aeneasr
Copy link
Member

aeneasr commented Apr 16, 2022

On the one hand I understand that some providers offer this capability as part of their APIs.

On the other, I see little added benefit to having ID Tokens supported in this flow. Most of the parameters (requested claims, auth_time, ...) do not apply to ROPC grant due since it's not in the OIDC specification. Thus, any implementation will be a custom interpretation of how this is supposed to work, with all the downsides of having to maintain this custom security feature for a long time.

Additionally, ROPC will be removed in OAuth2.1 ( https://oauth.net/2.1/ ).

For these reasons, I don't think that it makes sense to implement this. Also, because it contradicts the contribution and design guidelines which require standard track RFCs for feature requests as impactful as this one. Thus, I'll be closing this issue. If you disagree though, please feel free to comment or reopen!

@aeneasr aeneasr closed this as completed Apr 16, 2022
@james-d-elliott
Copy link
Contributor

It may be good to consider deprecating compose.OAuth2ResourceOwnerPasswordCredentialsFactory to communicate to people implementing fosite that ROPC is widely discouraged in general even by the IETF.

@aeneasr
Copy link
Member

aeneasr commented Apr 16, 2022

Makes sense! Contribs welcomed :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request.
Projects
None yet
Development

No branches or pull requests

4 participants