Add support for shared organization policies

Refactor configuration loading to use the go-githubapp/appconfig
package. This mostly adds support for shared organization policies (in
the .github repository by default), but should also make error messages
more accurate.

It also changes the policy link in the details view to point to the
resolved policy, instead of the local policy file.

README.md

@@ -41,15 +41,25 @@ UI to view the detailed approval status of any pull request.
 
 ## Configuration
 
-By default, the behavior of the bot is configured by a `.policy.yml` file at
-the root of the repository. When running your own instance of the server, a
-different file name and location can be configured. The configured name and
-location will be used instead of the default location.
+Policies are defined by a `.policy.yml` file at the root of the repository.
+You can change this path and file name when running your own instance of the
+server.
 
-- If the file does not exist, the `policy-bot` status check is not posted. This
-  means it is safe to enable `policy-bot` on all repositories in an organization.
-- The `.policy.yml` file is read from the most recent commit on the target branch
-  of each pull request.
+- The file is read from the most recent commit on the _target_ branch of each
+  pull request.
+
+- The file may contain a reference to a policy in a different repository (see
+  [Remote Policy Configuration](#remote-policy-configuration).)
+
+- If the file does not exist in the repository, `policy-bot` tries to load a
+  shared `policy.yml` file at the root of the `.github` repository in the same
+  organization. You can change this path and repository name when running your
+  own instance of the server.
+
+- If a policy does not exist in the repository or in the shared organization
+  repository, `policy-bot` does not post a status check on the pull request.
+  This means it is safe to enable `policy-bot` on all repositories in an
+  organization.
 
 ### policy.yml Specification
 
@@ -91,6 +101,7 @@ approval_rules:
 ```
 
 #### Notes on YAML Syntax
+
 The YAML language specification supports flow scalars (basic values like strings
 and numbers) in three formats:
 [single-quoted](https://yaml.org/spec/1.2/spec.html#id2788097),
@@ -108,6 +119,7 @@ escape characters, which can cause confusion when used for regex strings
   e.g. `^BREAKING CHANGE: (\w| )+$`
 
 #### Remote Policy Configuration
+
 You can also define a remote policy by specifying a repository, path, and ref
 (only repository is required). Instead of defining a `policy` key, you would
 define a `remote` key. Only 1 level of remote configuration is supported by design.

config/policy-bot.example.yml

@@ -75,17 +75,26 @@ sessions:
   # POLICYBOT_SESSIONS_KEY environment variable.
   key: "secretsessionkey"
 
-# Options for application behavior
-options:
-  # The path within repositories to find the policy.yml file
-  policy_path: .policy.yml
-  # The context prefix for status checks created by the bot
-  status_check_context: policy-bot
+# Options for application behavior. The defaults are shown below.
+#
+# options:
+#   # The path to the policy file in a repository.
+#   policy_path: .policy.yml
+#
+#   # The name of an organization repository to look in for a shared policy if
+#   # a repository does not define a policy file.
+#   shared_repository: .github
+#
+#   # The path to the policy file in the shared organization repository.
+#   shared_policy_path: policy.yml
+#
+#   # The context prefix for status checks created by the bot.
+#   status_check_context: policy-bot
 
 # Options for locating the frontend files. By default, the server uses appropriate
 # paths for the binary distribution and Docker container. For local development,
 # uncomment this section to use the alternate paths below.
-# 
+#
 # 'static' is the file system path to the assembled CSS and JS assets.
 # 'templates' is the file system path to the Go template files.
 #

server/handler/base.go

@@ -33,7 +33,10 @@ import (
 )
 
 const (
-	DefaultPolicyPath         = ".policy.yml"
+	DefaultPolicyPath       = ".policy.yml"
+	DefaultSharedRepository = ".github"
+	DefaultSharedPolicyPath = "policy.yml"
+
 	DefaultStatusCheckContext = "policy-bot"
 
 	LogKeyGitHubSHA = "github_sha"
@@ -53,6 +56,9 @@ type Base struct {
 type PullEvaluationOptions struct {
 	PolicyPath string `yaml:"policy_path"`
 
+	SharedRepository string `yaml:"shared_repository"`
+	SharedPolicyPath string `yaml:"shared_policy_path"`
+
 	// StatusCheckContext will be used to create the status context. It will be used in the following
 	// pattern: <StatusCheckContext>: <Base Branch Name>
 	StatusCheckContext string `yaml:"status_check_context"`
@@ -73,6 +79,12 @@ func (p *PullEvaluationOptions) FillDefaults() {
 	if p.PolicyPath == "" {
 		p.PolicyPath = DefaultPolicyPath
 	}
+	if p.SharedRepository == "" {
+		p.SharedRepository = DefaultSharedRepository
+	}
+	if p.SharedPolicyPath == "" {
+		p.SharedPolicyPath = DefaultSharedPolicyPath
+	}
 
 	if p.StatusCheckContext == "" {
 		p.StatusCheckContext = DefaultStatusCheckContext
@@ -151,17 +163,11 @@ func (b *Base) Evaluate(ctx context.Context, installationID int64, trigger commo
 		return err
 	}
 
-	fetchedConfig, err := b.ConfigFetcher.ConfigForPR(ctx, prctx, client)
-	if err != nil {
-		return errors.WithMessage(err, fmt.Sprintf("Failed to fetch configuration: %s", fetchedConfig))
-	}
-
+	fetchedConfig := b.ConfigFetcher.ConfigForPR(ctx, prctx, client)
 	evaluator, err := b.ValidateFetchedConfig(ctx, prctx, client, fetchedConfig, trigger)
-
 	if err != nil {
 		return err
 	}
-
 	if evaluator == nil {
 		return nil
 	}
@@ -174,32 +180,36 @@ func (b *Base) Evaluate(ctx context.Context, installationID int64, trigger commo
 	return b.RequestReviewsForResult(ctx, prctx, client, trigger, result)
 }
 
-func (b *Base) ValidateFetchedConfig(ctx context.Context, prctx pull.Context, client *github.Client, fetchedConfig FetchedConfig, trigger common.Trigger) (common.Evaluator, error) {
+func (b *Base) ValidateFetchedConfig(ctx context.Context, prctx pull.Context, client *github.Client, fc FetchedConfig, trigger common.Trigger) (common.Evaluator, error) {
 	logger := zerolog.Ctx(ctx)
 
-	if fetchedConfig.Missing() {
-		logger.Debug().Msg(fetchedConfig.Description())
+	switch {
+	case fc.LoadError != nil:
+		msg := fmt.Sprintf("Error loading policy from %s", fc.Source)
+		logger.Warn().Err(fc.LoadError).Msg(msg)
 
-		return nil, nil
-	}
+		b.PostStatus(ctx, prctx, client, "error", msg)
+		return nil, errors.Wrapf(fc.LoadError, "failed to load policy: %s: %s", fc.Source, fc.Path)
 
-	if fetchedConfig.Invalid() {
-		statusMessage := fetchedConfig.Description()
-		logger.Warn().Err(fetchedConfig.Error).Msg(statusMessage)
+	case fc.ParseError != nil:
+		msg := fmt.Sprintf("Invalid policy in %s: %s", fc.Source, fc.Path)
+		logger.Warn().Err(fc.ParseError).Msg(msg)
 
-		b.PostStatus(ctx, prctx, client, "error", statusMessage)
+		b.PostStatus(ctx, prctx, client, "error", msg)
+		return nil, errors.Wrapf(fc.ParseError, "failed to parse policy: %s: %s", fc.Source, fc.Path)
 
-		return nil, errors.Wrap(fetchedConfig.Error, statusMessage)
+	case fc.Config == nil:
+		logger.Debug().Msg("No policy defined for repository")
+		return nil, nil
 	}
 
-	evaluator, err := policy.ParsePolicy(fetchedConfig.Config)
+	evaluator, err := policy.ParsePolicy(fc.Config)
 	if err != nil {
-		statusMessage := fmt.Sprintf("Unable to parse policy defined by %s", fetchedConfig)
-		logger.Warn().Err(err).Msg(statusMessage)
-
-		b.PostStatus(ctx, prctx, client, "error", statusMessage)
+		msg := fmt.Sprintf("Invalid policy in %s: %s", fc.Source, fc.Path)
+		logger.Warn().Err(err).Msg(msg)
 
-		return nil, errors.Wrap(err, statusMessage)
+		b.PostStatus(ctx, prctx, client, "error", msg)
+		return nil, errors.Wrapf(err, "failed to create evaluator: %s: %s", fc.Source, fc.Path)
 	}
 
 	policyTrigger := evaluator.Trigger()
@@ -214,16 +224,15 @@ func (b *Base) ValidateFetchedConfig(ctx context.Context, prctx pull.Context, cl
 	return evaluator, nil
 }
 
-func (b *Base) EvaluateFetchedConfig(ctx context.Context, prctx pull.Context, client *github.Client, evaluator common.Evaluator, fetchedConfig FetchedConfig) (common.Result, error) {
+func (b *Base) EvaluateFetchedConfig(ctx context.Context, prctx pull.Context, client *github.Client, evaluator common.Evaluator, fc FetchedConfig) (common.Result, error) {
 	logger := zerolog.Ctx(ctx)
 
 	result := evaluator.Evaluate(ctx, prctx)
 	if result.Error != nil {
-		statusMessage := fmt.Sprintf("Error evaluating policy defined by %s", fetchedConfig)
-		logger.Warn().Err(result.Error).Msg(statusMessage)
-
-		b.PostStatus(ctx, prctx, client, "error", statusMessage)
+		msg := fmt.Sprintf("Error evaluating policy in %s: %s", fc.Source, fc.Path)
+		logger.Warn().Err(result.Error).Msg(msg)
 
+		b.PostStatus(ctx, prctx, client, "error", msg)
 		return result, result.Error
 	}
 

server/handler/details.go

@@ -20,6 +20,7 @@ import (
 	"net/url"
 	"path"
 	"strconv"
+	"strings"
 
 	"github.com/alexedwards/scs"
 	"github.com/bluekeyes/templatetree"
@@ -123,22 +124,16 @@ func (h *Details) ServeHTTP(w http.ResponseWriter, r *http.Request) error {
 	data.PullRequest = pr
 	data.User = user
 
-	config, err := h.ConfigFetcher.ConfigForPR(ctx, prctx, client)
+	config := h.ConfigFetcher.ConfigForPR(ctx, prctx, client)
 	data.PolicyURL = getPolicyURL(pr, config)
-	if err != nil {
-		data.Error = errors.WithMessage(err, fmt.Sprintf("Failed to fetch configuration: %s", config))
-		return h.render(w, data)
-	}
 
 	evaluator, err := h.Base.ValidateFetchedConfig(ctx, prctx, client, config, common.TriggerAll)
-
 	if err != nil {
 		data.Error = err
 		return h.render(w, data)
 	}
-
 	if evaluator == nil {
-		data.Error = errors.Errorf("Unable to evaluate: %s", config.Description())
+		data.Error = errors.Errorf("Invalid policy at %s: %s", config.Source, config.Path)
 		return h.render(w, data)
 	}
 
@@ -172,7 +167,12 @@ func (h *Details) render404(w http.ResponseWriter, owner, repo string, number in
 func getPolicyURL(pr *github.PullRequest, config FetchedConfig) string {
 	base := pr.GetBase().GetRepo().GetHTMLURL()
 	if u, _ := url.Parse(base); u != nil {
-		u.Path = path.Join(u.Path, "blob", pr.GetBase().GetRef(), config.Path)
+		// TODO(bkeyes): this format is not guaranteed by 'go-githubapp/appconfig'
+		srcParts := strings.Split(config.Source, "@")
+		if len(srcParts) != 2 {
+			return base
+		}
+		u.Path = path.Join(srcParts[0], "blob", srcParts[1], config.Path)
 		return u.String()
 	}
 	return base