From c7a6ee5c0bd8dcceac76dcb2100e81395d57560a Mon Sep 17 00:00:00 2001
From: Lanre Adelowo <adelowomailbox@gmail.com>
Date: Tue, 7 Aug 2018 02:59:42 +0100
Subject: [PATCH] Don't fail silently if trying to add a collaborator twice
 (#4533)

* don't fail silently if trying to add a collaborator twice

* fix translation text

* added collaborator test

* improvee testcases

* Added tests to make sure a collaborator cannot be added twice
---
 options/locale/locale_en-US.ini |   1 +
 routers/repo/setting.go         |   6 ++
 routers/repo/settings_test.go   | 102 ++++++++++++++++++++++++++++++++
 3 files changed, 109 insertions(+)

diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini
index 3d933b3728..bd13288b46 100644
--- a/options/locale/locale_en-US.ini
+++ b/options/locale/locale_en-US.ini
@@ -1025,6 +1025,7 @@ settings.transfer_succeed = The repository has been transferred.
 settings.confirm_delete = Delete Repository
 settings.add_collaborator = Add Collaborator
 settings.add_collaborator_success = The collaborator has been added.
+settings.add_collaborator_duplicate =The collaborator is already added to this repository.
 settings.delete_collaborator = Remove
 settings.collaborator_deletion = Remove Collaborator
 settings.collaborator_deletion_desc = Removing a collaborator will revoke their access to this repository. Continue?
diff --git a/routers/repo/setting.go b/routers/repo/setting.go
index fa3bd434d5..835ba0a751 100644
--- a/routers/repo/setting.go
+++ b/routers/repo/setting.go
@@ -401,6 +401,12 @@ func CollaborationPost(ctx *context.Context) {
 		}
 	}
 
+	if got, err := ctx.Repo.Repository.IsCollaborator(u.ID); err == nil && got {
+		ctx.Flash.Error(ctx.Tr("repo.settings.add_collaborator_duplicate"))
+		ctx.Redirect(ctx.Repo.RepoLink + "/settings/collaboration")
+		return
+	}
+
 	if err = ctx.Repo.Repository.AddCollaborator(u); err != nil {
 		ctx.ServerError("AddCollaborator", err)
 		return
diff --git a/routers/repo/settings_test.go b/routers/repo/settings_test.go
index 392c05f773..a8cc645075 100644
--- a/routers/repo/settings_test.go
+++ b/routers/repo/settings_test.go
@@ -10,6 +10,7 @@ import (
 
 	"code.gitea.io/gitea/models"
 	"code.gitea.io/gitea/modules/auth"
+	"code.gitea.io/gitea/modules/context"
 	"code.gitea.io/gitea/modules/test"
 
 	"github.com/stretchr/testify/assert"
@@ -59,3 +60,104 @@ func TestAddReadWriteOnlyDeployKey(t *testing.T) {
 		Mode:    models.AccessModeWrite,
 	})
 }
+
+func TestCollaborationPost(t *testing.T) {
+
+	models.PrepareTestEnv(t)
+	ctx := test.MockContext(t, "user2/repo1/issues/labels")
+	test.LoadUser(t, ctx, 2)
+	test.LoadUser(t, ctx, 4)
+	test.LoadRepo(t, ctx, 1)
+
+	ctx.Req.Form.Set("collaborator", "user4")
+
+	u := &models.User{
+		LowerName: "user2",
+		Type:      models.UserTypeIndividual,
+	}
+
+	re := &models.Repository{
+		ID:    2,
+		Owner: u,
+	}
+
+	repo := &context.Repository{
+		Owner:      u,
+		Repository: re,
+	}
+
+	ctx.Repo = repo
+
+	CollaborationPost(ctx)
+
+	assert.EqualValues(t, http.StatusFound, ctx.Resp.Status())
+
+	exists, err := re.IsCollaborator(4)
+	assert.NoError(t, err)
+	assert.True(t, exists)
+}
+
+func TestCollaborationPost_AddCollaboratorTwice(t *testing.T) {
+
+	models.PrepareTestEnv(t)
+	ctx := test.MockContext(t, "user2/repo1/issues/labels")
+	test.LoadUser(t, ctx, 2)
+	test.LoadUser(t, ctx, 4)
+	test.LoadRepo(t, ctx, 1)
+
+	ctx.Req.Form.Set("collaborator", "user4")
+
+	u := &models.User{
+		LowerName: "user2",
+		Type:      models.UserTypeIndividual,
+	}
+
+	re := &models.Repository{
+		ID:    2,
+		Owner: u,
+	}
+
+	repo := &context.Repository{
+		Owner:      u,
+		Repository: re,
+	}
+
+	ctx.Repo = repo
+
+	CollaborationPost(ctx)
+
+	assert.EqualValues(t, http.StatusFound, ctx.Resp.Status())
+
+	exists, err := re.IsCollaborator(4)
+	assert.NoError(t, err)
+	assert.True(t, exists)
+
+	// Try adding the same collaborator again
+	CollaborationPost(ctx)
+
+	assert.EqualValues(t, http.StatusFound, ctx.Resp.Status())
+	assert.NotEmpty(t, ctx.Flash.ErrorMsg)
+}
+
+func TestCollaborationPost_NonExistentUser(t *testing.T) {
+
+	models.PrepareTestEnv(t)
+	ctx := test.MockContext(t, "user2/repo1/issues/labels")
+	test.LoadUser(t, ctx, 2)
+	test.LoadRepo(t, ctx, 1)
+
+	ctx.Req.Form.Set("collaborator", "user34")
+
+	repo := &context.Repository{
+		Owner: &models.User{
+			LowerName: "user2",
+		},
+	}
+
+	ctx.Repo = repo
+
+	CollaborationPost(ctx)
+
+	assert.EqualValues(t, http.StatusFound, ctx.Resp.Status())
+	assert.NotEmpty(t, ctx.Flash.ErrorMsg)
+}