From eeeb28bbdeaff74747bf0b19e68a000c31bdcb0f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=
 <avarab@gmail.com>
Date: Mon, 14 Dec 2020 16:17:17 +0100
Subject: hooks: pass GL_ID & its JSON around as numeric

Let's not specify UserID in the ReceiveHooksPayload an a string, but
as an int64. Otherwise every other 3rd party JSON parser needs to do
this string to int dance when they want to get an ID out of it.
---
 internal/git/hooks_options.go      |  7 ++++++-
 internal/git/hooks_payload.go      |  9 +++++++--
 internal/git/hooks_payload_test.go | 24 +++++++++++++++++++-----
 3 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/internal/git/hooks_options.go b/internal/git/hooks_options.go
index f980b7d27..fa205e782 100644
--- a/internal/git/hooks_options.go
+++ b/internal/git/hooks_options.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"strconv"
 
 	"gitlab.com/gitlab-org/gitaly/internal/git/hooks"
 	"gitlab.com/gitlab-org/gitaly/internal/gitaly/config"
@@ -79,8 +80,12 @@ type ReceivePackRequest interface {
 // git-receive-pack(1).
 func WithReceivePackHooks(ctx context.Context, cfg config.Cfg, req ReceivePackRequest, protocol string) CmdOpt {
 	return func(cc *cmdCfg) error {
+		userId, err := strconv.ParseInt(req.GetGlId(), 10, 0)
+		if err != nil {
+			return err
+		}
 		if err := cc.configureHooks(ctx, req.GetRepository(), config.Config, &ReceiveHooksPayload{
-			UserID:   req.GetGlId(),
+			UserID:   userId,
 			Username: req.GetGlUsername(),
 			Protocol: protocol,
 		}); err != nil {
diff --git a/internal/git/hooks_payload.go b/internal/git/hooks_payload.go
index 8852a34f3..786e79c82 100644
--- a/internal/git/hooks_payload.go
+++ b/internal/git/hooks_payload.go
@@ -6,6 +6,7 @@ import (
 	"errors"
 	"fmt"
 	"strings"
+	"strconv"
 
 	"github.com/golang/protobuf/jsonpb"
 	"gitlab.com/gitlab-org/gitaly/internal/gitaly/config"
@@ -59,7 +60,7 @@ type ReceiveHooksPayload struct {
 	// Username contains the name of the user who has caused the hook to be executed.
 	Username string `json:"username"`
 	// UserID contains the ID of the user who has caused the hook to be executed.
-	UserID string `json:"userid"`
+	UserID int64 `json:"userid"`
 	// Protocol contains the protocol via which the hook was executed. This
 	// can be one of "web", "ssh" or "smarthttp".
 	Protocol string `json:"protocol"`
@@ -233,6 +234,10 @@ func fallbackReceiveHooksPayloadFromEnv(envs []string) (*ReceiveHooksPayload, er
 	if !ok {
 		return nil, errors.New("no user ID found in hooks environment")
 	}
+	userIDint, err := strconv.ParseInt(userID, 10, 0)
+	if err != nil {
+		return nil, fmt.Errorf("user ID found in hooks is non-numeric: %w", err)
+	}
 
 	username, ok := lookupEnv(envs, "GL_USERNAME")
 	if !ok {
@@ -241,7 +246,7 @@ func fallbackReceiveHooksPayloadFromEnv(envs []string) (*ReceiveHooksPayload, er
 
 	return &ReceiveHooksPayload{
 		Protocol: protocol,
-		UserID:   userID,
+		UserID:   userIDint,
 		Username: username,
 	}, nil
 }
diff --git a/internal/git/hooks_payload_test.go b/internal/git/hooks_payload_test.go
index 609b7d149..d1576458c 100644
--- a/internal/git/hooks_payload_test.go
+++ b/internal/git/hooks_payload_test.go
@@ -237,7 +237,7 @@ func TestHooksPayload(t *testing.T) {
 			InternalSocket:      "/path/to/socket",
 			InternalSocketToken: "secret",
 			ReceiveHooksPayload: &ReceiveHooksPayload{
-				UserID:   "1234",
+				UserID:   1234,
 				Username: "user",
 				Protocol: "ssh",
 			},
@@ -262,7 +262,7 @@ func TestHooksPayload(t *testing.T) {
 			InternalSocket:      config.Config.GitalyInternalSocketPath(),
 			InternalSocketToken: config.Config.Auth.Token,
 			ReceiveHooksPayload: &ReceiveHooksPayload{
-				UserID:   "1234",
+				UserID:   1234,
 				Username: "user",
 				Protocol: "ssh",
 			},
@@ -279,12 +279,26 @@ func TestHooksPayload(t *testing.T) {
 			"GL_PROTOCOL=ssh",
 		})
 		require.Error(t, err)
-		require.Contains(t, "no user ID found in hooks environment", err.Error())
+		require.Equal(t, "no user ID found in hooks environment", err.Error())
+	})
+
+	t.Run("payload with non-numeric GL_ID", func(t *testing.T) {
+		env, err := NewHooksPayload(config.Config, repo, nil, nil, nil).Env()
+		require.NoError(t, err)
+
+		_, err = HooksPayloadFromEnv([]string{
+			env,
+			"GL_USERNAME=user",
+			"GL_ID=1234xyz",
+			"GL_PROTOCOL=ssh",
+		})
+		require.Error(t, err)
+		require.Contains(t, err.Error(), "user ID found in hooks is non-numeric")
 	})
 
 	t.Run("receive hooks payload", func(t *testing.T) {
 		env, err := NewHooksPayload(config.Config, repo, nil, nil, &ReceiveHooksPayload{
-			UserID:   "1234",
+			UserID:   1234,
 			Username: "user",
 			Protocol: "ssh",
 		}).Env()
@@ -304,7 +318,7 @@ func TestHooksPayload(t *testing.T) {
 			InternalSocket:      config.Config.GitalyInternalSocketPath(),
 			InternalSocketToken: config.Config.Auth.Token,
 			ReceiveHooksPayload: &ReceiveHooksPayload{
-				UserID:   "1234",
+				UserID:   1234,
 				Username: "user",
 				Protocol: "ssh",
 			},
-- 
cgit v1.2.3