From afda2db9b8b7e13754ba75b98a35ded07b92307c Mon Sep 17 00:00:00 2001 From: caicandong <1290147055@qq.com> Date: Thu, 14 Sep 2023 19:42:18 +0800 Subject: [PATCH 1/6] Fix --- routers/api/v1/user/app.go | 24 +++++++++++++++++++++--- templates/swagger/v1_json.tmpl | 9 +++++++++ 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/routers/api/v1/user/app.go b/routers/api/v1/user/app.go index f89d53945fa0..d76862e91c5c 100644 --- a/routers/api/v1/user/app.go +++ b/routers/api/v1/user/app.go @@ -43,8 +43,14 @@ func ListAccessTokens(ctx *context.APIContext) { // responses: // "200": // "$ref": "#/responses/AccessTokenList" + // "403": + // "$ref": "#/responses/forbidden" - opts := auth_model.ListAccessTokensOptions{UserID: ctx.Doer.ID, ListOptions: utils.GetListOptions(ctx)} + if !ctx.Doer.IsAdmin && ctx.Doer != ctx.ContextUser { + ctx.Error(http.StatusForbidden, "", "Only admin or user self can list access tokens") + return + } + opts := auth_model.ListAccessTokensOptions{UserID: ctx.ContextUser.ID, ListOptions: utils.GetListOptions(ctx)} count, err := auth_model.CountAccessTokens(opts) if err != nil { @@ -95,11 +101,17 @@ func CreateAccessToken(ctx *context.APIContext) { // "$ref": "#/responses/AccessToken" // "400": // "$ref": "#/responses/error" + // "403": + // "$ref": "#/responses/forbidden" + if !ctx.Doer.IsAdmin && ctx.Doer != ctx.ContextUser { + ctx.Error(http.StatusForbidden, "", "Only admin or user self can list access tokens") + return + } form := web.GetForm(ctx).(*api.CreateAccessTokenOption) t := &auth_model.AccessToken{ - UID: ctx.Doer.ID, + UID: ctx.ContextUser.ID, Name: form.Name, } @@ -153,18 +165,24 @@ func DeleteAccessToken(ctx *context.APIContext) { // responses: // "204": // "$ref": "#/responses/empty" + // "403": + // "$ref": "#/responses/forbidden" // "404": // "$ref": "#/responses/notFound" // "422": // "$ref": "#/responses/error" + if !ctx.Doer.IsAdmin && ctx.Doer != ctx.ContextUser { + ctx.Error(http.StatusForbidden, "", "Only admin or user self can list access tokens") + return + } token := ctx.Params(":id") tokenID, _ := strconv.ParseInt(token, 0, 64) if tokenID == 0 { tokens, err := auth_model.ListAccessTokens(auth_model.ListAccessTokensOptions{ Name: token, - UserID: ctx.Doer.ID, + UserID: ctx.ContextUser.ID, }) if err != nil { ctx.Error(http.StatusInternalServerError, "ListAccessTokens", err) diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 88dc9ea1ce55..39c5a7fe117d 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -16359,6 +16359,9 @@ "responses": { "200": { "$ref": "#/responses/AccessTokenList" + }, + "403": { + "$ref": "#/responses/forbidden" } } }, @@ -16396,6 +16399,9 @@ }, "400": { "$ref": "#/responses/error" + }, + "403": { + "$ref": "#/responses/forbidden" } } } @@ -16430,6 +16436,9 @@ "204": { "$ref": "#/responses/empty" }, + "403": { + "$ref": "#/responses/forbidden" + }, "404": { "$ref": "#/responses/notFound" }, From 2c8635bbbcd8b7afa0d7dd8d5786c003c5999bdb Mon Sep 17 00:00:00 2001 From: caicandong <1290147055@qq.com> Date: Fri, 15 Sep 2023 08:32:58 +0800 Subject: [PATCH 2/6] fix error message --- routers/api/v1/user/app.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/routers/api/v1/user/app.go b/routers/api/v1/user/app.go index d76862e91c5c..38b179f286b0 100644 --- a/routers/api/v1/user/app.go +++ b/routers/api/v1/user/app.go @@ -105,7 +105,7 @@ func CreateAccessToken(ctx *context.APIContext) { // "$ref": "#/responses/forbidden" if !ctx.Doer.IsAdmin && ctx.Doer != ctx.ContextUser { - ctx.Error(http.StatusForbidden, "", "Only admin or user self can list access tokens") + ctx.Error(http.StatusForbidden, "", "Only admin or user self can create access tokens") return } form := web.GetForm(ctx).(*api.CreateAccessTokenOption) @@ -173,7 +173,7 @@ func DeleteAccessToken(ctx *context.APIContext) { // "$ref": "#/responses/error" if !ctx.Doer.IsAdmin && ctx.Doer != ctx.ContextUser { - ctx.Error(http.StatusForbidden, "", "Only admin or user self can list access tokens") + ctx.Error(http.StatusForbidden, "", "Only admin or user self can delete access tokens") return } token := ctx.Params(":id") From 52860e6db78e490b5a3dbcf4a257b7b26042d58e Mon Sep 17 00:00:00 2001 From: caicandong <1290147055@qq.com> Date: Fri, 15 Sep 2023 17:58:08 +0800 Subject: [PATCH 3/6] refactor --- routers/api/v1/api.go | 12 +++++++++++- routers/api/v1/user/app.go | 12 ------------ 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index fd7d3687acbc..a1597cc68ecb 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -367,6 +367,16 @@ func reqOwner() func(ctx *context.APIContext) { } } +// reqSelfOrAdmin doer should be the same as the contextUser or site admin +func reqSelfOrAdmin() func(ctx *context.APIContext) { + return func(ctx *context.APIContext) { + if !ctx.IsUserSiteAdmin() && ctx.ContextUser != ctx.Doer { + ctx.Error(http.StatusForbidden, "reqSelfOrAdmin", "doer should be the site admin or be same as the contextUser") + return + } + } +} + // reqAdmin user should be an owner or a collaborator with admin write of a repository, or site admin func reqAdmin() func(ctx *context.APIContext) { return func(ctx *context.APIContext) { @@ -907,7 +917,7 @@ func Routes() *web.Route { m.Combo("").Get(user.ListAccessTokens). Post(bind(api.CreateAccessTokenOption{}), reqToken(), user.CreateAccessToken) m.Combo("/{id}").Delete(reqToken(), user.DeleteAccessToken) - }, reqBasicOrRevProxyAuth()) + }, reqSelfOrAdmin(), reqBasicOrRevProxyAuth()) m.Get("/activities/feeds", user.ListUserActivityFeeds) }, context_service.UserAssignmentAPI()) diff --git a/routers/api/v1/user/app.go b/routers/api/v1/user/app.go index 38b179f286b0..e1a1d2831a60 100644 --- a/routers/api/v1/user/app.go +++ b/routers/api/v1/user/app.go @@ -46,10 +46,6 @@ func ListAccessTokens(ctx *context.APIContext) { // "403": // "$ref": "#/responses/forbidden" - if !ctx.Doer.IsAdmin && ctx.Doer != ctx.ContextUser { - ctx.Error(http.StatusForbidden, "", "Only admin or user self can list access tokens") - return - } opts := auth_model.ListAccessTokensOptions{UserID: ctx.ContextUser.ID, ListOptions: utils.GetListOptions(ctx)} count, err := auth_model.CountAccessTokens(opts) @@ -104,10 +100,6 @@ func CreateAccessToken(ctx *context.APIContext) { // "403": // "$ref": "#/responses/forbidden" - if !ctx.Doer.IsAdmin && ctx.Doer != ctx.ContextUser { - ctx.Error(http.StatusForbidden, "", "Only admin or user self can create access tokens") - return - } form := web.GetForm(ctx).(*api.CreateAccessTokenOption) t := &auth_model.AccessToken{ @@ -172,10 +164,6 @@ func DeleteAccessToken(ctx *context.APIContext) { // "422": // "$ref": "#/responses/error" - if !ctx.Doer.IsAdmin && ctx.Doer != ctx.ContextUser { - ctx.Error(http.StatusForbidden, "", "Only admin or user self can delete access tokens") - return - } token := ctx.Params(":id") tokenID, _ := strconv.ParseInt(token, 0, 64) From d19f12817282017a6facae7adb5cb2aff13dd819 Mon Sep 17 00:00:00 2001 From: caicandong <1290147055@qq.com> Date: Sat, 16 Sep 2023 22:56:35 +0800 Subject: [PATCH 4/6] add permission test --- tests/integration/api_token_test.go | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/tests/integration/api_token_test.go b/tests/integration/api_token_test.go index 1c63d07f2202..ebfb02f58134 100644 --- a/tests/integration/api_token_test.go +++ b/tests/integration/api_token_test.go @@ -40,6 +40,29 @@ func TestAPIDeleteMissingToken(t *testing.T) { MakeRequest(t, req, http.StatusNotFound) } +// TestAPIGetTokensPermission ensures that Only admin can create token for other users +func TestAPIGetTokensPermission(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + // admin can get tokens for other users + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + req := NewRequestf(t, "GET", "/api/v1/users/user2/tokens") + req = AddBasicAuthHeader(req, user.Name) + MakeRequest(t, req, http.StatusOK) + + // non-admin can get tokens for himself + user = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + req = NewRequestf(t, "GET", "/api/v1/users/user2/tokens") + req = AddBasicAuthHeader(req, user.Name) + MakeRequest(t, req, http.StatusOK) + + // non-admin can't get tokens for other users + user = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4}) + req = NewRequestf(t, "GET", "/api/v1/users/user2/tokens") + req = AddBasicAuthHeader(req, user.Name) + MakeRequest(t, req, http.StatusForbidden) +} + type permission struct { category auth_model.AccessTokenScopeCategory level auth_model.AccessTokenScopeLevel From d161f497b93b3cb35eb1d499a68ccaafc77a7b70 Mon Sep 17 00:00:00 2001 From: silverwind Date: Sat, 16 Sep 2023 16:58:26 +0200 Subject: [PATCH 5/6] Update tests/integration/api_token_test.go --- tests/integration/api_token_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/api_token_test.go b/tests/integration/api_token_test.go index ebfb02f58134..49512e784225 100644 --- a/tests/integration/api_token_test.go +++ b/tests/integration/api_token_test.go @@ -40,7 +40,7 @@ func TestAPIDeleteMissingToken(t *testing.T) { MakeRequest(t, req, http.StatusNotFound) } -// TestAPIGetTokensPermission ensures that Only admin can create token for other users +// TestAPIGetTokensPermission ensures that only admin can create token for other users func TestAPIGetTokensPermission(t *testing.T) { defer tests.PrepareTestEnv(t)() From a2a0b7612c7870d745aaae6d9a59c58a49228b6d Mon Sep 17 00:00:00 2001 From: CaiCandong <50507092+CaiCandong@users.noreply.github.com> Date: Sat, 16 Sep 2023 23:00:57 +0800 Subject: [PATCH 6/6] fix comment --- tests/integration/api_token_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/api_token_test.go b/tests/integration/api_token_test.go index 49512e784225..a71392298230 100644 --- a/tests/integration/api_token_test.go +++ b/tests/integration/api_token_test.go @@ -40,7 +40,7 @@ func TestAPIDeleteMissingToken(t *testing.T) { MakeRequest(t, req, http.StatusNotFound) } -// TestAPIGetTokensPermission ensures that only admin can create token for other users +// TestAPIGetTokensPermission ensures that only the admin can get tokens from other users func TestAPIGetTokensPermission(t *testing.T) { defer tests.PrepareTestEnv(t)()