Skip to content

Commit

Permalink
IAM: Implement correct OAuth2 error handling
Browse files Browse the repository at this point in the history
  • Loading branch information
@reinkrul
reinkrul committed Sep 26, 2023
1 parent 7238d6f commit e814b34
Show file tree
Hide file tree
Showing 9 changed files with 402 additions and 71 deletions.
49 changes: 16 additions & 33 deletions auth/api/iam/api.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ import (
"github.com/nuts-foundation/nuts-node/auth/log"
"github.com/nuts-foundation/nuts-node/core"
"github.com/nuts-foundation/nuts-node/vcr"
"github.com/nuts-foundation/nuts-node/vcr/openid4vci"
"github.com/nuts-foundation/nuts-node/vdr/didservice"
vdr "github.com/nuts-foundation/nuts-node/vdr/types"
"html/template"
Expand Down Expand Up @@ -80,8 +79,7 @@ func (r Wrapper) Routes(router core.EchoRouter) {
// Add http.Request to context, to allow reading URL query parameters
requestCtx := context.WithValue(ctx.Request().Context(), httpRequestContextKey, ctx.Request())
ctx.SetRequest(ctx.Request().WithContext(requestCtx))
// TODO: Do we need a generic error handler?
// ctx.Set(core.ErrorWriterContextKey, &protocolErrorWriter{})
ctx.Set(core.ErrorWriterContextKey, &oauth2ErrorWriter{})
return f(ctx, request)
}
},
Expand Down Expand Up @@ -118,30 +116,21 @@ func (r Wrapper) HandleTokenRequest(ctx context.Context, request HandleTokenRequ
// Options:
// - OpenID4VCI
// - OpenID4VP, vp_token is sent in Token Response
panic("not implemented")
case "vp_token":
// Options:
// - service-to-service vp_token flow
panic("not implemented")
case "urn:ietf:params:oauth:grant-type:pre-authorized_code":
// Options:
// - OpenID4VCI
panic("not implemented")
default:
// TODO: Don't use openid4vci package for errors
return nil, openid4vci.Error{
Code: openid4vci.InvalidRequest,
StatusCode: http.StatusBadRequest,
//Description: "invalid grant type",
return nil, OAuth2Error{
Code: InvalidRequest,
Description: "unsupported grant_type",
}
}

// TODO: Handle?
//scope, err := handler(request.Body.AdditionalProperties)
//if err != nil {
// return nil, err
//}
// TODO: Generate access token with scope
return HandleTokenRequest200JSONResponse(TokenResponse{
AccessToken: "",
}), nil
}

// HandleAuthorizeRequest handles calls to the authorization endpoint for starting an authorization code flow.
Expand All @@ -161,7 +150,10 @@ func (r Wrapper) HandleAuthorizeRequest(ctx context.Context, request HandleAutho
// TODO: Spec says that the redirect URI is optional, but it's not clear what to do if it's not provided.
// Threat models say it's unsafe to omit redirect_uri.
// See https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.1
return nil, errors.New("missing redirect URI")
return nil, OAuth2Error{
Code: InvalidRequest,
Description: "redirect_uri is required",
}
}

