Fix the bug when ssh clone with redirect user or repository (#36039) (#36090)

Backport #36039 by @lunny

Fix #36026 

The redirect should be checked when original user/repo doesn't exist.

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>

routers/private/serv.go

@@ -108,21 +108,19 @@ func ServCommand(ctx *context.PrivateContext) {
 		results.RepoName = repoName[:len(repoName)-5]
 	}
 
-	// Check if there is a user redirect for the requested owner
-	redirectedUserID, err := user_model.LookupUserRedirect(ctx, results.OwnerName)
-	if err == nil {
-		owner, err := user_model.GetUserByID(ctx, redirectedUserID)
-		if err == nil {
-			log.Info("User %s has been redirected to %s", results.OwnerName, owner.Name)
-			results.OwnerName = owner.Name
-		} else {
-			log.Warn("User %s has a redirect to user with ID %d, but no user with this ID could be found. Trying without redirect...", results.OwnerName, redirectedUserID)
-		}
-	}
-
 	owner, err := user_model.GetUserByName(ctx, results.OwnerName)
 	if err != nil {
-		if user_model.IsErrUserNotExist(err) {
+		if !user_model.IsErrUserNotExist(err) {
+			log.Error("Unable to get repository owner: %s/%s Error: %v", results.OwnerName, results.RepoName, err)
+			ctx.JSON(http.StatusForbidden, private.Response{
+				UserMsg: fmt.Sprintf("Unable to get repository owner: %s/%s %v", results.OwnerName, results.RepoName, err),
+			})
+			return
+		}
+
+		// Check if there is a user redirect for the requested owner
+		redirectedUserID, err := user_model.LookupUserRedirect(ctx, results.OwnerName)
+		if err != nil {
 			// User is fetching/cloning a non-existent repository
 			log.Warn("Failed authentication attempt (cannot find repository: %s/%s) from %s", results.OwnerName, results.RepoName, ctx.RemoteAddr())
 			ctx.JSON(http.StatusNotFound, private.Response{
@@ -130,11 +128,20 @@ func ServCommand(ctx *context.PrivateContext) {
 			})
 			return
 		}
-		log.Error("Unable to get repository owner: %s/%s Error: %v", results.OwnerName, results.RepoName, err)
+
-		ctx.JSON(http.StatusForbidden, private.Response{
+		redirectUser, err := user_model.GetUserByID(ctx, redirectedUserID)
-			UserMsg: fmt.Sprintf("Unable to get repository owner: %s/%s %v", results.OwnerName, results.RepoName, err),
+		if err != nil {
-		})
+			// User is fetching/cloning a non-existent repository
-		return
+			log.Warn("Failed authentication attempt (cannot find repository: %s/%s) from %s", results.OwnerName, results.RepoName, ctx.RemoteAddr())
+			ctx.JSON(http.StatusNotFound, private.Response{
+				UserMsg: fmt.Sprintf("Cannot find repository: %s/%s", results.OwnerName, results.RepoName),
+			})
+			return
+		}
+
+		log.Info("User %s has been redirected to %s", results.OwnerName, redirectUser.Name)
+		results.OwnerName = redirectUser.Name
+		owner = redirectUser
 	}
 	if !owner.IsOrganization() && !owner.IsActive {
 		ctx.JSON(http.StatusForbidden, private.Response{
@@ -143,24 +150,33 @@ func ServCommand(ctx *context.PrivateContext) {
 		return
 	}
 
-	redirectedRepoID, err := repo_model.LookupRedirect(ctx, owner.ID, results.RepoName)
-	if err == nil {
-		redirectedRepo, err := repo_model.GetRepositoryByID(ctx, redirectedRepoID)
-		if err == nil {
-			log.Info("Repository %s/%s has been redirected to %s/%s", results.OwnerName, results.RepoName, redirectedRepo.OwnerName, redirectedRepo.Name)
-			results.RepoName = redirectedRepo.Name
-			results.OwnerName = redirectedRepo.OwnerName
-			owner.ID = redirectedRepo.OwnerID
-		} else {
-			log.Warn("Repo %s/%s has a redirect to repo with ID %d, but no repo with this ID could be found. Trying without redirect...", results.OwnerName, results.RepoName, redirectedRepoID)
-		}
-	}
-
 	// Now get the Repository and set the results section
 	repoExist := true
 	repo, err := repo_model.GetRepositoryByName(ctx, owner.ID, results.RepoName)
 	if err != nil {
-		if repo_model.IsErrRepoNotExist(err) {
+		if !repo_model.IsErrRepoNotExist(err) {
+			log.Error("Unable to get repository: %s/%s Error: %v", results.OwnerName, results.RepoName, err)
+			ctx.JSON(http.StatusInternalServerError, private.Response{
+				Err: fmt.Sprintf("Unable to get repository: %s/%s %v", results.OwnerName, results.RepoName, err),
+			})
+			return
+		}
+
+		redirectedRepoID, err := repo_model.LookupRedirect(ctx, owner.ID, results.RepoName)
+		if err == nil {
+			redirectedRepo, err := repo_model.GetRepositoryByID(ctx, redirectedRepoID)
+			if err == nil {
+				log.Info("Repository %s/%s has been redirected to %s/%s", results.OwnerName, results.RepoName, redirectedRepo.OwnerName, redirectedRepo.Name)
+				results.RepoName = redirectedRepo.Name
+				results.OwnerName = redirectedRepo.OwnerName
+				repo = redirectedRepo
+				owner.ID = redirectedRepo.OwnerID
+			} else {
+				log.Warn("Repo %s/%s has a redirect to repo with ID %d, but no repo with this ID could be found. Trying without redirect...", results.OwnerName, results.RepoName, redirectedRepoID)
+			}
+		}
+
+		if repo == nil {
 			repoExist = false
 			if mode == perm.AccessModeRead {
 				// User is fetching/cloning a non-existent repository
@@ -170,13 +186,6 @@ func ServCommand(ctx *context.PrivateContext) {
 				})
 				return
 			}
-			// else fallthrough (push-to-create may kick in below)
-		} else {
-			log.Error("Unable to get repository: %s/%s Error: %v", results.OwnerName, results.RepoName, err)
-			ctx.JSON(http.StatusInternalServerError, private.Response{
-				Err: fmt.Sprintf("Unable to get repository: %s/%s %v", results.OwnerName, results.RepoName, err),
-			})
-			return
 		}
 	}
 

tests/integration/git_ssh_redirect_test.go

@@ -6,9 +6,13 @@ package integration
 import (
 	"fmt"
 	"net/url"
+	"os"
 	"testing"
 
 	auth_model "code.gitea.io/gitea/models/auth"
+	"code.gitea.io/gitea/modules/structs"
+
+	"github.com/stretchr/testify/assert"
 )
 
 func TestGitSSHRedirect(t *testing.T) {
@@ -16,7 +20,8 @@ func TestGitSSHRedirect(t *testing.T) {
 }
 
 func testGitSSHRedirect(t *testing.T, u *url.URL) {
-	apiTestContext := NewAPITestContext(t, "user2", "repo1", auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteUser)
+	apiTestContext := NewAPITestContext(t, "user2", "repo1", auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteUser, auth_model.AccessTokenScopeWriteOrganization)
+	session := loginUser(t, "user2")
 
 	withKeyFile(t, "my-testing-key", func(keyFile string) {
 		t.Run("CreateUserKey", doAPICreateUserKey(apiTestContext, "test-key", keyFile))
@@ -38,5 +43,39 @@ func testGitSSHRedirect(t *testing.T, u *url.URL) {
 				t.Run("Clone", doGitClone(t.TempDir(), cloneURL))
 			})
 		}
+
+		doAPICreateOrganization(apiTestContext, &structs.CreateOrgOption{
+			UserName: "olduser2",
+			FullName: "Old User2",
+		})(t)
+
+		cloneURL := createSSHUrl("olduser2/repo1.git", u)
+		t.Run("Clone Should Fail", doGitCloneFail(cloneURL))
+
+		doAPICreateOrganizationRepository(apiTestContext, "olduser2", &structs.CreateRepoOption{
+			Name:     "repo1",
+			AutoInit: true,
+		})(t)
+		testEditFile(t, session, "olduser2", "repo1", "master", "README.md", "This is olduser2's repo1\n")
+
+		dstDir := t.TempDir()
+		t.Run("Clone", doGitClone(dstDir, cloneURL))
+		readMEContent, err := os.ReadFile(dstDir + "/README.md")
+		assert.NoError(t, err)
+		assert.Equal(t, "This is olduser2's repo1\n", string(readMEContent))
+
+		apiTestContext2 := NewAPITestContext(t, "user2", "oldrepo1", auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteUser, auth_model.AccessTokenScopeWriteOrganization)
+		doAPICreateRepository(apiTestContext2, false)(t)
+		testEditFile(t, session, "user2", "oldrepo1", "master", "README.md", "This is user2's oldrepo1\n")
+
+		dstDir = t.TempDir()
+		cloneURL = createSSHUrl("user2/oldrepo1.git", u)
+		t.Run("Clone", doGitClone(dstDir, cloneURL))
+		readMEContent, err = os.ReadFile(dstDir + "/README.md")
+		assert.NoError(t, err)
+		assert.Equal(t, "This is user2's oldrepo1\n", string(readMEContent))
+
+		cloneURL = createSSHUrl("olduser2/oldrepo1.git", u)
+		t.Run("Clone Should Fail", doGitCloneFail(cloneURL))
 	})
 }