From 5bc0e74b534ac72c8c699e02c004b3be0a69a149 Mon Sep 17 00:00:00 2001 From: jinapurapu <65002498+jinapurapu@users.noreply.github.com> Date: Tue, 12 Dec 2023 16:09:56 -0800 Subject: [PATCH] Omit detailedMessage from login errors (#3136) --- restapi/errors.go | 9 +++++- restapi/errors_test.go | 62 +++++++++++++++++++++++-------------- sso-integration/sso_test.go | 3 -- 3 files changed, 46 insertions(+), 28 deletions(-) diff --git a/restapi/errors.go b/restapi/errors.go index 06f0a4c1d..a3f6df765 100644 --- a/restapi/errors.go +++ b/restapi/errors.go @@ -53,6 +53,7 @@ var ( ErrAvoidSelfAccountDelete = errors.New("logged in user cannot be deleted by itself") ErrAccessDenied = errors.New("access denied") ErrOauth2Provider = errors.New("unable to contact configured identity provider") + ErrOauth2Login = errors.New("unable to login using configured identity provider") ErrNonUniqueAccessKey = errors.New("access key already in use") ErrRemoteTierExists = errors.New("specified remote tier already exists") ErrRemoteTierNotFound = errors.New("specified remote tier was not found") @@ -84,10 +85,12 @@ type CodedAPIError struct { func ErrorWithContext(ctx context.Context, err ...interface{}) *CodedAPIError { errorCode := 500 errorMessage := ErrDefault.Error() + var detailedMessage string var err1 error var exists bool if len(err) > 0 { if err1, exists = err[0].(error); exists { + detailedMessage = err1.Error() var lastError error if len(err) > 1 { if err2, lastExists := err[1].(error); lastExists { @@ -105,6 +108,7 @@ func ErrorWithContext(ctx context.Context, err ...interface{}) *CodedAPIError { errorMessage = ErrNotFound.Error() } if errors.Is(err1, ErrInvalidLogin) { + detailedMessage = "" errorCode = 401 errorMessage = ErrInvalidLogin.Error() } @@ -114,10 +118,12 @@ func ErrorWithContext(ctx context.Context, err ...interface{}) *CodedAPIError { } // If the last error is ErrInvalidLogin, this is a login failure if errors.Is(lastError, ErrInvalidLogin) { + detailedMessage = "" errorCode = 401 errorMessage = err1.Error() } if strings.Contains(err1.Error(), ErrLoginNotAllowed.Error()) { + detailedMessage = "" errorCode = 400 errorMessage = ErrLoginNotAllowed.Error() } @@ -211,6 +217,7 @@ func ErrorWithContext(ctx context.Context, err ...interface{}) *CodedAPIError { errorMessage = ErrAccessDenied.Error() } if madmin.ToErrorResponse(err1).Code == "InvalidAccessKeyId" { + errorCode = 401 errorMessage = ErrInvalidSession.Error() } @@ -261,7 +268,7 @@ func ErrorWithContext(ctx context.Context, err ...interface{}) *CodedAPIError { } } } - return &CodedAPIError{Code: errorCode, APIError: &models.APIError{Message: errorMessage, DetailedMessage: err1.Error()}} + return &CodedAPIError{Code: errorCode, APIError: &models.APIError{Message: errorMessage, DetailedMessage: detailedMessage}} } // Error receives an errors object and parse it against k8sErrors, returns the right errors code paired with a generic errors message diff --git a/restapi/errors_test.go b/restapi/errors_test.go index e6f6ad23f..c82750312 100644 --- a/restapi/errors_test.go +++ b/restapi/errors_test.go @@ -44,37 +44,38 @@ func TestError(t *testing.T) { } appErrors := map[string]expectedError{ - "ErrDefault": {code: 500, err: ErrDefault}, - "ErrInvalidLogin": {code: 401, err: ErrInvalidLogin}, - "ErrForbidden": {code: 403, err: ErrForbidden}, - "ErrFileTooLarge": {code: 413, err: ErrFileTooLarge}, - "ErrInvalidSession": {code: 401, err: ErrInvalidSession}, - "ErrNotFound": {code: 404, err: ErrNotFound}, - "ErrGroupAlreadyExists": {code: 400, err: ErrGroupAlreadyExists}, - "ErrInvalidErasureCodingValue": {code: 400, err: ErrInvalidErasureCodingValue}, - "ErrBucketBodyNotInRequest": {code: 400, err: ErrBucketBodyNotInRequest}, - "ErrBucketNameNotInRequest": {code: 400, err: ErrBucketNameNotInRequest}, - "ErrGroupBodyNotInRequest": {code: 400, err: ErrGroupBodyNotInRequest}, - "ErrGroupNameNotInRequest": {code: 400, err: ErrGroupNameNotInRequest}, - "ErrPolicyNameNotInRequest": {code: 400, err: ErrPolicyNameNotInRequest}, - "ErrPolicyBodyNotInRequest": {code: 400, err: ErrPolicyBodyNotInRequest}, - "ErrInvalidEncryptionAlgorithm": {code: 500, err: ErrInvalidEncryptionAlgorithm}, - "ErrSSENotConfigured": {code: 404, err: ErrSSENotConfigured}, - "ErrBucketLifeCycleNotConfigured": {code: 404, err: ErrBucketLifeCycleNotConfigured}, - "ErrChangePassword": {code: 403, err: ErrChangePassword}, - "ErrInvalidLicense": {code: 404, err: ErrInvalidLicense}, - "ErrLicenseNotFound": {code: 404, err: ErrLicenseNotFound}, - "ErrAvoidSelfAccountDelete": {code: 403, err: ErrAvoidSelfAccountDelete}, - "ErrAccessDenied": {code: 403, err: ErrAccessDenied}, + "ErrDefault": {code: 500, err: ErrDefault}, + + "ErrForbidden": {code: 403, err: ErrForbidden}, + "ErrFileTooLarge": {code: 413, err: ErrFileTooLarge}, + "ErrInvalidSession": {code: 401, err: ErrInvalidSession}, + "ErrNotFound": {code: 404, err: ErrNotFound}, + "ErrGroupAlreadyExists": {code: 400, err: ErrGroupAlreadyExists}, + "ErrInvalidErasureCodingValue": {code: 400, err: ErrInvalidErasureCodingValue}, + "ErrBucketBodyNotInRequest": {code: 400, err: ErrBucketBodyNotInRequest}, + "ErrBucketNameNotInRequest": {code: 400, err: ErrBucketNameNotInRequest}, + "ErrGroupBodyNotInRequest": {code: 400, err: ErrGroupBodyNotInRequest}, + "ErrGroupNameNotInRequest": {code: 400, err: ErrGroupNameNotInRequest}, + "ErrPolicyNameNotInRequest": {code: 400, err: ErrPolicyNameNotInRequest}, + "ErrPolicyBodyNotInRequest": {code: 400, err: ErrPolicyBodyNotInRequest}, + "ErrInvalidEncryptionAlgorithm": {code: 500, err: ErrInvalidEncryptionAlgorithm}, + "ErrSSENotConfigured": {code: 404, err: ErrSSENotConfigured}, + "ErrBucketLifeCycleNotConfigured": {code: 404, err: ErrBucketLifeCycleNotConfigured}, + "ErrChangePassword": {code: 403, err: ErrChangePassword}, + "ErrInvalidLicense": {code: 404, err: ErrInvalidLicense}, + "ErrLicenseNotFound": {code: 404, err: ErrLicenseNotFound}, + "ErrAvoidSelfAccountDelete": {code: 403, err: ErrAvoidSelfAccountDelete}, + "ErrNonUniqueAccessKey": {code: 500, err: ErrNonUniqueAccessKey}, "ErrRemoteTierExists": {code: 400, err: ErrRemoteTierExists}, "ErrRemoteTierNotFound": {code: 400, err: ErrRemoteTierNotFound}, "ErrRemoteTierUppercase": {code: 400, err: ErrRemoteTierUppercase}, "ErrRemoteTierBucketNotFound": {code: 400, err: ErrRemoteTierBucketNotFound}, "ErrRemoteInvalidCredentials": {code: 403, err: ErrRemoteInvalidCredentials}, + "ErrTooFewNodes": {code: 500, err: ErrTooFewNodes}, "ErrUnableToGetTenantUsage": {code: 500, err: ErrUnableToGetTenantUsage}, "ErrTooManyNodes": {code: 500, err: ErrTooManyNodes}, - "ErrTooFewNodes": {code: 500, err: ErrTooFewNodes}, + "ErrAccessDenied": {code: 403, err: ErrAccessDenied}, "ErrTooFewAvailableNodes": {code: 500, err: ErrTooFewAvailableNodes}, "ErrFewerThanFourNodes": {code: 500, err: ErrFewerThanFourNodes}, "ErrUnableToGetTenantLogs": {code: 500, err: ErrUnableToGetTenantLogs}, @@ -96,6 +97,7 @@ func TestError(t *testing.T) { }, }) } + tests = append(tests, testError{ name: "passing multiple errors but ErrInvalidLogin is last", @@ -104,9 +106,21 @@ func TestError(t *testing.T) { }, want: &CodedAPIError{ Code: int(401), - APIError: &models.APIError{Message: ErrDefault.Error(), DetailedMessage: ErrDefault.Error()}, + APIError: &models.APIError{Message: ErrDefault.Error(), DetailedMessage: ""}, }, }) + tests = append(tests, + testError{ + name: "login error omits detailedMessage", + args: args{ + err: []interface{}{ErrInvalidLogin}, + }, + want: &CodedAPIError{ + Code: int(401), + APIError: &models.APIError{Message: ErrInvalidLogin.Error(), DetailedMessage: ""}, + }, + }) + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { got := Error(tt.args.err...) diff --git a/sso-integration/sso_test.go b/sso-integration/sso_test.go index 7607bd4d7..d3c4d0559 100644 --- a/sso-integration/sso_test.go +++ b/sso-integration/sso_test.go @@ -288,7 +288,4 @@ func TestBadLogin(t *testing.T) { log.Println(err) assert.Nil(err) } - detailedMessage := result2.DetailedMessage - fmt.Println(detailedMessage) - assert.Equal("expected 'code' response type - got [], login not allowed", detailedMessage) }