From 95b41b2866e5432299533fe6ba94901f9726ee38 Mon Sep 17 00:00:00 2001 From: CatalinSnyk Date: Thu, 21 Aug 2025 10:55:09 +0300 Subject: [PATCH] fix: respect the reason required org setting for ignore creation --- pkg/local_workflows/ignore_workflow/config.go | 78 +++++- .../ignore_workflow/ignore_workflow.go | 13 +- .../ignore_workflow/ignore_workflow_test.go | 233 +++++++++++++++++- .../ignore_workflow/response.go | 8 +- 4 files changed, 317 insertions(+), 15 deletions(-) diff --git a/pkg/local_workflows/ignore_workflow/config.go b/pkg/local_workflows/ignore_workflow/config.go index 1a6f4b1ee..041bd3a45 100644 --- a/pkg/local_workflows/ignore_workflow/config.go +++ b/pkg/local_workflows/ignore_workflow/config.go @@ -8,6 +8,7 @@ import ( "github.com/snyk/error-catalog-golang-public/cli" "github.com/snyk/go-application-framework/internal/api" + "github.com/snyk/go-application-framework/internal/api/contract" policyApi "github.com/snyk/go-application-framework/internal/api/policy/2024-10-15" "github.com/snyk/go-application-framework/pkg/configuration" "github.com/snyk/go-application-framework/pkg/local_workflows/local_models" @@ -40,26 +41,55 @@ func addCreateIgnoreDefaultConfigurationValues(invocationCtx workflow.Invocation config.AddDefaultValue(ReasonKey, func(_ configuration.Configuration, existingValue interface{}) (interface{}, error) { isSet := config.IsSet(ReasonKey) - return defaultFuncWithValidator(existingValue, isSet, isValidReason) + reasonRequired := config.GetBool(ConfigIgnoreReasonRequired) + return defaultFuncWithValidator(existingValue, isSet, getReasonValidator(reasonRequired)) }) } +func getOrgIgnoreSettings(engine workflow.Engine) (*contract.OrgSettingsResponse, error) { + config := engine.GetConfiguration() + org := config.GetString(configuration.ORGANIZATION) + client := engine.GetNetworkAccess().GetHttpClient() + url := config.GetString(configuration.API_URL) + apiClient := api.NewApi(url, client) + + settings, err := apiClient.GetOrgSettings(org) + if err != nil { + engine.GetLogger().Err(err).Msg("Failed to access settings.") + return nil, err + } + + engine.GetConfiguration().Set(ConfigIgnoreSettings, settings) + return settings, nil +} + +func getOrgIgnoreSettingsConfig(engine workflow.Engine) configuration.DefaultValueFunction { + callback := func(_ configuration.Configuration, existingValue interface{}) (interface{}, error) { + if existingValue != nil { + return existingValue, nil + } + + response, err := getOrgIgnoreSettings(engine) + if err != nil { + engine.GetLogger().Err(err).Msg("Failed to access settings.") + return nil, err + } + + return response, nil + } + return callback +} + func getOrgIgnoreApprovalEnabled(engine workflow.Engine) configuration.DefaultValueFunction { return func(_ configuration.Configuration, existingValue interface{}) (interface{}, error) { if existingValue != nil { return existingValue, nil } - config := engine.GetConfiguration() - org := config.GetString(configuration.ORGANIZATION) - client := engine.GetNetworkAccess().GetHttpClient() - url := config.GetString(configuration.API_URL) - apiClient := api.NewApi(url, client) - - settings, err := apiClient.GetOrgSettings(org) + settings, err := getOrgIgnoreSettings(engine) if err != nil { engine.GetLogger().Err(err).Msg("Failed to access settings.") - return nil, err + return false, err } if settings.Ignores != nil && settings.Ignores.ApprovalWorkflowEnabled { @@ -70,6 +100,26 @@ func getOrgIgnoreApprovalEnabled(engine workflow.Engine) configuration.DefaultVa } } +func getOrgIgnoreReasonRequired(engine workflow.Engine) configuration.DefaultValueFunction { + return func(_ configuration.Configuration, existingValue interface{}) (interface{}, error) { + if existingValue != nil { + return existingValue, nil + } + + settings, err := getOrgIgnoreSettings(engine) + if err != nil { + engine.GetLogger().Err(err).Msg("Failed to access settings.") + return false, err + } + + if settings.Ignores != nil { + return settings.Ignores.ReasonRequired, nil + } + + return false, nil + } +} + func remoteRepoUrlDefaultFunc(existingValue interface{}, config configuration.Configuration) (interface{}, error) { if existingValue != nil && existingValue != "" { return existingValue, nil @@ -148,6 +198,16 @@ func isValidFindingsId(input string) error { return nil } +func getReasonValidator(reasonRequired bool) func(string) error { + if reasonRequired { + return isValidReason + } + + return func(input string) error { + return nil + } +} + func isValidReason(input string) error { if input == "" { return cli.NewValidationFailureError("The ignore reason cannot be empty. Provide a justification for ignoring this finding.") diff --git a/pkg/local_workflows/ignore_workflow/ignore_workflow.go b/pkg/local_workflows/ignore_workflow/ignore_workflow.go index 5b745d55d..bb8227030 100644 --- a/pkg/local_workflows/ignore_workflow/ignore_workflow.go +++ b/pkg/local_workflows/ignore_workflow/ignore_workflow.go @@ -62,6 +62,8 @@ const ( interactiveIgnoreRequestSubmissionMessage = "✅ Your ignore request has been submitted." ConfigIgnoreApprovalEnabled = "internal_iaw_enabled" + ConfigIgnoreReasonRequired = "internal_ignore_reason_required" + ConfigIgnoreSettings = "internal_ignore_settings" ) var reasonPromptHelpMap = map[string]string{ @@ -90,7 +92,9 @@ func InitIgnoreWorkflows(engine workflow.Engine) error { return err } + engine.GetConfiguration().AddDefaultValue(ConfigIgnoreSettings, getOrgIgnoreSettingsConfig(engine)) engine.GetConfiguration().AddDefaultValue(ConfigIgnoreApprovalEnabled, getOrgIgnoreApprovalEnabled(engine)) + engine.GetConfiguration().AddDefaultValue(ConfigIgnoreReasonRequired, getOrgIgnoreReasonRequired(engine)) return nil } @@ -115,6 +119,11 @@ func ignoreCreateWorkflowEntryPoint(invocationCtx workflow.InvocationContext, _ return nil, disabledError } + reasonRequired, reasonRequiredError := config.GetBoolWithError(ConfigIgnoreReasonRequired) + if reasonRequiredError != nil { + return nil, reasonRequiredError + } + interactive := config.GetBool(InteractiveKey) addCreateIgnoreDefaultConfigurationValues(invocationCtx) @@ -184,7 +193,7 @@ func ignoreCreateWorkflowEntryPoint(invocationCtx workflow.InvocationContext, _ if !ok { reasonHelp = reasonPromptHelp } - reason, err = promptIfEmpty(reason, userInterface, reasonHelp, reasonDescription, isValidReason) + reason, err = promptIfEmpty(reason, userInterface, reasonHelp, reasonDescription, getReasonValidator(reasonRequired)) if err != nil { return nil, err } @@ -213,7 +222,7 @@ func ignoreCreateWorkflowEntryPoint(invocationCtx workflow.InvocationContext, _ return nil, cli.NewEmptyFlagOptionError("The expiration flag is required and cannot be empty. Provide it using the --expiration flag. The date format is YYYY-MM-DD or 'never' for no expiration.") } - if reason == "" { + if reasonRequired && reason == "" { return nil, cli.NewEmptyFlagOptionError("The reason flag is required and cannot be empty. Provide it using the --reason flag.") } diff --git a/pkg/local_workflows/ignore_workflow/ignore_workflow_test.go b/pkg/local_workflows/ignore_workflow/ignore_workflow_test.go index 05707e22a..966aa18f2 100644 --- a/pkg/local_workflows/ignore_workflow/ignore_workflow_test.go +++ b/pkg/local_workflows/ignore_workflow/ignore_workflow_test.go @@ -322,6 +322,72 @@ func Test_ignoreCreateWorkflowEntryPoint(t *testing.T) { assert.NotNil(t, err, "Should return an error when IAW FF is disabled") assert.Nil(t, result, "result should be nil when IAW FF is disabled") }) + t.Run("reason not required - non-interactive mode success with empty reason", func(t *testing.T) { + expectedFindingsId := uuid.New().String() + expectedIgnoreType := string(policyApi.WontFix) + policyId := uuid.New().String() + responseMock := fmt.Sprintf(`{ + "data": { + "id": "%s", + "type": "policy", + "attributes": { + "action": { + "data": { + "ignore_type": "%s" + } + }, + "action_type": "%s", + "conditions_group": { + "conditions": [ + { + "field": "%s", + "operator": "%s", + "value": "%s" + } + ], + "logical_operator": "%s" + }, + "created_by": { + "email": "%s" + }, + "review": "%s" + } +}}`, policyId, expectedIgnoreType, policyApi.PolicyAttributesActionTypeIgnore, policyApi.Snykassetfindingv1, policyApi.Includes, expectedFindingsId, policyApi.And, expectedUser, policyApi.PolicyReviewPending) + invocationContext := setupMockIgnoreContext(t, responseMock, http.StatusCreated) + config := invocationContext.GetConfiguration() + config.Set(InteractiveKey, false) + config.Set(ConfigIgnoreReasonRequired, false) + + config.Set(FindingsIdKey, expectedFindingsId) + config.Set(IgnoreTypeKey, expectedIgnoreType) + config.Set(ReasonKey, "") + config.Set(ExpirationKey, "never") + config.Set(RemoteRepoUrlKey, testRepoUrl) + config.Set(configuration.API_URL, "https://api.snyk.io") + config.Set(EnrichResponseKey, true) + + result, err := ignoreCreateWorkflowEntryPoint(invocationContext, nil) + assert.NoError(t, err) + assert.Greater(t, len(result), 0) + }) + t.Run("non-interactive mode failure - reason required but not provided", func(t *testing.T) { + expectedFindingsId := uuid.New().String() + expectedIgnoreType := string(policyApi.WontFix) + invocationContext := setupMockIgnoreContext(t, "{}", http.StatusCreated) + config := invocationContext.GetConfiguration() + config.Set(ConfigIgnoreReasonRequired, true) + config.Set(InteractiveKey, false) + + config.Set(FindingsIdKey, expectedFindingsId) + config.Set(IgnoreTypeKey, expectedIgnoreType) + config.Set(ReasonKey, "") + config.Set(ExpirationKey, "never") + config.Set(RemoteRepoUrlKey, testRepoUrl) + + result, err := ignoreCreateWorkflowEntryPoint(invocationContext, nil) + assert.Error(t, err) + assert.Nil(t, result) + }) } func setupInteractiveMockContext(t *testing.T, mockApiResponse string, mockApiStatusCode int) (*mocks.MockInvocationContext, *mocks.MockUserInterface) { @@ -583,6 +649,46 @@ func Test_InteractiveIgnoreWorkflow(t *testing.T) { _, err := ignoreCreateWorkflowEntryPoint(invocationCtx, nil) assert.Error(t, err, "Expected to have an error regarding invalid ignore type") }) + + t.Run("interactive mode - reason required but not provided", func(t *testing.T) { + invocationCtx, mockUI := setupInteractiveMockContext(t, "", http.StatusCreated) + config := invocationCtx.GetConfiguration() + config.Set(ConfigIgnoreReasonRequired, true) + + mockUI.EXPECT().Input(gomock.Eq(findingsIdDescription)).Return(findingId, nil).Times(1) + mockUI.EXPECT().Input(gomock.Eq(ignoreTypeDescription)).Return(ignoreType, nil).Times(1) + mockUI.EXPECT().Input(gomock.Eq(expirationDescription)).Return(expiration, nil).Times(1) + + mockUI.EXPECT().Input(gomock.Eq(reasonDescription)).Return("", nil).Times(1) + + _, err := ignoreCreateWorkflowEntryPoint(invocationCtx, nil) + assert.Error(t, err) + }) + + t.Run("interactive mode - reason not required and not provided", func(t *testing.T) { + mockResponse := getSuccessfulPolicyResponse(policyId.String(), findingId, ignoreType, "", email, &expirationDate) + invocationCtx, mockUI := setupInteractiveMockContext(t, mockResponse, http.StatusCreated) + config := invocationCtx.GetConfiguration() + config.Set(ConfigIgnoreReasonRequired, false) + + mockUI.EXPECT().Input(gomock.Eq(findingsIdDescription)).Return(findingId, nil).Times(1) + mockUI.EXPECT().Input(gomock.Eq(ignoreTypeDescription)).Return(ignoreType, nil).Times(1) + mockUI.EXPECT().Input(gomock.Eq(expirationDescription)).Return(expiration, nil).Times(1) + mockUI.EXPECT().Input(gomock.Eq(remoteRepoUrlDescription)).Return(repoUrl, nil).Times(1) + mockUI.EXPECT().Input(gomock.Eq(reasonDescription)).Return("", nil).Times(1) + + result, err := ignoreCreateWorkflowEntryPoint(invocationCtx, nil) + assert.NoError(t, err) + assert.NotNil(t, result) + assert.Greater(t, len(result), 0) + + var policyResp sarif.Suppression + data, ok := result[0].GetPayload().([]byte) + assert.True(t, ok) + err = json.Unmarshal(data, &policyResp) + assert.NoError(t, err, "Should parse JSON response") + assert.Equal(t, "", policyResp.Justification) + }) } func getSuccessfulPolicyResponse(policyIdStr, findingId, ignoreTypeStr, reasonStr, userEmailStr string, expiration *time.Time) string { @@ -772,7 +878,7 @@ func Test_getOrgIgnoreApprovalEnabled(t *testing.T) { mockEngine.EXPECT().GetConfiguration().Return(mockConfig) mockEngine.EXPECT().GetNetworkAccess().Return(mockNetworkAccess) - mockEngine.EXPECT().GetLogger().Return(&logger) + mockEngine.EXPECT().GetLogger().Return(&logger).AnyTimes() mockConfig.EXPECT().GetString(configuration.ORGANIZATION).Return(orgId) mockConfig.EXPECT().GetString(configuration.API_URL).Return(apiUrl) mockNetworkAccess.EXPECT().GetHttpClient().Return(httpClient) @@ -781,7 +887,11 @@ func Test_getOrgIgnoreApprovalEnabled(t *testing.T) { result, err := defaultValueFunc(nil, nil) assert.Error(t, err) - assert.Nil(t, result) + assert.NotNil(t, result) + + ok, isOk := result.(bool) + assert.True(t, isOk) + assert.False(t, ok) assert.Contains(t, err.Error(), "unable to retrieve org settings") }) } @@ -808,7 +918,8 @@ func setupMockEngineForOrgSettings(t *testing.T, response *contract.OrgSettingsR } }) - mockEngine.EXPECT().GetConfiguration().Return(mockConfig) + mockConfig.EXPECT().Set(ConfigIgnoreSettings, gomock.Any()) + mockEngine.EXPECT().GetConfiguration().Return(mockConfig).AnyTimes() mockEngine.EXPECT().GetNetworkAccess().Return(mockNetworkAccess) mockConfig.EXPECT().GetString(configuration.ORGANIZATION).Return(orgId) mockConfig.EXPECT().GetString(configuration.API_URL).Return(apiUrl) @@ -817,3 +928,119 @@ func setupMockEngineForOrgSettings(t *testing.T, response *contract.OrgSettingsR defaultValueFunc := getOrgIgnoreApprovalEnabled(mockEngine) return defaultValueFunc(nil, nil) } + +func Test_getOrgIgnoreReasonRequired(t *testing.T) { + t.Run("returns existing value when not nil", func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockEngine := mocks.NewMockEngine(ctrl) + defaultValueFunc := getOrgIgnoreReasonRequired(mockEngine) + + result, err := defaultValueFunc(nil, true) + assert.NoError(t, err) + assert.Equal(t, true, result) + + result, err = defaultValueFunc(nil, false) + assert.NoError(t, err) + assert.Equal(t, false, result) + }) + + t.Run("reason required enabled", func(t *testing.T) { + result, err := setupMockEngineForOrgReasonRequired(t, &contract.OrgSettingsResponse{ + Ignores: &contract.OrgIgnoreSettings{ReasonRequired: true}, + }) + + assert.NoError(t, err) + assert.Equal(t, true, result) + }) + + t.Run("reason required disabled", func(t *testing.T) { + result, err := setupMockEngineForOrgReasonRequired(t, &contract.OrgSettingsResponse{ + Ignores: &contract.OrgIgnoreSettings{ReasonRequired: false}, + }) + + assert.NoError(t, err) + assert.Equal(t, false, result) + }) + + t.Run("ignores field is nil", func(t *testing.T) { + result, err := setupMockEngineForOrgReasonRequired(t, &contract.OrgSettingsResponse{ + Ignores: nil, + }) + + assert.NoError(t, err) + assert.Equal(t, false, result) + }) + + t.Run("API call fails", func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + logger := zerolog.Logger{} + orgId := uuid.New().String() + apiUrl := "https://api.snyk.io" + + mockEngine := mocks.NewMockEngine(ctrl) + mockConfig := mocks.NewMockConfiguration(ctrl) + mockNetworkAccess := mocks.NewMockNetworkAccess(ctrl) + + httpClient := localworkflows.NewTestClient(func(req *http.Request) *http.Response { + return &http.Response{ + StatusCode: http.StatusInternalServerError, + Body: io.NopCloser(bytes.NewBufferString("Internal Server Error")), + } + }) + + mockEngine.EXPECT().GetConfiguration().Return(mockConfig) + mockEngine.EXPECT().GetNetworkAccess().Return(mockNetworkAccess) + mockEngine.EXPECT().GetLogger().Return(&logger).AnyTimes() + mockConfig.EXPECT().GetString(configuration.ORGANIZATION).Return(orgId) + mockConfig.EXPECT().GetString(configuration.API_URL).Return(apiUrl) + mockNetworkAccess.EXPECT().GetHttpClient().Return(httpClient) + + defaultValueFunc := getOrgIgnoreReasonRequired(mockEngine) + result, err := defaultValueFunc(nil, nil) + + assert.Error(t, err) + assert.NotNil(t, result) + + ok, isOk := result.(bool) + assert.True(t, isOk) + assert.False(t, ok) + assert.Contains(t, err.Error(), "unable to retrieve org settings") + }) +} + +func setupMockEngineForOrgReasonRequired(t *testing.T, response *contract.OrgSettingsResponse) (interface{}, error) { + t.Helper() + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + orgId := uuid.New().String() + apiUrl := "https://api.snyk.io" + + responseJSON, err := json.Marshal(response) + assert.NoError(t, err) + + mockEngine := mocks.NewMockEngine(ctrl) + mockConfig := mocks.NewMockConfiguration(ctrl) + mockNetworkAccess := mocks.NewMockNetworkAccess(ctrl) + + httpClient := localworkflows.NewTestClient(func(req *http.Request) *http.Response { + return &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(bytes.NewBuffer(responseJSON)), + } + }) + + mockConfig.EXPECT().Set(ConfigIgnoreSettings, gomock.Any()) + mockEngine.EXPECT().GetConfiguration().Return(mockConfig).AnyTimes() + mockEngine.EXPECT().GetNetworkAccess().Return(mockNetworkAccess) + mockConfig.EXPECT().GetString(configuration.ORGANIZATION).Return(orgId) + mockConfig.EXPECT().GetString(configuration.API_URL).Return(apiUrl) + mockNetworkAccess.EXPECT().GetHttpClient().Return(httpClient) + + defaultValueFunc := getOrgIgnoreReasonRequired(mockEngine) + return defaultValueFunc(nil, nil) +} diff --git a/pkg/local_workflows/ignore_workflow/response.go b/pkg/local_workflows/ignore_workflow/response.go index 4fcc6aa32..3cd8b8537 100644 --- a/pkg/local_workflows/ignore_workflow/response.go +++ b/pkg/local_workflows/ignore_workflow/response.go @@ -33,9 +33,15 @@ func policyResponseToSarifSuppression(policyResponse *v20241015.PolicyResponse) expiresStr := policyResponse.Attributes.Action.Data.Expires.Format(time.RFC3339) expires = &expiresStr } + + justification := "" + if policyResponse.Attributes.Action.Data.Reason != nil { + justification = *policyResponse.Attributes.Action.Data.Reason + } + return &sarif.Suppression{ Guid: policyResponse.Id.String(), - Justification: *policyResponse.Attributes.Action.Data.Reason, + Justification: justification, Status: policyReviewToSarifStatus(policyResponse.Attributes.Review), Properties: sarif.SuppressionProperties{ Expiration: expires,