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

@@ -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?

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

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)
+}