Fix GetCredentialsBy API (#54)

* fix: GetCredentialsBy api * fix: test * sanitized logs Co-authored-by: Gabe <gcohen@squareup.com>
TBD54566975 · Jul 11, 2022 · 2368b87 · 2368b87
1 parent 9c60a26
commit 2368b87
Show file tree

Hide file tree

Showing 6 changed files with 48 additions and 27 deletions.
diff --git a/internal/util/util.go b/internal/util/util.go
@@ -2,9 +2,10 @@ package util
 
 import (
 	"fmt"
+	"strings"
+
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
-	"strings"
 )
 
 // GetMethodForDID gets a DID method from a did, the second part of the did (e.g. did:test:abcd, the method is 'test')
@@ -31,6 +32,12 @@ func LoggingNewError(msg string) error {
 
 // LoggingErrorMsg is a utility to combine logging an error, and returning and error with a message
 func LoggingErrorMsg(err error, msg string) error {
-	logrus.WithError(err).Error(msg)
+	logrus.WithError(err).Error(SanitizingLog(msg))
 	return errors.Wrap(err, msg)
 }
+
+// Sanitizing before it is logged
+func SanitizingLog(log string) string {
+	escapedLog := strings.Replace(log, "\n", "", -1)
+	return strings.Replace(escapedLog, "\r", "", -1)
+}
diff --git a/pkg/server/framework/util.go b/pkg/server/framework/util.go
@@ -2,6 +2,8 @@ package framework
 
 import (
 	"context"
+	"net/http"
+
 	"github.com/dimfeld/httptreemux/v5"
 )
 
@@ -14,3 +16,12 @@ func GetParam(ctx context.Context, param string) *string {
 	}
 	return &method
 }
+
+// GetQueryValue is a utility to get a parameter value from the query string, nil if not found
+func GetQueryValue(r *http.Request, param string) *string {
+	v := r.URL.Query().Get(param)
+	if v == "" {
+		return nil
+	}
+	return &v
+}
diff --git a/pkg/server/router/credential.go b/pkg/server/router/credential.go
@@ -8,6 +8,7 @@ import (
 	credsdk "github.com/TBD54566975/ssi-sdk/credential"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
+	"github.com/tbd54566975/ssi-service/internal/util"
 	"github.com/tbd54566975/ssi-service/pkg/server/framework"
 	"github.com/tbd54566975/ssi-service/pkg/service/credential"
 	svcframework "github.com/tbd54566975/ssi-service/pkg/service/framework"
@@ -149,9 +150,9 @@ type GetCredentialsResponse struct {
 // @Failure      500      {string}  string  "Internal server error"
 // @Router       /v1/credentials [get]
 func (cr CredentialRouter) GetCredentials(ctx context.Context, w http.ResponseWriter, r *http.Request) error {
-	issuer := framework.GetParam(ctx, IssuerParam)
-	schema := framework.GetParam(ctx, SchemaParam)
-	subject := framework.GetParam(ctx, SubjectParam)
+	issuer := framework.GetQueryValue(r, IssuerParam)
+	schema := framework.GetQueryValue(r, SchemaParam)
+	subject := framework.GetQueryValue(r, SubjectParam)
 
 	err := framework.NewRequestErrorMsg("must use one of the following query parameters: issuer, subject, schema", http.StatusBadRequest)
 
@@ -175,7 +176,7 @@ func (cr CredentialRouter) GetCredentials(ctx context.Context, w http.ResponseWr
 func (cr CredentialRouter) getCredentialsByIssuer(issuer string, ctx context.Context, w http.ResponseWriter, r *http.Request) error {
 	gotCredentials, err := cr.service.GetCredentialsByIssuer(credential.GetCredentialByIssuerRequest{Issuer: issuer})
 	if err != nil {
-		errMsg := fmt.Sprintf("could not get credentials for issuer: %s", issuer)
+		errMsg := fmt.Sprintf("could not get credentials for issuer: %s", util.SanitizingLog(issuer))
 		logrus.WithError(err).Error(errMsg)
 		return framework.NewRequestErrorMsg(errMsg, http.StatusInternalServerError)
 	}
@@ -187,7 +188,7 @@ func (cr CredentialRouter) getCredentialsByIssuer(issuer string, ctx context.Con
 func (cr CredentialRouter) getCredentialsBySubject(subject string, ctx context.Context, w http.ResponseWriter, r *http.Request) error {
 	gotCredentials, err := cr.service.GetCredentialsBySubject(credential.GetCredentialBySubjectRequest{Subject: subject})
 	if err != nil {
-		errMsg := fmt.Sprintf("could not get credentials for subject: %s", subject)
+		errMsg := fmt.Sprintf("could not get credentials for subject: %s", util.SanitizingLog(subject))
 		logrus.WithError(err).Error(errMsg)
 		return framework.NewRequestErrorMsg(errMsg, http.StatusInternalServerError)
 	}
@@ -199,7 +200,7 @@ func (cr CredentialRouter) getCredentialsBySubject(subject string, ctx context.C
 func (cr CredentialRouter) getCredentialsBySchema(schema string, ctx context.Context, w http.ResponseWriter, r *http.Request) error {
 	gotCredentials, err := cr.service.GetCredentialsBySchema(credential.GetCredentialBySchemaRequest{Schema: schema})
 	if err != nil {
-		errMsg := fmt.Sprintf("could not get credentials for schema: %s", schema)
+		errMsg := fmt.Sprintf("could not get credentials for schema: %s", util.SanitizingLog(schema))
 		logrus.WithError(err).Error(errMsg)
 		return framework.NewRequestErrorMsg(errMsg, http.StatusInternalServerError)
 	}

diff --git a/pkg/server/server_test.go b/pkg/server/server_test.go
@@ -4,6 +4,13 @@ import (
 	"bytes"
 	"context"
 	"fmt"
+	"io"
+	"net/http"
+	"net/http/httptest"
+	"os"
+	"testing"
+	"time"
+
 	credsdk "github.com/TBD54566975/ssi-sdk/credential"
 	"github.com/TBD54566975/ssi-sdk/crypto"
 	"github.com/dimfeld/httptreemux/v5"
@@ -19,12 +26,6 @@ import (
 	svcframework "github.com/tbd54566975/ssi-service/pkg/service/framework"
 	"github.com/tbd54566975/ssi-service/pkg/service/schema"
 	"github.com/tbd54566975/ssi-service/pkg/storage"
-	"io"
-	"net/http"
-	"net/http/httptest"
-	"os"
-	"testing"
-	"time"
 )
 
 func TestHealthCheckAPI(t *testing.T) {
@@ -532,8 +533,8 @@ func TestCredentialAPI(t *testing.T) {
 		assert.NoError(tt, err)
 
 		// get credential by schema
-		req = httptest.NewRequest(http.MethodGet, "https://ssi-service.com/v1/credential", nil)
-		err = credService.GetCredentials(newRequestContextWithParams(map[string]string{"schema": schemaID}), w, req)
+		req = httptest.NewRequest(http.MethodGet, fmt.Sprintf("https://ssi-service.com/v1/credential?schema=%s", schemaID), nil)
+		err = credService.GetCredentials(newRequestContext(), w, req)
 		assert.NoError(tt, err)
 
 		var getCredsResp router.GetCredentialsResponse
@@ -575,8 +576,8 @@ func TestCredentialAPI(t *testing.T) {
 		assert.NoError(tt, err)
 
 		// get credential by issuer id
-		req = httptest.NewRequest(http.MethodGet, "https://ssi-service.com/v1/credential", nil)
-		err = credService.GetCredentials(newRequestContextWithParams(map[string]string{"issuer": issuerID}), w, req)
+		req = httptest.NewRequest(http.MethodGet, fmt.Sprintf("https://ssi-service.com/v1/credential?issuer=%s", issuerID), nil)
+		err = credService.GetCredentials(newRequestContext(), w, req)
 		assert.NoError(tt, err)
 
 		var getCredsResp router.GetCredentialsResponse
@@ -618,8 +619,8 @@ func TestCredentialAPI(t *testing.T) {
 		assert.NoError(tt, err)
 
 		// get credential by subject id
-		req = httptest.NewRequest(http.MethodGet, "https://ssi-service.com/v1/credential", nil)
-		err = credService.GetCredentials(newRequestContextWithParams(map[string]string{"subject": subjectID}), w, req)
+		req = httptest.NewRequest(http.MethodGet, fmt.Sprintf("https://ssi-service.com/v1/credential?subject=%s", subjectID), nil)
+		err = credService.GetCredentials(newRequestContext(), w, req)
 		assert.NoError(tt, err)
 
 		var getCredsResp router.GetCredentialsResponse

diff --git a/pkg/service/credential/credential.go b/pkg/service/credential/credential.go
@@ -149,7 +149,7 @@ func (s Service) GetCredential(request GetCredentialRequest) (*GetCredentialResp
 
 func (s Service) GetCredentialsByIssuer(request GetCredentialByIssuerRequest) (*GetCredentialsResponse, error) {
 
-	logrus.Debugf("getting credential(s) for issuer: %s", request.Issuer)
+	logrus.Debugf("getting credential(s) for issuer: %s", util.SanitizingLog(request.Issuer))
 
 	gotCreds, err := s.storage.GetCredentialsByIssuer(request.Issuer)
 	if err != nil {
@@ -168,7 +168,7 @@ func (s Service) GetCredentialsByIssuer(request GetCredentialByIssuerRequest) (*
 
 func (s Service) GetCredentialsBySubject(request GetCredentialBySubjectRequest) (*GetCredentialsResponse, error) {
 
-	logrus.Debugf("getting credential(s) for subject: %s", request.Subject)
+	logrus.Debugf("getting credential(s) for subject: %s", util.SanitizingLog(request.Subject))
 
 	gotCreds, err := s.storage.GetCredentialsBySubject(request.Subject)
 	if err != nil {
@@ -187,7 +187,7 @@ func (s Service) GetCredentialsBySubject(request GetCredentialBySubjectRequest)
 
 func (s Service) GetCredentialsBySchema(request GetCredentialBySchemaRequest) (*GetCredentialsResponse, error) {
 
-	logrus.Debugf("getting credential(s) for schema: %s", request.Schema)
+	logrus.Debugf("getting credential(s) for schema: %s", util.SanitizingLog(request.Schema))
 
 	gotCreds, err := s.storage.GetCredentialsBySchema(request.Schema)
 	if err != nil {

diff --git a/pkg/service/credential/storage/bolt.go b/pkg/service/credential/storage/bolt.go
@@ -2,12 +2,13 @@ package storage
 
 import (
 	"fmt"
+	"strings"
+
 	"github.com/goccy/go-json"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	"github.com/tbd54566975/ssi-service/internal/util"
 	"github.com/tbd54566975/ssi-service/pkg/storage"
-	"strings"
 )
 
 const (
@@ -94,7 +95,7 @@ func (b BoltCredentialStorage) GetCredentialsByIssuer(issuer string) ([]StoredCr
 		}
 	}
 	if len(issuerKeys) == 0 {
-		logrus.Warnf("no credentials found for issuer: %s", issuer)
+		logrus.Warnf("no credentials found for issuer: %s", util.SanitizingLog(issuer))
 		return nil, nil
 	}
 
@@ -138,7 +139,7 @@ func (b BoltCredentialStorage) GetCredentialsBySubject(subject string) ([]Stored
 		}
 	}
 	if len(subjectKeys) == 0 {
-		logrus.Warnf("no credentials found for subject: %s", subject)
+		logrus.Warnf("no credentials found for subject: %s", util.SanitizingLog(subject))
 		return nil, nil
 	}
 
@@ -183,7 +184,7 @@ func (b BoltCredentialStorage) GetCredentialsBySchema(schema string) ([]StoredCr
 		}
 	}
 	if len(schemaKeys) == 0 {
-		logrus.Warnf("no credentials found for schema: %s", schema)
+		logrus.Warnf("no credentials found for schema: %s", util.SanitizingLog(schema))
 		return nil, nil
 	}