From a74f317a326a69524d0762c6868aa4bb0b3649a9 Mon Sep 17 00:00:00 2001 From: Dmitry Zenovich Date: Mon, 30 Dec 2024 19:03:38 +0300 Subject: [PATCH 1/4] suppress output of TestAskHintRequest_UnmarshalJSON, use more strict query expectation regexp in TestAskHintRequest_UnmarshalJSON --- app/api/items/ask_hint_test.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/app/api/items/ask_hint_test.go b/app/api/items/ask_hint_test.go index a0dde18cf..e00bddbee 100644 --- a/app/api/items/ask_hint_test.go +++ b/app/api/items/ask_hint_test.go @@ -15,6 +15,7 @@ import ( "github.com/France-ioi/AlgoreaBackend/v2/app/payloadstest" "github.com/France-ioi/AlgoreaBackend/v2/app/token" "github.com/France-ioi/AlgoreaBackend/v2/app/tokentest" + "github.com/France-ioi/AlgoreaBackend/v2/testhelpers/testoutput" ) func TestAskHintRequest_UnmarshalJSON(t *testing.T) { @@ -127,12 +128,14 @@ func TestAskHintRequest_UnmarshalJSON(t *testing.T) { for _, tt := range tests { tt := tt t.Run(tt.name, func(t *testing.T) { + testoutput.SuppressIfPasses(t) + db, mock := database.NewDBMock() defer func() { _ = db.Close() }() if tt.mockDB { - mockQuery := mock.ExpectQuery(regexp.QuoteMeta("SELECT public_key " + - "FROM `platforms` JOIN items ON items.platform_id = platforms.id WHERE (items.id = ?) LIMIT 1")). + mockQuery := mock.ExpectQuery("^" + regexp.QuoteMeta("SELECT public_key "+ + "FROM `platforms` JOIN items ON items.platform_id = platforms.id WHERE (items.id = ?) LIMIT 1") + "$"). WithArgs(tt.itemID) if tt.platform != nil { From f3295ddfee46e268e4ff854083e50263388e4d94 Mon Sep 17 00:00:00 2001 From: Dmitry Zenovich Date: Mon, 30 Dec 2024 19:37:28 +0300 Subject: [PATCH 2/4] Partially revert "Hint request service: Do not allow unsigned requests and throw error if there is no key for the platform associated with the item" to handle unexpected errors properly This reverts commit 0bfbc7a84cb47286e177b248047ccc67cb7e487f. --- app/api/items/ask_hint.go | 2 +- app/api/items/save_grade.go | 3 ++- app/token/token.go | 33 +++++++++++++++++++++++++++++++++ app/token/token_test.go | 17 +++++++++++++++++ 4 files changed, 53 insertions(+), 2 deletions(-) diff --git a/app/api/items/ask_hint.go b/app/api/items/ask_hint.go index 9768a9b60..97a3ee1a0 100644 --- a/app/api/items/ask_hint.go +++ b/app/api/items/ask_hint.go @@ -248,7 +248,7 @@ func (requestData *AskHintRequest) unmarshalHintToken(wrapper *askHintRequestWra wrapper.HintRequestedToken.Bytes(), "hint_requested", ) - if err != nil { + if err != nil && !token.IsUnexpectedError(err) { return err } service.MustNotBeError(err) diff --git a/app/api/items/save_grade.go b/app/api/items/save_grade.go index 44026829b..714f11729 100644 --- a/app/api/items/save_grade.go +++ b/app/api/items/save_grade.go @@ -306,9 +306,10 @@ func (requestData *saveGradeRequestParsed) unmarshalScoreToken(wrapper *saveGrad wrapper.ScoreToken.Bytes(), "score_token", ) - if err != nil { + if err != nil && !token.IsUnexpectedError(err) { return err } + service.MustNotBeError(err) } if !hasScoreToken || !hasPlatformKey { diff --git a/app/token/token.go b/app/token/token.go index 714f48ebb..3d315b8f8 100644 --- a/app/token/token.go +++ b/app/token/token.go @@ -133,6 +133,24 @@ func Generate(payload map[string]interface{}, privateKey *rsa.PrivateKey) []byte return token } +// UnexpectedError represents an unexpected error so that we could differentiate it from expected errors. +type UnexpectedError struct { + err error +} + +// Error returns a string representation for an unexpected error. +func (ue *UnexpectedError) Error() string { + return ue.err.Error() +} + +// IsUnexpectedError returns true if its argument is an unexpected error. +func IsUnexpectedError(err error) bool { + if _, unexpected := err.(*UnexpectedError); unexpected { + return true + } + return false +} + // UnmarshalDependingOnItemPlatform unmarshals a token from JSON representation // using a platform's public key for given itemID. // The function returns nil (success) if the platform doesn't use tokens. @@ -144,11 +162,14 @@ func UnmarshalDependingOnItemPlatform( tokenFieldName string, ) (platformHasKey bool, err error) { targetRefl := reflect.ValueOf(target) + defer recoverPanics(&err) publicKey, err := store.Platforms().GetPublicKeyByItemID(itemID) if gorm.IsRecordNotFoundError(err) { return false, fmt.Errorf("cannot find the platform for item %d", itemID) } + mustNotBeError(err) + if publicKey == "" { return false, nil } @@ -175,3 +196,15 @@ func UnmarshalDependingOnItemPlatform( return true, nil } + +func mustNotBeError(err error) { + if err != nil { + panic(err) + } +} + +func recoverPanics(err *error) { // nolint:gocritic + if r := recover(); r != nil { + *err = &UnexpectedError{err: r.(error)} + } +} diff --git a/app/token/token_test.go b/app/token/token_test.go index a41b299cf..c45dde857 100644 --- a/app/token/token_test.go +++ b/app/token/token_test.go @@ -396,3 +396,20 @@ func createTmpPrivateKeyFile(key []byte) (*os.File, error) { _, err = tmpFilePrivate.Write(key) return tmpFilePrivate, err } + +func Test_recoverPanics_and_mustNotBeError(t *testing.T) { + expectedError := errors.New("some error") + err := func() (err error) { + defer recoverPanics(&err) + mustNotBeError(expectedError) + return nil + }() + assert.Equal(t, &UnexpectedError{expectedError}, err) + assert.Equal(t, expectedError.Error(), err.Error()) +} + +func Test_UnexpectedError(t *testing.T) { + assert.True(t, IsUnexpectedError(&UnexpectedError{err: errors.New("some error")})) + assert.False(t, IsUnexpectedError(errors.New("some error"))) + assert.False(t, IsUnexpectedError(nil)) +} From 587f335d7828cc80ff94e40fd8aff65238ace0be Mon Sep 17 00:00:00 2001 From: Dmitry Zenovich Date: Mon, 30 Dec 2024 19:51:31 +0300 Subject: [PATCH 3/4] add test TestAskHintRequest_UnmarshalJSON_DBError checking that DB errors are handled correctly in AskHintRequest_UnmarshalJSON() --- app/api/items/ask_hint_test.go | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/app/api/items/ask_hint_test.go b/app/api/items/ask_hint_test.go index e00bddbee..e26c6c405 100644 --- a/app/api/items/ask_hint_test.go +++ b/app/api/items/ask_hint_test.go @@ -179,3 +179,27 @@ func TestAskHintRequest_UnmarshalJSON(t *testing.T) { }) } } + +func TestAskHintRequest_UnmarshalJSON_DBError(t *testing.T) { + testoutput.SuppressIfPasses(t) + + db, mock := database.NewDBMock() + defer func() { _ = db.Close() }() + + expectedError := errors.New("error") + mock.ExpectQuery("^" + regexp.QuoteMeta("SELECT public_key "+ + "FROM `platforms` JOIN items ON items.platform_id = platforms.id WHERE (items.id = ?) LIMIT 1") + "$"). + WithArgs(901756573345831409).WillReturnError(expectedError) + + r := &AskHintRequest{ + store: database.NewDataStore(db), + publicKey: tokentest.AlgoreaPlatformPublicKeyParsed, + } + assert.PanicsWithError(t, expectedError.Error(), func() { + _ = r.UnmarshalJSON([]byte(fmt.Sprintf(`{"task_token": %q, "hint_requested": %q}`, + token.Generate(payloadstest.TaskPayloadFromAlgoreaPlatform, tokentest.AlgoreaPlatformPrivateKeyParsed), + token.Generate(payloadstest.HintPayloadFromTaskPlatform, tokentest.TaskPlatformPrivateKeyParsed), + ))) + }) + assert.NoError(t, mock.ExpectationsWereMet()) +} From 7af568a7d859f8b1d1b0a1dbb26ca2db44f1c553 Mon Sep 17 00:00:00 2001 From: Dmitry Zenovich Date: Mon, 30 Dec 2024 21:59:58 +0300 Subject: [PATCH 4/4] add test Test_saveGradeRequestParsed_UnmarshalJSON checking that DB errors are handled correctly in saveGradeRequestParsed.UnmarshalJSON() --- app/api/items/save_grade_test.go | 40 ++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) create mode 100644 app/api/items/save_grade_test.go diff --git a/app/api/items/save_grade_test.go b/app/api/items/save_grade_test.go new file mode 100644 index 000000000..25f756129 --- /dev/null +++ b/app/api/items/save_grade_test.go @@ -0,0 +1,40 @@ +package items + +import ( + "errors" + "fmt" + "regexp" + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/France-ioi/AlgoreaBackend/v2/app/database" + "github.com/France-ioi/AlgoreaBackend/v2/app/payloadstest" + "github.com/France-ioi/AlgoreaBackend/v2/app/token" + "github.com/France-ioi/AlgoreaBackend/v2/app/tokentest" + "github.com/France-ioi/AlgoreaBackend/v2/testhelpers/testoutput" +) + +func Test_saveGradeRequestParsed_UnmarshalJSON(t *testing.T) { + testoutput.SuppressIfPasses(t) + + db, mock := database.NewDBMock() + defer func() { _ = db.Close() }() + + expectedError := errors.New("error") + mock.ExpectQuery("^" + regexp.QuoteMeta("SELECT public_key "+ + "FROM `platforms` JOIN items ON items.platform_id = platforms.id WHERE (items.id = ?) LIMIT 1") + "$"). + WithArgs(901756573345831409).WillReturnError(expectedError) + + r := saveGradeRequestParsed{ + store: database.NewDataStore(db), + publicKey: tokentest.AlgoreaPlatformPublicKeyParsed, + } + assert.PanicsWithError(t, expectedError.Error(), func() { + _ = r.UnmarshalJSON([]byte(fmt.Sprintf(`{"score_token": %q, "answer_token": %q}`, + token.Generate(payloadstest.ScorePayloadFromGrader, tokentest.AlgoreaPlatformPrivateKeyParsed), + token.Generate(payloadstest.AnswerPayloadFromAlgoreaPlatform, tokentest.AlgoreaPlatformPrivateKeyParsed), + ))) + }) + assert.NoError(t, mock.ExpectationsWereMet()) +}