From 3cf03f2cbf8c4073a511f94e769f8fa6eccf4858 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Gryglicki?= Date: Tue, 25 Aug 2026 07:23:08 +0000 Subject: [PATCH 1/3] M2 milestone - implement remaining BE features MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Łukasz Gryglicki Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot) Assisted by [Claude](https://claude.ai) --- .../emails/contact_cla_manager_templates.go | 15 +- cla-backend-go/events/event_data.go | 52 +++++++ cla-backend-go/events/event_data_test.go | 37 +++++ cla-backend-go/events/event_types.go | 2 + cla-backend-go/signatures/dbmodels.go | 4 + cla-backend-go/signatures/mocks/mock_repo.go | 15 ++ .../signatures/mocks/mock_service.go | 1 + cla-backend-go/signatures/repository.go | 81 +++++++++++ cla-backend-go/swagger/cla.v2.yaml | 14 +- .../common/icla-invalidation-input.yaml | 16 +++ .../common/my-cla-manager-request-result.yaml | 2 +- .../common/my-cla-manager-request.yaml | 8 +- cla-backend-go/swagger/common/my-cla.yaml | 5 +- cla-backend-go/utils/string_utils.go | 31 +++- cla-backend-go/utils/string_utils_test.go | 25 ++++ .../v2/my_clas/cla_managers_test.go | 64 +++++++++ cla-backend-go/v2/my_clas/handlers.go | 4 +- cla-backend-go/v2/my_clas/prefetch.go | 19 +-- cla-backend-go/v2/my_clas/service.go | 57 +++++--- cla-backend-go/v2/my_clas/service_test.go | 124 ++++++++++++++-- cla-backend-go/v2/sign/helpers.go | 4 + cla-backend-go/v2/sign/icla_block_test.go | 81 +++++++++++ cla-backend-go/v2/sign/service.go | 49 +++++++ cla-backend-go/v2/signatures/handlers.go | 2 +- cla-backend-go/v2/signatures/service.go | 23 ++- cla-backend-go/v2/signatures/service_test.go | 133 ++++++++++++++++++ docs/MY_CLAS_API.md | 10 +- 27 files changed, 822 insertions(+), 56 deletions(-) create mode 100644 cla-backend-go/swagger/common/icla-invalidation-input.yaml create mode 100644 cla-backend-go/utils/string_utils_test.go create mode 100644 cla-backend-go/v2/sign/icla_block_test.go diff --git a/cla-backend-go/emails/contact_cla_manager_templates.go b/cla-backend-go/emails/contact_cla_manager_templates.go index cee2f4598..7bdc927fa 100644 --- a/cla-backend-go/emails/contact_cla_manager_templates.go +++ b/cla-backend-go/emails/contact_cla_manager_templates.go @@ -12,20 +12,32 @@ type ContactClaManagerTemplateParams struct { RequestAction string ContributorName string ContributorIdentity string + ContributorEmail string CompanyName string ProjectName string CLAGroupName string OptionalMessage string + ContactOnly bool } const ( // ContactClaManagerTemplateName is email template name for ContactClaManagerTemplate ContactClaManagerTemplateName = "ContactClaManagerTemplate" // ContactClaManagerTemplate is the email sent to the selected CLA managers when a - // contributor requests removal from or (re-)approval under the company CCLA + // contributor requests removal/approval or sends a contact-only message ContactClaManagerTemplate = `

Hello CLA Manager,

This is a notification email from EasyCLA regarding the project {{.ProjectName}} and CLA Group {{.CLAGroupName}}.

+{{if .ContactOnly}} +

{{.ContributorName}} ({{.ContributorIdentity}}) has sent you a message about their employee acknowledgement +under the {{.CompanyName}} corporate CLA. You are receiving this message as a CLA Manager from {{.CompanyName}} for {{.ProjectName}}.

+

The contributor's message:

+
{{.OptionalMessage}}
+{{if .ContributorEmail}} +

You can reply to the contributor at {{.ContributorEmail}}.

+{{end}} +

This is a message only - no change was requested and none has been made.

+{{else}}

{{.ContributorName}} ({{.ContributorIdentity}}) has requested {{.RequestAction}} for their employee acknowledgement under the {{.CompanyName}} corporate CLA. You are receiving this message as a CLA Manager from {{.CompanyName}} for {{.ProjectName}}.

{{if .OptionalMessage}} @@ -34,6 +46,7 @@ under the {{.CompanyName}} corporate CLA. You are receiving this message as a CL {{end}}

To act on this request, please log into the EasyCLA Corporate Console and update the Approved List for {{.CompanyName}} accordingly. No change has been made automatically.

+{{end}} ` ) diff --git a/cla-backend-go/events/event_data.go b/cla-backend-go/events/event_data.go index aee58b928..1e1c34a00 100644 --- a/cla-backend-go/events/event_data.go +++ b/cla-backend-go/events/event_data.go @@ -125,6 +125,10 @@ type GitHubProjectDeletedEventData struct { // SignatureProjectInvalidatedEventData data model type SignatureProjectInvalidatedEventData struct { InvalidatedCount int + SignatureID string + InvalidatedBy string + Reason string + InvalidationNote string } // SignatureInvalidatedApprovalRejectionEventData data model @@ -173,6 +177,9 @@ type CompanyACLUserAddedEventData struct { UserLFID string } +// CompanySanctionedEventData data model +type CompanySanctionedEventData struct{} + // CLATemplateCreatedEventData data model type CLATemplateCreatedEventData struct { TemplateName string @@ -793,6 +800,13 @@ func (ed *CompanyACLUserAddedEventData) GetEventDetailsString(args *LogEventArgs return data, true } +// GetEventDetailsString returns the details string for this event +func (ed *CompanySanctionedEventData) GetEventDetailsString(args *LogEventArgs) (string, bool) { + data := fmt.Sprintf("The company %s was flagged as sanctioned by sanctions screening", args.CompanyName) + data = data + "." + return data, true +} + // GetEventDetailsString returns the details string for this event func (ed *CLATemplateCreatedEventData) GetEventDetailsString(args *LogEventArgs) (string, bool) { data := "A CLA Group template was created or updated" // nolint @@ -1475,6 +1489,9 @@ func (ed *GitHubProjectDeletedEventData) GetEventDetailsString(args *LogEventArg // GetEventDetailsString returns the details string for this event func (ed *SignatureProjectInvalidatedEventData) GetEventDetailsString(args *LogEventArgs) (string, bool) { + if ed.SignatureID != "" { + return ed.singleSignatureText(args, true), true + } data := fmt.Sprintf("%d Signatures were invalidated (approved set to false) due to CLA Group/Project: %s deletion", ed.InvalidatedCount, args.ProjectName) if args.UserName != "" { @@ -1484,6 +1501,32 @@ func (ed *SignatureProjectInvalidatedEventData) GetEventDetailsString(args *LogE return data, true } +// singleSignatureText renders the admin ICLA invalidation wording (SignatureID set) shared by +// the details and summary strings +func (ed *SignatureProjectInvalidatedEventData) singleSignatureText(args *LogEventArgs, capitalized bool) string { + lead := "the signature" + if capitalized { + lead = "The signature" + } + data := fmt.Sprintf("%s %s was invalidated (approved set to false)", lead, ed.SignatureID) + if args.UserName != "" { + data = data + fmt.Sprintf(" for the user %s", args.UserName) + } + if args.ProjectName != "" { + data = data + fmt.Sprintf(" for the project %s", args.ProjectName) + } + if ed.InvalidatedBy != "" { + data = data + fmt.Sprintf(" by the administrator %s", ed.InvalidatedBy) + } + if ed.Reason != "" { + data = data + fmt.Sprintf(", reason: %s", ed.Reason) + } + if ed.InvalidationNote != "" { + data = data + fmt.Sprintf(", note: %s", ed.InvalidationNote) + } + return data + "." +} + // GetEventDetailsString returns the details string for this event func (ed *SignatureInvalidatedApprovalRejectionEventData) GetEventDetailsString(args *LogEventArgs) (string, bool) { reason := noReason @@ -1904,6 +1947,12 @@ func (ed *CompanyACLUserAddedEventData) GetEventSummaryString(args *LogEventArgs return data, true } +// GetEventSummaryString returns the summary string for this event +func (ed *CompanySanctionedEventData) GetEventSummaryString(args *LogEventArgs) (string, bool) { + data := fmt.Sprintf("The company %s was flagged as sanctioned by sanctions screening.", args.CompanyName) + return data, true +} + // GetEventSummaryString returns the summary string for this event func (ed *CLATemplateCreatedEventData) GetEventSummaryString(args *LogEventArgs) (string, bool) { // Same output as the details @@ -2651,6 +2700,9 @@ func (ed *GitHubProjectDeletedEventData) GetEventSummaryString(args *LogEventArg // GetEventSummaryString returns the summary string for this event func (ed *SignatureProjectInvalidatedEventData) GetEventSummaryString(args *LogEventArgs) (string, bool) { + if ed.SignatureID != "" { + return ed.singleSignatureText(args, false), true + } data := fmt.Sprintf("%d signatures were invalidated (approved set to false) due to CLA Group/Project %s deletion", ed.InvalidatedCount, args.ProjectName) if args.CLAGroupName != "" { diff --git a/cla-backend-go/events/event_data_test.go b/cla-backend-go/events/event_data_test.go index 6aaf6877d..4d76d0a71 100644 --- a/cla-backend-go/events/event_data_test.go +++ b/cla-backend-go/events/event_data_test.go @@ -167,3 +167,40 @@ func TestContactCLAManagerRequestCreatedEventData(t *testing.T) { summary, _ = eventData.GetEventSummaryString(args) assert.Contains(t, summary, "with a message") } + +func TestSignatureProjectInvalidatedEventDataSingleSignature(t *testing.T) { + bulk := &SignatureProjectInvalidatedEventData{InvalidatedCount: 3} + args := &LogEventArgs{UserName: testUser, ProjectName: "My Project"} + + details, containsPII := bulk.GetEventDetailsString(args) + assert.True(t, containsPII) + assert.Contains(t, details, "3 Signatures were invalidated (approved set to false) due to CLA Group/Project: My Project deletion") + + single := &SignatureProjectInvalidatedEventData{ + SignatureID: "sig-1", + InvalidatedBy: "admin-user", + Reason: "compliance", + InvalidationNote: "per legal review", + } + details, containsPII = single.GetEventDetailsString(args) + assert.True(t, containsPII) + assert.Equal(t, "The signature sig-1 was invalidated (approved set to false) for the user john for the project My Project by the administrator admin-user, reason: compliance, note: per legal review.", details) + summary, _ := single.GetEventSummaryString(args) + assert.Equal(t, "the signature sig-1 was invalidated (approved set to false) for the user john for the project My Project by the administrator admin-user, reason: compliance, note: per legal review.", summary) + + bare := &SignatureProjectInvalidatedEventData{SignatureID: "sig-2"} + details, _ = bare.GetEventDetailsString(&LogEventArgs{}) + assert.Equal(t, "The signature sig-2 was invalidated (approved set to false).", details) +} + +func TestCompanySanctionedEventData(t *testing.T) { + eventData := &CompanySanctionedEventData{} + args := &LogEventArgs{CompanyName: "Flagged Corp"} + + details, containsPII := eventData.GetEventDetailsString(args) + assert.True(t, containsPII) + assert.Equal(t, "The company Flagged Corp was flagged as sanctioned by sanctions screening.", details) + summary, containsPII := eventData.GetEventSummaryString(args) + assert.True(t, containsPII) + assert.Equal(t, "The company Flagged Corp was flagged as sanctioned by sanctions screening.", summary) +} diff --git a/cla-backend-go/events/event_types.go b/cla-backend-go/events/event_types.go index fec253f4e..9fa897c62 100644 --- a/cla-backend-go/events/event_types.go +++ b/cla-backend-go/events/event_types.go @@ -59,6 +59,8 @@ const ( CompanyACLRequestApproved = "company_acl.request_approved" CompanyACLRequestDenied = "company_acl.request_denied" + CompanySanctioned = "company.sanctioned" + ContactCLAManagerRequestCreated = "contact_cla_manager_request.created" CCLAApprovalListRequestCreated = "ccla_approval_list_request.created" diff --git a/cla-backend-go/signatures/dbmodels.go b/cla-backend-go/signatures/dbmodels.go index 86c043b58..e1e4b531b 100644 --- a/cla-backend-go/signatures/dbmodels.go +++ b/cla-backend-go/signatures/dbmodels.go @@ -46,6 +46,10 @@ type ItemSignature struct { UserDocusignDateSigned string `json:"user_docusign_date_signed,omitempty"` AutoCreateECLA bool `json:"auto_create_ecla,omitempty"` UserDocusignRawXML string `json:"user_docusign_raw_xml,omitempty"` + DateInvalidated string `json:"date_invalidated,omitempty"` + InvalidatedBy string `json:"invalidated_by,omitempty"` + InvalidationReason string `json:"invalidation_reason,omitempty"` + InvalidationNote string `json:"invalidation_note,omitempty"` } // DBManagersModel is a database model for only the ACL/Manager column diff --git a/cla-backend-go/signatures/mocks/mock_repo.go b/cla-backend-go/signatures/mocks/mock_repo.go index f12194287..a4627eea2 100644 --- a/cla-backend-go/signatures/mocks/mock_repo.go +++ b/cla-backend-go/signatures/mocks/mock_repo.go @@ -1,5 +1,6 @@ // Copyright The Linux Foundation and each contributor to CommunityBridge. // SPDX-License-Identifier: MIT +// // Code generated by MockGen. DO NOT EDIT. // Source: signatures/repository.go @@ -526,6 +527,20 @@ func (mr *MockSignatureRepositoryMockRecorder) InvalidateProjectRecord(ctx, sign return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "InvalidateProjectRecord", reflect.TypeOf((*MockSignatureRepository)(nil).InvalidateProjectRecord), ctx, signatureID, note) } +// InvalidateProjectRecordWithMetadata mocks base method. +func (m *MockSignatureRepository) InvalidateProjectRecordWithMetadata(ctx context.Context, signatureID, note string, metadata *signatures0.InvalidationMetadata) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "InvalidateProjectRecordWithMetadata", ctx, signatureID, note, metadata) + ret0, _ := ret[0].(error) + return ret0 +} + +// InvalidateProjectRecordWithMetadata indicates an expected call of InvalidateProjectRecordWithMetadata. +func (mr *MockSignatureRepositoryMockRecorder) InvalidateProjectRecordWithMetadata(ctx, signatureID, note, metadata interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "InvalidateProjectRecordWithMetadata", reflect.TypeOf((*MockSignatureRepository)(nil).InvalidateProjectRecordWithMetadata), ctx, signatureID, note, metadata) +} + // ProjectSignatures mocks base method. func (m *MockSignatureRepository) ProjectSignatures(ctx context.Context, projectID string) (*models.Signatures, error) { m.ctrl.T.Helper() diff --git a/cla-backend-go/signatures/mocks/mock_service.go b/cla-backend-go/signatures/mocks/mock_service.go index 08bf9c177..9f2c6a039 100644 --- a/cla-backend-go/signatures/mocks/mock_service.go +++ b/cla-backend-go/signatures/mocks/mock_service.go @@ -1,5 +1,6 @@ // Copyright The Linux Foundation and each contributor to CommunityBridge. // SPDX-License-Identifier: MIT +// // Code generated by MockGen. DO NOT EDIT. // Source: signatures/service.go diff --git a/cla-backend-go/signatures/repository.go b/cla-backend-go/signatures/repository.go index d71b52745..d40f2b1c3 100644 --- a/cla-backend-go/signatures/repository.go +++ b/cla-backend-go/signatures/repository.go @@ -70,6 +70,7 @@ type SignatureRepository interface { DeleteGithubOrganizationFromApprovalList(ctx context.Context, signatureID, githubOrganizationID string) ([]models.GithubOrg, error) ValidateProjectRecord(ctx context.Context, signatureID, note string) error InvalidateProjectRecord(ctx context.Context, signatureID, note string) error + InvalidateProjectRecordWithMetadata(ctx context.Context, signatureID, note string, metadata *InvalidationMetadata) error UpdateEnvelopeDetails(ctx context.Context, signatureID, envelopeID string, signURL *string) (*models.Signature, error) CreateSignature(ctx context.Context, signature *ItemSignature) error UpdateSignature(ctx context.Context, signatureID string, updates map[string]interface{}) error @@ -2085,6 +2086,13 @@ func (repo repository) ProjectSignatures(ctx context.Context, projectID string) }, nil } +// InvalidationMetadata carries the invalidation attribution stored on the signature record +type InvalidationMetadata struct { + InvalidatedBy string + Reason string + Note string +} + // InvalidateProjectRecord invalidates the specified project record by setting the signature_approved flag to false func (repo repository) InvalidateProjectRecord(ctx context.Context, signatureID, note string) error { f := logrus.Fields{ @@ -2129,6 +2137,79 @@ func (repo repository) InvalidateProjectRecord(ctx context.Context, signatureID, return nil } +// InvalidateProjectRecordWithMetadata invalidates the specified project record by setting the +// signature_approved flag to false and records the invalidation attribution: date_invalidated +// (kept from the first invalidation), invalidated_by, invalidation_reason, invalidation_note +func (repo repository) InvalidateProjectRecordWithMetadata(ctx context.Context, signatureID, note string, metadata *InvalidationMetadata) error { + f := logrus.Fields{ + "functionName": "v1.signatures.repository.InvalidateProjectRecordWithMetadata", + utils.XREQUESTID: ctx.Value(utils.XREQUESTID), + "signatureID": signatureID, + } + + signatureTableName := fmt.Sprintf("cla-%s-signatures", repo.stage) + + _, now := utils.CurrentTime() + + expressionAttributeNames := map[string]*string{} + expressionAttributeValues := map[string]*dynamodb.AttributeValue{} + updateExpression := "SET " // nolint + + expressionAttributeNames["#A"] = aws.String("signature_approved") + expressionAttributeValues[":a"] = &dynamodb.AttributeValue{BOOL: aws.Bool(false)} + updateExpression = updateExpression + " #A = :a," + + expressionAttributeNames["#S"] = aws.String("note") + expressionAttributeValues[":s"] = &dynamodb.AttributeValue{S: aws.String(note)} + updateExpression = updateExpression + " #S = :s," + + expressionAttributeNames["#DI"] = aws.String("date_invalidated") + expressionAttributeValues[":di"] = &dynamodb.AttributeValue{S: aws.String(now)} + updateExpression = updateExpression + " #DI = if_not_exists(#DI, :di)," + + if metadata != nil { + if metadata.InvalidatedBy != "" { + expressionAttributeNames["#IB"] = aws.String("invalidated_by") + expressionAttributeValues[":ib"] = &dynamodb.AttributeValue{S: aws.String(metadata.InvalidatedBy)} + updateExpression = updateExpression + " #IB = :ib," + } + if metadata.Reason != "" { + expressionAttributeNames["#IR"] = aws.String("invalidation_reason") + expressionAttributeValues[":ir"] = &dynamodb.AttributeValue{S: aws.String(metadata.Reason)} + updateExpression = updateExpression + " #IR = :ir," + } + if metadata.Note != "" { + expressionAttributeNames["#IN"] = aws.String("invalidation_note") + expressionAttributeValues[":in"] = &dynamodb.AttributeValue{S: aws.String(metadata.Note)} + updateExpression = updateExpression + " #IN = :in," + } + } + + expressionAttributeNames["#M"] = aws.String("date_modified") + expressionAttributeValues[":m"] = &dynamodb.AttributeValue{S: aws.String(now)} + updateExpression = updateExpression + " #M = :m" + + input := &dynamodb.UpdateItemInput{ + Key: map[string]*dynamodb.AttributeValue{ + "signature_id": { + S: aws.String(signatureID), + }, + }, + ExpressionAttributeNames: expressionAttributeNames, + ExpressionAttributeValues: expressionAttributeValues, + UpdateExpression: &updateExpression, + TableName: aws.String(signatureTableName), + } + + _, updateErr := repo.dynamoDBClient.UpdateItem(input) + if updateErr != nil { + log.WithFields(f).Warnf("error updating signature_approved for signature_id : %s error : %v ", signatureID, updateErr) + return updateErr + } + + return nil +} + // ValidateProjectRecord validates the specified project record by setting the signature_approved flag to true func (repo repository) ValidateProjectRecord(ctx context.Context, signatureID, note string) error { f := logrus.Fields{ diff --git a/cla-backend-go/swagger/cla.v2.yaml b/cla-backend-go/swagger/cla.v2.yaml index 95b46013a..f5e2f6fab 100644 --- a/cla-backend-go/swagger/cla.v2.yaml +++ b/cla-backend-go/swagger/cla.v2.yaml @@ -2882,8 +2882,8 @@ paths: /my-clas/{signatureID}/cla-manager-requests: post: - summary: Request removal or approval from the CLA managers of an ECLA - description: Records a contributor request against their own ECLA and emails it to the selected CLA managers of the covering company CCLA - removal (take me off the coverage) or approval (add me back to the approval list). recipients must be a non-empty subset of the managers from GET /my-clas/{signatureID}/cla-managers; empty only when none resolves, and the request is then recorded without email. No signature state changes. Unknown, not-owned and ICLA signature IDs return 404. An ECLA flagged for a sanctioned company is deliberately still accepted - a removal request is legitimate there. Identity-ownership enforcement and trusted-caller token verification are as in GET /my-clas + summary: Request removal or approval from, or send a message to, the CLA managers of an ECLA + description: Records a contributor request against their own ECLA and emails it to the selected CLA managers of the covering company CCLA - removal (take me off the coverage), approval (add me back to the approval list) or contact (just deliver the contributor's free-form message, required and non-blank for this type, no change requested). recipients must be a non-empty subset of the managers from GET /my-clas/{signatureID}/cla-managers; empty only when none resolves, and the request is then recorded without email. No signature state changes. Unknown, not-owned and ICLA signature IDs return 404. An ECLA flagged for a sanctioned company is deliberately still accepted - a removal request is legitimate there. Identity-ownership enforcement and trusted-caller token verification are as in GET /my-clas operationId: createMyClaManagerRequest parameters: - $ref: "#/parameters/x-request-id" @@ -3801,7 +3801,7 @@ paths: /cla-group/{claGroupID}/user/{userID}/icla: put: summary: Invalidate ICLA record - description: Invalidates a given ICLA record for a user + description: Invalidates a given ICLA record for a user - also stamps date_invalidated, invalidated_by and the optional invalidation_reason/invalidation_note from the body operationId: invalidateICLA parameters: - $ref: "#/parameters/x-request-id" @@ -3810,6 +3810,11 @@ paths: - $ref: "#/parameters/x-username" - $ref: "#/parameters/path-claGroupID" - $ref: "#/parameters/path-userID" + - name: body + in: body + required: false + schema: + $ref: '#/definitions/icla-invalidation-input' responses: '200': description: 'Success' @@ -5289,6 +5294,9 @@ definitions: my-cla-manager-request-result: $ref: './common/my-cla-manager-request-result.yaml' + icla-invalidation-input: + $ref: './common/icla-invalidation-input.yaml' + my-identity-list: $ref: './common/my-identity-list.yaml' diff --git a/cla-backend-go/swagger/common/icla-invalidation-input.yaml b/cla-backend-go/swagger/common/icla-invalidation-input.yaml new file mode 100644 index 000000000..1cccf1fce --- /dev/null +++ b/cla-backend-go/swagger/common/icla-invalidation-input.yaml @@ -0,0 +1,16 @@ +# Copyright The Linux Foundation and each contributor to CommunityBridge. +# SPDX-License-Identifier: MIT + +type: object +x-nullable: false +title: ICLA Invalidation Input +description: Optional invalidation metadata recorded on the signature - omitting the body preserves the previous behavior +properties: + reason: + type: string + enum: [signed-in-error, should-be-corporate, compliance, other] + description: Stored as invalidation_reason on the signature record + note: + type: string + maxLength: 2048 + description: Free-text note stored as invalidation_note, separate from the general-purpose note audit trail diff --git a/cla-backend-go/swagger/common/my-cla-manager-request-result.yaml b/cla-backend-go/swagger/common/my-cla-manager-request-result.yaml index a8b39796b..4ed396390 100644 --- a/cla-backend-go/swagger/common/my-cla-manager-request-result.yaml +++ b/cla-backend-go/swagger/common/my-cla-manager-request-result.yaml @@ -14,7 +14,7 @@ properties: description: ECLA signature ID (UUID) the request refers to requestType: type: string - enum: [removal, approval] + enum: [removal, approval, contact] description: The request type status: type: string diff --git a/cla-backend-go/swagger/common/my-cla-manager-request.yaml b/cla-backend-go/swagger/common/my-cla-manager-request.yaml index fed062c72..9aa75359d 100644 --- a/cla-backend-go/swagger/common/my-cla-manager-request.yaml +++ b/cla-backend-go/swagger/common/my-cla-manager-request.yaml @@ -4,14 +4,14 @@ type: object x-nullable: false title: My CLA Manager Request -description: A contributor removal or approval request to the CLA managers of the CCLA covering their ECLA, delivered by email - no signature state changes +description: A contributor removal, approval or contact request to the CLA managers of the CCLA covering their ECLA, delivered by email - no signature state changes required: - requestType properties: requestType: type: string - enum: [removal, approval] - description: removal - ask to be removed from the CCLA coverage; approval - ask to be (re-)added to the approval list + enum: [removal, approval, contact] + description: removal - ask to be removed from the CCLA coverage; approval - ask to be (re-)added to the approval list; contact - just send a free-form message to the CLA managers, no change is requested recipients: type: array description: LF usernames of the CLA managers to notify - a non-empty subset of the resolved managers; empty only when zero managers resolve, and the request is then recorded without email @@ -20,4 +20,4 @@ properties: message: type: string maxLength: 4096 - description: Optional contributor message included in the notification email + description: Contributor message included in the notification email - optional for removal and approval, required (non-blank) for contact; control characters other than newlines and tabs are stripped diff --git a/cla-backend-go/swagger/common/my-cla.yaml b/cla-backend-go/swagger/common/my-cla.yaml index c8c4bdb60..3d30c1cb5 100644 --- a/cla-backend-go/swagger/common/my-cla.yaml +++ b/cla-backend-go/swagger/common/my-cla.yaml @@ -48,6 +48,9 @@ properties: type: boolean x-omitempty: false description: signature_approved flag - false when the signature was invalidated + invalidatedAt: + type: string + description: Stored date_invalidated, stamped at the first invalidation - only present on records invalidated after this field was introduced valid: type: boolean x-omitempty: false @@ -104,7 +107,7 @@ properties: description: True when the employer is currently flagged by sanctions screening - always present, always false on ICLA rows flaggedAt: type: string - description: The employer's stored sanctioned_date, stamped at the first live detection - the response time only when no date is stored and stamping it failed; present only when flagged is true + description: The employer's stored sanctioned_date (the revocation date), stamped at the first live detection and refreshed when a cleared employer is flagged again; present only when flagged is true and a stored date exists flaggedCheck: type: string enum: [live, stored, unavailable] diff --git a/cla-backend-go/utils/string_utils.go b/cla-backend-go/utils/string_utils.go index 934550ba2..06505d26d 100644 --- a/cla-backend-go/utils/string_utils.go +++ b/cla-backend-go/utils/string_utils.go @@ -3,7 +3,10 @@ package utils -import "strings" +import ( + "strings" + "unicode" +) // TrimRemoveTrailingComma trims the whitespace on the specified string and removes the trailing comma func TrimRemoveTrailingComma(input string) string { @@ -40,3 +43,29 @@ func GetFirstAndLastName(firstAndLastName string) (string, string) { return strings.TrimSpace(userFirstName), strings.TrimSpace(userLastName) } + +// SanitizePlainText normalizes user-supplied free text: CR/LF variants become newlines, other +// control characters are dropped, the result is trimmed (HTML escaping is the renderer's job) +func SanitizePlainText(text string) string { + text = strings.ReplaceAll(text, "\r\n", "\n") + text = strings.ReplaceAll(text, "\r", "\n") + var builder strings.Builder + builder.Grow(len(text)) + for _, r := range text { + if r == '\n' || r == '\t' || !unicode.IsControl(r) { + builder.WriteRune(r) + } + } + return strings.TrimSpace(builder.String()) +} + +// SanitizeSingleLine strips every control character so user-influenced values cannot inject +// email header separators +func SanitizeSingleLine(value string) string { + return strings.Map(func(r rune) rune { + if unicode.IsControl(r) { + return -1 + } + return r + }, value) +} diff --git a/cla-backend-go/utils/string_utils_test.go b/cla-backend-go/utils/string_utils_test.go new file mode 100644 index 000000000..34eda9711 --- /dev/null +++ b/cla-backend-go/utils/string_utils_test.go @@ -0,0 +1,25 @@ +// Copyright The Linux Foundation and each contributor to CommunityBridge. +// SPDX-License-Identifier: MIT + +package utils + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestSanitizePlainText(t *testing.T) { + assert.Equal(t, "", SanitizePlainText("")) + assert.Equal(t, "", SanitizePlainText(" \r\n \x07\x1b \t ")) + assert.Equal(t, "one\ntwo\nthree", SanitizePlainText("one\r\ntwo\rthree"), "CR and CRLF normalize to LF") + assert.Equal(t, "keep\ttabs\nand lines", SanitizePlainText("keep\ttabs\nand lines")) + assert.Equal(t, "bell stripped", SanitizePlainText("bell\x07 stripped\x00")) + assert.Equal(t, "trimmed", SanitizePlainText(" trimmed \n")) +} + +func TestSanitizeSingleLine(t *testing.T) { + assert.Equal(t, "", SanitizeSingleLine("")) + assert.Equal(t, "Subject line", SanitizeSingleLine("Subject\r\n line\x07")) + assert.Equal(t, "no tabs", SanitizeSingleLine("no\t tabs")) +} diff --git a/cla-backend-go/v2/my_clas/cla_managers_test.go b/cla-backend-go/v2/my_clas/cla_managers_test.go index 2ddab5250..736933e54 100644 --- a/cla-backend-go/v2/my_clas/cla_managers_test.go +++ b/cla-backend-go/v2/my_clas/cla_managers_test.go @@ -24,6 +24,11 @@ type fakeEvents struct { } func (f *fakeEvents) LogEventWithContext(_ context.Context, args *events.LogEventArgs) { + // Mirror the identity gate in events.service.LogEventWithContext: events without a + // top-level UserID or LfUsername are dropped in production. + if args == nil || args.EventType == "" || args.EventData == nil || (args.UserID == "" && args.LfUsername == "") { + return + } f.logged = append(f.logged, args) } @@ -483,3 +488,62 @@ func TestSignedIdentityFallbacks(t *testing.T) { assert.True(t, isClaManager(&v1Models.Signature{SignatureACL: []v1Models.User{{Username: "SomeOne"}}}, "someone"), "the plain username matches too") } + +func TestCreateMyClaManagerRequestContact(t *testing.T) { + repo, signaturesService, companies := managersFixture() + repo.byLFUsername["someone"][0].LfEmail = someoneEmail + svc, eventsService, sent := newRequestTestService(repo, signaturesService, companies) + + result, err := svc.CreateMyClaManagerRequest(context.Background(), &Caller{Username: "someone"}, &Identity{}, "sig-ecla", + requestInput("contact", []string{"manager-one"}, "Hi there,\r\nplease advise.\x07")) + require.NoError(t, err) + assert.Equal(t, "contact", result.RequestType) + assert.Equal(t, "sent", result.Status) + + require.Len(t, *sent, 1) + email := (*sent)[0] + assert.Equal(t, "EasyCLA: Message from Some One regarding Good Corp", email.subject) + assert.Contains(t, email.body, "Hi <b>there</b>,\nplease advise.", "the message is HTML-escaped, CRLF-normalized and stripped of control characters") + assert.NotContains(t, email.body, "there") + assert.Contains(t, email.body, "You can reply to the contributor at "+someoneEmail) + assert.Contains(t, email.body, "This is a message only - no change was requested and none has been made") + assert.NotContains(t, email.body, "has requested") + + require.Len(t, eventsService.logged, 1) + eventData, ok := eventsService.logged[0].EventData.(*events.ContactCLAManagerRequestCreatedEventData) + require.True(t, ok) + assert.Equal(t, "contact", eventData.RequestType) + assert.Equal(t, "Hi there,\nplease advise.", eventData.Message) +} + +func TestCreateMyClaManagerRequestContactRequiresMessage(t *testing.T) { + repo, signaturesService, companies := managersFixture() + svc, eventsService, sent := newRequestTestService(repo, signaturesService, companies) + caller := &Caller{Username: "someone"} + + _, err := svc.CreateMyClaManagerRequest(context.Background(), caller, &Identity{}, "sig-ecla", + requestInput("contact", []string{"manager-one"}, "")) + assert.ErrorIs(t, err, ErrMissingMessage) + + _, err = svc.CreateMyClaManagerRequest(context.Background(), caller, &Identity{}, "sig-ecla", + requestInput("contact", []string{"manager-one"}, " \r\n \x07\x1b ")) + assert.ErrorIs(t, err, ErrMissingMessage, "a message that sanitizes to nothing cannot be sent") + + assert.Empty(t, *sent) + assert.Empty(t, eventsService.logged) +} + +func TestCreateMyClaManagerRequestSubjectStaysSingleLine(t *testing.T) { + repo, signaturesService, companies := managersFixture() + repo.byLFUsername["someone"][0].Username = "Evil\r\nBcc: victim@example.org" + svc, _, sent := newRequestTestService(repo, signaturesService, companies) + + _, err := svc.CreateMyClaManagerRequest(context.Background(), &Caller{Username: "someone"}, &Identity{}, "sig-ecla", + requestInput("removal", []string{"manager-one"}, "")) + require.NoError(t, err) + require.Len(t, *sent, 1) + subject := (*sent)[0].subject + assert.NotContains(t, subject, "\n") + assert.NotContains(t, subject, "\r") + assert.Contains(t, subject, "request from EvilBcc: victim@example.org for Good Corp") +} diff --git a/cla-backend-go/v2/my_clas/handlers.go b/cla-backend-go/v2/my_clas/handlers.go index f34f3afda..291fe1bdf 100644 --- a/cla-backend-go/v2/my_clas/handlers.go +++ b/cla-backend-go/v2/my_clas/handlers.go @@ -206,8 +206,8 @@ func Configure(api *operations.EasyclaAPI, service Service, callerVerifier Calle result, err := service.CreateMyClaManagerRequest(ctx, &Caller{Username: currentUsername, Admin: admin, Trusted: trusted}, requested, params.SignatureID, ¶ms.Body) if err != nil { - if errors.Is(err, ErrInvalidRecipients) { - log.WithFields(f).WithError(err).Warn("invalid recipients") + if errors.Is(err, ErrInvalidRecipients) || errors.Is(err, ErrMissingMessage) { + log.WithFields(f).WithError(err).Warn("invalid CLA manager request input") return myClasOps.NewCreateMyClaManagerRequestBadRequest().WithXRequestID(reqID).WithPayload(utils.ErrorResponseBadRequest(reqID, err.Error())) } msg := "unable to create the CLA manager request" diff --git a/cla-backend-go/v2/my_clas/prefetch.go b/cla-backend-go/v2/my_clas/prefetch.go index 1248e0aed..5b60f8f7d 100644 --- a/cla-backend-go/v2/my_clas/prefetch.go +++ b/cla-backend-go/v2/my_clas/prefetch.go @@ -98,11 +98,11 @@ func (s *service) prefetch(ctx context.Context, refs []claRef) (*claData, error) cclas: make(map[string]*v1Models.Signature), approvals: make(map[string]eclaCoverage), } - claGroupIDs, companyIDs, eclaRefs := distinctRefs(refs) + claGroupIDs, companyIDs, companyActors, eclaRefs := distinctRefs(refs) group, groupCtx := errgroup.WithContext(ctx) group.Go(func() error { return s.loadProjects(groupCtx, data, claGroupIDs) }) - group.Go(func() error { return s.loadEmployers(groupCtx, data, companyIDs) }) + group.Go(func() error { return s.loadEmployers(groupCtx, data, companyIDs, companyActors) }) group.Go(func() error { return s.loadCoverage(groupCtx, data, eclaRefs) }) if err := group.Wait(); err != nil { return nil, err @@ -110,11 +110,13 @@ func (s *service) prefetch(ctx context.Context, refs []claRef) (*claData, error) return data, nil } -// distinctRefs collects the keys to resolve: one per CLA Group, one per employer and one per (CLA -// Group, employer) pair, carrying the user records that pair must be evaluated for -func distinctRefs(refs []claRef) ([]string, []string, []eclaRef) { +// distinctRefs collects the keys to resolve: one per CLA Group, one per employer (with the first +// referencing user as the audit actor) and one per (CLA Group, employer) pair, carrying the user +// records that pair must be evaluated for +func distinctRefs(refs []claRef) ([]string, []string, map[string]*v1Models.User, []eclaRef) { var claGroupIDs, companyIDs []string var eclaRefs []eclaRef + companyActors := make(map[string]*v1Models.User) seenClaGroup := make(map[string]bool) seenCompany := make(map[string]bool) seenEcla := make(map[string]int) @@ -132,6 +134,7 @@ func distinctRefs(refs []claRef) ([]string, []string, []eclaRef) { if !seenCompany[companyID] { seenCompany[companyID] = true companyIDs = append(companyIDs, companyID) + companyActors[companyID] = ref.user } key := cclaKey(claGroupID, companyID) index, ok := seenEcla[key] @@ -145,7 +148,7 @@ func distinctRefs(refs []claRef) ([]string, []string, []eclaRef) { eclaRefs[index].users = append(eclaRefs[index].users, ref.user) } } - return claGroupIDs, companyIDs, eclaRefs + return claGroupIDs, companyIDs, companyActors, eclaRefs } // loadProjects resolves each distinct CLA Group's name and its project name and logo. These @@ -185,7 +188,7 @@ func (s *service) loadProjects(ctx context.Context, data *claData, claGroupIDs [ // loadEmployers looks up each distinct employer and screens it for sanctions in the same chain, // so a slow screen never delays another employer. A failed lookup degrades that employer's rows. -func (s *service) loadEmployers(ctx context.Context, data *claData, companyIDs []string) error { +func (s *service) loadEmployers(ctx context.Context, data *claData, companyIDs []string, companyActors map[string]*v1Models.User) error { f := logrus.Fields{ "functionName": "v2.my_clas.prefetch.loadEmployers", utils.XREQUESTID: ctx.Value(utils.XREQUESTID), @@ -203,7 +206,7 @@ func (s *service) loadEmployers(ctx context.Context, data *claData, companyIDs [ return nil } companyModels[i] = companyModel - states[i] = s.companySanctions(groupCtx, companyModel) + states[i] = s.companySanctions(groupCtx, companyModel, companyActors[companyID]) return nil }) } diff --git a/cla-backend-go/v2/my_clas/service.go b/cla-backend-go/v2/my_clas/service.go index 9aa4409fa..447a8fa08 100644 --- a/cla-backend-go/v2/my_clas/service.go +++ b/cla-backend-go/v2/my_clas/service.go @@ -147,6 +147,9 @@ type EventsService interface { // managers - empty is valid only when none resolves var ErrInvalidRecipients = errors.New("recipients must be a non-empty subset of the CLA managers returned by the cla-managers endpoint - empty only when no CLA manager resolves") +// ErrMissingMessage is returned when a contact request carries no (non-blank) message +var ErrMissingMessage = errors.New("message is required for a contact request and must not be blank") + // Service interface defines the My CLAs service methods type Service interface { GetMyClas(ctx context.Context, caller *Caller, requested *Identity) (*models.MyClaList, error) @@ -253,6 +256,9 @@ func (s *service) GetMyClas(ctx context.Context, caller *Caller, requested *Iden DocumentMinorVersion: int64(sig.SignatureDocumentMinorVersion), } row.SignedVia, row.SignedAs = signedIdentity(sig) + if sig.DateInvalidated != "" { + row.InvalidatedAt = utils.FormatTimeString(sig.DateInvalidated) + } if sig.SignatureUserCompanyID == "" { row.ClaType = utils.ClaTypeICLA @@ -269,13 +275,8 @@ func (s *service) GetMyClas(ctx context.Context, caller *Caller, requested *Iden sanction := data.sanctions[sig.SignatureUserCompanyID] row.Flagged = sanction.flagged row.FlaggedCheck = sanction.check - if sanction.flagged { - if sanction.date != "" { - row.FlaggedAt = utils.FormatTimeString(sanction.date) - } else { - // Flagged with no stored date and none could be stamped; report when it was seen. - _, row.FlaggedAt = utils.CurrentTime() - } + if sanction.flagged && sanction.date != "" { + row.FlaggedAt = utils.FormatTimeString(sanction.date) } coverage := data.coverage(sig, ref.user, sanction.flagged) row.Valid = sig.SignatureApproved && coverage.covered @@ -385,9 +386,10 @@ func (s *service) GetMyClaManagers(ctx context.Context, caller *Caller, requeste }, nil } -// CreateMyClaManagerRequest emails a removal/approval request against the caller's own ECLA to the -// selected CLA managers and logs the audit event that is its receipt - nil means unknown, -// not-owned, unsigned or ICLA signature ID, ErrInvalidRecipients an invalid recipients list +// CreateMyClaManagerRequest emails a removal/approval/contact request against the caller's own +// ECLA to the selected CLA managers and logs the audit event that is its receipt - nil means +// unknown, not-owned, unsigned or ICLA signature ID, ErrInvalidRecipients an invalid recipients +// list, ErrMissingMessage a contact request without a message func (s *service) CreateMyClaManagerRequest(ctx context.Context, caller *Caller, requested *Identity, signatureID string, input *models.MyClaManagerRequest) (*models.MyClaManagerRequestResult, error) { f := logrus.Fields{ "functionName": "v2.my_clas.service.CreateMyClaManagerRequest", @@ -437,7 +439,10 @@ func (s *service) CreateMyClaManagerRequest(ctx context.Context, caller *Caller, } requestType := utils.StringValue(input.RequestType) - message := strings.TrimSpace(input.Message) + message := utils.SanitizePlainText(input.Message) + if requestType == models.MyClaManagerRequestRequestTypeContact && message == "" { + return nil, ErrMissingMessage + } contributorName := userModel.Username if contributorName == "" { contributorName = identity.LfUsername @@ -454,10 +459,12 @@ func (s *service) CreateMyClaManagerRequest(ctx context.Context, caller *Caller, RequestAction: requestAction(requestType), ContributorName: contributorName, ContributorIdentity: contributorIdentity, + ContributorEmail: utils.GetBestEmail(userModel), CompanyName: details.companyName, ProjectName: details.projectName, CLAGroupName: details.claGroupName, OptionalMessage: message, + ContactOnly: requestType == models.MyClaManagerRequestRequestTypeContact, }) if err != nil { log.WithFields(f).WithError(err).Warn("unable to render the contact CLA manager email") @@ -474,7 +481,7 @@ func (s *service) CreateMyClaManagerRequest(ctx context.Context, caller *Caller, requestID := requestUUID.String() if len(recipientEmails) > 0 { - subject := fmt.Sprintf("EasyCLA: %s request from %s for %s", requestAction(requestType), contributorName, details.companyName) + subject := utils.SanitizeSingleLine(requestSubject(requestType, contributorName, details.companyName)) if sendErr := s.sendEmail(subject, body, recipientEmails); sendErr != nil { log.WithFields(f).WithError(sendErr).Warn("unable to send the contact CLA manager email") return nil, sendErr @@ -629,6 +636,13 @@ func requestAction(requestType string) string { return "approval under the corporate CLA" } +func requestSubject(requestType, contributorName, companyName string) string { + if requestType == models.MyClaManagerRequestRequestTypeContact { + return fmt.Sprintf("EasyCLA: Message from %s regarding %s", contributorName, companyName) + } + return fmt.Sprintf("EasyCLA: %s request from %s for %s", requestAction(requestType), contributorName, companyName) +} + func isClaManager(ccla *v1Models.Signature, lfUsername string) bool { if ccla == nil || lfUsername == "" { return false @@ -1077,14 +1091,14 @@ func (s *service) sanctionsMode() string { // companySanctions screens one employer, live where possible. An unreadable employer is // unavailable, never an absent answer. -func (s *service) companySanctions(ctx context.Context, companyModel *v1Models.Company) sanctionState { +func (s *service) companySanctions(ctx context.Context, companyModel *v1Models.Company, actor *v1Models.User) sanctionState { if companyModel == nil { return sanctionState{check: models.MyClaFlaggedCheckUnavailable} } state := sanctionState{flagged: companyModel.IsSanctioned, check: models.MyClaFlaggedCheckStored, date: companyModel.SanctionedDate} if s.sanctions != nil { state.flagged, state.check = s.sanctions.ScreenCompany(ctx, companyModel) - s.persistLiveSanction(ctx, companyModel, &state) + s.persistLiveSanction(ctx, companyModel, &state, actor) } return state } @@ -1093,7 +1107,7 @@ func (s *service) companySanctions(ctx context.Context, companyModel *v1Models.C // the reported date stops moving with every listing. A record already carrying the date is left // alone - restamping it here would drift on each page view - and a failed write only costs this // employer its stored date, never the listing. -func (s *service) persistLiveSanction(ctx context.Context, companyModel *v1Models.Company, state *sanctionState) { +func (s *service) persistLiveSanction(ctx context.Context, companyModel *v1Models.Company, state *sanctionState, actor *v1Models.User) { if !state.flagged || state.check != models.MyClaFlaggedCheckLive || (companyModel.IsSanctioned && companyModel.SanctionedDate != "") { return } @@ -1102,8 +1116,9 @@ func (s *service) persistLiveSanction(ctx context.Context, companyModel *v1Model utils.XREQUESTID: ctx.Value(utils.XREQUESTID), "companyID": companyModel.CompanyID, } + newSanction := !companyModel.IsSanctioned if err := s.companyRepo.UpdateCompanySanctionStatus(ctx, companyModel.CompanyID, true, sanctionOriginSSS); err != nil { - log.WithFields(f).WithError(err).Warnf("unable to persist the live sanction for company %s - reporting the observation time instead", companyModel.CompanyID) + log.WithFields(f).WithError(err).Warnf("unable to persist the live sanction for company %s - reporting the flag without a date", companyModel.CompanyID) // A retained date belongs to the previous, cleared sanction - drop it rather than // report it as this flag's date. state.date = "" @@ -1111,6 +1126,16 @@ func (s *service) persistLiveSanction(ctx context.Context, companyModel *v1Model } log.WithFields(f).Warnf("live screen flagged company %s, persisted the sanction with origin=%s", companyModel.CompanyID, sanctionOriginSSS) _, state.date = utils.CurrentTime() + if newSanction && s.eventsService != nil && actor != nil { + s.eventsService.LogEventWithContext(ctx, &events.LogEventArgs{ + EventType: events.CompanySanctioned, + UserID: actor.UserID, + LfUsername: actor.LfUsername, + UserModel: actor, + CompanyModel: companyModel, + EventData: &events.CompanySanctionedEventData{}, + }) + } } // assignMyClaStatus sets the contributor-facing status independently of approved/valid. A diff --git a/cla-backend-go/v2/my_clas/service_test.go b/cla-backend-go/v2/my_clas/service_test.go index 2effb88c4..52437be6c 100644 --- a/cla-backend-go/v2/my_clas/service_test.go +++ b/cla-backend-go/v2/my_clas/service_test.go @@ -12,6 +12,7 @@ import ( "testing" "time" + "github.com/linuxfoundation/easycla/cla-backend-go/events" v1Models "github.com/linuxfoundation/easycla/cla-backend-go/gen/v1/models" "github.com/linuxfoundation/easycla/cla-backend-go/gen/v2/models" "github.com/linuxfoundation/easycla/cla-backend-go/projects_cla_groups" @@ -1136,11 +1137,12 @@ func TestGetMyClasPersistsFirstLiveSanction(t *testing.T) { const storedDate = "2024-01-15T10:11:12.000000+0000" tests := []struct { - name string - company *v1Models.Company - writeErr error - wantWrites int - wantFlaggedAt string + name string + company *v1Models.Company + writeErr error + wantWrites int + wantFlaggedAt string + wantNoFlaggedAt bool }{ { name: "first live detection is persisted", @@ -1159,10 +1161,11 @@ func TestGetMyClasPersistsFirstLiveSanction(t *testing.T) { wantFlaggedAt: "2024-01-15T10:11:12Z", }, { - name: "a failed write reports the observation, not the cleared episode's date", - company: &v1Models.Company{CompanyID: "company-1", CompanyName: "Unwritable Corp", SanctionedDate: storedDate}, - writeErr: errors.New("dynamodb unavailable"), - wantWrites: 0, + name: "a failed write reports the flag without a date", + company: &v1Models.Company{CompanyID: "company-1", CompanyName: "Unwritable Corp", SanctionedDate: storedDate}, + writeErr: errors.New("dynamodb unavailable"), + wantWrites: 0, + wantNoFlaggedAt: true, }, } @@ -1196,7 +1199,9 @@ func TestGetMyClasPersistsFirstLiveSanction(t *testing.T) { } assert.True(t, row.Flagged) assert.Equal(t, models.MyClaFlaggedCheckLive, row.FlaggedCheck) - if tc.wantFlaggedAt != "" { + if tc.wantNoFlaggedAt { + assert.Empty(t, row.FlaggedAt, "a flag without a trustworthy date is reported without one") + } else if tc.wantFlaggedAt != "" { assert.Equal(t, tc.wantFlaggedAt, row.FlaggedAt, "the stored date is reported, not the response time") } else { assert.NotEmpty(t, row.FlaggedAt) @@ -1430,3 +1435,102 @@ func TestIdentityIsEmpty(t *testing.T) { assert.False(t, (&Identity{GitlabUsernames: []string{"someone"}}).IsEmpty()) assert.False(t, (&Identity{GerritUsernames: []string{"someone"}}).IsEmpty()) } +func TestGetMyClasEmitsCompanySanctionedEvent(t *testing.T) { + const storedDate = "2024-01-15T10:11:12.000000+0000" + + tests := []struct { + name string + company *v1Models.Company + writeErr error + wantEvents int + }{ + { + name: "a fresh flag is logged", + company: &v1Models.Company{CompanyID: "company-1", CompanyName: "Newly Flagged Corp"}, + wantEvents: 1, + }, + { + name: "a re-flag after a clear is logged", + company: &v1Models.Company{CompanyID: "company-1", CompanyName: "Repeat Corp", SanctionedDate: storedDate}, + wantEvents: 1, + }, + { + name: "an already sanctioned employer is not re-logged", + company: &v1Models.Company{CompanyID: "company-1", CompanyName: "Known Corp", IsSanctioned: true, SanctionedDate: storedDate}, + wantEvents: 0, + }, + { + name: "a date backfill for a known sanction is not logged", + company: &v1Models.Company{CompanyID: "company-1", CompanyName: "Dateless Corp", IsSanctioned: true}, + wantEvents: 0, + }, + { + name: "a failed persist is not logged", + company: &v1Models.Company{CompanyID: "company-1", CompanyName: "Unwritable Corp"}, + writeErr: errors.New("dynamodb unavailable"), + wantEvents: 0, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + userA := &v1Models.User{UserID: "user-a", LfUsername: "someone"} + companies := &fakeCompanies{ + byID: map[string]*v1Models.Company{"company-1": tc.company}, + writeErr: tc.writeErr, + } + repo := &fakeRepo{ + byUserID: map[string][]*signatures.ItemSignature{"user-a": {ecla("sig-1", "company-1", "2024-01-01T00:00:00Z", true)}}, + byLFUsername: map[string][]*v1Models.User{"someone": {userA}}, + } + signaturesService := &fakeSignatures{ + cclas: map[string]*v1Models.Signature{"cla-group-1|company-1": {SignatureID: "ccla-1"}}, + approvedUserIDs: map[string]bool{"user-a": true}, + } + svc := newTestService(repo, &fakePlatform{}, signaturesService, companies, &fakeClaGroups{}) + svc.sanctions = &fakeScreener{mode: models.MyClaListSssModeRequired, flagged: map[string]bool{"company-1": true}} + eventsLog := &fakeEvents{} + svc.eventsService = eventsLog + + _, err := svc.GetMyClas(context.Background(), &Caller{Username: "someone"}, &Identity{}) + require.NoError(t, err) + + require.Len(t, eventsLog.logged, tc.wantEvents) + if tc.wantEvents > 0 { + logged := eventsLog.logged[0] + assert.Equal(t, events.CompanySanctioned, logged.EventType) + assert.Equal(t, "user-a", logged.UserID, "a top-level user identity is required or the events service drops the event") + assert.Same(t, tc.company, logged.CompanyModel, "the company model is passed so the events service needs no extra lookup") + assert.Same(t, userA, logged.UserModel, "the listing user whose employer was screened is the event actor") + _, ok := logged.EventData.(*events.CompanySanctionedEventData) + assert.True(t, ok) + } + }) + } +} + +func TestGetMyClasInvalidatedAt(t *testing.T) { + userA := &v1Models.User{UserID: "user-a", LfUsername: "someone"} + invalidated := icla("sig-invalidated", "user-a", "cla-group-1", "2024-01-01T00:00:00Z", false) + invalidated.DateInvalidated = "2024-03-04T05:06:07.000000+0000" + invalidated.InvalidatedBy = "admin-user" + valid := icla("sig-valid", "user-a", "cla-group-1", "2024-02-01T00:00:00Z", true) + + repo := &fakeRepo{ + byUserID: map[string][]*signatures.ItemSignature{"user-a": {invalidated, valid}}, + byLFUsername: map[string][]*v1Models.User{"someone": {userA}}, + } + svc := newTestService(repo, &fakePlatform{}, &fakeSignatures{}, &fakeCompanies{}, &fakeClaGroups{}) + + result, err := svc.GetMyClas(context.Background(), &Caller{Username: "someone"}, &Identity{}) + require.NoError(t, err) + byID := map[string]models.MyCla{} + for _, row := range result.Clas { + byID[row.SignatureID] = row + } + + assert.Equal(t, "2024-03-04T05:06:07Z", byID["sig-invalidated"].InvalidatedAt) + assert.Equal(t, models.MyClaStatusInvalidated, byID["sig-invalidated"].Status) + assert.Empty(t, byID["sig-valid"].InvalidatedAt) + assert.Equal(t, models.MyClaStatusValid, byID["sig-valid"].Status) +} diff --git a/cla-backend-go/v2/sign/helpers.go b/cla-backend-go/v2/sign/helpers.go index 33f9b089f..d779eabe4 100644 --- a/cla-backend-go/v2/sign/helpers.go +++ b/cla-backend-go/v2/sign/helpers.go @@ -152,12 +152,16 @@ func (s service) hasUserSigned(ctx context.Context, user *models.User, projectID } // Check if company is sanctioned before allowing ECLA acknowledgement + wasSanctioned := companyModel.IsSanctioned sanctioned, sanctionErr := s.checkCompanyCompliance(ctx, companyModel) if sanctionErr != nil { log.WithFields(f).WithError(sanctionErr).Warnf("failed to check company compliance for company: %s", companyID) return &hasSigned, &companyAffiliation, sanctionErr } if sanctioned { + if !wasSanctioned { + s.logCompanySanctionedEvent(ctx, companyModel, user, "") + } sanctionedErr := fmt.Errorf("company %s is sanctioned", companyID) log.WithFields(f).WithError(sanctionedErr).Error("company is sanctioned") return &hasSigned, &companyAffiliation, sanctionedErr diff --git a/cla-backend-go/v2/sign/icla_block_test.go b/cla-backend-go/v2/sign/icla_block_test.go new file mode 100644 index 000000000..ce7bfc7dd --- /dev/null +++ b/cla-backend-go/v2/sign/icla_block_test.go @@ -0,0 +1,81 @@ +// Copyright The Linux Foundation and each contributor to CommunityBridge. +// SPDX-License-Identifier: MIT + +package sign + +import ( + "context" + "testing" + + "github.com/golang/mock/gomock" + "github.com/linuxfoundation/easycla/cla-backend-go/events" + eventsMock "github.com/linuxfoundation/easycla/cla-backend-go/events/mock" + v1Models "github.com/linuxfoundation/easycla/cla-backend-go/gen/v1/models" + "github.com/stretchr/testify/assert" +) + +func TestHasInvalidatedIcla(t *testing.T) { + assert.False(t, hasInvalidatedIcla(nil)) + assert.False(t, hasInvalidatedIcla([]*v1Models.Signature{nil})) + assert.False(t, hasInvalidatedIcla([]*v1Models.Signature{ + {SignatureID: "in-progress", SignatureSigned: false, SignatureApproved: true}, + {SignatureID: "valid", SignatureSigned: true, SignatureApproved: true}, + {SignatureID: "abandoned", SignatureSigned: false, SignatureApproved: false}, + })) + assert.True(t, hasInvalidatedIcla([]*v1Models.Signature{ + {SignatureID: "valid", SignatureSigned: true, SignatureApproved: true}, + {SignatureID: "invalidated", SignatureSigned: true, SignatureApproved: false}, + }), "a signed but unapproved ICLA marks an administrator invalidation") +} + +func TestLogCompanySanctionedEventIdentity(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + comp := &v1Models.Company{CompanyID: "company-1", CompanyName: "Flagged Corp"} + tests := []struct { + name string + userModel *v1Models.User + lfUsername string + wantUserID string + wantLfUsername string + }{ + { + name: "a user model supplies the identity the events gate requires", + userModel: &v1Models.User{UserID: "user-1", LfUsername: "contributor"}, + wantUserID: "user-1", + wantLfUsername: "contributor", + }, + { + name: "an explicit lf username is kept", + userModel: &v1Models.User{UserID: "user-1", LfUsername: "contributor"}, + lfUsername: "manager", + wantUserID: "user-1", + wantLfUsername: "manager", + }, + { + name: "an lf username alone passes the gate", + lfUsername: "manager", + wantLfUsername: "manager", + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + mockEvents := eventsMock.NewMockService(ctrl) + var logged *events.LogEventArgs + mockEvents.EXPECT().LogEventWithContext(gomock.Any(), gomock.Any()).Do( + func(_ context.Context, args *events.LogEventArgs) { + logged = args + }) + svc := &service{eventsService: mockEvents} + svc.logCompanySanctionedEvent(context.Background(), comp, tc.userModel, tc.lfUsername) + if assert.NotNil(t, logged) { + assert.True(t, logged.UserID != "" || logged.LfUsername != "", "the events service drops events without a top-level user identity") + assert.Equal(t, tc.wantUserID, logged.UserID) + assert.Equal(t, tc.wantLfUsername, logged.LfUsername) + assert.Same(t, comp, logged.CompanyModel) + assert.Equal(t, events.CompanySanctioned, logged.EventType) + } + }) + } +} diff --git a/cla-backend-go/v2/sign/service.go b/cla-backend-go/v2/sign/service.go index 56e71676a..b98b2cff8 100644 --- a/cla-backend-go/v2/sign/service.go +++ b/cla-backend-go/v2/sign/service.go @@ -72,6 +72,7 @@ var ( ErrCCLANotEnabled = errors.New("corporate license agreement is not enabled with this project") ErrTemplateNotConfigured = errors.New("cla template not configured for this project") ErrNotInOrg error + ErrIclaInvalidated = errors.New("an individual CLA for this CLA Group was invalidated by an administrator - signing a new individual CLA for this CLA Group is not permitted") ) // ProjectRepo contains project repo methods @@ -272,12 +273,16 @@ func (s *service) RequestCorporateSignature(ctx context.Context, lfUsername stri return nil, fmt.Errorf("company not found") } + wasSanctioned := comp.IsSanctioned sanctioned, sanctionErr := s.checkCompanyCompliance(ctx, comp) if sanctionErr != nil { log.WithFields(f).WithError(sanctionErr).Error("failed to check company compliance") return nil, sanctionErr } if sanctioned { + if !wasSanctioned { + s.logCompanySanctionedEvent(ctx, comp, nil, lfUsername) + } if input.CompanySfid != nil { err = fmt.Errorf("company %s requires further review for trade compliance", *input.CompanySfid) } else { @@ -1259,10 +1264,14 @@ func (s *service) SignedCorporateCallback(ctx context.Context, payload []byte, c // Sanctions gate: re-screen the company before finalizing the CCLA. A company can // become blocked (manual/admin or SSS) between the DocuSign request and this // completion callback; do not finalize a corporate CLA for a sanctioned company. + wasSanctioned := companyModel.IsSanctioned if sanctioned, complianceErr := s.checkCompanyCompliance(ctx, companyModel); complianceErr != nil { log.WithFields(f).WithError(complianceErr).Warnf("company compliance check failed in corporate callback for company %s; not finalizing CCLA", companyID) return complianceErr } else if sanctioned { + if !wasSanctioned { + s.logCompanySanctionedEvent(ctx, companyModel, user, "") + } log.WithFields(f).Warnf("company %s requires further review for trade compliance; refusing to finalize corporate CLA in callback", companyID) return fmt.Errorf("company %s requires further review for trade compliance; corporate CLA cannot be finalized", companyID) } @@ -1412,6 +1421,10 @@ func (s *service) RequestIndividualSignature(ctx context.Context, input *models. return nil, err } log.WithFields(f).Debugf("found %d signatures for user: %s", len(userSignatures.Signatures), *input.UserID) + if hasInvalidatedIcla(userSignatures.Signatures) { + log.WithFields(f).Warnf("user %s has an invalidated ICLA for project %s - blocking a new individual signature", *input.UserID, *input.ProjectID) + return nil, ErrIclaInvalidated + } latestSignature := getLatestSignature(userSignatures.Signatures) // loading latest document @@ -2455,6 +2468,15 @@ func getLatestSignature(signatures []*v1Models.Signature) *v1Models.Signature { return latestSignature } +func hasInvalidatedIcla(signatures []*v1Models.Signature) bool { + for _, signature := range signatures { + if signature != nil && signature.SignatureSigned && !signature.SignatureApproved { + return true + } + } + return false +} + func (s *service) RequestIndividualSignatureGerrit(ctx context.Context, input *models.IndividualSignatureInput) (*models.IndividualSignatureOutput, error) { f := logrus.Fields{ "functionName": "sign.RequestIndividualSignatureGerrit", @@ -2502,6 +2524,11 @@ func (s *service) RequestIndividualSignatureGerrit(ctx context.Context, input *m return nil, err } + if hasInvalidatedIcla(userSignatures.Signatures) { + log.WithFields(f).Warnf("user %s has an invalidated ICLA for project %s - blocking a new individual signature", *input.UserID, *input.ProjectID) + return nil, ErrIclaInvalidated + } + latestSignature := getLatestSignature(userSignatures.Signatures) //loading latest document @@ -3232,6 +3259,28 @@ func (s *service) checkCompanyCompliance(ctx context.Context, company *v1Models. return sanctioned, nil } +// logCompanySanctionedEvent records the audit event for a company newly flagged as sanctioned +func (s *service) logCompanySanctionedEvent(ctx context.Context, comp *v1Models.Company, userModel *v1Models.User, lfUsername string) { + if s.eventsService == nil || comp == nil || (userModel == nil && lfUsername == "") { + return + } + args := &events.LogEventArgs{ + EventType: events.CompanySanctioned, + UserModel: userModel, + LfUsername: lfUsername, + CompanyModel: comp, + EventData: &events.CompanySanctionedEventData{}, + } + // LogEventWithContext requires a top-level UserID or LfUsername before it consults UserModel. + if userModel != nil { + args.UserID = userModel.UserID + if args.LfUsername == "" { + args.LfUsername = userModel.LfUsername + } + } + s.eventsService.LogEventWithContext(ctx, args) +} + // complianceUnavailable returns the screening decision for a path that could not // produce a live SSS result: block when SSS is required, otherwise honor the // persisted sanction state. The specific cause is carried by resultErr. diff --git a/cla-backend-go/v2/signatures/handlers.go b/cla-backend-go/v2/signatures/handlers.go index 86db493e7..1136cbed5 100644 --- a/cla-backend-go/v2/signatures/handlers.go +++ b/cla-backend-go/v2/signatures/handlers.go @@ -1269,7 +1269,7 @@ func Configure(api *operations.EasyclaAPI, claGroupService service.Service, proj InvalidatedCount: 1, }, } - err := v2SignatureService.InvalidateICLA(ctx, params.ClaGroupID, params.UserID, authUser, eventsService, eventArgs) + err := v2SignatureService.InvalidateICLA(ctx, params.ClaGroupID, params.UserID, authUser, eventsService, eventArgs, ¶ms.Body) if err != nil { msg := "unable to invalidate icla" log.WithFields(f).Warn(msg) diff --git a/cla-backend-go/v2/signatures/service.go b/cla-backend-go/v2/signatures/service.go index 4e8bb08ff..83d77a907 100644 --- a/cla-backend-go/v2/signatures/service.go +++ b/cla-backend-go/v2/signatures/service.go @@ -57,7 +57,7 @@ type ServiceInterface interface { GetSignedDocument(ctx context.Context, signatureID string) (*models.SignedDocument, error) GetSignedIclaZipPdf(claGroupID string) (*models.URLObject, error) GetSignedCclaZipPdf(claGroupID string) (*models.URLObject, error) - InvalidateICLA(ctx context.Context, claGroupID string, userID string, authUser *auth.User, eventsService events.Service, eventArgs *events.LogEventArgs) error + InvalidateICLA(ctx context.Context, claGroupID string, userID string, authUser *auth.User, eventsService events.Service, eventArgs *events.LogEventArgs, input *models.IclaInvalidationInput) error EclaAutoCreate(ctx context.Context, signatureID string, autoCreateECLA bool) error IsUserAuthorized(ctx context.Context, lfid, claGroupId string) (*models.LfidAuthorizedResponse, error) } @@ -351,8 +351,9 @@ func (s *Service) GetClaGroupCorporateContributors(ctx context.Context, params v return &resp, nil } -// InvalidateICLA invalidates the specified signature record using the supplied parameters -func (s *Service) InvalidateICLA(ctx context.Context, claGroupID string, userID string, authUser *auth.User, eventsService events.Service, eventArgs *events.LogEventArgs) error { +// InvalidateICLA invalidates the specified signature record using the supplied parameters - +// input optionally carries the invalidation reason and note recorded on the record +func (s *Service) InvalidateICLA(ctx context.Context, claGroupID string, userID string, authUser *auth.User, eventsService events.Service, eventArgs *events.LogEventArgs, input *models.IclaInvalidationInput) error { f := logrus.Fields{ "functionName": "v2.signatures.service.InvalidateICLA", "claGroupID": claGroupID, @@ -388,7 +389,14 @@ func (s *Service) InvalidateICLA(ctx context.Context, claGroupID string, userID log.WithFields(f).Debug("invalidating signature record ...") note := fmt.Sprintf("Signature invalidated (approved set to false) by %s for %s ", authUser.UserName, utils.GetBestUsername(user)) - err := s.v1SignatureRepo.InvalidateProjectRecord(ctx, icla.SignatureID, note) + metadata := &signatures.InvalidationMetadata{ + InvalidatedBy: authUser.UserName, + } + if input != nil { + metadata.Reason = input.Reason + metadata.Note = utils.SanitizePlainText(input.Note) + } + err := s.v1SignatureRepo.InvalidateProjectRecordWithMetadata(ctx, icla.SignatureID, note, metadata) if err != nil { log.WithFields(f).Debug("unable to invalidate icla record") return err @@ -414,7 +422,14 @@ func (s *Service) InvalidateICLA(ctx context.Context, claGroupID string, userID eventArgs.UserName = utils.GetBestUsername(user) eventArgs.UserModel = user + eventArgs.UserID = user.UserID eventArgs.ProjectName = claGroup.ProjectName + if eventData, ok := eventArgs.EventData.(*events.SignatureProjectInvalidatedEventData); ok { + eventData.SignatureID = icla.SignatureID + eventData.InvalidatedBy = authUser.UserName + eventData.Reason = metadata.Reason + eventData.InvalidationNote = metadata.Note + } // Log event eventsService.LogEventWithContext(ctx, eventArgs) diff --git a/cla-backend-go/v2/signatures/service_test.go b/cla-backend-go/v2/signatures/service_test.go index 99935bc29..bef89ee8a 100644 --- a/cla-backend-go/v2/signatures/service_test.go +++ b/cla-backend-go/v2/signatures/service_test.go @@ -13,10 +13,14 @@ import ( "github.com/linuxfoundation/easycla/cla-backend-go/utils" // mock_signatures "github.com/linuxfoundation/easycla/cla-backend-go/v2/signatures/mock_v1_signatures" + "github.com/LF-Engineering/lfx-kit/auth" "github.com/golang/mock/gomock" mock_company "github.com/linuxfoundation/easycla/cla-backend-go/company/mocks" + "github.com/linuxfoundation/easycla/cla-backend-go/events" + eventsMock "github.com/linuxfoundation/easycla/cla-backend-go/events/mock" ini "github.com/linuxfoundation/easycla/cla-backend-go/init" mock_project "github.com/linuxfoundation/easycla/cla-backend-go/project/mocks" + v1Signatures "github.com/linuxfoundation/easycla/cla-backend-go/signatures" mock_v1_signatures "github.com/linuxfoundation/easycla/cla-backend-go/signatures/mocks" mock_users "github.com/linuxfoundation/easycla/cla-backend-go/v2/signatures/mock_users" "github.com/stretchr/testify/assert" @@ -287,3 +291,132 @@ func TestService_IsUserAuthorized(t *testing.T) { }) } } + +func TestService_InvalidateICLA(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + awsSession, err := ini.GetAWSSession() + if err != nil { + assert.Fail(t, "unable to create AWS session") + } + + ctx := context.Background() + approved, signed := true, true + + mockSignatureService := mock_v1_signatures.NewMockSignatureService(ctrl) + mockSignatureService.EXPECT().GetIndividualSignature(ctx, "cla-group-1", "user-1", &approved, &signed). + Return(&v1Models.Signature{SignatureID: "sig-1"}, nil) + + mockProjectService := mock_project.NewMockService(ctrl) + mockProjectService.EXPECT().GetCLAGroupByID(ctx, "cla-group-1"). + Return(&v1Models.ClaGroup{ProjectName: "My Project", Version: "v2"}, nil) + + mockUserService := mock_users.NewMockService(ctrl) + mockUserService.EXPECT().GetUser("user-1"). + Return(&v1Models.User{UserID: "user-1", LfUsername: "contributor", Username: "Contributor"}, nil) + + mockRepo := mock_v1_signatures.NewMockSignatureRepository(ctrl) + var gotNote string + var gotMetadata *v1Signatures.InvalidationMetadata + mockRepo.EXPECT().InvalidateProjectRecordWithMetadata(ctx, "sig-1", gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, _, note string, metadata *v1Signatures.InvalidationMetadata) error { + gotNote = note + gotMetadata = metadata + return nil + }) + + mockEvents := eventsMock.NewMockService(ctrl) + var logged *events.LogEventArgs + mockEvents.EXPECT().LogEventWithContext(ctx, gomock.Any()).Do( + func(_ context.Context, args *events.LogEventArgs) { + logged = args + }) + + service := NewService(awsSession, "", mockProjectService, nil, mockSignatureService, nil, mockRepo, mockUserService, nil) + + eventArgs := &events.LogEventArgs{ + EventType: events.InvalidatedSignature, + EventData: &events.SignatureProjectInvalidatedEventData{InvalidatedCount: 1}, + } + input := &models.IclaInvalidationInput{Reason: "compliance", Note: "per legal\r\nreview\x07"} + err = service.InvalidateICLA(ctx, "cla-group-1", "user-1", &auth.User{UserName: "admin-user"}, mockEvents, eventArgs, input) + assert.Nil(t, err) + + assert.Contains(t, gotNote, "Signature invalidated (approved set to false) by admin-user for Contributor") + if assert.NotNil(t, gotMetadata) { + assert.Equal(t, "admin-user", gotMetadata.InvalidatedBy) + assert.Equal(t, "compliance", gotMetadata.Reason) + assert.Equal(t, "per legal\nreview", gotMetadata.Note, "the note is sanitized before it is stored") + } + + if assert.NotNil(t, logged) { + eventData, ok := logged.EventData.(*events.SignatureProjectInvalidatedEventData) + if assert.True(t, ok) { + assert.Equal(t, "sig-1", eventData.SignatureID) + assert.Equal(t, "admin-user", eventData.InvalidatedBy) + assert.Equal(t, "compliance", eventData.Reason) + assert.Equal(t, "per legal\nreview", eventData.InvalidationNote) + } + assert.Equal(t, "Contributor", logged.UserName) + assert.Equal(t, "user-1", logged.UserID, "a top-level user identity is required or the events service drops the event") + assert.Equal(t, "My Project", logged.ProjectName) + } +} + +func TestService_InvalidateICLAWithoutBody(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + awsSession, err := ini.GetAWSSession() + if err != nil { + assert.Fail(t, "unable to create AWS session") + } + + ctx := context.Background() + approved, signed := true, true + + mockSignatureService := mock_v1_signatures.NewMockSignatureService(ctrl) + mockSignatureService.EXPECT().GetIndividualSignature(ctx, "cla-group-1", "user-1", &approved, &signed). + Return(&v1Models.Signature{SignatureID: "sig-1"}, nil) + + mockProjectService := mock_project.NewMockService(ctrl) + mockProjectService.EXPECT().GetCLAGroupByID(ctx, "cla-group-1"). + Return(&v1Models.ClaGroup{ProjectName: "My Project", Version: "v2"}, nil) + + mockUserService := mock_users.NewMockService(ctrl) + mockUserService.EXPECT().GetUser("user-1"). + Return(&v1Models.User{UserID: "user-1", LfUsername: "contributor"}, nil) + + mockRepo := mock_v1_signatures.NewMockSignatureRepository(ctrl) + var gotMetadata *v1Signatures.InvalidationMetadata + mockRepo.EXPECT().InvalidateProjectRecordWithMetadata(ctx, "sig-1", gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, _, _ string, metadata *v1Signatures.InvalidationMetadata) error { + gotMetadata = metadata + return nil + }) + + mockEvents := eventsMock.NewMockService(ctrl) + var logged *events.LogEventArgs + mockEvents.EXPECT().LogEventWithContext(ctx, gomock.Any()).Do( + func(_ context.Context, args *events.LogEventArgs) { + logged = args + }) + + service := NewService(awsSession, "", mockProjectService, nil, mockSignatureService, nil, mockRepo, mockUserService, nil) + + eventArgs := &events.LogEventArgs{ + EventType: events.InvalidatedSignature, + EventData: &events.SignatureProjectInvalidatedEventData{InvalidatedCount: 1}, + } + err = service.InvalidateICLA(ctx, "cla-group-1", "user-1", &auth.User{UserName: "admin-user"}, mockEvents, eventArgs, nil) + assert.Nil(t, err) + if assert.NotNil(t, logged) { + assert.Equal(t, "user-1", logged.UserID, "a top-level user identity is required or the events service drops the event") + } + if assert.NotNil(t, gotMetadata) { + assert.Equal(t, "admin-user", gotMetadata.InvalidatedBy) + assert.Empty(t, gotMetadata.Reason) + assert.Empty(t, gotMetadata.Note) + } +} diff --git a/docs/MY_CLAS_API.md b/docs/MY_CLAS_API.md index b56b194fc..61a2febb0 100644 --- a/docs/MY_CLAS_API.md +++ b/docs/MY_CLAS_API.md @@ -29,7 +29,7 @@ recycled-alias trade-off is under "Known limitations". | `GET` | `/v4/my-clas/{signatureID}/pdf` | Time-limited (15 min) presigned S3 URL for a signed ICLA PDF owned by the provided identity | | `GET` | `/v4/my-clas/identities` | List the deduplicated `:` identities the authenticated user owns (no query params) | | `GET` | `/v4/my-clas/{signatureID}/cla-managers` | List the CLA managers of the CCLA covering an ECLA owned by the provided identity | -| `POST` | `/v4/my-clas/{signatureID}/cla-manager-requests` | Email a removal/approval request for an owned ECLA to selected CLA managers (M2; swagger-documented) | +| `POST` | `/v4/my-clas/{signatureID}/cla-manager-requests` | Email a removal/approval request or a contact-only message for an owned ECLA to selected CLA managers (M2; swagger-documented; `requestType=contact` requires a non-blank `message`) | ## Changed repositories and branches @@ -403,8 +403,9 @@ question (the lookup/domain/status logic is duplicated from `v2/sign`'s listings. An employer *currently* sanctioned and already carrying the date is never restamped by the listing; one that was cleared but retained its date is restamped on the next live detection, and the signing and legacy SSS flows restamp on every flagged detection by design. -A failed write costs that employer only its stored date (the row then reports the observation -time), and the listing never clears a flag. +A failed write costs that employer only its stored date (the flag is then reported without a +date), and the listing never clears a flag. The first persist of a new sanction also logs a +`company.sanctioned` event. ### Response — `200 my-cla-list` @@ -475,7 +476,8 @@ Field reference (`my-cla` rows): | `valid` | bool | Computed as defined above | | `status` | `valid` \| `needs_attention` \| `revoked` \| `invalidated` \| `unknown` | Contributor-facing standing, computed independently of `approved`/`valid` (see step 5). New values may be added in a future spec revision (generated clients validate the enum strictly) | | `statusReason` | `not_on_approval_list` \| `unknown` | Why the standing is not `valid`; omitted for every other status and on every ICLA | -| `flagged` / `flaggedAt` | bool / string | ECLA only: the employer is currently flagged by sanctions screening, and the company's stored `sanctioned_date` — stamped at the first live detection, so the response time only when no date is stored and stamping it failed | +| `flagged` / `flaggedAt` | bool / string | ECLA only: the employer is currently flagged by sanctions screening, and the company's stored `sanctioned_date` — stamped at the first live detection; `flaggedAt` is omitted when no stored date exists (issue #1370: the revocation date) | +| `invalidatedAt` | string | The record's `date_invalidated` — stamped by the PCC admin ICLA invalidation (kept from the first invalidation); omitted for records invalidated before the field existed (issue #1732) | | `flaggedCheck` | `live` \| `stored` \| `unavailable` | ECLA only: how `flagged` was obtained (see step 5). `unavailable` means the value is the persisted flag and may be stale | | `signedVia` / `signedAs` | string | The platform signed via (`github`, `gitlab`, `gerrit` — the last also covers LF SSO signings identified by email) and the account signed as; omitted when the record carries no such identity | | `claManager` | bool | ECLA only: the owning user is a CLA manager of the employer's CCLA for this CLA Group | From 8958c59628d5bd214c033710714f9712d1ffc45e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Gryglicki?= Date: Tue, 25 Aug 2026 08:23:57 +0000 Subject: [PATCH 2/3] M2: MyCLAs contact requests, invalidation/sanction metadata, ICLA re-sign block - address AI feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Łukasz Gryglicki Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot) Assisted by [Claude](https://claude.ai) --- cla-backend-go/signatures/repository.go | 58 ++++++++++------- cla-backend-go/signatures/repository_test.go | 52 +++++++++++++++ cla-backend-go/v2/sign/icla_block_test.go | 66 ++++++++++++++++++++ cla-backend-go/v2/sign/service.go | 30 ++++++++- 4 files changed, 180 insertions(+), 26 deletions(-) create mode 100644 cla-backend-go/signatures/repository_test.go diff --git a/cla-backend-go/signatures/repository.go b/cla-backend-go/signatures/repository.go index d40f2b1c3..b96e38c3c 100644 --- a/cla-backend-go/signatures/repository.go +++ b/cla-backend-go/signatures/repository.go @@ -2138,8 +2138,10 @@ func (repo repository) InvalidateProjectRecord(ctx context.Context, signatureID, } // InvalidateProjectRecordWithMetadata invalidates the specified project record by setting the -// signature_approved flag to false and records the invalidation attribution: date_invalidated -// (kept from the first invalidation), invalidated_by, invalidation_reason, invalidation_note +// signature_approved flag to false and records the invalidation attribution. The attribution +// attributes (date_invalidated, invalidated_by, invalidation_reason, invalidation_note) are +// first-write-wins so a re-invalidation never destroys the record of a prior invalidation; +// attributes missing on pre-feature records are still populated. func (repo repository) InvalidateProjectRecordWithMetadata(ctx context.Context, signatureID, note string, metadata *InvalidationMetadata) error { f := logrus.Fields{ "functionName": "v1.signatures.repository.InvalidateProjectRecordWithMetadata", @@ -2151,6 +2153,32 @@ func (repo repository) InvalidateProjectRecordWithMetadata(ctx context.Context, _, now := utils.CurrentTime() + expressionAttributeNames, expressionAttributeValues, updateExpression := invalidationUpdateExpression(note, now, metadata) + + input := &dynamodb.UpdateItemInput{ + Key: map[string]*dynamodb.AttributeValue{ + "signature_id": { + S: aws.String(signatureID), + }, + }, + ExpressionAttributeNames: expressionAttributeNames, + ExpressionAttributeValues: expressionAttributeValues, + UpdateExpression: &updateExpression, + TableName: aws.String(signatureTableName), + } + + _, updateErr := repo.dynamoDBClient.UpdateItem(input) + if updateErr != nil { + log.WithFields(f).Warnf("error updating signature_approved for signature_id : %s error : %v ", signatureID, updateErr) + return updateErr + } + + return nil +} + +// invalidationUpdateExpression assembles the invalidation update: approval revoked, note replaced, +// every attribution attribute first-write-wins via if_not_exists, date_modified refreshed. +func invalidationUpdateExpression(note, now string, metadata *InvalidationMetadata) (map[string]*string, map[string]*dynamodb.AttributeValue, string) { expressionAttributeNames := map[string]*string{} expressionAttributeValues := map[string]*dynamodb.AttributeValue{} updateExpression := "SET " // nolint @@ -2171,17 +2199,17 @@ func (repo repository) InvalidateProjectRecordWithMetadata(ctx context.Context, if metadata.InvalidatedBy != "" { expressionAttributeNames["#IB"] = aws.String("invalidated_by") expressionAttributeValues[":ib"] = &dynamodb.AttributeValue{S: aws.String(metadata.InvalidatedBy)} - updateExpression = updateExpression + " #IB = :ib," + updateExpression = updateExpression + " #IB = if_not_exists(#IB, :ib)," } if metadata.Reason != "" { expressionAttributeNames["#IR"] = aws.String("invalidation_reason") expressionAttributeValues[":ir"] = &dynamodb.AttributeValue{S: aws.String(metadata.Reason)} - updateExpression = updateExpression + " #IR = :ir," + updateExpression = updateExpression + " #IR = if_not_exists(#IR, :ir)," } if metadata.Note != "" { expressionAttributeNames["#IN"] = aws.String("invalidation_note") expressionAttributeValues[":in"] = &dynamodb.AttributeValue{S: aws.String(metadata.Note)} - updateExpression = updateExpression + " #IN = :in," + updateExpression = updateExpression + " #IN = if_not_exists(#IN, :in)," } } @@ -2189,25 +2217,7 @@ func (repo repository) InvalidateProjectRecordWithMetadata(ctx context.Context, expressionAttributeValues[":m"] = &dynamodb.AttributeValue{S: aws.String(now)} updateExpression = updateExpression + " #M = :m" - input := &dynamodb.UpdateItemInput{ - Key: map[string]*dynamodb.AttributeValue{ - "signature_id": { - S: aws.String(signatureID), - }, - }, - ExpressionAttributeNames: expressionAttributeNames, - ExpressionAttributeValues: expressionAttributeValues, - UpdateExpression: &updateExpression, - TableName: aws.String(signatureTableName), - } - - _, updateErr := repo.dynamoDBClient.UpdateItem(input) - if updateErr != nil { - log.WithFields(f).Warnf("error updating signature_approved for signature_id : %s error : %v ", signatureID, updateErr) - return updateErr - } - - return nil + return expressionAttributeNames, expressionAttributeValues, updateExpression } // ValidateProjectRecord validates the specified project record by setting the signature_approved flag to true diff --git a/cla-backend-go/signatures/repository_test.go b/cla-backend-go/signatures/repository_test.go new file mode 100644 index 000000000..8864db3dd --- /dev/null +++ b/cla-backend-go/signatures/repository_test.go @@ -0,0 +1,52 @@ +// Copyright The Linux Foundation and each contributor to CommunityBridge. +// SPDX-License-Identifier: MIT + +package signatures + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestInvalidationUpdateExpression(t *testing.T) { + const now = "2024-05-06T07:08:09.000000+0000" + + names, values, expr := invalidationUpdateExpression("a note", now, &InvalidationMetadata{ + InvalidatedBy: "admin-user", + Reason: "compliance", + Note: "per legal review", + }) + + assert.Contains(t, expr, "#A = :a") + assert.Contains(t, expr, "#S = :s") + assert.Contains(t, expr, "#DI = if_not_exists(#DI, :di)") + assert.Contains(t, expr, "#IB = if_not_exists(#IB, :ib)", "a re-invalidation must not overwrite the first actor") + assert.Contains(t, expr, "#IR = if_not_exists(#IR, :ir)", "a re-invalidation must not overwrite the first reason") + assert.Contains(t, expr, "#IN = if_not_exists(#IN, :in)", "a re-invalidation must not overwrite the first note") + assert.Contains(t, expr, "#M = :m") + + assert.Equal(t, "invalidated_by", *names["#IB"]) + assert.Equal(t, "invalidation_reason", *names["#IR"]) + assert.Equal(t, "invalidation_note", *names["#IN"]) + assert.Equal(t, "admin-user", *values[":ib"].S) + assert.Equal(t, "compliance", *values[":ir"].S) + assert.Equal(t, "per legal review", *values[":in"].S) + assert.Equal(t, now, *values[":di"].S) + assert.False(t, *values[":a"].BOOL) +} + +func TestInvalidationUpdateExpressionWithoutMetadata(t *testing.T) { + const now = "2024-05-06T07:08:09.000000+0000" + + for _, metadata := range []*InvalidationMetadata{nil, {}} { + names, values, expr := invalidationUpdateExpression("a note", now, metadata) + + assert.NotContains(t, expr, "#IB") + assert.NotContains(t, expr, "#IR") + assert.NotContains(t, expr, "#IN") + assert.Contains(t, expr, "#DI = if_not_exists(#DI, :di)") + assert.NotContains(t, names, "#IB") + assert.NotContains(t, values, ":ib") + } +} diff --git a/cla-backend-go/v2/sign/icla_block_test.go b/cla-backend-go/v2/sign/icla_block_test.go index ce7bfc7dd..e7405b630 100644 --- a/cla-backend-go/v2/sign/icla_block_test.go +++ b/cla-backend-go/v2/sign/icla_block_test.go @@ -5,12 +5,15 @@ package sign import ( "context" + "errors" "testing" "github.com/golang/mock/gomock" "github.com/linuxfoundation/easycla/cla-backend-go/events" eventsMock "github.com/linuxfoundation/easycla/cla-backend-go/events/mock" v1Models "github.com/linuxfoundation/easycla/cla-backend-go/gen/v1/models" + sigs "github.com/linuxfoundation/easycla/cla-backend-go/gen/v1/restapi/operations/signatures" + mock_v1_signatures "github.com/linuxfoundation/easycla/cla-backend-go/signatures/mocks" "github.com/stretchr/testify/assert" ) @@ -28,6 +31,69 @@ func TestHasInvalidatedIcla(t *testing.T) { }), "a signed but unapproved ICLA marks an administrator invalidation") } +func TestUserHasInvalidatedIclaExhaustiveLookup(t *testing.T) { + userName := "contributor" + projectID := "cla-group-1" + callParams := sigs.GetUserSignaturesParams{UserID: "user-1", UserName: &userName} + + tests := []struct { + name string + signatures []*v1Models.Signature + lookupErr error + wantBlocked bool + wantErr bool + }{ + { + name: "an invalidated ICLA anywhere in the exhaustive result blocks", + signatures: []*v1Models.Signature{ + {SignatureID: "valid", SignatureSigned: true, SignatureApproved: true}, + {SignatureID: "invalidated", SignatureSigned: true, SignatureApproved: false}, + }, + wantBlocked: true, + }, + { + name: "a clean history does not block", + signatures: []*v1Models.Signature{ + {SignatureID: "valid", SignatureSigned: true, SignatureApproved: true}, + }, + }, + { + name: "a failed lookup is propagated", + lookupErr: errors.New("dynamodb unavailable"), + wantErr: true, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockSignatures := mock_v1_signatures.NewMockSignatureService(ctrl) + mockSignatures.EXPECT().GetUserSignatures(gomock.Any(), gomock.Any(), &projectID).DoAndReturn( + func(_ context.Context, params sigs.GetUserSignaturesParams, _ *string) (*v1Models.Signatures, error) { + if assert.NotNil(t, params.PageSize, "the block check must request an exhaustive page size, not the default of 10") { + assert.GreaterOrEqual(t, *params.PageSize, int64(1000)) + } + assert.Equal(t, "user-1", params.UserID) + if tc.lookupErr != nil { + return nil, tc.lookupErr + } + return &v1Models.Signatures{Signatures: tc.signatures}, nil + }) + + svc := &service{signatureService: mockSignatures} + blocked, err := svc.userHasInvalidatedIcla(context.Background(), callParams, &projectID) + if tc.wantErr { + assert.Error(t, err) + return + } + assert.NoError(t, err) + assert.Equal(t, tc.wantBlocked, blocked) + assert.Nil(t, callParams.PageSize, "the caller's default-sized params must stay untouched") + }) + } +} + func TestLogCompanySanctionedEventIdentity(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() diff --git a/cla-backend-go/v2/sign/service.go b/cla-backend-go/v2/sign/service.go index b98b2cff8..a1e046feb 100644 --- a/cla-backend-go/v2/sign/service.go +++ b/cla-backend-go/v2/sign/service.go @@ -1421,7 +1421,12 @@ func (s *service) RequestIndividualSignature(ctx context.Context, input *models. return nil, err } log.WithFields(f).Debugf("found %d signatures for user: %s", len(userSignatures.Signatures), *input.UserID) - if hasInvalidatedIcla(userSignatures.Signatures) { + blocked, blockErr := s.userHasInvalidatedIcla(ctx, sigParams, input.ProjectID) + if blockErr != nil { + log.WithFields(f).WithError(blockErr).Warnf("unable to check for an invalidated ICLA for user: %s", *input.UserID) + return nil, blockErr + } + if blocked { log.WithFields(f).Warnf("user %s has an invalidated ICLA for project %s - blocking a new individual signature", *input.UserID, *input.ProjectID) return nil, ErrIclaInvalidated } @@ -2477,6 +2482,22 @@ func hasInvalidatedIcla(signatures []*v1Models.Signature) bool { return false } +// iclaBlockPageSize exhausts a user's ICLA records for the CLA group so the invalidated-ICLA +// check cannot miss a record beyond the default page of 10. +const iclaBlockPageSize int64 = 1000 + +// userHasInvalidatedIcla runs the invalidated-ICLA check on a dedicated exhaustive lookup, +// leaving the caller's default-sized lookup untouched. +func (s *service) userHasInvalidatedIcla(ctx context.Context, params sigs.GetUserSignaturesParams, projectID *string) (bool, error) { + pageSize := iclaBlockPageSize + params.PageSize = &pageSize + userSignatures, err := s.signatureService.GetUserSignatures(ctx, params, projectID) + if err != nil { + return false, err + } + return hasInvalidatedIcla(userSignatures.Signatures), nil +} + func (s *service) RequestIndividualSignatureGerrit(ctx context.Context, input *models.IndividualSignatureInput) (*models.IndividualSignatureOutput, error) { f := logrus.Fields{ "functionName": "sign.RequestIndividualSignatureGerrit", @@ -2524,7 +2545,12 @@ func (s *service) RequestIndividualSignatureGerrit(ctx context.Context, input *m return nil, err } - if hasInvalidatedIcla(userSignatures.Signatures) { + blocked, blockErr := s.userHasInvalidatedIcla(ctx, sigParams, input.ProjectID) + if blockErr != nil { + log.WithFields(f).WithError(blockErr).Warnf("unable to check for an invalidated ICLA for user: %s", *input.UserID) + return nil, blockErr + } + if blocked { log.WithFields(f).Warnf("user %s has an invalidated ICLA for project %s - blocking a new individual signature", *input.UserID, *input.ProjectID) return nil, ErrIclaInvalidated } From 43477c7f24aa2b300d4aaee7c9418e57887bcd78 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Gryglicki?= Date: Tue, 25 Aug 2026 08:49:56 +0000 Subject: [PATCH 3/3] M2: MyCLAs contact requests, invalidation/sanction metadata, ICLA re-sign block - address AI feedback - 2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Łukasz Gryglicki Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot) Assisted by [Claude](https://claude.ai) --- cla-backend-go/v2/sign/icla_block_test.go | 86 ++++++++++++++++------- cla-backend-go/v2/sign/service.go | 25 ++++--- 2 files changed, 79 insertions(+), 32 deletions(-) diff --git a/cla-backend-go/v2/sign/icla_block_test.go b/cla-backend-go/v2/sign/icla_block_test.go index e7405b630..c8e753b81 100644 --- a/cla-backend-go/v2/sign/icla_block_test.go +++ b/cla-backend-go/v2/sign/icla_block_test.go @@ -34,33 +34,57 @@ func TestHasInvalidatedIcla(t *testing.T) { func TestUserHasInvalidatedIclaExhaustiveLookup(t *testing.T) { userName := "contributor" projectID := "cla-group-1" - callParams := sigs.GetUserSignaturesParams{UserID: "user-1", UserName: &userName} + + type page struct { + signatures []*v1Models.Signature + lastKeyScanned string + err error + } + valid := &v1Models.Signature{SignatureID: "valid", SignatureSigned: true, SignatureApproved: true} + invalidated := &v1Models.Signature{SignatureID: "invalidated", SignatureSigned: true, SignatureApproved: false} tests := []struct { name string - signatures []*v1Models.Signature - lookupErr error + pages []page wantBlocked bool wantErr bool }{ { - name: "an invalidated ICLA anywhere in the exhaustive result blocks", - signatures: []*v1Models.Signature{ - {SignatureID: "valid", SignatureSigned: true, SignatureApproved: true}, - {SignatureID: "invalidated", SignatureSigned: true, SignatureApproved: false}, + name: "a hit on the first page blocks without fetching further pages", + pages: []page{{signatures: []*v1Models.Signature{valid, invalidated}, lastKeyScanned: "cursor-1"}}, + wantBlocked: true, + }, + { + name: "an invalidated ICLA beyond the first page still blocks", + pages: []page{ + {signatures: []*v1Models.Signature{valid}, lastKeyScanned: "cursor-1"}, + {signatures: []*v1Models.Signature{invalidated}}, }, wantBlocked: true, }, { - name: "a clean history does not block", - signatures: []*v1Models.Signature{ - {SignatureID: "valid", SignatureSigned: true, SignatureApproved: true}, + name: "a clean multi-page history does not block", + pages: []page{ + {signatures: []*v1Models.Signature{valid}, lastKeyScanned: "cursor-1"}, + {signatures: []*v1Models.Signature{valid}}, }, }, { - name: "a failed lookup is propagated", - lookupErr: errors.New("dynamodb unavailable"), - wantErr: true, + name: "a clean single page does not block", + pages: []page{{signatures: []*v1Models.Signature{valid}}}, + }, + { + name: "a failed lookup is propagated", + pages: []page{{err: errors.New("dynamodb unavailable")}}, + wantErr: true, + }, + { + name: "a failure on a later page is propagated", + pages: []page{ + {signatures: []*v1Models.Signature{valid}, lastKeyScanned: "cursor-1"}, + {err: errors.New("dynamodb unavailable")}, + }, + wantErr: true, }, } for _, tc := range tests { @@ -68,28 +92,42 @@ func TestUserHasInvalidatedIclaExhaustiveLookup(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() + callParams := sigs.GetUserSignaturesParams{UserID: "user-1", UserName: &userName} + calls := 0 mockSignatures := mock_v1_signatures.NewMockSignatureService(ctrl) mockSignatures.EXPECT().GetUserSignatures(gomock.Any(), gomock.Any(), &projectID).DoAndReturn( func(_ context.Context, params sigs.GetUserSignaturesParams, _ *string) (*v1Models.Signatures, error) { - if assert.NotNil(t, params.PageSize, "the block check must request an exhaustive page size, not the default of 10") { - assert.GreaterOrEqual(t, *params.PageSize, int64(1000)) - } - assert.Equal(t, "user-1", params.UserID) - if tc.lookupErr != nil { - return nil, tc.lookupErr + if assert.Less(t, calls, len(tc.pages), "no lookups expected past the last page") { + p := tc.pages[calls] + calls++ + if assert.NotNil(t, params.PageSize, "the block check must request an exhaustive page size, not the default of 10") { + assert.GreaterOrEqual(t, *params.PageSize, int64(1000)) + } + assert.Equal(t, "user-1", params.UserID) + if calls == 1 { + assert.Nil(t, params.NextKey) + } else if assert.NotNil(t, params.NextKey, "follow-up lookups must carry the pagination cursor") { + assert.Equal(t, tc.pages[calls-2].lastKeyScanned, *params.NextKey) + } + if p.err != nil { + return nil, p.err + } + return &v1Models.Signatures{Signatures: p.signatures, LastKeyScanned: p.lastKeyScanned}, nil } - return &v1Models.Signatures{Signatures: tc.signatures}, nil - }) + return &v1Models.Signatures{}, nil + }).Times(len(tc.pages)) svc := &service{signatureService: mockSignatures} blocked, err := svc.userHasInvalidatedIcla(context.Background(), callParams, &projectID) if tc.wantErr { assert.Error(t, err) - return + } else { + assert.NoError(t, err) + assert.Equal(t, tc.wantBlocked, blocked) } - assert.NoError(t, err) - assert.Equal(t, tc.wantBlocked, blocked) + assert.Equal(t, len(tc.pages), calls, "every prepared page is consumed and none beyond") assert.Nil(t, callParams.PageSize, "the caller's default-sized params must stay untouched") + assert.Nil(t, callParams.NextKey, "the caller's cursor must stay untouched") }) } } diff --git a/cla-backend-go/v2/sign/service.go b/cla-backend-go/v2/sign/service.go index a1e046feb..8054d6c02 100644 --- a/cla-backend-go/v2/sign/service.go +++ b/cla-backend-go/v2/sign/service.go @@ -2482,20 +2482,29 @@ func hasInvalidatedIcla(signatures []*v1Models.Signature) bool { return false } -// iclaBlockPageSize exhausts a user's ICLA records for the CLA group so the invalidated-ICLA -// check cannot miss a record beyond the default page of 10. +// iclaBlockPageSize is the per-call cap for the invalidated-ICLA lookup, far above the +// default page of 10; the helper pages past it when needed. const iclaBlockPageSize int64 = 1000 -// userHasInvalidatedIcla runs the invalidated-ICLA check on a dedicated exhaustive lookup, -// leaving the caller's default-sized lookup untouched. +// userHasInvalidatedIcla runs the invalidated-ICLA check on a dedicated lookup, following the +// pagination cursor until a hit or exhaustion, leaving the caller's default-sized lookup untouched. func (s *service) userHasInvalidatedIcla(ctx context.Context, params sigs.GetUserSignaturesParams, projectID *string) (bool, error) { pageSize := iclaBlockPageSize params.PageSize = &pageSize - userSignatures, err := s.signatureService.GetUserSignatures(ctx, params, projectID) - if err != nil { - return false, err + for { + userSignatures, err := s.signatureService.GetUserSignatures(ctx, params, projectID) + if err != nil { + return false, err + } + if hasInvalidatedIcla(userSignatures.Signatures) { + return true, nil + } + if userSignatures.LastKeyScanned == "" { + return false, nil + } + nextKey := userSignatures.LastKeyScanned + params.NextKey = &nextKey } - return hasInvalidatedIcla(userSignatures.Signatures), nil } func (s *service) RequestIndividualSignatureGerrit(ctx context.Context, input *models.IndividualSignatureInput) (*models.IndividualSignatureOutput, error) {