From 3c5eb71ba8b9ff2c5eb4b2c79385cf1e696f24b2 Mon Sep 17 00:00:00 2001
From: Simone Gotti <simone.gotti@gmail.com>
Date: Fri, 29 Mar 2019 12:09:32 +0100
Subject: [PATCH] runservice: make and check that group paths are absolute

---
 internal/services/gateway/webhook.go          | 22 ++++++++++++++-----
 .../runservice/scheduler/command/command.go   |  8 +++++++
 internal/services/runservice/types/types.go   |  7 +++---
 3 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/internal/services/gateway/webhook.go b/internal/services/gateway/webhook.go
index 5c4391d..4373d7a 100644
--- a/internal/services/gateway/webhook.go
+++ b/internal/services/gateway/webhook.go
@@ -61,6 +61,16 @@ const (
 	AnnotationPullRequestLink = "pull_request_link"
 )
 
+type GroupType string
+
+const (
+	GroupTypeProject     GroupType = "project"
+	GroupTypeUser        GroupType = "user"
+	GroupTypeBranch      GroupType = "branch"
+	GroupTypeTag         GroupType = "tag"
+	GroupTypePullRequest GroupType = "pr"
+)
+
 func genAnnotationVirtualBranch(webhookData *types.WebhookData) string {
 	switch webhookData.Event {
 	case types.WebhookEventPush:
@@ -74,16 +84,16 @@ func genAnnotationVirtualBranch(webhookData *types.WebhookData) string {
 	panic(fmt.Errorf("invalid webhook event type: %q", webhookData.Event))
 }
 
-func genGroup(baseGroupID string, webhookData *types.WebhookData) string {
+func genGroup(baseGroupType GroupType, baseGroupID string, webhookData *types.WebhookData) string {
 	// we pathescape the branch name to handle branches with slashes and make the
 	// branch a single path entry
 	switch webhookData.Event {
 	case types.WebhookEventPush:
-		return path.Join(baseGroupID, "branch-"+url.PathEscape(webhookData.Branch))
+		return path.Join("/", string(baseGroupType), baseGroupID, string(GroupTypeBranch), url.PathEscape(webhookData.Branch))
 	case types.WebhookEventTag:
-		return path.Join(baseGroupID, "tag-"+url.PathEscape(webhookData.Tag))
+		return path.Join("/", string(baseGroupType), baseGroupID, string(GroupTypeTag), url.PathEscape(webhookData.Tag))
 	case types.WebhookEventPullRequest:
-		return path.Join(baseGroupID, "pr-"+url.PathEscape(webhookData.PullRequestID))
+		return path.Join("/", string(baseGroupType), baseGroupID, string(GroupTypePullRequest), url.PathEscape(webhookData.PullRequestID))
 	}
 
 	panic(fmt.Errorf("invalid webhook event type: %q", webhookData.Event))
@@ -288,9 +298,9 @@ func (h *webhooksHandler) handleWebhook(r *http.Request) (int, string, error) {
 
 	var group string
 	if !isUserBuild {
-		group = genGroup(webhookData.ProjectID, webhookData)
+		group = genGroup(GroupTypeProject, webhookData.ProjectID, webhookData)
 	} else {
-		group = genGroup(userID, webhookData)
+		group = genGroup(GroupTypeUser, userID, webhookData)
 	}
 
 	if err := h.createRuns(ctx, data, group, annotations, env, variables, webhookData); err != nil {
diff --git a/internal/services/runservice/scheduler/command/command.go b/internal/services/runservice/scheduler/command/command.go
index 839767c..05f4e9d 100644
--- a/internal/services/runservice/scheduler/command/command.go
+++ b/internal/services/runservice/scheduler/command/command.go
@@ -16,6 +16,7 @@ package command
 
 import (
 	"context"
+	"path"
 	"time"
 
 	uuid "github.com/satori/go.uuid"
@@ -146,6 +147,13 @@ func (s *CommandHandler) newRun(ctx context.Context, req *RunCreateRequest) (*ty
 	rc := req.RunConfig
 	var run *types.Run
 
+	if req.Group == "" {
+		return nil, errors.Errorf("run group is empty")
+	}
+	if !path.IsAbs(req.Group) {
+		return nil, errors.Errorf("run group %q must be an absolute path", req.Group)
+	}
+
 	// generate a new run sequence that will be the same for the run, runconfig and rundata
 	seq, err := sequence.IncSequence(ctx, s.e, common.EtcdRunSequenceKey)
 	if err != nil {
diff --git a/internal/services/runservice/types/types.go b/internal/services/runservice/types/types.go
index 4eed09b..1333a94 100644
--- a/internal/services/runservice/types/types.go
+++ b/internal/services/runservice/types/types.go
@@ -79,10 +79,9 @@ type Run struct {
 	Counter uint64 `json:"counter,omitempty"`
 
 	// Group is the run group of the run. Every run is assigned to a specific group
-	// i.e. project/$projectid/$branch
-	// i.e. user/$projectid/$branch (for a user run)
-	// this is the format that will be used to archive the runs in the lts. It's
-	// also needed to fetch them when they aren't indexed in the readdb.
+	// The format is /$grouptypes/groupname(/$grouptype/groupname ...)
+	// i.e. /project/$projectid/branch/$branchname
+	//      /project/$projectid/pr/$prid
 	Group string `json:"group,omitempty"`
 
 	// Annotations contain custom run properties