From 2f1a737769b2ecd64c7c62e94cabf4bd72e89b69 Mon Sep 17 00:00:00 2001 From: KN4CK3R Date: Fri, 20 Sep 2024 21:00:39 +0200 Subject: [PATCH] Fix incorrect `/tokens` api (#32085) Fixes #32078 - Add missing scopes output. - Disallow empty scope. --------- Co-authored-by: Lunny Xiao (cherry picked from commit 08adbc468f8875fd4763c3656b334203c11adc0a) (cherry picked from commit 526054332acb221e061d3900bba2dc6e012da52d) --- routers/api/v1/user/app.go | 5 +++++ tests/integration/api_token_test.go | 31 ++++++++++------------------- 2 files changed, 16 insertions(+), 20 deletions(-) diff --git a/routers/api/v1/user/app.go b/routers/api/v1/user/app.go index b61ebac7d0..d5b20f7703 100644 --- a/routers/api/v1/user/app.go +++ b/routers/api/v1/user/app.go @@ -118,6 +118,10 @@ func CreateAccessToken(ctx *context.APIContext) { ctx.Error(http.StatusBadRequest, "AccessTokenScope.Normalize", fmt.Errorf("invalid access token scope provided: %w", err)) return } + if scope == "" { + ctx.Error(http.StatusBadRequest, "AccessTokenScope", "access token must have a scope") + return + } t.Scope = scope if err := auth_model.NewAccessToken(ctx, t); err != nil { @@ -129,6 +133,7 @@ func CreateAccessToken(ctx *context.APIContext) { Token: t.Token, ID: t.ID, TokenLastEight: t.TokenLastEight, + Scopes: t.Scope.StringSlice(), }) } diff --git a/tests/integration/api_token_test.go b/tests/integration/api_token_test.go index 9c7bf37330..01d18ef6f1 100644 --- a/tests/integration/api_token_test.go +++ b/tests/integration/api_token_test.go @@ -23,10 +23,10 @@ func TestAPICreateAndDeleteToken(t *testing.T) { defer tests.PrepareTestEnv(t)() user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) - newAccessToken := createAPIAccessTokenWithoutCleanUp(t, "test-key-1", user, nil) + newAccessToken := createAPIAccessTokenWithoutCleanUp(t, "test-key-1", user, []auth_model.AccessTokenScope{auth_model.AccessTokenScopeAll}) deleteAPIAccessToken(t, newAccessToken, user) - newAccessToken = createAPIAccessTokenWithoutCleanUp(t, "test-key-2", user, nil) + newAccessToken = createAPIAccessTokenWithoutCleanUp(t, "test-key-2", user, []auth_model.AccessTokenScope{auth_model.AccessTokenScopeAll}) deleteAPIAccessToken(t, newAccessToken, user) } @@ -72,19 +72,19 @@ func TestAPIDeleteTokensPermission(t *testing.T) { user4 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4}) // admin can delete tokens for other users - createAPIAccessTokenWithoutCleanUp(t, "test-key-1", user2, nil) + createAPIAccessTokenWithoutCleanUp(t, "test-key-1", user2, []auth_model.AccessTokenScope{auth_model.AccessTokenScopeAll}) req := NewRequest(t, "DELETE", "/api/v1/users/"+user2.LoginName+"/tokens/test-key-1"). AddBasicAuth(admin.Name) MakeRequest(t, req, http.StatusNoContent) // non-admin can delete tokens for himself - createAPIAccessTokenWithoutCleanUp(t, "test-key-2", user2, nil) + createAPIAccessTokenWithoutCleanUp(t, "test-key-2", user2, []auth_model.AccessTokenScope{auth_model.AccessTokenScopeAll}) req = NewRequest(t, "DELETE", "/api/v1/users/"+user2.LoginName+"/tokens/test-key-2"). AddBasicAuth(user2.Name) MakeRequest(t, req, http.StatusNoContent) // non-admin can't delete tokens for other users - createAPIAccessTokenWithoutCleanUp(t, "test-key-3", user2, nil) + createAPIAccessTokenWithoutCleanUp(t, "test-key-3", user2, []auth_model.AccessTokenScope{auth_model.AccessTokenScopeAll}) req = NewRequest(t, "DELETE", "/api/v1/users/"+user2.LoginName+"/tokens/test-key-3"). AddBasicAuth(user4.Name) MakeRequest(t, req, http.StatusForbidden) @@ -520,7 +520,7 @@ func runTestCase(t *testing.T, testCase *requiredScopeTestCase, user *user_model unauthorizedScopes = append(unauthorizedScopes, cateogoryUnauthorizedScopes...) } - accessToken := createAPIAccessTokenWithoutCleanUp(t, "test-token", user, &unauthorizedScopes) + accessToken := createAPIAccessTokenWithoutCleanUp(t, "test-token", user, unauthorizedScopes) defer deleteAPIAccessToken(t, accessToken, user) // Request the endpoint. Verify that permission is denied. @@ -532,20 +532,12 @@ func runTestCase(t *testing.T, testCase *requiredScopeTestCase, user *user_model // createAPIAccessTokenWithoutCleanUp Create an API access token and assert that // creation succeeded. The caller is responsible for deleting the token. -func createAPIAccessTokenWithoutCleanUp(t *testing.T, tokenName string, user *user_model.User, scopes *[]auth_model.AccessTokenScope) api.AccessToken { +func createAPIAccessTokenWithoutCleanUp(t *testing.T, tokenName string, user *user_model.User, scopes []auth_model.AccessTokenScope) api.AccessToken { payload := map[string]any{ - "name": tokenName, - } - if scopes != nil { - for _, scope := range *scopes { - scopes, scopesExists := payload["scopes"].([]string) - if !scopesExists { - scopes = make([]string, 0) - } - scopes = append(scopes, string(scope)) - payload["scopes"] = scopes - } + "name": tokenName, + "scopes": scopes, } + log.Debug("Requesting creation of token with scopes: %v", scopes) req := NewRequestWithJSON(t, "POST", "/api/v1/users/"+user.LoginName+"/tokens", payload). AddBasicAuth(user.Name) @@ -563,8 +555,7 @@ func createAPIAccessTokenWithoutCleanUp(t *testing.T, tokenName string, user *us return newAccessToken } -// createAPIAccessTokenWithoutCleanUp Delete an API access token and assert that -// deletion succeeded. +// deleteAPIAccessToken deletes an API access token and assert that deletion succeeded. func deleteAPIAccessToken(t *testing.T, accessToken api.AccessToken, user *user_model.User) { req := NewRequestf(t, "DELETE", "/api/v1/users/"+user.LoginName+"/tokens/%d", accessToken.ID). AddBasicAuth(user.Name)