From f9682bf9b5c3512067df8b668bf8b0845a577da8 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 5 Oct 2024 13:49:23 -0700 Subject: [PATCH 1/5] Move delete deploy keys into service layer --- models/asymkey/error.go | 5 ++- models/repo.go | 48 --------------------- services/asymkey/deploy_key.go | 79 +++++++++++++++++++++++++++++++++- services/repository/delete.go | 13 ++---- 4 files changed, 83 insertions(+), 62 deletions(-) diff --git a/models/asymkey/error.go b/models/asymkey/error.go index 03bc82302f100..2e65d76612982 100644 --- a/models/asymkey/error.go +++ b/models/asymkey/error.go @@ -217,6 +217,7 @@ func (err ErrGPGKeyAccessDenied) Unwrap() error { // ErrKeyAccessDenied represents a "KeyAccessDenied" kind of error. type ErrKeyAccessDenied struct { UserID int64 + RepoID int64 KeyID int64 Note string } @@ -228,8 +229,8 @@ func IsErrKeyAccessDenied(err error) bool { } func (err ErrKeyAccessDenied) Error() string { - return fmt.Sprintf("user does not have access to the key [user_id: %d, key_id: %d, note: %s]", - err.UserID, err.KeyID, err.Note) + return fmt.Sprintf("user does not have access to the key [user_id: %d, repo_id: %d, key_id: %d, note: %s]", + err.UserID, err.RepoID, err.KeyID, err.Note) } func (err ErrKeyAccessDenied) Unwrap() error { diff --git a/models/repo.go b/models/repo.go index 0dc8ee5df33e7..3e9c52fdd9c06 100644 --- a/models/repo.go +++ b/models/repo.go @@ -6,15 +6,12 @@ package models import ( "context" - "fmt" "strconv" _ "image/jpeg" // Needed for jpeg support - asymkey_model "code.gitea.io/gitea/models/asymkey" "code.gitea.io/gitea/models/db" issues_model "code.gitea.io/gitea/models/issues" - access_model "code.gitea.io/gitea/models/perm/access" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unit" user_model "code.gitea.io/gitea/models/user" @@ -315,48 +312,3 @@ func DoctorUserStarNum(ctx context.Context) (err error) { return err } - -// DeleteDeployKey delete deploy keys -func DeleteDeployKey(ctx context.Context, doer *user_model.User, id int64) error { - key, err := asymkey_model.GetDeployKeyByID(ctx, id) - if err != nil { - if asymkey_model.IsErrDeployKeyNotExist(err) { - return nil - } - return fmt.Errorf("GetDeployKeyByID: %w", err) - } - - // Check if user has access to delete this key. - if !doer.IsAdmin { - repo, err := repo_model.GetRepositoryByID(ctx, key.RepoID) - if err != nil { - return fmt.Errorf("GetRepositoryByID: %w", err) - } - has, err := access_model.IsUserRepoAdmin(ctx, repo, doer) - if err != nil { - return fmt.Errorf("GetUserRepoPermission: %w", err) - } else if !has { - return asymkey_model.ErrKeyAccessDenied{ - UserID: doer.ID, - KeyID: key.ID, - Note: "deploy", - } - } - } - - if _, err := db.DeleteByID[asymkey_model.DeployKey](ctx, key.ID); err != nil { - return fmt.Errorf("delete deploy key [%d]: %w", key.ID, err) - } - - // Check if this is the last reference to same key content. - has, err := asymkey_model.IsDeployKeyExistByKeyID(ctx, key.KeyID) - if err != nil { - return err - } else if !has { - if _, err = db.DeleteByID[asymkey_model.PublicKey](ctx, key.KeyID); err != nil { - return err - } - } - - return nil -} diff --git a/services/asymkey/deploy_key.go b/services/asymkey/deploy_key.go index 324688c53408e..6e52793b77247 100644 --- a/services/asymkey/deploy_key.go +++ b/services/asymkey/deploy_key.go @@ -5,12 +5,75 @@ package asymkey import ( "context" + "fmt" - "code.gitea.io/gitea/models" + asymkey_model "code.gitea.io/gitea/models/asymkey" "code.gitea.io/gitea/models/db" + access_model "code.gitea.io/gitea/models/perm/access" + repo_model "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" ) +func DeleteRepoDeployKeys(ctx context.Context, doer *user_model.User, repoID int64) (int, error) { + deployKeys, err := db.Find[asymkey_model.DeployKey](ctx, asymkey_model.ListDeployKeysOptions{RepoID: repoID}) + if err != nil { + return 0, fmt.Errorf("listDeployKeys: %w", err) + } + + if err := checkDeployPerm(ctx, doer, repoID, 0); err != nil { + return 0, err + } + + for _, dKey := range deployKeys { + if err := deleteDeployKeyFromDB(ctx, doer, dKey); err != nil { + return 0, fmt.Errorf("deleteDeployKeys: %w", err) + } + } + return len(deployKeys), nil +} + +// checkDeployPerm Check if user has access to delete this key. +func checkDeployPerm(ctx context.Context, doer *user_model.User, repoID, keyID int64) error { + if doer.IsAdmin { + return nil + } + repo, err := repo_model.GetRepositoryByID(ctx, repoID) + if err != nil { + return fmt.Errorf("GetRepositoryByID: %w", err) + } + has, err := access_model.IsUserRepoAdmin(ctx, repo, doer) + if err != nil { + return fmt.Errorf("IsUserRepoAdmin: %w", err) + } else if !has { + return asymkey_model.ErrKeyAccessDenied{ + UserID: doer.ID, + RepoID: repoID, + KeyID: keyID, + Note: "deploy", + } + } + return nil +} + +// deleteDeployKeyFromDB delete deploy keys from database +func deleteDeployKeyFromDB(ctx context.Context, doer *user_model.User, key *asymkey_model.DeployKey) error { + if _, err := db.DeleteByID[asymkey_model.DeployKey](ctx, key.ID); err != nil { + return fmt.Errorf("delete deploy key [%d]: %w", key.ID, err) + } + + // Check if this is the last reference to same key content. + has, err := asymkey_model.IsDeployKeyExistByKeyID(ctx, key.KeyID) + if err != nil { + return err + } else if !has { + if _, err = db.DeleteByID[asymkey_model.PublicKey](ctx, key.KeyID); err != nil { + return err + } + } + + return nil +} + // DeleteDeployKey deletes deploy key from its repository authorized_keys file if needed. func DeleteDeployKey(ctx context.Context, doer *user_model.User, id int64) error { dbCtx, committer, err := db.TxContext(ctx) @@ -19,7 +82,19 @@ func DeleteDeployKey(ctx context.Context, doer *user_model.User, id int64) error } defer committer.Close() - if err := models.DeleteDeployKey(dbCtx, doer, id); err != nil { + key, err := asymkey_model.GetDeployKeyByID(ctx, id) + if err != nil { + if asymkey_model.IsErrDeployKeyNotExist(err) { + return nil + } + return fmt.Errorf("GetDeployKeyByID: %w", err) + } + + if err := checkDeployPerm(ctx, doer, key.RepoID, key.ID); err != nil { + return err + } + + if err := deleteDeployKeyFromDB(dbCtx, doer, key); err != nil { return err } if err := committer.Commit(); err != nil { diff --git a/services/repository/delete.go b/services/repository/delete.go index e580833140891..7340c486d0214 100644 --- a/services/repository/delete.go +++ b/services/repository/delete.go @@ -7,11 +7,9 @@ import ( "context" "fmt" - "code.gitea.io/gitea/models" actions_model "code.gitea.io/gitea/models/actions" activities_model "code.gitea.io/gitea/models/activities" admin_model "code.gitea.io/gitea/models/admin" - asymkey_model "code.gitea.io/gitea/models/asymkey" "code.gitea.io/gitea/models/db" git_model "code.gitea.io/gitea/models/git" issues_model "code.gitea.io/gitea/models/issues" @@ -76,16 +74,11 @@ func DeleteRepositoryDirectly(ctx context.Context, doer *user_model.User, repoID } // Delete Deploy Keys - deployKeys, err := db.Find[asymkey_model.DeployKey](ctx, asymkey_model.ListDeployKeysOptions{RepoID: repoID}) + deleted, err := asymkey_service.DeleteRepoDeployKeys(ctx, doer, repoID) if err != nil { - return fmt.Errorf("listDeployKeys: %w", err) - } - needRewriteKeysFile := len(deployKeys) > 0 - for _, dKey := range deployKeys { - if err := models.DeleteDeployKey(ctx, doer, dKey.ID); err != nil { - return fmt.Errorf("deleteDeployKeys: %w", err) - } + return err } + needRewriteKeysFile := deleted > 0 if cnt, err := sess.ID(repoID).Delete(&repo_model.Repository{}); err != nil { return err From 044d12ce91015ede94367bfc9a0a4f83cf68b1fb Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 5 Oct 2024 16:52:10 -0700 Subject: [PATCH 2/5] Fix lint and remove unnecessary check --- services/asymkey/deploy_key.go | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/services/asymkey/deploy_key.go b/services/asymkey/deploy_key.go index 6e52793b77247..484ef7513540c 100644 --- a/services/asymkey/deploy_key.go +++ b/services/asymkey/deploy_key.go @@ -14,18 +14,15 @@ import ( user_model "code.gitea.io/gitea/models/user" ) +// DeleteRepoDeployKeys deletes all deploy keys of a repository. permissions check should be done outside func DeleteRepoDeployKeys(ctx context.Context, doer *user_model.User, repoID int64) (int, error) { deployKeys, err := db.Find[asymkey_model.DeployKey](ctx, asymkey_model.ListDeployKeysOptions{RepoID: repoID}) if err != nil { return 0, fmt.Errorf("listDeployKeys: %w", err) } - if err := checkDeployPerm(ctx, doer, repoID, 0); err != nil { - return 0, err - } - for _, dKey := range deployKeys { - if err := deleteDeployKeyFromDB(ctx, doer, dKey); err != nil { + if err := deleteDeployKeyFromDB(ctx, dKey); err != nil { return 0, fmt.Errorf("deleteDeployKeys: %w", err) } } @@ -56,7 +53,7 @@ func checkDeployPerm(ctx context.Context, doer *user_model.User, repoID, keyID i } // deleteDeployKeyFromDB delete deploy keys from database -func deleteDeployKeyFromDB(ctx context.Context, doer *user_model.User, key *asymkey_model.DeployKey) error { +func deleteDeployKeyFromDB(ctx context.Context, key *asymkey_model.DeployKey) error { if _, err := db.DeleteByID[asymkey_model.DeployKey](ctx, key.ID); err != nil { return fmt.Errorf("delete deploy key [%d]: %w", key.ID, err) } @@ -94,7 +91,7 @@ func DeleteDeployKey(ctx context.Context, doer *user_model.User, id int64) error return err } - if err := deleteDeployKeyFromDB(dbCtx, doer, key); err != nil { + if err := deleteDeployKeyFromDB(dbCtx, key); err != nil { return err } if err := committer.Commit(); err != nil { From 30b3881354ba1adb531ecb4bc7453c87bda33257 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 5 Oct 2024 17:28:31 -0700 Subject: [PATCH 3/5] Fix test --- services/asymkey/main_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/services/asymkey/main_test.go b/services/asymkey/main_test.go index 3505b26f699a6..1cdc39933d43a 100644 --- a/services/asymkey/main_test.go +++ b/services/asymkey/main_test.go @@ -8,6 +8,7 @@ import ( "code.gitea.io/gitea/models/unittest" + _ "code.gitea.io/gitea/models" _ "code.gitea.io/gitea/models/actions" _ "code.gitea.io/gitea/models/activities" ) From c1f1727570f4ceebfc743ce8c214dfae1d5e18be Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 4 Dec 2024 22:29:57 -0800 Subject: [PATCH 4/5] Remove unnecessary parameter --- routers/api/v1/repo/key.go | 2 +- routers/web/repo/setting/deploy_key.go | 2 +- services/asymkey/deploy_key.go | 31 ++++---------------------- 3 files changed, 6 insertions(+), 29 deletions(-) diff --git a/routers/api/v1/repo/key.go b/routers/api/v1/repo/key.go index e5115980eb0a9..060694d085f98 100644 --- a/routers/api/v1/repo/key.go +++ b/routers/api/v1/repo/key.go @@ -279,7 +279,7 @@ func DeleteDeploykey(ctx *context.APIContext) { // "404": // "$ref": "#/responses/notFound" - if err := asymkey_service.DeleteDeployKey(ctx, ctx.Doer, ctx.PathParamInt64(":id")); err != nil { + if err := asymkey_service.DeleteDeployKey(ctx, ctx.Repo.Repository, ctx.PathParamInt64(":id")); err != nil { if asymkey_model.IsErrKeyAccessDenied(err) { ctx.Error(http.StatusForbidden, "", "You do not have access to this key") } else { diff --git a/routers/web/repo/setting/deploy_key.go b/routers/web/repo/setting/deploy_key.go index abc3eb4af18b6..193562528bf83 100644 --- a/routers/web/repo/setting/deploy_key.go +++ b/routers/web/repo/setting/deploy_key.go @@ -99,7 +99,7 @@ func DeployKeysPost(ctx *context.Context) { // DeleteDeployKey response for deleting a deploy key func DeleteDeployKey(ctx *context.Context) { - if err := asymkey_service.DeleteDeployKey(ctx, ctx.Doer, ctx.FormInt64("id")); err != nil { + if err := asymkey_service.DeleteDeployKey(ctx, ctx.Repo.Repository, ctx.FormInt64("id")); err != nil { ctx.Flash.Error("DeleteDeployKey: " + err.Error()) } else { ctx.Flash.Success(ctx.Tr("repo.settings.deploy_key_deletion_success")) diff --git a/services/asymkey/deploy_key.go b/services/asymkey/deploy_key.go index 484ef7513540c..b3c957ec3446b 100644 --- a/services/asymkey/deploy_key.go +++ b/services/asymkey/deploy_key.go @@ -9,7 +9,6 @@ import ( asymkey_model "code.gitea.io/gitea/models/asymkey" "code.gitea.io/gitea/models/db" - access_model "code.gitea.io/gitea/models/perm/access" repo_model "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" ) @@ -29,29 +28,6 @@ func DeleteRepoDeployKeys(ctx context.Context, doer *user_model.User, repoID int return len(deployKeys), nil } -// checkDeployPerm Check if user has access to delete this key. -func checkDeployPerm(ctx context.Context, doer *user_model.User, repoID, keyID int64) error { - if doer.IsAdmin { - return nil - } - repo, err := repo_model.GetRepositoryByID(ctx, repoID) - if err != nil { - return fmt.Errorf("GetRepositoryByID: %w", err) - } - has, err := access_model.IsUserRepoAdmin(ctx, repo, doer) - if err != nil { - return fmt.Errorf("IsUserRepoAdmin: %w", err) - } else if !has { - return asymkey_model.ErrKeyAccessDenied{ - UserID: doer.ID, - RepoID: repoID, - KeyID: keyID, - Note: "deploy", - } - } - return nil -} - // deleteDeployKeyFromDB delete deploy keys from database func deleteDeployKeyFromDB(ctx context.Context, key *asymkey_model.DeployKey) error { if _, err := db.DeleteByID[asymkey_model.DeployKey](ctx, key.ID); err != nil { @@ -72,7 +48,8 @@ func deleteDeployKeyFromDB(ctx context.Context, key *asymkey_model.DeployKey) er } // DeleteDeployKey deletes deploy key from its repository authorized_keys file if needed. -func DeleteDeployKey(ctx context.Context, doer *user_model.User, id int64) error { +// Permissions check should be done outside. +func DeleteDeployKey(ctx context.Context, repo *repo_model.Repository, id int64) error { dbCtx, committer, err := db.TxContext(ctx) if err != nil { return err @@ -87,8 +64,8 @@ func DeleteDeployKey(ctx context.Context, doer *user_model.User, id int64) error return fmt.Errorf("GetDeployKeyByID: %w", err) } - if err := checkDeployPerm(ctx, doer, key.RepoID, key.ID); err != nil { - return err + if key.RepoID != repo.ID { + return fmt.Errorf("deploy key %d does not belong to repository %d", id, repo.ID) } if err := deleteDeployKeyFromDB(dbCtx, key); err != nil { From 958cc8677fec5f1dd48fc31437da30f42b958972 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 5 Dec 2024 10:23:56 -0800 Subject: [PATCH 5/5] Remove unnecessary parameter --- services/asymkey/deploy_key.go | 3 +-- services/repository/delete.go | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/services/asymkey/deploy_key.go b/services/asymkey/deploy_key.go index b3c957ec3446b..9e5a6d6292b98 100644 --- a/services/asymkey/deploy_key.go +++ b/services/asymkey/deploy_key.go @@ -10,11 +10,10 @@ import ( asymkey_model "code.gitea.io/gitea/models/asymkey" "code.gitea.io/gitea/models/db" repo_model "code.gitea.io/gitea/models/repo" - user_model "code.gitea.io/gitea/models/user" ) // DeleteRepoDeployKeys deletes all deploy keys of a repository. permissions check should be done outside -func DeleteRepoDeployKeys(ctx context.Context, doer *user_model.User, repoID int64) (int, error) { +func DeleteRepoDeployKeys(ctx context.Context, repoID int64) (int, error) { deployKeys, err := db.Find[asymkey_model.DeployKey](ctx, asymkey_model.ListDeployKeysOptions{RepoID: repoID}) if err != nil { return 0, fmt.Errorf("listDeployKeys: %w", err) diff --git a/services/repository/delete.go b/services/repository/delete.go index 5fcad4cee138d..61e39fe1056a1 100644 --- a/services/repository/delete.go +++ b/services/repository/delete.go @@ -74,7 +74,7 @@ func DeleteRepositoryDirectly(ctx context.Context, doer *user_model.User, repoID } // Delete Deploy Keys - deleted, err := asymkey_service.DeleteRepoDeployKeys(ctx, doer, repoID) + deleted, err := asymkey_service.DeleteRepoDeployKeys(ctx, repoID) if err != nil { return err }