Repository: ossf/allstar

Issue:
When should issues be closed?
Currently the GitHub issue code is only triggered when:

- Allstar is enabled
- The policy is enabled
- The policy action is `issue`.

At that point. Allstar will ensure an issue is open if the policy fails, and ensure the issue is closed if the policy passes.

In any other case, Allstar does not check the issue status (open or closed). This means, Allstar will leave issues open if:

- The policy or Allstar is disabled
- The action is changed to `log` or `fix`.
- The `issueRepo` is set, unset, or changed.
- Allstar is uninstalled. (Can never close in this case).

Should Allstar try to close issues in some or all of these cases? If so, which ones? Allstar doesn't keep state, so this means it will search for issues in all those cases whenever a repo is checked.

Hints from issue comments:
@azeemshaikh38: My 2 cents - let's say a policy is enabled with `issue` action and is seeing violations. The 3 possible updates the user could make:

1. Update the policy i.e disable or update some policy params (like add a binary artifact exception). In this case, we should ensure that the issue gets closed if the new policy params cause no violations (including the case where policy gets disabled). 
2. Update the action type. Let's say the action type changed from `issue` to `email`, it would makes sense to me if the old issue did not get auto-closed. `issue` -> `fix` is a special case of this so I'm ok if the issue is left open in this case too. Another reason for me saying this is that, since AllStar doesn't keep state, we will need to add it only to support this which may not be worth the effort.
3. Update the `issueRepo`. Same as (2), we can leave the older issue open since we won't be spamming it anyways.

When we make architectural changes to AllStar backend for scaling, we can consider keeping state so that we have better support for (2) and (3). 

@brianrussell2 fyi.@agonzalez-plume: Interestingly, I just updated Allstar branchprotection mode from issue -> log while diagnosing excessive Github emails going to some people (not all emails were from Allstar). I had wondered if that would close the Issue or not. I guess it doesn't, which is what I expected. 

I do wonder if the priority for the Allstar check for a class of repos gets excluded or cancelled after initial configuration. If I optOut a class of repos, should that close the Issue? In that case, I believe it should close the issue.@markdboyd: @jeffmendoza I was just wondering about this myself. My opinion is that we should update Allstar to close issues if the action is set to "fix". Currently, you could have this experience:

- Action is set to "issue". Allstar runs scans, finds repos out of compliance, and creates Github issues.
- Then, action is set to "fix" to automatically fix the failing repos, but the Github issues remain open unless you set action back to "issue" and let the scans re-run

I can't really think of a downside to having issues closed if action is set to "fix" and I'm happy to implement this if you think it makes sense @jeffmendoza: Thanks for that fix @markdboyd ! We probably want to close issues in other cases as well, so leaving this open for more discussion.@None: In some of the cases listed it makes sense to close issues, so it might be best to do it if issueEnsure isn't explicitly called.

While there isn't much to do about the uninstalled case, I think a solution to all the other cases (*except possibly `issueRepo` being changed? would have to look closer at how issueRepo is considered in enforce/issue*) is to add a new feature to the issue package to be used in enforce `runPolicies()`.

