Skip to content

Commit

Permalink
Adding validation for request input and added more UTs. (#12)
Browse files Browse the repository at this point in the history
Change Log:
1. Added validate() Method and corresponding UTs for all the requests:
This method sanity checks the input.
2. Added UTs for error.go module. Also made some methods generic in
error.go.
3. Increased codecov coverage from 70% to 80%
4. Moved `plugin.go` from `cli/internal/mock/plugin.go` →
`internal/mock/plugin.go` to keep all the mocks in one place.

---------

Signed-off-by: Pritesh Bandi <[email protected]>
Co-authored-by: Patrick Zheng <[email protected]>
  • Loading branch information
priteshbandi and Two-Hearts authored Jan 23, 2024
1 parent 43178dd commit 4df400e
Show file tree
Hide file tree
Showing 17 changed files with 565 additions and 28 deletions.
2 changes: 1 addition & 1 deletion .github/.codecov.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,4 @@ coverage:
status:
project:
default:
target: 70%
target: 80%
15 changes: 12 additions & 3 deletions cli/cli.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
// 2. Read and unmarshal input
// 3. Execute relevant plugin functions
// 4. marshals output

package cli

import (
Expand Down Expand Up @@ -123,7 +122,7 @@ func (c *CLI) Execute(ctx context.Context, args []string) {
func (c *CLI) printVersion(ctx context.Context) {
md := c.getMetadata(ctx, c.pl)

fmt.Printf("%s - %s\nVersion: %s \n", md.Name, md.Description, md.Version)
fmt.Printf("%s - %s\nVersion: %s\n", md.Name, md.Description, md.Version)
}

// validateArgs validate commands/arguments passed to executable.
Expand All @@ -135,11 +134,21 @@ func (c *CLI) validateArgs(ctx context.Context, args []string) {
}

// unmarshalRequest reads input from std.in and unmarshal it into given request struct
func (c *CLI) unmarshalRequest(request any) error {
func (c *CLI) unmarshalRequest(request plugin.Request) error {
if err := json.NewDecoder(os.Stdin).Decode(request); err != nil {
c.logger.Errorf("%s unmarshalling error :%v", reflect.TypeOf(request), err)
return plugin.NewJSONParsingError(plugin.ErrorMsgMalformedInput)
}

if err := request.Validate(); err != nil {
c.logger.Errorf("%s validation error :%v", reflect.TypeOf(request), err)
var plError *plugin.Error
if errors.As(err, &plError) {
return plugin.NewValidationErrorf("%s: %s", plugin.ErrorMsgMalformedInput, plError.Message)
}
return plugin.NewValidationErrorf("%s", plugin.ErrorMsgMalformedInput)
}

return nil
}

Expand Down
28 changes: 16 additions & 12 deletions cli/cli_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import (
"strings"
"testing"

"github.com/notaryproject/notation-plugin-framework-go/cli/internal/mock"
"github.com/notaryproject/notation-plugin-framework-go/internal/mock"
"github.com/notaryproject/notation-plugin-framework-go/plugin"
)

Expand All @@ -34,7 +34,7 @@ var errorCli, _ = New(mock.NewPlugin(true))
func TestNewWithLogger(t *testing.T) {
_, err := NewWithLogger(nil, &discardLogger{})
if err == nil {
t.Fatalf("NewWithLogger() expected error but not found")
t.Errorf("NewWithLogger() expected error but not found")
}
}

Expand All @@ -56,7 +56,6 @@ func TestMarshalResponse(t *testing.T) {
}

func TestMarshalResponseError(t *testing.T) {

_, err := cli.marshalResponse(nil, fmt.Errorf("expected error thrown"))
assertErr(t, err, plugin.ErrorCodeGeneric)

Expand Down Expand Up @@ -89,17 +88,17 @@ func TestUnmarshalRequestError(t *testing.T) {
var request plugin.DescribeKeyRequest
err := cli.unmarshalRequest(&request)
if err == nil {
t.Fatalf("unmarshalRequest() expected error but not found")
t.Errorf("unmarshalRequest() expected error but not found")
}

plgErr, ok := err.(*plugin.Error)
if !ok {
t.Fatalf("unmarshalRequest() expected error of type plugin.Error but found %s", reflect.TypeOf(err))
t.Errorf("unmarshalRequest() expected error of type plugin.Error but found %s", reflect.TypeOf(err))
}

expectedErrStr := "{\"errorCode\":\"VALIDATION_ERROR\",\"errorMessage\":\"Input is not a valid JSON\"}"
if plgErr.Error() != expectedErrStr {
t.Fatalf("unmarshalRequest() expected error string to be %s but found %s", expectedErrStr, plgErr.Error())
t.Errorf("unmarshalRequest() expected error string to be %s but found %s", expectedErrStr, plgErr.Error())
}
}

Expand All @@ -115,48 +114,54 @@ func TestGetMetadataError(t *testing.T) {
if e, ok := err.(*exec.ExitError); ok && !e.Success() {
return
}
t.Fatalf("process ran with err %v, want exit status 1", err)
t.Errorf("process ran with err %v, want exit status 1", err)
}

func TestExecuteSuccess(t *testing.T) {
sigGenCli, _ := New(mock.NewSigGeneratorPlugin(false))
tests := map[string]struct {
c *CLI
in string
op string
}{
string(plugin.CommandGetMetadata): {
c: cli,
in: "{}",
op: "{\"name\":\"Example Plugin\",\"description\":\"This is an description of example plugin. 🍺\",\"version\":\"1.0.0\",\"url\":\"https://example.com/notation/plugin\",\"capabilities\":[\"SIGNATURE_VERIFIER.TRUSTED_IDENTITY\",\"SIGNATURE_VERIFIER.REVOCATION_CHECK\",\"SIGNATURE_GENERATOR.ENVELOPE\"]}",
},
string(plugin.Version): {
c: cli,
op: "Example Plugin - This is an description of example plugin. 🍺\nVersion: 1.0.0 \n",
in: "",
op: "Example Plugin - This is an description of example plugin. 🍺\nVersion: 1.0.0\n",
},
string(plugin.CommandGenerateEnvelope): {
c: cli,
op: "{\"signatureEnvelope\":\"\",\"signatureEnvelopeType\":\"\",\"annotations\":{\"manifestAnntnKey1\":\"value1\"}}",
in: "{\"contractVersion\":\"1.0\",\"keyId\":\"someKeyId\",\"payloadType\":\"somePT\",\"signatureEnvelopeType\":\"someSET\",\"payload\":\"em9w\"}",
op: "{\"signatureEnvelope\":\"\",\"signatureEnvelopeType\":\"someSET\",\"annotations\":{\"manifestAnntnKey1\":\"value1\"}}",
},
string(plugin.CommandVerifySignature): {
c: cli,
in: "{\"contractVersion\":\"1.0\",\"signature\":{\"criticalAttributes\":{\"contentType\":\"someCT\",\"signingScheme\":\"someSigningScheme\"},\"unprocessedAttributes\":null,\"certificateChain\":[\"emFw\",\"em9w\"]},\"trustPolicy\":{\"trustedIdentities\":null,\"signatureVerification\":[\"SIGNATURE_GENERATOR.RAW\"]}}",
op: "{\"verificationResults\":{\"SIGNATURE_VERIFIER.REVOCATION_CHECK\":{\"success\":true,\"reason\":\"Not revoked\"},\"SIGNATURE_VERIFIER.TRUSTED_IDENTITY\":{\"success\":true,\"reason\":\"Valid trusted Identity\"}},\"processedAttributes\":[]}",
},
string(plugin.CommandGenerateSignature): {
c: sigGenCli,
in: "{\"contractVersion\":\"1.0\",\"keyId\":\"someKeyId\",\"keySpec\":\"EC-384\",\"hashAlgorithm\":\"SHA-384\",\"payload\":\"em9w\"}",
op: "{\"keyId\":\"someKeyId\",\"signature\":\"YWJjZA==\",\"signingAlgorithm\":\"RSASSA-PSS-SHA-256\",\"certificateChain\":[\"YWJjZA==\",\"d3h5eg==\"]}",
},
string(plugin.CommandDescribeKey): {
c: sigGenCli,
in: "{\"contractVersion\":\"1.0\",\"keyId\":\"someKeyId\"}",
op: "{\"keyId\":\"someKeyId\",\"keySpec\":\"RSA-2048\"}",
},
}
for name, test := range tests {
t.Run(name, func(t *testing.T) {
closer := setupReader("{}")
closer := setupReader(test.in)
defer closer()
op := captureStdOut(func() {
test.c.Execute(context.Background(), []string{"notation", name})
})
fmt.Println(op)
if op != test.op {
t.Errorf("Execute() with '%s' args, expected '%s' but got '%s'", name, test.op, op)
}
Expand Down Expand Up @@ -206,6 +211,5 @@ func assertErr(t *testing.T, err error, code plugin.ErrorCode) {
}
t.Errorf("mismatch in error code: \n expected: %s\n actual : %s", code, plgErr.ErrCode)
}

t.Errorf("expected error of type PluginError but found %s", reflect.TypeOf(err))
}
2 changes: 1 addition & 1 deletion cli/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func getValidArgs(md *plugin.GetMetadataResponse) []string {

// deliverError print to standard error and then return nonzero exit code
func deliverError(message string) {
_, _ = fmt.Fprintln(os.Stderr, message)
_, _ = fmt.Fprint(os.Stderr, message)
os.Exit(1)
}

Expand Down
9 changes: 5 additions & 4 deletions example/plugin.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,10 +91,11 @@ func (p *ExamplePlugin) VerifySignature(ctx context.Context, req *plugin.VerifyS

func (p *ExamplePlugin) GetMetadata(ctx context.Context, req *plugin.GetMetadataRequest) (*plugin.GetMetadataResponse, error) {
return &plugin.GetMetadataResponse{
Name: "Example Plugin",
Description: "This is an description of example plugin. 🍺",
URL: "https://example.com/notation/plugin",
Version: "1.0.0",
SupportedContractVersions: []string{plugin.ContractVersion},
Name: "com.example.plugin",
Description: "This is an description of example plugin",
URL: "https://example.com/notation/plugin",
Version: "1.0.0",
Capabilities: []plugin.Capability{
plugin.CapabilityEnvelopeGenerator,
plugin.CapabilityTrustedIdentityVerifier,
Expand Down
File renamed without changes.
13 changes: 13 additions & 0 deletions plugin/describekey.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,19 @@ func (DescribeKeyRequest) Command() Command {
return CommandDescribeKey
}

// Validate validates DescribeKeyRequest struct
func (r DescribeKeyRequest) Validate() error {
if r.ContractVersion == "" {
return NewValidationError("contractVersion cannot be empty")
}

if r.KeyID == "" {
return NewValidationError("keyId cannot be empty")
}

return nil
}

// DescribeKeyResponse is the response of a describe-key request.
type DescribeKeyResponse struct {
// The same key id as passed in the request.
Expand Down
72 changes: 72 additions & 0 deletions plugin/describekey_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
// Copyright The Notary Project Authors.
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package plugin

import (
"fmt"
"testing"
)

func TestDescribeKeyRequest_Validate(t *testing.T) {
reqs := []DescribeKeyRequest{
getDescribeKeyRequest(ContractVersion, "someKeyId"),
{
ContractVersion: ContractVersion,
KeyID: "someKeyId",
PluginConfig: map[string]string{"someKey": "someValue"}},
}

for _, req := range reqs {
if err := req.Validate(); err != nil {
t.Errorf("VerifySignatureRequest#Validate failed with error: %+v", err)
}
}
}

func TestDescribeKeyRequest_Validate_Error(t *testing.T) {
testCases := []struct {
name string
req DescribeKeyRequest
}{
{name: "contractVersion", req: getDescribeKeyRequest("", "someKeyId")},
{name: "keyId", req: getDescribeKeyRequest(ContractVersion, "")},
}

for _, testcase := range testCases {
t.Run(testcase.name, func(t *testing.T) {
if err := testcase.req.Validate(); err != nil {
expMsg := fmt.Sprintf("{\"errorCode\":\"VALIDATION_ERROR\",\"errorMessage\":\"%s cannot be empty\"}", testcase.name)
if err.Error() != expMsg {
t.Errorf("expected error message '%s' but got '%s'", expMsg, err.Error())
}
} else {
t.Error("DescribeKeyRequest#Validate didn't returned error")
}
})
}
}

func TestDescribeKeyRequest_Command(t *testing.T) {
req := getDescribeKeyRequest(ContractVersion, "someKeyId")
if cmd := req.Command(); cmd != CommandDescribeKey {
t.Errorf("DescribeKeyRequest#Command, expected %s but returned %s", CommandDescribeKey, cmd)
}
}

func getDescribeKeyRequest(cv, kid string) DescribeKeyRequest {
return DescribeKeyRequest{
ContractVersion: cv,
KeyID: kid,
}
}
11 changes: 5 additions & 6 deletions plugin/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,7 @@ const (
ErrorMsgMalformedOutputFmt string = "Failed to generate response. Error: %s"
)

// Error is used when the signature associated is no longer
// valid.
// Error is used to return a well-formed error response as per NotaryProject specification.
type Error struct {
ErrCode ErrorCode `json:"errorCode"`
Message string `json:"errorMessage,omitempty"`
Expand All @@ -53,8 +52,8 @@ func NewGenericError(msg string) *Error {
return NewError(ErrorCodeGeneric, msg)
}

func NewGenericErrorf(format string, msg string) *Error {
return NewError(ErrorCodeGeneric, fmt.Sprintf(format, msg))
func NewGenericErrorf(format string, msg ...any) *Error {
return NewError(ErrorCodeGeneric, fmt.Sprintf(format, msg...))
}

func NewUnsupportedError(msg string) *Error {
Expand All @@ -65,8 +64,8 @@ func NewValidationError(msg string) *Error {
return NewError(ErrorCodeValidation, msg)
}

func NewValidationErrorf(format string, msg string) *Error {
return NewError(ErrorCodeValidation, fmt.Sprintf(format, msg))
func NewValidationErrorf(format string, msg ...any) *Error {
return NewError(ErrorCodeValidation, fmt.Sprintf(format, msg...))
}

func NewUnsupportedContractVersionError(version string) *Error {
Expand Down
65 changes: 65 additions & 0 deletions plugin/errors_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
// Copyright The Notary Project Authors.
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package plugin

import (
"testing"
)

func TestNewError(t *testing.T) {
msg := "someMSg"
err := NewError(ErrorCodeValidation, msg)
if err != nil {
if ErrorCodeValidation != err.ErrCode {
t.Errorf("NewError, expected errorCode '%s' but found'%s'", ErrorCodeValidation, err.ErrCode)
}

if msg != err.Message {
t.Errorf("NewError, expected message'%s' but found '%s'", msg, err.Message)
}

if err.Metadata != nil {
t.Errorf("NewError, expected metadata to be nil but found '%s'", err.Metadata)
}

expError := "{\"errorCode\":\"VALIDATION_ERROR\",\"errorMessage\":\"someMSg\"}"
if expError != err.Error() {
t.Errorf("NewError#Error, expected error to be '%s' but found '%s'", expError, err.Error())

}

} else {
t.Error("NewError didn't return an error")
}
}

func TestErrorCodes(t *testing.T) {
testCases := []struct {
err *Error
errCode ErrorCode
}{
{err: NewValidationError(""), errCode: ErrorCodeValidation},
{err: NewValidationErrorf("%s", ""), errCode: ErrorCodeValidation},
{err: NewUnsupportedError(""), errCode: ErrorCodeValidation},
{err: NewGenericError(""), errCode: ErrorCodeGeneric},
{err: NewGenericErrorf("%s", ""), errCode: ErrorCodeGeneric},
{err: NewJSONParsingError(""), errCode: ErrorCodeValidation},
{err: NewUnsupportedContractVersionError(""), errCode: ErrorCodeUnsupportedContractVersion},
}
for _, test := range testCases {
if test.errCode != test.err.ErrCode {
t.Errorf("Expected errorCode %s but found %s", test.errCode, test.err.ErrCode)
}
}
}
5 changes: 5 additions & 0 deletions plugin/metadata.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,11 @@ func (GetMetadataRequest) Command() Command {
return CommandGetMetadata
}

// Validate validates GetMetadataRequest struct
func (GetMetadataRequest) Validate() error {
return nil
}

// GetMetadataResponse provided by the plugin.
type GetMetadataResponse struct {
Name string `json:"name"`
Expand Down
Loading

0 comments on commit 4df400e

Please sign in to comment.