From 565332cda05cc4c2d7d88326d38e6f1e572d4757 Mon Sep 17 00:00:00 2001 From: John Doak <42280444+element-of-surprise@users.noreply.github.com> Date: Wed, 12 Apr 2023 13:55:43 -0700 Subject: [PATCH] Adding some better diagnostics for len(scopes) == 0 (#403) * Adding some better diagnostics for len(scopes) == 0 * Try again --------- Co-authored-by: John Doak --- apps/internal/oauth/oauth.go | 65 +++++++++++++++++++++++++++++++++--- 1 file changed, 60 insertions(+), 5 deletions(-) diff --git a/apps/internal/oauth/oauth.go b/apps/internal/oauth/oauth.go index 5f136933..c3080759 100644 --- a/apps/internal/oauth/oauth.go +++ b/apps/internal/oauth/oauth.go @@ -76,12 +76,17 @@ func (t *Client) ResolveEndpoints(ctx context.Context, authorityInfo authority.I return t.Resolver.ResolveEndpoints(ctx, authorityInfo, userPrincipalName) } +// AADInstanceDiscovery attempts to discover a tenant endpoint (used in OIDC auth with an authorization endpoint). +// This is done by AAD which allows for aliasing of tenants (windows.sts.net is the same as login.windows.com). func (t *Client) AADInstanceDiscovery(ctx context.Context, authorityInfo authority.Info) (authority.InstanceDiscoveryResponse, error) { return t.Authority.AADInstanceDiscovery(ctx, authorityInfo) } // AuthCode returns a token based on an authorization code. func (t *Client) AuthCode(ctx context.Context, req accesstokens.AuthCodeRequest) (accesstokens.TokenResponse, error) { + if err := scopeError(req.AuthParams); err != nil { + return accesstokens.TokenResponse{}, err + } if err := t.resolveEndpoint(ctx, &req.AuthParams, ""); err != nil { return accesstokens.TokenResponse{}, err } @@ -107,6 +112,9 @@ func (t *Client) Credential(ctx context.Context, authParams authority.AuthParams } tr, err := cred.TokenProvider(ctx, params) if err != nil { + if len(scopes) == 0 { + return accesstokens.TokenResponse{}, fmt.Errorf("token request had an empty authority.AuthParams.Scopes, which may cause the following error: %w", err) + } return accesstokens.TokenResponse{}, err } return accesstokens.TokenResponse{ @@ -134,6 +142,9 @@ func (t *Client) Credential(ctx context.Context, authParams authority.AuthParams // Credential acquires a token from the authority using a client credentials grant. func (t *Client) OnBehalfOf(ctx context.Context, authParams authority.AuthParams, cred *accesstokens.Credential) (accesstokens.TokenResponse, error) { + if err := scopeError(authParams); err != nil { + return accesstokens.TokenResponse{}, err + } if err := t.resolveEndpoint(ctx, &authParams, ""); err != nil { return accesstokens.TokenResponse{}, err } @@ -145,20 +156,35 @@ func (t *Client) OnBehalfOf(ctx context.Context, authParams authority.AuthParams if err != nil { return accesstokens.TokenResponse{}, err } - return t.AccessTokens.FromUserAssertionClientCertificate(ctx, authParams, authParams.UserAssertion, jwt) + tr, err := t.AccessTokens.FromUserAssertionClientCertificate(ctx, authParams, authParams.UserAssertion, jwt) + if err != nil { + return accesstokens.TokenResponse{}, err + } + return tr, nil } func (t *Client) Refresh(ctx context.Context, reqType accesstokens.AppType, authParams authority.AuthParams, cc *accesstokens.Credential, refreshToken accesstokens.RefreshToken) (accesstokens.TokenResponse, error) { + if err := scopeError(authParams); err != nil { + return accesstokens.TokenResponse{}, err + } if err := t.resolveEndpoint(ctx, &authParams, ""); err != nil { return accesstokens.TokenResponse{}, err } - return t.AccessTokens.FromRefreshToken(ctx, reqType, authParams, cc, refreshToken.Secret) + tr, err := t.AccessTokens.FromRefreshToken(ctx, reqType, authParams, cc, refreshToken.Secret) + if err != nil { + return accesstokens.TokenResponse{}, err + } + return tr, nil } // UsernamePassword retrieves a token where a username and password is used. However, if this is // a user realm of "Federated", this uses SAML tokens. If "Managed", uses normal username/password. func (t *Client) UsernamePassword(ctx context.Context, authParams authority.AuthParams) (accesstokens.TokenResponse, error) { + if err := scopeError(authParams); err != nil { + return accesstokens.TokenResponse{}, err + } + if authParams.AuthorityInfo.AuthorityType == authority.ADFS { if err := t.resolveEndpoint(ctx, &authParams, authParams.Username); err != nil { return accesstokens.TokenResponse{}, err @@ -178,15 +204,24 @@ func (t *Client) UsernamePassword(ctx context.Context, authParams authority.Auth case authority.Federated: mexDoc, err := t.WSTrust.Mex(ctx, userRealm.FederationMetadataURL) if err != nil { - return accesstokens.TokenResponse{}, fmt.Errorf("problem getting mex doc from federated url(%s): %w", userRealm.FederationMetadataURL, err) + err = fmt.Errorf("problem getting mex doc from federated url(%s): %w", userRealm.FederationMetadataURL, err) + return accesstokens.TokenResponse{}, err } saml, err := t.WSTrust.SAMLTokenInfo(ctx, authParams, userRealm.CloudAudienceURN, mexDoc.UsernamePasswordEndpoint) if err != nil { - return accesstokens.TokenResponse{}, fmt.Errorf("problem getting SAML token info: %w", err) + err = fmt.Errorf("problem getting SAML token info: %w", err) + return accesstokens.TokenResponse{}, err } - return t.AccessTokens.FromSamlGrant(ctx, authParams, saml) + tr, err := t.AccessTokens.FromSamlGrant(ctx, authParams, saml) + if err != nil { + return accesstokens.TokenResponse{}, err + } + return tr, nil case authority.Managed: + if len(authParams.Scopes) == 0 { + return accesstokens.TokenResponse{}, fmt.Errorf("token request had an empty authority.AuthParams.Scopes, which may cause the following error: %w", err) + } return t.AccessTokens.FromUsernamePassword(ctx, authParams) } return accesstokens.TokenResponse{}, errors.New("unknown account type") @@ -274,6 +309,10 @@ func isWaitDeviceCodeErr(err error) bool { // DeviceCode returns a DeviceCode object that can be used to get the code that must be entered on the second // device and optionally the token once the code has been entered on the second device. func (t *Client) DeviceCode(ctx context.Context, authParams authority.AuthParams) (DeviceCode, error) { + if err := scopeError(authParams); err != nil { + return DeviceCode{}, err + } + if err := t.resolveEndpoint(ctx, &authParams, ""); err != nil { return DeviceCode{}, err } @@ -294,3 +333,19 @@ func (t *Client) resolveEndpoint(ctx context.Context, authParams *authority.Auth authParams.Endpoints = endpoints return nil } + +// scopeError takes an authority.AuthParams and returns an error +// if len(AuthParams.Scope) == 0. +func scopeError(a authority.AuthParams) error { + // TODO(someone): we could look deeper at the message to determine if + // it's a scope error, but this is a good start. + /* + {error":"invalid_scope","error_description":"AADSTS1002012: The provided value for scope + openid offline_access profile is not valid. Client credential flows must have a scope value + with /.default suffixed to the resource identifier (application ID URI)...} + */ + if len(a.Scopes) == 0 { + return fmt.Errorf("token request had an empty authority.AuthParams.Scopes, which is invalid") + } + return nil +}