diff --git a/cla-backend-go/v2/member-service/client.go b/cla-backend-go/v2/member-service/client.go index 390deba76..01234c3f7 100644 --- a/cla-backend-go/v2/member-service/client.go +++ b/cla-backend-go/v2/member-service/client.go @@ -87,10 +87,71 @@ func NewClient(cfg Config) (*Client, error) { return &Client{cfg: cfg, httpClient: &http.Client{Timeout: 30 * time.Second}}, nil } +const sfidSuffixChars = "ABCDEFGHIJKLMNOPQRSTUVWXYZ012345" + +// sfid18 returns the canonical 18-character form of a 15- or 18-character Salesforce ID; the +// gateway matches the path id against b2b_org:<18-char> FGA tuples, so a 15-character id is +// always refused. +func sfid18(id string) (string, bool) { + id = strings.TrimSpace(id) + if len(id) != 15 && len(id) != 18 { + return "", false + } + var b [18]byte + copy(b[:], id[:15]) + for _, c := range b[:15] { + if !(c >= 'A' && c <= 'Z' || c >= 'a' && c <= 'z' || c >= '0' && c <= '9') { + return "", false + } + } + if len(id) == 18 && !restoreSFIDCase(b[:15], id[15:]) { + return "", false + } + for g := 0; g < 3; g++ { + bits := 0 + for j := 0; j < 5; j++ { + if c := b[g*5+j]; c >= 'A' && c <= 'Z' { + bits |= 1 << j + } + } + b[15+g] = sfidSuffixChars[bits] + } + return string(b[:]), true +} + +// restoreSFIDCase re-applies the letter case encoded by the case-insensitive 3-character suffix +// (bit j of suffix character g set <=> position g*5+j is an uppercase letter). +func restoreSFIDCase(id []byte, suffix string) bool { + for g := 0; g < 3; g++ { + s := suffix[g] + if s >= 'a' && s <= 'z' { + s -= 'a' - 'A' + } + bits := strings.IndexByte(sfidSuffixChars, s) + if bits < 0 { + return false + } + for j := 0; j < 5; j++ { + c := &id[g*5+j] + switch { + case bits&(1<= 'A' && *c <= 'Z' { + *c += 'a' - 'A' + } + case *c >= 'a' && *c <= 'z': + *c -= 'a' - 'A' + case *c < 'A' || *c > 'Z': + return false + } + } + } + return true +} + // GetB2BOrg returns the registered B2B org (dry-run liveness check; member-service reads Salesforce). func (c *Client) GetB2BOrg(ctx context.Context, uid string) (*B2BOrg, error) { - uid = strings.TrimSpace(uid) - if len(uid) != 15 && len(uid) != 18 { + uid, ok := sfid18(uid) + if !ok { return nil, ErrInvalidSFID } tok, err := c.getToken(ctx) @@ -109,8 +170,8 @@ func (c *Client) GetB2BOrg(ctx context.Context, uid string) (*B2BOrg, error) { // RegisterB2BOrg registers the Salesforce account as a B2B org (idempotent on the member-service // side) and returns the resulting record. func (c *Client) RegisterB2BOrg(ctx context.Context, sfid string) (*B2BOrg, error) { - sfid = strings.TrimSpace(sfid) - if len(sfid) != 15 && len(sfid) != 18 { + sfid, ok := sfid18(sfid) + if !ok { return nil, ErrInvalidSFID } tok, err := c.getToken(ctx) diff --git a/cla-backend-go/v2/member-service/client_test.go b/cla-backend-go/v2/member-service/client_test.go index a851ef682..93e31338f 100644 --- a/cla-backend-go/v2/member-service/client_test.go +++ b/cla-backend-go/v2/member-service/client_test.go @@ -14,10 +14,13 @@ import ( "github.com/stretchr/testify/require" ) +const syntheticSFID18 = "001Ab00000CdEfGIAV" + type fakeMemberService struct { t *testing.T tokenCalls int getCalls int + getPaths []string registerBody []map[string]string status int response interface{} @@ -44,11 +47,12 @@ func (f *fakeMemberService) ServeHTTP(w http.ResponseWriter, r *http.Request) { assert.Equal(f.t, "client_credentials", req["grant_type"]) assert.Equal(f.t, "https://member.example/", req["audience"]) f.encode(w, map[string]interface{}{"access_token": "member-token", "token_type": "Bearer", "expires_in": 3600}) - case "/b2b_orgs/0014100000Te0G7AAJ": + case "/b2b_orgs/0014100000Te0G7AAJ", "/b2b_orgs/" + syntheticSFID18: assert.Equal(f.t, http.MethodGet, r.Method) assert.Equal(f.t, "Bearer member-token", r.Header.Get("Authorization")) assert.Equal(f.t, "1", r.URL.Query().Get("v")) f.getCalls++ + f.getPaths = append(f.getPaths, r.URL.Path) w.WriteHeader(f.status) if f.response != nil { f.encode(w, f.response) @@ -114,6 +118,14 @@ func TestRegisterB2BOrg(t *testing.T) { require.NoError(t, err) assert.Equal(t, "Infosys Limited", org.Name) assert.Equal(t, 1, fake.getCalls) + org, err = client.GetB2BOrg(context.Background(), " 0014100000Te0G7 ") + require.NoError(t, err, "15-char ids are sent in the 18-char form the gateway matches tuples on") + assert.Equal(t, "Infosys Limited", org.Name) + assert.Equal(t, 2, fake.getCalls) + fake.status = http.StatusCreated + _, err = client.RegisterB2BOrg(context.Background(), "0014100000Te0G7") + require.NoError(t, err) + assert.Equal(t, map[string]string{"sfid": "0014100000Te0G7AAJ"}, fake.registerBody[len(fake.registerBody)-1]) fake.status = http.StatusNotFound _, err = client.GetB2BOrg(context.Background(), "0014100000Te0G7AAJ") assert.ErrorIs(t, err, ErrOrgNotFound) @@ -121,6 +133,66 @@ func TestRegisterB2BOrg(t *testing.T) { assert.ErrorIs(t, err, ErrInvalidSFID) } +func TestSFID18(t *testing.T) { + // real Account id pairs from the dev companies table + for in, want := range map[string]string{ + "0014100000Te0Rk": "0014100000Te0RkAAJ", + "0012h00000hFI9F": "0012h00000hFI9FAAW", + "0014100000Te0G7": "0014100000Te0G7AAJ", + "0012M00002VjHnZ": "0012M00002VjHnZQAV", + "0012M00002p9y2q": "0012M00002p9y2qQAA", + "0014100000Te0yq": "0014100000Te0yqAAB", + "0014100000Te0RkAAJ": "0014100000Te0RkAAJ", + "0014100000Te0Rkaaj": "0014100000Te0RkAAJ", + " 0012M00002VjHnZ\n": "0012M00002VjHnZQAV", + // synthetic pair: the suffix restores the letter case of any case-folded 18-char form + "001Ab00000CdEfG": syntheticSFID18, + syntheticSFID18: syntheticSFID18, + "001ab00000cdefgiav": syntheticSFID18, + "001AB00000CDEFGIAV": syntheticSFID18, + "001aB00000cDeFgIaV": syntheticSFID18, + "001Ab00000CdEfGiav": syntheticSFID18, + "001ab00000cdefg": "001ab00000cdefgAAA", + } { + got, ok := sfid18(in) + assert.True(t, ok, in) + assert.Equal(t, want, got, in) + } + // suffix outside A-Z/0-5, or an uppercase bit on a digit position, is malformed + for _, in := range []string{"", "0014100000Te0R", "0014100000Te0RkA", "0014100000Te0RkAAJX", "0014100000Te0R-", "lf-not-an-sfid-xxx", + "001Ab00000CdEfGIA6", "001Ab00000CdEfGIA-", "001Ab00000CdEfG-AV", "001Ab00000CdEfGJAV", "001Ab00000CdEfGIBV"} { + got, ok := sfid18(in) + assert.False(t, ok, in) + assert.Empty(t, got, in) + } +} + +func TestB2BOrgCaseFoldedSFID(t *testing.T) { + fake := &fakeMemberService{status: http.StatusOK, response: map[string]string{"uid": syntheticSFID18, "name": "Synthetic Org"}} + client := newTestClient(t, fake) + + org, err := client.GetB2BOrg(context.Background(), "001ab00000cdefgiav") + require.NoError(t, err) + assert.Equal(t, syntheticSFID18, org.UID) + assert.Equal(t, []string{"/b2b_orgs/" + syntheticSFID18}, fake.getPaths, "GET path carries the case-restored canonical id") + + fake.status = http.StatusCreated + _, err = client.RegisterB2BOrg(context.Background(), "001AB00000CDEFGIAV") + require.NoError(t, err) + assert.Equal(t, []map[string]string{{"sfid": syntheticSFID18}}, fake.registerBody, "POST payload carries the case-restored canonical id") + + client = newTestClient(t, fake) + for _, in := range []string{"001Ab00000CdEfGJAV", "001Ab00000CdEfGIA6"} { + _, err = client.GetB2BOrg(context.Background(), in) + assert.ErrorIs(t, err, ErrInvalidSFID, in) + _, err = client.RegisterB2BOrg(context.Background(), in) + assert.ErrorIs(t, err, ErrInvalidSFID, in) + } + assert.Equal(t, 1, fake.getCalls, "malformed suffixes never reach the service") + assert.Len(t, fake.registerBody, 1) + assert.Equal(t, 1, fake.tokenCalls, "malformed suffixes are refused before a token is minted") +} + func TestRegisterB2BOrgErrors(t *testing.T) { fake := &fakeMemberService{status: http.StatusNotFound, response: map[string]string{"name": "NotFound", "message": "b2b org not found"}} client := newTestClient(t, fake) diff --git a/cla-backend-go/v2/signatures/ecla_invalidate_test.go b/cla-backend-go/v2/signatures/ecla_invalidate_test.go index 4ebedbccc..6581659fb 100644 --- a/cla-backend-go/v2/signatures/ecla_invalidate_test.go +++ b/cla-backend-go/v2/signatures/ecla_invalidate_test.go @@ -35,6 +35,8 @@ import ( "github.com/stretchr/testify/require" ) +const eclaForbiddenMessage = utils.EasyCLA403Forbidden + " - unable to invalidate ecla - error: not authorized to invalidate this employee acknowledgment" + type capturedEmail struct { subject string body string @@ -70,6 +72,76 @@ func eclaEventArgs() *events.LogEventArgs { } } +func parentCCLA(managers ...string) *v1Models.Signature { + ccla := &v1Models.Signature{SignatureID: "ccla-1", SignatureACL: []v1Models.User{}} + for _, manager := range managers { + ccla.SignatureACL = append(ccla.SignatureACL, v1Models.User{LfUsername: manager}) + } + return ccla +} + +// expectParentCCLALookup pins the parent lookup to the acknowledgment's company and cla group and to an approved, signed ccla +func expectParentCCLALookup(ctx context.Context, t *testing.T, mockRepo *mock_v1_signatures.MockSignatureRepository, ccla *v1Models.Signature, err error) { + mockRepo.EXPECT().GetCorporateSignature(ctx, "cla-group-1", "company-1", gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, _, _ string, approved, signed *bool) (*v1Models.Signature, error) { + if assert.NotNil(t, approved, "the parent lookup must ask for an approved ccla") { + assert.True(t, *approved, "the parent lookup must ask for an approved ccla") + } + if assert.NotNil(t, signed, "the parent lookup must ask for a signed ccla") { + assert.True(t, *signed, "the parent lookup must ask for a signed ccla") + } + return ccla, err + }) +} + +// deniedEclaFixture wires the strict mocks of a refused invalidation: the parent lookup is the last +// permitted repository call - any invalidation, re-invalidation, user/cla group lookup, email or event fails the test +type deniedEclaFixture struct { + repo *mock_v1_signatures.MockSignatureRepository + events *eventsMock.MockService + sender *capturingEmailSender + svc *Service +} + +func newDeniedEclaFixture(ctx context.Context, t *testing.T, ctrl *gomock.Controller, sig *v1Signatures.ItemSignature) *deniedEclaFixture { + awsSession, err := ini.GetAWSSession() + require.NoError(t, err, "unable to create AWS session") + + mockRepo := mock_v1_signatures.NewMockSignatureRepository(ctrl) + mockRepo.EXPECT().GetItemSignature(ctx, "sig-1").Return(sig, nil) + mockRepo.EXPECT().InvalidateProjectRecordWithMetadata(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(0) + mockRepo.EXPECT().ReinvalidateProjectRecordWithMetadata(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(0) + + mockCompanyService := mock_company.NewMockIService(ctrl) + mockCompanyService.EXPECT().GetCompany(ctx, "company-1"). + Return(&v1Models.Company{CompanyID: "company-1", CompanyExternalID: "comp-sfid", CompanyName: "Acme"}, nil) + + mockProjectClaGroupsRepo := mock_projects_cla_groups.NewMockRepository(ctrl) + mockProjectClaGroupsRepo.EXPECT().GetProjectsIdsForClaGroup(ctx, "cla-group-1"). + Return([]*projects_cla_groups.ProjectClaGroup{{ProjectSFID: "proj-sfid"}}, nil) + + mockUserService := mock_users.NewMockService(ctrl) + mockUserService.EXPECT().GetUser(gomock.Any()).Times(0) + + mockProjectService := mock_project.NewMockService(ctrl) + mockProjectService.EXPECT().GetCLAGroupByID(gomock.Any(), gomock.Any()).Times(0) + + mockEvents := eventsMock.NewMockService(ctrl) + mockEvents.EXPECT().LogEventWithContext(gomock.Any(), gomock.Any()).Times(0) + + sender := &capturingEmailSender{} + prevSender := utils.GetEmailSender() + utils.SetEmailSender(sender) + t.Cleanup(func() { utils.SetEmailSender(prevSender) }) + + return &deniedEclaFixture{ + repo: mockRepo, + events: mockEvents, + sender: sender, + svc: NewService(awsSession, "", mockProjectService, mockCompanyService, nil, mockProjectClaGroupsRepo, mockRepo, mockUserService, nil), + } +} + func TestService_InvalidateECLA(t *testing.T) { t.Setenv("DISABLE_LOCAL_PERMISSION_CHECKS", "false") @@ -107,6 +179,7 @@ func TestService_InvalidateECLA(t *testing.T) { mockProjectClaGroupsRepo := mock_projects_cla_groups.NewMockRepository(ctrl) mockProjectClaGroupsRepo.EXPECT().GetProjectsIdsForClaGroup(ctx, "cla-group-1"). Return([]*projects_cla_groups.ProjectClaGroup{{ProjectSFID: "proj-other"}, {ProjectSFID: "proj-sfid"}}, nil) + expectParentCCLALookup(ctx, t, mockRepo, parentCCLA("org-admin"), nil) mockUserService := mock_users.NewMockService(ctrl) mockUserService.EXPECT().GetUser("user-1"). @@ -226,6 +299,7 @@ func TestService_InvalidateECLAAfterApprovalListRemoval(t *testing.T) { mockProjectClaGroupsRepo := mock_projects_cla_groups.NewMockRepository(ctrl) mockProjectClaGroupsRepo.EXPECT().GetProjectsIdsForClaGroup(ctx, "cla-group-1"). Return([]*projects_cla_groups.ProjectClaGroup{{ProjectSFID: "proj-sfid"}}, nil) + expectParentCCLALookup(ctx, t, mockRepo, parentCCLA("org-admin"), nil) mockUserService := mock_users.NewMockService(ctrl) mockUserService.EXPECT().GetUser("user-1"). @@ -288,6 +362,7 @@ func TestService_InvalidateECLAAfterApprovalListRemoval(t *testing.T) { mockProjectClaGroupsRepo := mock_projects_cla_groups.NewMockRepository(ctrl) mockProjectClaGroupsRepo.EXPECT().GetProjectsIdsForClaGroup(ctx, "cla-group-1"). Return([]*projects_cla_groups.ProjectClaGroup{{ProjectSFID: "proj-sfid"}}, nil) + expectParentCCLALookup(ctx, t, mockRepo, parentCCLA("org-admin"), nil) mockUserService := mock_users.NewMockService(ctrl) mockUserService.EXPECT().GetUser("user-1"). @@ -342,6 +417,7 @@ func TestService_InvalidateECLASanctionedCompany(t *testing.T) { mockProjectClaGroupsRepo := mock_projects_cla_groups.NewMockRepository(ctrl) mockProjectClaGroupsRepo.EXPECT().GetProjectsIdsForClaGroup(ctx, "cla-group-1"). Return([]*projects_cla_groups.ProjectClaGroup{{ProjectSFID: "proj-sfid"}}, nil) + expectParentCCLALookup(ctx, t, mockRepo, parentCCLA("org-admin"), nil) service := NewService(awsSession, "", nil, mockCompanyService, nil, mockProjectClaGroupsRepo, mockRepo, nil, nil) @@ -353,6 +429,31 @@ func TestService_InvalidateECLASanctionedCompany(t *testing.T) { assert.Equal(t, "comp-sfid", sanctionedErr.CompanySFID) }) + t.Run("parent acl denial is checked before the sanctions gate", func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + ctx := context.Background() + + mockRepo := mock_v1_signatures.NewMockSignatureRepository(ctrl) + mockRepo.EXPECT().GetItemSignature(ctx, "sig-1").Return(eclaItemSignature(), nil) + expectParentCCLALookup(ctx, t, mockRepo, parentCCLA("cla-manager"), nil) + + mockCompanyService := mock_company.NewMockIService(ctrl) + mockCompanyService.EXPECT().GetCompany(ctx, "company-1").Return(sanctionedCompany, nil) + + mockProjectClaGroupsRepo := mock_projects_cla_groups.NewMockRepository(ctrl) + mockProjectClaGroupsRepo.EXPECT().GetProjectsIdsForClaGroup(ctx, "cla-group-1"). + Return([]*projects_cla_groups.ProjectClaGroup{{ProjectSFID: "proj-sfid"}}, nil) + + service := NewService(awsSession, "", nil, mockCompanyService, nil, mockProjectClaGroupsRepo, mockRepo, nil, nil) + + result, err := service.InvalidateECLA(ctx, "cla-group-1", "sig-1", managerUser, nil, eclaEventArgs(), nil) + assert.Nil(t, result) + assert.ErrorIs(t, err, errEclaForbidden, "a non-manager must not learn the sanction status") + var sanctionedErr *utils.SanctionedCompanyError + assert.False(t, errors.As(err, &sanctionedErr)) + }) + t.Run("authorization is checked before the sanctions gate", func(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() @@ -453,6 +554,7 @@ func TestService_InvalidateECLAValidation(t *testing.T) { mockProjectClaGroupsRepo := mock_projects_cla_groups.NewMockRepository(ctrl) mockProjectClaGroupsRepo.EXPECT().GetProjectsIdsForClaGroup(ctx, "cla-group-1"). Return([]*projects_cla_groups.ProjectClaGroup{{ProjectSFID: "proj-sfid"}}, nil) + expectParentCCLALookup(ctx, t, mockRepo, parentCCLA("org-admin"), nil) mockUserService := mock_users.NewMockService(ctrl) mockUserService.EXPECT().GetUser("user-1"). @@ -488,6 +590,7 @@ func TestService_InvalidateECLAValidation(t *testing.T) { mockProjectClaGroupsRepo := mock_projects_cla_groups.NewMockRepository(ctrl) mockProjectClaGroupsRepo.EXPECT().GetProjectsIdsForClaGroup(ctx, "cla-group-1"). Return([]*projects_cla_groups.ProjectClaGroup{{ProjectSFID: "proj-sfid"}}, nil).AnyTimes() + mockRepo.EXPECT().GetCorporateSignature(ctx, "cla-group-1", "company-1", gomock.Any(), gomock.Any()).Return(parentCCLA("org-admin"), nil).AnyTimes() service := NewService(awsSession, "", nil, mockCompanyService, nil, mockProjectClaGroupsRepo, mockRepo, nil, nil) @@ -500,6 +603,96 @@ func TestService_InvalidateECLAValidation(t *testing.T) { } } +func TestService_InvalidateECLARequiresParentCCLAManager(t *testing.T) { + t.Setenv("DISABLE_LOCAL_PERMISSION_CHECKS", "false") + + managerUser := &auth.User{UserName: "org-admin", Email: "org-admin@example.com", ACL: auth.ACL{Allowed: true, Scopes: []auth.Scope{{Type: auth.ProjectOrganization, ID: "proj-sfid|comp-sfid"}}}} + staffAdmin := &auth.User{UserName: "staff-admin", Email: "staff@example.com", ACL: auth.ACL{Admin: true, Allowed: true}} + noScopeUser := &auth.User{UserName: "no-scope", Email: "no-scope@example.com", ACL: auth.ACL{Allowed: true}} + lookupDown := errors.New("dynamo down") + + nilACL := parentCCLA() + nilACL.SignatureACL = nil + + sameEmailOnly := parentCCLA("someone-else") + sameEmailOnly.SignatureACL[0].LfEmail = strfmt.Email(managerUser.Email) + + ownACL := eclaItemSignature() + ownACL.SignatureACL = []string{"org-admin"} + + for _, tc := range []struct { + name string + sig *v1Signatures.ItemSignature + ccla *v1Models.Signature + lookupErr error + expectedErr error + }{ + {name: "caller absent from a nonempty parent acl", ccla: parentCCLA("cla-manager", "another-manager"), expectedErr: errEclaForbidden}, + {name: "nil parent acl", ccla: nilACL, expectedErr: errEclaForbidden}, + {name: "empty parent acl", ccla: parentCCLA(), expectedErr: errEclaForbidden}, + {name: "username differing only by case", ccla: parentCCLA("Org-Admin"), expectedErr: errEclaForbidden}, + {name: "username differing only by surrounding whitespace", ccla: parentCCLA(" org-admin "), expectedErr: errEclaForbidden}, + {name: "same email on a different manager", ccla: sameEmailOnly, expectedErr: errEclaForbidden}, + {name: "membership in the acknowledgment's own acl", sig: ownACL, ccla: parentCCLA("cla-manager"), expectedErr: errEclaForbidden}, + {name: "no approved and signed parent ccla", expectedErr: errEclaForbidden}, + {name: "parent lookup failure is propagated", lookupErr: lookupDown, expectedErr: lookupDown}, + } { + t.Run(tc.name, func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + ctx := context.Background() + + sig := tc.sig + if sig == nil { + sig = eclaItemSignature() + } + fx := newDeniedEclaFixture(ctx, t, ctrl, sig) + expectParentCCLALookup(ctx, t, fx.repo, tc.ccla, tc.lookupErr) + + result, err := fx.svc.InvalidateECLA(ctx, "cla-group-1", "sig-1", managerUser, fx.events, eclaEventArgs(), &models.EclaInvalidationInput{Reason: "compliance"}) + assert.Nil(t, result) + assert.ErrorIs(t, err, tc.expectedErr) + assert.Empty(t, fx.sender.sent, "a refused invalidation sends no notification") + }) + } + + // the parent acl is an additional condition: without the acs scope it is not even consulted + for _, authUser := range []*auth.User{staffAdmin, noScopeUser} { + t.Run("parent acl membership without the acs scope: "+authUser.UserName, func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + ctx := context.Background() + + fx := newDeniedEclaFixture(ctx, t, ctrl, eclaItemSignature()) + fx.repo.EXPECT().GetCorporateSignature(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(parentCCLA(authUser.UserName), nil).Times(0) + + result, err := fx.svc.InvalidateECLA(ctx, "cla-group-1", "sig-1", authUser, fx.events, eclaEventArgs(), nil) + assert.Nil(t, result) + assert.ErrorIs(t, err, errEclaForbidden) + assert.Empty(t, fx.sender.sent) + }) + } + + t.Run("the lookup is pinned to the acknowledgment's internal company and cla group", func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + ctx := context.Background() + + fx := newDeniedEclaFixture(ctx, t, ctrl, eclaItemSignature()) + // company-2 is another signing entity of the same organization (same SFID) and cla-group-2 another + // agreement of company-1 - both list the caller, neither is the acknowledgment's parent + fx.repo.EXPECT().GetCorporateSignature(ctx, "cla-group-1", "company-2", gomock.Any(), gomock.Any()).Return(parentCCLA("org-admin"), nil).Times(0) + fx.repo.EXPECT().GetCorporateSignature(ctx, "cla-group-2", "company-1", gomock.Any(), gomock.Any()).Return(parentCCLA("org-admin"), nil).Times(0) + expectParentCCLALookup(ctx, t, fx.repo, parentCCLA("cla-manager"), nil) + + result, err := fx.svc.InvalidateECLA(ctx, "cla-group-1", "sig-1", managerUser, fx.events, eclaEventArgs(), nil) + assert.Nil(t, result) + assert.ErrorIs(t, err, errEclaForbidden) + assert.Empty(t, fx.sender.sent) + }) +} + type fakeEclaInvalidateService struct { ServiceInterface result *models.EclaInvalidateResult @@ -562,6 +755,12 @@ func TestInvalidateECLAHandlerMapping(t *testing.T) { if tc.expectedStatus == http.StatusOK { assert.JSONEq(t, `{"signature_id":"sig-1","cla_group_id":"cla-group-1","company_id":"company-1","user_id":"user-1"}`, recorder.Body.String()) } + if tc.name == "forbidden" { + var payload map[string]interface{} + require.NoError(t, json.Unmarshal(recorder.Body.Bytes(), &payload)) + assert.Equal(t, utils.String403, payload["Code"]) + assert.Equal(t, eclaForbiddenMessage, payload["Message"]) + } if tc.name == "sanctioned company" { var payload map[string]interface{} require.NoError(t, json.Unmarshal(recorder.Body.Bytes(), &payload)) @@ -573,6 +772,66 @@ func TestInvalidateECLAHandlerMapping(t *testing.T) { } } +func TestInvalidateECLAHandlerParentACLDenial(t *testing.T) { + t.Setenv("DISABLE_LOCAL_PERMISSION_CHECKS", "false") + + awsSession, err := ini.GetAWSSession() + if err != nil { + assert.Fail(t, "unable to create AWS session") + } + + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + // the handler builds its own request context - match it loosely and pin the business arguments + mockRepo := mock_v1_signatures.NewMockSignatureRepository(ctrl) + mockRepo.EXPECT().GetItemSignature(gomock.Any(), "sig-1").Return(eclaItemSignature(), nil) + mockRepo.EXPECT().GetCorporateSignature(gomock.Any(), "cla-group-1", "company-1", gomock.Any(), gomock.Any()).Return(parentCCLA("cla-manager"), nil) + mockRepo.EXPECT().InvalidateProjectRecordWithMetadata(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(0) + mockRepo.EXPECT().ReinvalidateProjectRecordWithMetadata(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(0) + + mockCompanyService := mock_company.NewMockIService(ctrl) + mockCompanyService.EXPECT().GetCompany(gomock.Any(), "company-1"). + Return(&v1Models.Company{CompanyID: "company-1", CompanyExternalID: "comp-sfid", CompanyName: "Acme"}, nil) + + mockProjectClaGroupsRepo := mock_projects_cla_groups.NewMockRepository(ctrl) + mockProjectClaGroupsRepo.EXPECT().GetProjectsIdsForClaGroup(gomock.Any(), "cla-group-1"). + Return([]*projects_cla_groups.ProjectClaGroup{{ProjectSFID: "proj-sfid"}}, nil) + + mockEvents := eventsMock.NewMockService(ctrl) + mockEvents.EXPECT().LogEventWithContext(gomock.Any(), gomock.Any()).Times(0) + + sender := &capturingEmailSender{} + prevSender := utils.GetEmailSender() + utils.SetEmailSender(sender) + t.Cleanup(func() { utils.SetEmailSender(prevSender) }) + + v2Service := NewService(awsSession, "", nil, mockCompanyService, nil, mockProjectClaGroupsRepo, mockRepo, nil, nil) + api := operations.NewEasyclaAPI(nil) + Configure(api, nil, nil, mockCompanyService, nil, nil, mockEvents, v2Service, mockProjectClaGroupsRepo) + require.NotNil(t, api.SignaturesInvalidateECLAHandler) + + authUser := &auth.User{UserName: "org-admin", Email: "org-admin@example.com", ACL: auth.ACL{Allowed: true, Scopes: []auth.Scope{{Type: auth.ProjectOrganization, ID: "proj-sfid|comp-sfid"}}}} + username, email, reqID := authUser.UserName, authUser.Email, "req-3127" + recorder := httptest.NewRecorder() + api.SignaturesInvalidateECLAHandler.Handle(sigOps.InvalidateECLAParams{ + HTTPRequest: httptest.NewRequest(http.MethodPut, "/v4/cla-group/cla-group-1/ecla/sig-1/invalidate", nil), + XUSERNAME: &username, XEMAIL: &email, XREQUESTID: &reqID, + ClaGroupID: "cla-group-1", SignatureID: "sig-1", + Body: models.EclaInvalidationInput{Reason: "compliance"}, + }, authUser).WriteResponse(recorder, runtime.JSONProducer()) + + assert.Equal(t, http.StatusForbidden, recorder.Code, recorder.Body.String()) + assert.Equal(t, "req-3127", recorder.Header().Get("X-Request-Id")) + var payload map[string]interface{} + require.NoError(t, json.Unmarshal(recorder.Body.Bytes(), &payload)) + assert.Equal(t, "403", payload["Code"]) + assert.Equal(t, "req-3127", payload["x-request-id"]) + assert.Equal(t, eclaForbiddenMessage, payload["Message"]) + assert.NotContains(t, payload, "signature_id", "a refusal carries no success payload") + assert.Empty(t, sender.sent) +} + func TestEclaInvalidateJSONContracts(t *testing.T) { result, err := json.Marshal(models.EclaInvalidateResult{}) assert.Nil(t, err) diff --git a/cla-backend-go/v2/signatures/service.go b/cla-backend-go/v2/signatures/service.go index 1a5e0c84e..d1e69de3e 100644 --- a/cla-backend-go/v2/signatures/service.go +++ b/cla-backend-go/v2/signatures/service.go @@ -49,7 +49,7 @@ var ( errNotEcla = errors.New("signature is not an employee acknowledgment (ecla)") errEclaWrongClaGroup = errors.New("ecla does not belong to the specified cla group") errEclaAlreadyInvalidated = errors.New("ecla already invalidated") - errEclaForbidden = errors.New("not authorized for the ecla company and project scope") + errEclaForbidden = errors.New("not authorized to invalidate this employee acknowledgment") ) // ServiceInterface contains method of v2 signature service @@ -498,6 +498,10 @@ func (s *Service) InvalidateECLA(ctx context.Context, claGroupID string, signatu return nil, errEclaForbidden } + if cclaErr := s.requireParentCCLAManager(ctx, f, claGroupID, sig.SignatureUserCompanyID, authUser); cclaErr != nil { + return nil, cclaErr + } + if sanctionedErr := utils.CheckCompanySanctioned(companyModel); sanctionedErr != nil { log.WithFields(f).Warnf("company %s is sanctioned - rejecting InvalidateECLA", companyModel.CompanyID) return nil, sanctionedErr @@ -593,6 +597,21 @@ func (s *Service) InvalidateECLA(ctx context.Context, claGroupID string, signatu } // EclaAutoCreate this routine updates the CCLA signature record by adjusting the auto_create_ecla column to the specified value + +// requireParentCCLAManager refuses callers outside the approved, signed parent ccla's acl (lfx-self-serve#3127) +func (s *Service) requireParentCCLAManager(ctx context.Context, f logrus.Fields, claGroupID, companyID string, authUser *auth.User) error { + approved, signed := true, true + ccla, err := s.v1SignatureRepo.GetCorporateSignature(ctx, claGroupID, companyID, &approved, &signed) + if err != nil { + log.WithFields(f).WithError(err).Warn("unable to load the parent ccla signature") + return err + } + if ccla == nil || !utils.CurrentUserInACL(authUser, ccla.SignatureACL) { + log.WithFields(f).Debug("caller is not a cla manager of the parent ccla - rejecting InvalidateECLA") + return errEclaForbidden + } + return nil +} func (s *Service) EclaAutoCreate(ctx context.Context, signatureID string, autoCreateECLA bool) error { f := logrus.Fields{ "functionName": "v2.signatures.service.EclaAutoCreate", diff --git a/docs/M3_ORG_LENS_API.md b/docs/M3_ORG_LENS_API.md index fcadbf002..bfa5a7370 100644 --- a/docs/M3_ORG_LENS_API.md +++ b/docs/M3_ORG_LENS_API.md @@ -214,7 +214,11 @@ acknowledgment, mirroring the ICLA invalidate internals: sets (enum: `signed-in-error`, `should-be-corporate`, `compliance`, `other`) and `note` (≤2048 chars) — logs the event and emails the employee. 400 when the signature is not an employee acknowledgment or belongs to another CLA group, 409 when already invalidated; the response echoes -`signature_id`, `cla_group_id`, `company_id`, `user_id`. +`signature_id`, `cla_group_id`, `company_id`, `user_id`. On top of the shared auth below the caller +must be listed in the parent CCLA's `signature_acl` (the exact `CurrentUserInACL` match used by +approval-list and Auto ECLA writes): no approved, signed parent CCLA for the acknowledgment's +company and CLA group, or an ACL that does not list the caller (including an empty one), returns +403 ([lfx-self-serve#3127](https://github.com/linuxfoundation/lfx-self-serve/issues/3127)). Auth for all five: `project|organization` tree scope for the project/company pair, LF admin disallowed (ACS resources `cla_manager_request_admin`,