First, fetch all Allstar issues using the [repo issues endpoint](https://docs.github.com/en/rest/issues/issues#list-repository-issues), setting creator to the app's username (if doing so works correctly), and then, when ensure is called, remove that issue from the list.

Once all ensureIssue calls are done, close issues remaining in list.

A struct to achieve that might look something vaguely like this and would exist in the issue package:

```go
type IssueCurator struct {
    Issues []*github.Issue
}

func NewIssueCurator(ctx context.Context, c *github.Client, owner, repo string) error {
    iss, _, _ := c.Issues.ListByRepo(ctx, owner, repo, {Creator: test using Creator here with c.User.Get(ctx, "") ? })
    return &IssueCurator{iss}
}

func (i *IssueCurator) Ensure(ctx context.Context, c *github.Client, owner, repo, policy, text string) error {
    // ensure issue like usual
    // have ensure return whether an issue already existed, and its number if so
    // if already exist, remove from i.Issues the issue with that ID so it doesn't get closed
}

func (i *IssueCurator) Close(ctx context.Context, c *github.Client, owner, repo string) error {
    // close all remaining issues from i.Issues
}
```@github-actions: This issue has been marked stale because it has been open for 60 days with no activity.@justaugustus: Closing as the discussion is complete.

Solution patch:
diff --git a/pkg/enforce/enforce.go b/pkg/enforce/enforce.go
index 9228196..363b701 100644
--- a/pkg/enforce/enforce.go
+++ b/pkg/enforce/enforce.go
@@ -36,7 +36,7 @@ var policiesGetPolicies func() []policydef.Policy
 var issueEnsure func(ctx context.Context, c *github.Client, owner, repo, policy, text string) error
 var issueClose func(ctx context.Context, c *github.Client, owner, repo, policy string) error
 var getAppInstallations func(ctx context.Context, ghc ghclients.GhClientsInterface) ([]*github.Installation, error)
-var getAppInstallationRepos func(ctx context.Context, ghc ghclients.GhClientsInterface, ic *github.Client) ([]*github.Repository, error)
+var getAppInstallationRepos func(ctx context.Context, ghc ghclients.GhClientsInterface, ic *github.Client) ([]*github.Repository, *github.Response, error)
 var isBotEnabled func(ctx context.Context, c *github.Client, owner, repo string) bool
 
 type EnforceResults = map[string]bool
@@ -81,12 +81,22 @@ func EnforceAll(ctx context.Context, ghc ghclients.GhClientsInterface) (EnforceA
 			return nil, err
 		}
 
-		repos, err := getAppInstallationRepos(ctx, ghc, ic)
+		repos, _, err := getAppInstallationRepos(ctx, ghc, ic)
+		// FIXME, not getting a rsp for this one, instead I think it is a special
+		// error that I need to introspect. just continue on all errors here
+		// temporarily to fix prod.
+		// if err != nil && rsp != nil && rsp.StatusCode == http.StatusForbidden {
+		// 	log.Error().
+		// 		Err(err).
+		// 		Msg("Skip installation, forbidden.")
+		// 	continue
+		// }
 		if err != nil {
 			log.Error().
 				Err(err).
 				Msg("Unexpected error listing installation repos.")
-			return nil, err
+			// return nil, err
+			continue
 		}
 
 		err = nil
@@ -155,15 +165,16 @@ func getAppInstallationsReal(ctx context.Context, ghc ghclients.GhClientsInterfa
 	return insts, nil
 }
 
-func getAppInstallationReposReal(ctx context.Context, ghc ghclients.GhClientsInterface, ic *github.Client) ([]*github.Repository, error) {
+func getAppInstallationReposReal(ctx context.Context, ghc ghclients.GhClientsInterface, ic *github.Client) ([]*github.Repository, *github.Response, error) {
 	var repos []*github.Repository
 	opt := &github.ListOptions{
 		PerPage: 100,
 	}
 	var err error
+	var resp *github.Response
 	for {
 		var rs *github.ListRepositories
-		var resp *github.Response
+		resp = nil
 		rs, resp, err = ic.Apps.ListRepos(ctx, opt)
 		if err != nil {
 			break
@@ -174,7 +185,7 @@ func getAppInstallationReposReal(ctx context.Context, ghc ghclients.GhClientsInt
 		}
 		opt.Page = resp.NextPage
 	}
-	return repos, err
+	return repos, resp, err
 }
 
 // EnforceJob is a reconcilation job that enforces policies on all repos every
@@ -248,7 +259,7 @@ func RunPolicies(ctx context.Context, c *github.Client, owner, repo string, enab
 					Msg("Unknown action configured.")
 			}
 		}
-		if r.Pass && a == "issue" {
+		if r.Pass && (a == "issue" || a == "fix") {
 			err := issueClose(ctx, c, owner, repo, p.Name())
 			if err != nil {
 				return nil, err



Test patch:
diff --git a/pkg/enforce/enforce_test.go b/pkg/enforce/enforce_test.go
index a8ee88b..46efea6 100644
--- a/pkg/enforce/enforce_test.go
+++ b/pkg/enforce/enforce_test.go
@@ -149,6 +149,19 @@ func TestRunPolicies(t *testing.T) {
 				"Test policy": false,
 			},
 		},
+		{
+			Name: "CloseIssueOnFix",
+			Res: policyRepoResults{
+				"fake-repo": policydef.Result{Enabled: true, Pass: true},
+			},
+			Action:       "fix",
+			ShouldFix:    false,
+			ShouldEnsure: false,
+			ShouldClose:  true,
+			ExpEnforceResults: EnforceResults{
+				"Test policy": true,
+			},
+		},
 		{
 			Name: "PolicyDisabled",
 			Res: policyRepoResults{
@@ -230,7 +243,7 @@ func TestEnforceAll(t *testing.T) {
 		insts = append(insts, inst)
 		return insts, nil
 	}
-	getAppInstallationRepos = func(ctx context.Context, ghc ghclients.GhClientsInterface, ic *github.Client) ([]*github.Repository, error) {
+	getAppInstallationRepos = func(ctx context.Context, ghc ghclients.GhClientsInterface, ic *github.Client) ([]*github.Repository, *github.Response, error) {
 		var repos []*github.Repository
 		repo1Name := "repo1"
 		repo2Name := "repo2"
@@ -250,7 +263,7 @@ func TestEnforceAll(t *testing.T) {
 			},
 		}
 		repos = append(repos, newRepos...)
-		return repos, nil
+		return repos, nil, nil
 	}
 	isBotEnabled = func(ctx context.Context, c *github.Client, owner, repo string) bool {
 		return true