switch session.ResponseType {
Expand All @@ -171,33 +163,24 @@ func (r Wrapper) HandleAuthorizeRequest(ctx context.Context, request HandleAutho
// - OpenID4VCI; authorization code flow for credential issuance to (end-user) wallet
// - OpenID4VP, vp_token is sent in Token Response; authorization code flow for presentation exchange (not required a.t.m.)
// TODO: Switch on parameters to right flow
panic("not implemented")
case responseTypeVPToken:
// Options:
// - OpenID4VP flow, vp_token is sent in Authorization Response
// TODO: Check parameters for right flow
// TODO: Do we actually need this? (probably not)
panic("not implemented")
case responseTypeVPIDToken:
// Options:
// - OpenID4VP+SIOP flow, vp_token is sent in Authorization Response
return r.handlePresentationRequest(params, session)
default:
// TODO: This should be a redirect?
// TODO: Don't use openid4vci package for errors
return nil, openid4vci.Error{
Code: openid4vci.InvalidRequest,
StatusCode: http.StatusBadRequest,
//Description: "invalid/unsupported response_type",
return nil, OAuth2Error{
Code: UnsupportedResponseType,
RedirectURI: session.RedirectURI,
}
}

// No handler could handle the request
// TODO: This should be a redirect?
// TODO: Don't use openid4vci package for errors
return nil, openid4vci.Error{
Code: openid4vci.InvalidRequest,
StatusCode: http.StatusBadRequest,
//Description: "missing or invalid parameters",
}
}

// OAuthAuthorizationServerMetadata returns the Authorization Server's metadata
Expand Down
97 changes: 80 additions & 17 deletions auth/api/iam/api_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
package iam

import (
"context"
"errors"
ssi "github.com/nuts-foundation/go-did"
"github.com/nuts-foundation/go-did/did"
Expand All @@ -29,18 +30,20 @@ import (
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.uber.org/mock/gomock"
"net/http"
"net/url"
"testing"
)

var nutsDID = did.MustParseDID("did:nuts:123")

func TestWrapper_OAuthAuthorizationServerMetadata(t *testing.T) {
testDID := did.MustParseDID("did:nuts:123")
t.Run("ok", func(t *testing.T) {
// 200
ctx := newTestClient(t)
ctx.vdr.EXPECT().IsOwner(nil, testDID).Return(true, nil)
ctx.vdr.EXPECT().IsOwner(nil, nutsDID).Return(true, nil)

res, err := ctx.client.OAuthAuthorizationServerMetadata(nil, OAuthAuthorizationServerMetadataRequestObject{Id: testDID.ID})
res, err := ctx.client.OAuthAuthorizationServerMetadata(nil, OAuthAuthorizationServerMetadataRequestObject{Id: nutsDID.ID})

require.NoError(t, err)
assert.IsType(t, OAuthAuthorizationServerMetadata200JSONResponse{}, res)
Expand All @@ -49,9 +52,9 @@ func TestWrapper_OAuthAuthorizationServerMetadata(t *testing.T) {
t.Run("error - did not managed by this node", func(t *testing.T) {
//404
ctx := newTestClient(t)
ctx.vdr.EXPECT().IsOwner(nil, testDID)
ctx.vdr.EXPECT().IsOwner(nil, nutsDID)

res, err := ctx.client.OAuthAuthorizationServerMetadata(nil, OAuthAuthorizationServerMetadataRequestObject{Id: testDID.ID})
res, err := ctx.client.OAuthAuthorizationServerMetadata(nil, OAuthAuthorizationServerMetadataRequestObject{Id: nutsDID.ID})

assert.Equal(t, 404, statusCodeFrom(err))
assert.EqualError(t, err, "authz server metadata: did not owned")
Expand All @@ -60,9 +63,9 @@ func TestWrapper_OAuthAuthorizationServerMetadata(t *testing.T) {
t.Run("error - did does not exist", func(t *testing.T) {
//404
ctx := newTestClient(t)
ctx.vdr.EXPECT().IsOwner(nil, testDID).Return(false, vdr.ErrNotFound)
ctx.vdr.EXPECT().IsOwner(nil, nutsDID).Return(false, vdr.ErrNotFound)

res, err := ctx.client.OAuthAuthorizationServerMetadata(nil, OAuthAuthorizationServerMetadataRequestObject{Id: testDID.ID})
res, err := ctx.client.OAuthAuthorizationServerMetadata(nil, OAuthAuthorizationServerMetadataRequestObject{Id: nutsDID.ID})

assert.Equal(t, 404, statusCodeFrom(err))
assert.EqualError(t, err, "authz server metadata: unable to find the DID document")
Expand All @@ -71,9 +74,9 @@ func TestWrapper_OAuthAuthorizationServerMetadata(t *testing.T) {
t.Run("error - internal error 500", func(t *testing.T) {
//500
ctx := newTestClient(t)
ctx.vdr.EXPECT().IsOwner(nil, testDID).Return(false, errors.New("unknown error"))
ctx.vdr.EXPECT().IsOwner(nil, nutsDID).Return(false, errors.New("unknown error"))

res, err := ctx.client.OAuthAuthorizationServerMetadata(nil, OAuthAuthorizationServerMetadataRequestObject{Id: testDID.ID})
res, err := ctx.client.OAuthAuthorizationServerMetadata(nil, OAuthAuthorizationServerMetadataRequestObject{Id: nutsDID.ID})

assert.Equal(t, 500, statusCodeFrom(err))
assert.EqualError(t, err, "authz server metadata: unknown error")
Expand All @@ -82,7 +85,6 @@ func TestWrapper_OAuthAuthorizationServerMetadata(t *testing.T) {
}

func TestWrapper_GetWebDID(t *testing.T) {
nutsDID := did.MustParseDID("did:nuts:123")
webDID := did.MustParseDID("did:web:example.com:iam:123")
publicURL := ssi.MustParseURI("https://example.com").URL
webDIDBaseURL := publicURL.JoinPath("/iam")
Expand Down Expand Up @@ -124,37 +126,98 @@ func TestWrapper_GetWebDID(t *testing.T) {
}

func TestWrapper_GetOAuthClientMetadata(t *testing.T) {
did := did.MustParseDID("did:nuts:123")
t.Run("ok", func(t *testing.T) {
ctx := newTestClient(t)
ctx.vdr.EXPECT().IsOwner(nil, did).Return(true, nil)
ctx.vdr.EXPECT().IsOwner(nil, nutsDID).Return(true, nil)

res, err := ctx.client.OAuthClientMetadata(nil, OAuthClientMetadataRequestObject{Id: did.ID})
res, err := ctx.client.OAuthClientMetadata(nil, OAuthClientMetadataRequestObject{Id: nutsDID.ID})

require.NoError(t, err)
assert.IsType(t, OAuthClientMetadata200JSONResponse{}, res)
})
t.Run("error - did not managed by this node", func(t *testing.T) {
ctx := newTestClient(t)
ctx.vdr.EXPECT().IsOwner(nil, did)
ctx.vdr.EXPECT().IsOwner(nil, nutsDID)

res, err := ctx.client.OAuthClientMetadata(nil, OAuthClientMetadataRequestObject{Id: did.ID})
res, err := ctx.client.OAuthClientMetadata(nil, OAuthClientMetadataRequestObject{Id: nutsDID.ID})

assert.Equal(t, 404, statusCodeFrom(err))
assert.Nil(t, res)
})
t.Run("error - internal error 500", func(t *testing.T) {
ctx := newTestClient(t)
ctx.vdr.EXPECT().IsOwner(nil, did).Return(false, errors.New("unknown error"))
ctx.vdr.EXPECT().IsOwner(nil, nutsDID).Return(false, errors.New("unknown error"))

res, err := ctx.client.OAuthClientMetadata(nil, OAuthClientMetadataRequestObject{Id: did.ID})
res, err := ctx.client.OAuthClientMetadata(nil, OAuthClientMetadataRequestObject{Id: nutsDID.ID})

assert.Equal(t, 500, statusCodeFrom(err))
assert.EqualError(t, err, "unknown error")
assert.Nil(t, res)
})
}

func TestWrapper_HandleAuthorizeRequest(t *testing.T) {
t.Run("missing redirect_uri", func(t *testing.T) {
ctx := newTestClient(t)

res, err := ctx.client.HandleAuthorizeRequest(requestContext(map[string]string{}), HandleAuthorizeRequestRequestObject{
Id: nutsDID.String(),
})

requireOAuthError(t, err, InvalidRequest, "redirect_uri is required")
assert.Nil(t, res)
})
t.Run("unsupported response type", func(t *testing.T) {
ctx := newTestClient(t)

res, err := ctx.client.HandleAuthorizeRequest(requestContext(map[string]string{
"redirect_uri": "https://example.com",
"response_type": "unsupported",
}), HandleAuthorizeRequestRequestObject{
Id: nutsDID.String(),
})

requireOAuthError(t, err, UnsupportedResponseType, "")
assert.Nil(t, res)
})
}

func TestWrapper_HandleTokenRequest(t *testing.T) {
t.Run("unsupported grant type", func(t *testing.T) {
ctx := newTestClient(t)

res, err := ctx.client.HandleTokenRequest(nil, HandleTokenRequestRequestObject{
Id: nutsDID.String(),
Body: &HandleTokenRequestFormdataRequestBody{
GrantType: "unsupported",
},
})

requireOAuthError(t, err, InvalidRequest, "unsupported grant_type")
assert.Nil(t, res)
})
}

func requireOAuthError(t *testing.T, err error, errorCode ErrorCode, errorDescription string) {
var oauthErr OAuth2Error
require.ErrorAs(t, err, &oauthErr)
assert.Equal(t, errorCode, oauthErr.Code)
assert.Equal(t, errorDescription, oauthErr.Description)
}

func requestContext(queryParams map[string]string) context.Context {
vals := url.Values{}
for key, value := range queryParams {
vals.Add(key, value)
}
httpRequest := &http.Request{
URL: &url.URL{
RawQuery: vals.Encode(),
},
}
return context.WithValue(audit.TestContext(), httpRequestContextKey, httpRequest)
}

// statusCodeFrom returns the statuscode if err is core.HTTPStatusCodeError, or 0 if it isn't
func statusCodeFrom(err error) int {
var SE core.HTTPStatusCodeError
Expand Down
17 changes: 6 additions & 11 deletions auth/api/iam/authorized_code.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import (
"fmt"
"github.com/labstack/echo/v4"
"github.com/nuts-foundation/nuts-node/core"
"github.com/nuts-foundation/nuts-node/vcr/openid4vci"
"html/template"
"net/http"
"net/url"
Expand Down Expand Up @@ -92,20 +91,16 @@ func (a authorizedCodeFlow) handleAuthConsent(c echo.Context) error {

func (a authorizedCodeFlow) validateCode(params map[string]string) (string, error) {
code, ok := params["code"]
invalidCodeError := OAuth2Error{
Code: InvalidRequest,
Description: "missing or invalid code parameter",
}
if !ok {
return "", openid4vci.Error{
Code: openid4vci.InvalidRequest,
StatusCode: http.StatusBadRequest,
//Description: "missing or invalid code parameter",
}
return "", invalidCodeError
}
session := a.sessions.Get(code)
if session == nil {
return "", openid4vci.Error{
Code: openid4vci.InvalidRequest,
StatusCode: http.StatusBadRequest,
//Description: "invalid code",
}
return "", invalidCodeError
}
return session.Scope, nil
}
Expand Down
Loading

0 comments on commit e814b34

Please sign in to comment.