From dba7d88523cab6c63ab4a7e0fa98af3a1235846d Mon Sep 17 00:00:00 2001 From: Hugo Date: Thu, 25 May 2023 15:43:52 +0100 Subject: [PATCH] fix(handlers/scopes): Whitespace on Name and Description fields (#3235) This commit fixes the behaviour of scopes where whitespace was allowed (and thus persisted) in the Name and Description fields. It also adds some more tests for scope Create and Updateto cover more error cases. For scope update, `handlers.ValidateUpdateRequest` exists, however this was not used as it does not work properly for requests where the `global` scope is updated. In `ValidateUpdateRequest`, an ID check is performed and it assumes there's always a resource prefix (eg: "p", "o") and an identifier (random numbers and letters) after the prefix. This fails for `global`. See `handlers.ValidId`. --- .../handlers/scopes/scope_service.go | 71 +++--- .../handlers/scopes/scope_service_test.go | 212 +++++++++++++++++- 2 files changed, 250 insertions(+), 33 deletions(-) diff --git a/internal/daemon/controller/handlers/scopes/scope_service.go b/internal/daemon/controller/handlers/scopes/scope_service.go index bc2cd53bd..c5cba363e 100644 --- a/internal/daemon/controller/handlers/scopes/scope_service.go +++ b/internal/daemon/controller/handlers/scopes/scope_service.go @@ -847,39 +847,26 @@ func validateGetRequest(req *pbs.GetScopeRequest) error { } func validateCreateRequest(req *pbs.CreateScopeRequest) error { - badFields := map[string]string{} - item := req.GetItem() - if item.GetScopeId() == "" { - badFields["scope_id"] = "Missing value for scope_id." - } - switch item.GetType() { - case scope.Global.String(): - badFields["type"] = "Cannot create a global scope." - case scope.Org.String(): - if !strings.EqualFold(scope.Global.String(), item.GetScopeId()) { - badFields["type"] = "Org scopes can only be created under the global scope." + return handlers.ValidateCreateRequest(req.GetItem(), func() map[string]string { + badFields := map[string]string{} + item := req.GetItem() + if item.GetScopeId() == "" { + badFields["scope_id"] = "Missing value for scope_id." } - case scope.Project.String(): - if !handlers.ValidId(handlers.Id(item.GetScopeId()), scope.Org.Prefix()) { - badFields["type"] = "Project scopes can only be created under an org scope." + switch item.GetType() { + case scope.Global.String(): + badFields["type"] = "Cannot create a global scope." + case scope.Org.String(): + if !strings.EqualFold(scope.Global.String(), item.GetScopeId()) { + badFields["type"] = "Org scopes can only be created under the global scope." + } + case scope.Project.String(): + if !handlers.ValidId(handlers.Id(item.GetScopeId()), scope.Org.Prefix()) { + badFields["type"] = "Project scopes can only be created under an org scope." + } } - } - if item.GetId() != "" { - badFields["id"] = "This is a read only field." - } - if item.GetCreatedTime() != nil { - badFields["created_time"] = "This is a read only field." - } - if item.GetUpdatedTime() != nil { - badFields["updated_time"] = "This is a read only field." - } - if item.GetVersion() != 0 { - badFields["version"] = "This cannot be specified at create time." - } - if len(badFields) > 0 { - return handlers.InvalidArgumentErrorf("Error in provided request.", badFields) - } - return nil + return badFields + }) } func validateUpdateRequest(req *pbs.UpdateScopeRequest) error { @@ -932,6 +919,28 @@ func validateUpdateRequest(req *pbs.UpdateScopeRequest) error { if item.GetPrimaryAuthMethodId().GetValue() != "" && !handlers.ValidId(handlers.Id(item.GetPrimaryAuthMethodId().GetValue()), globals.PasswordAuthMethodPrefix, globals.OidcAuthMethodPrefix, globals.LdapAuthMethodPrefix) { badFields["primary_auth_method_id"] = "Improperly formatted identifier." } + if item.GetName() != nil { + trimmed := strings.TrimSpace(item.GetName().GetValue()) + switch { + case trimmed == "": + badFields["name"] = "Cannot set empty string as name" + case !handlers.ValidNameDescription(trimmed): + badFields["name"] = "Name contains unprintable characters" + default: + item.GetName().Value = trimmed + } + } + if item.GetDescription() != nil { + trimmed := strings.TrimSpace(item.GetDescription().GetValue()) + switch { + case trimmed == "": + badFields["description"] = "Cannot set empty string as description" + case !handlers.ValidNameDescription(trimmed): + badFields["description"] = "Description contains unprintable characters" + default: + item.GetDescription().Value = trimmed + } + } if len(badFields) > 0 { return handlers.InvalidArgumentErrorf("Error in provided request.", badFields) } diff --git a/internal/daemon/controller/handlers/scopes/scope_service_test.go b/internal/daemon/controller/handlers/scopes/scope_service_test.go index 5ff5f2ff8..7abbf797c 100644 --- a/internal/daemon/controller/handlers/scopes/scope_service_test.go +++ b/internal/daemon/controller/handlers/scopes/scope_service_test.go @@ -9,6 +9,7 @@ import ( "fmt" "strings" "testing" + "unicode" "github.com/golang/protobuf/ptypes/wrappers" "github.com/google/go-cmp/cmp" @@ -738,6 +739,56 @@ func TestCreate(t *testing.T) { }, }, }, + { + name: "Create a valid org with leading and trailing whitespace on name and description", + scopeId: scope.Global.String(), + req: &pbs.CreateScopeRequest{ + Item: &pb.Scope{ + ScopeId: scope.Global.String(), + Description: &wrapperspb.StringValue{Value: " test description with whitespace "}, + Type: scope.Org.String(), + Name: &wrapperspb.StringValue{Value: " test org name with whitespace "}, + }, + }, + res: &pbs.CreateScopeResponse{ + Uri: "scopes/o_", + Item: &pb.Scope{ + ScopeId: scope.Global.String(), + Scope: &pb.ScopeInfo{Id: scope.Global.String(), Type: scope.Global.String(), Name: scope.Global.String(), Description: "Global Scope"}, + Description: &wrapperspb.StringValue{Value: "test description with whitespace"}, // assert the whitespace is trimmed + Name: &wrapperspb.StringValue{Value: "test org name with whitespace"}, // assert the whitespace is trimmed + Version: 1, + Type: scope.Org.String(), + AuthorizedActions: testAuthorizedActions, + AuthorizedCollectionActions: orgAuthorizedCollectionActions, + }, + }, + }, + { + name: "Create a global type scope", + scopeId: scope.Global.String(), + req: &pbs.CreateScopeRequest{ + Item: &pb.Scope{ + ScopeId: scope.Global.String(), + Description: &wrapperspb.StringValue{Value: "desc"}, + Type: scope.Global.String(), + }, + }, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), + }, + { + name: "Create with a custom version number", + scopeId: scope.Global.String(), + req: &pbs.CreateScopeRequest{ + Item: &pb.Scope{ + ScopeId: scope.Global.String(), + Description: &wrapperspb.StringValue{Value: "desc"}, + Type: scope.Org.String(), + Version: 5, + }, + }, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), + }, { name: "Project with bad type specified", scopeId: defaultOrg.GetPublicId(), @@ -809,7 +860,26 @@ func TestCreate(t *testing.T) { require.NoError(err, "Error when getting new project service.") if name != "" { - name = fmt.Sprintf("%s-%t", name, withUserId) + // Test cases can specify names with leading and trailing + // whitespace to test for Boundary's whitespace removal. + // Since we're adding to the name before we send the request + // off, we need to make sure we keep it as we found it. + + // Find where leading whitespace ends, add withUserId there. + idx := 0 + for i, r := range name { + if !unicode.IsSpace(r) { + idx = i + break + } + } + + if idx == 0 { + name = fmt.Sprintf("%t-%s", withUserId, name) + } else { + name = fmt.Sprintf("%s%t-%s", name[:idx], withUserId, name[idx:]) + } + req.GetItem().GetName().Value = name } var userId string @@ -866,7 +936,7 @@ func TestCreate(t *testing.T) { } // Clear all values which are hard to compare against. - assert.Equal(name, got.GetItem().GetName().GetValue()) + assert.Equal(strings.TrimSpace(name), got.GetItem().GetName().GetValue()) got.Item.Name = tc.res.GetItem().GetName() got.Uri, tc.res.Uri = "", "" got.Item.Id, tc.res.Item.Id = "", "" @@ -994,6 +1064,33 @@ func TestUpdate(t *testing.T) { }, }, }, + { + name: "Update org name and description with whitespace", + scopeId: scope.Global.String(), + req: &pbs.UpdateScopeRequest{ + UpdateMask: &field_mask.FieldMask{ + Paths: []string{"name", "description"}, + }, + Item: &pb.Scope{ + Name: &wrapperspb.StringValue{Value: " new name "}, + Description: &wrapperspb.StringValue{Value: " new desc "}, + Type: scope.Org.String(), + }, + }, + res: &pbs.UpdateScopeResponse{ + Item: &pb.Scope{ + Id: org.GetPublicId(), + ScopeId: scope.Global.String(), + Scope: &pb.ScopeInfo{Id: scope.Global.String(), Type: scope.Global.String(), Name: scope.Global.String(), Description: "Global Scope"}, + Name: &wrapperspb.StringValue{Value: "new name"}, + Description: &wrapperspb.StringValue{Value: "new desc"}, + CreatedTime: org.GetCreateTime().GetTimestamp(), + Type: scope.Org.String(), + AuthorizedActions: testAuthorizedActions, + AuthorizedCollectionActions: orgAuthorizedCollectionActions, + }, + }, + }, { name: "Update global", scopeId: scope.Global.String(), @@ -1083,6 +1180,117 @@ func TestUpdate(t *testing.T) { }, err: handlers.ApiErrorWithCode(codes.InvalidArgument), }, + { + name: "Invalidly formatted scope id - org scope", + scopeId: org.GetPublicId(), + req: &pbs.UpdateScopeRequest{ + Id: "o_!@£$*", + UpdateMask: &field_mask.FieldMask{Paths: []string{}}, + Item: &pb.Scope{ + Name: &wrapperspb.StringValue{Value: "updated name"}, + Description: &wrapperspb.StringValue{Value: "updated desc"}, + }, + }, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), + }, + { + name: "Invalidly formatted scope id - proj scope", + scopeId: proj.GetPublicId(), + req: &pbs.UpdateScopeRequest{ + Id: "p_!@£$*", + UpdateMask: &field_mask.FieldMask{Paths: []string{}}, + Item: &pb.Scope{ + Name: &wrapperspb.StringValue{Value: "updated name"}, + Description: &wrapperspb.StringValue{Value: "updated desc"}, + }, + }, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), + }, + { + name: "Invalidly formatted scope id - unknown scope", + scopeId: org.GetPublicId(), + req: &pbs.UpdateScopeRequest{ + Id: "bla_123", + UpdateMask: &field_mask.FieldMask{Paths: []string{}}, + Item: &pb.Scope{ + Name: &wrapperspb.StringValue{Value: "updated name"}, + Description: &wrapperspb.StringValue{Value: "updated desc"}, + }, + }, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), + }, + { + name: "Unknown scope type - org scope", + scopeId: org.GetPublicId(), + req: &pbs.UpdateScopeRequest{ + Id: org.GetPublicId(), + UpdateMask: &field_mask.FieldMask{Paths: []string{}}, + Item: &pb.Scope{ + Type: "test", + Name: &wrapperspb.StringValue{Value: "updated name"}, + Description: &wrapperspb.StringValue{Value: "updated desc"}, + }, + }, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), + }, + { + name: "Unknown scope type - proj scope", + scopeId: proj.GetPublicId(), + req: &pbs.UpdateScopeRequest{ + Id: proj.GetPublicId(), + UpdateMask: &field_mask.FieldMask{Paths: []string{}}, + Item: &pb.Scope{ + Type: "test", + Name: &wrapperspb.StringValue{Value: "updated name"}, + Description: &wrapperspb.StringValue{Value: "updated desc"}, + }, + }, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), + }, + { + name: "Unknown primary auth method id", + scopeId: proj.GetPublicId(), + req: &pbs.UpdateScopeRequest{ + Id: proj.GetPublicId(), + UpdateMask: &field_mask.FieldMask{Paths: []string{}}, + Item: &pb.Scope{ + Name: &wrapperspb.StringValue{Value: "updated name"}, + Description: &wrapperspb.StringValue{Value: "updated desc"}, + PrimaryAuthMethodId: &wrapperspb.StringValue{Value: "test_123"}, + }, + }, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), + }, + { + name: "Update org name and description with whitespace only", + scopeId: scope.Global.String(), + req: &pbs.UpdateScopeRequest{ + UpdateMask: &field_mask.FieldMask{ + Paths: []string{"name", "description"}, + }, + Item: &pb.Scope{ + Name: &wrapperspb.StringValue{Value: " "}, + Description: &wrapperspb.StringValue{Value: " "}, + Type: scope.Org.String(), + }, + }, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), + }, + { + name: "Update org name and description with non-printable characters", + scopeId: scope.Global.String(), + req: &pbs.UpdateScopeRequest{ + UpdateMask: &field_mask.FieldMask{ + Paths: []string{"name", "description"}, + }, + Item: &pb.Scope{ + Name: &wrapperspb.StringValue{Value: " na\u200Bme "}, + Description: &wrapperspb.StringValue{Value: " desc\u200Bription "}, + Type: scope.Org.String(), + }, + }, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), + }, { name: "Only non-existant paths in Mask", scopeId: org.GetPublicId(),