Revert "Fix reference token privilege escalation (#1540)" (#1571)

Author: reshmifrog

utils/config/tokenrefresh.go

@@ -2,13 +2,11 @@ package config
 
 import (
 	"errors"
-	"fmt"
 	"sync"
 	"time"
 
 	"github.com/jfrog/jfrog-client-go/access"
 	accessservices "github.com/jfrog/jfrog-client-go/access/services"
-	clientutils "github.com/jfrog/jfrog-client-go/utils"
 	"github.com/jfrog/jfrog-client-go/utils/errorutils"
 
 	"github.com/jfrog/jfrog-cli-core/v2/utils/coreutils"
@@ -185,56 +183,23 @@ func writeNewTokens(serverConfiguration *ServerDetails, serverId, accessToken, r
 }
 
 func createTokensForConfig(serverDetails *ServerDetails, expirySeconds int) (auth.CreateTokenResponseData, error) {
-	expiresIn := uint(max(expirySeconds, 0)) // #nosec G115 -- expirySeconds is validated positive by callers
-	createTokenParams := accessservices.CreateTokenParams{
-		CommonTokenParams: auth.CommonTokenParams{
-			Scope:       "applied-permissions/user",
-			ExpiresIn:   &expiresIn,
-			Refreshable: clientutils.Pointer(true),
-		},
-		Username: serverDetails.User,
-	}
-
-	// First, try with the original credentials (basic auth: user + password).
-	servicesManager, err := createAccessTokensServiceManager(serverDetails)
+	servicesManager, err := createArtifactoryTokensServiceManager(serverDetails)
 	if err != nil {
 		return auth.CreateTokenResponseData{}, err
 	}
-	newToken, err := servicesManager.CreateAccessToken(createTokenParams)
-	if err == nil {
-		return newToken, nil
-	}
 
-	// If basic auth failed and a password is available, retry using it as a Bearer token.
-	// This handles reference tokens, which the Access service can resolve server-side.
-	if serverDetails.Password != "" {
-		bearerDetails := serverDetailsForBearerAuth(serverDetails, serverDetails.Password)
-		servicesManager, err = createAccessTokensServiceManager(bearerDetails)
-		if err != nil {
-			return auth.CreateTokenResponseData{}, err
-		}
-		newToken, err = servicesManager.CreateAccessToken(createTokenParams)
-		if err == nil {
-			return newToken, nil
-		}
-		log.Debug("Access token creation with Bearer auth failed: " + err.Error())
-	}
+	createTokenParams := services.NewCreateTokenParams()
+	createTokenParams.Username = serverDetails.User
+	createTokenParams.ExpiresIn = expirySeconds
+	// User-scoped token
+	createTokenParams.Scope = "member-of-groups:*"
+	createTokenParams.Refreshable = true
 
-	return auth.CreateTokenResponseData{}, fmt.Errorf(
-		"automatic token creation via the Access API failed: %s. "+
-			"If your JFrog Platform version does not support the Access API, please upgrade. "+
-			"Alternatively, use the --basic-auth-only flag to skip automatic token creation",
-		err.Error())
-}
-
-// serverDetailsForBearerAuth returns a ServerDetails configured to authenticate using the
-// given token as a Bearer token, with user/password credentials cleared.
-func serverDetailsForBearerAuth(original *ServerDetails, token string) *ServerDetails {
-	details := *original
-	details.AccessToken = token
-	details.User = ""
-	details.Password = ""
-	return &details
+	newToken, err := servicesManager.CreateToken(createTokenParams)
+	if err != nil {
+		return auth.CreateTokenResponseData{}, err
+	}
+	return newToken, nil
 }
 
 func CreateInitialRefreshableTokensIfNeeded(serverDetails *ServerDetails) (err error) {
@@ -256,10 +221,9 @@ func CreateInitialRefreshableTokensIfNeeded(serverDetails *ServerDetails) (err e
 		return
 	}
 
-	newToken, tokenErr := createTokensForConfig(serverDetails, serverDetails.ArtifactoryTokenRefreshInterval*60)
-	if tokenErr != nil {
-		serverDetails.ArtifactoryTokenRefreshInterval = 0
-		return nil
+	newToken, err := createTokensForConfig(serverDetails, serverDetails.ArtifactoryTokenRefreshInterval*60)
+	if err != nil {
+		return
 	}
 	// Remove initializing value.
 	serverDetails.ArtifactoryTokenRefreshInterval = 0

utils/config/tokenrefresh_test.go

@@ -1,16 +1,10 @@
 package config
 
 import (
-	"encoding/json"
-	"net/http"
-	"net/http/httptest"
-	"strings"
-	"sync"
 	"testing"
 
 	configtests "github.com/jfrog/jfrog-cli-core/v2/utils/config/tests"
 	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
 )
 
 func TestCreateInitialRefreshableTokensIfNeededEarlyReturns(t *testing.T) {
@@ -202,11 +196,21 @@ func TestCreateInitialRefreshableTokensIfNeededValidInputs(t *testing.T) {
 
 			err = CreateInitialRefreshableTokensIfNeeded(serverDetailsCopy)
 
-			// With the Access API migration, token creation fails gracefully when no server is reachable:
-			// the function returns nil error and resets the interval to 0 (disabling token refresh).
-			assert.NoError(t, err, "Should not return error — graceful fallback to basic auth")
-			assert.Equal(t, 0, serverDetailsCopy.ArtifactoryTokenRefreshInterval,
-				"ArtifactoryTokenRefreshInterval should be reset to 0 after token creation attempt")
+			if tt.expectError {
+				assert.Error(t, err, "Expected an error but got none")
+			} else if err == nil {
+				// Note: This will fail if createTokensForConfig requires actual Artifactory connection
+				// In that case, this test would need to be an integration test with a mock server
+				assert.Equal(t, tt.expectedIntervalAfterCall, serverDetailsCopy.ArtifactoryTokenRefreshInterval,
+					"ArtifactoryTokenRefreshInterval should be reset to 0 after successful token creation")
+				// Verify tokens were set (if no error occurred)
+				// Note: This assumes createTokensForConfig succeeded
+				// In a real scenario, you'd need to mock the Artifactory service
+				if tt.shouldCreateTokens {
+					assert.NotEmpty(t, serverDetailsCopy.AccessToken, "AccessToken should be set after successful creation")
+					assert.NotEmpty(t, serverDetailsCopy.ArtifactoryRefreshToken, "ArtifactoryRefreshToken should be set after successful creation")
+				}
+			}
 		})
 	}
 }
@@ -302,11 +306,20 @@ func TestCreateInitialRefreshableTokensIfNeededInputValidation(t *testing.T) {
 			// 1. Return early (if conditions are met)
 			// 2. Attempt to create tokens and potentially fail due to invalid input
 			// We validate that the function handles the input gracefully
-			// With graceful fallback, token creation failures return nil error
-			assert.NoError(t, err, "Should not return error — graceful fallback")
-			if initialInterval > 0 {
-				assert.Equal(t, 0, serverDetailsCopy.ArtifactoryTokenRefreshInterval,
-					"Interval should be reset to 0 after token creation attempt")
+			if err != nil {
+				// If there's an error, it should be due to invalid configuration
+				// The interval should be reset to 0 if token creation was attempted
+				if serverDetailsCopy.ArtifactoryTokenRefreshInterval == 0 {
+					// Token creation was attempted but failed
+					assert.Error(t, err, tt.description)
+				}
+			} else {
+				// If no error, either early return occurred or tokens were created successfully
+				if initialInterval > 0 && serverDetailsCopy.ArtifactoryTokenRefreshInterval == 0 {
+					// Tokens were created successfully
+					assert.NotEmpty(t, serverDetailsCopy.AccessToken, "AccessToken should be set after successful creation")
+					assert.NotEmpty(t, serverDetailsCopy.ArtifactoryRefreshToken, "ArtifactoryRefreshToken should be set after successful creation")
+				}
 			}
 		})
 	}
@@ -557,156 +570,3 @@ func TestCreateInitialRefreshableTokensIfNeededBranchCoverage(t *testing.T) {
 		assert.NotNil(t, serverDetails)
 	})
 }
-
-func TestAccessAPITokenCreation_MockServer(t *testing.T) {
-	cleanUpTempEnv := configtests.CreateTempEnv(t, false)
-	defer cleanUpTempEnv()
-
-	var mu sync.Mutex
-	var accessAPIHit bool
-	var authMethod string
-
-	mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		if r.URL.Path == "/access/api/v1/tokens" && r.Method == http.MethodPost {
-			mu.Lock()
-			accessAPIHit = true
-			if _, _, ok := r.BasicAuth(); ok {
-				authMethod = "basic"
-			} else if strings.HasPrefix(r.Header.Get("Authorization"), "Bearer ") {
-				authMethod = "bearer"
-			}
-			mu.Unlock()
-			w.Header().Set("Content-Type", "application/json")
-			// #nosec G101 -- mock test credentials, not real tokens
-			resp := map[string]interface{}{
-				"access_token":  "mock-jwt-token",   // #nosec G101
-				"refresh_token": "mock-refresh-token", // #nosec G101
-				"expires_in":    3600,
-				"scope":         "applied-permissions/user",
-				"token_type":    "Bearer",
-			}
-			_ = json.NewEncoder(w).Encode(resp)
-			return
-		}
-		w.WriteHeader(http.StatusNotFound)
-	}))
-	defer mockServer.Close()
-
-	platformURL := mockServer.URL + "/"
-
-	serverDetails := &ServerDetails{
-		ServerId:                        "test-access-api",
-		ArtifactoryTokenRefreshInterval: 60,
-		ArtifactoryRefreshToken:         "",
-		AccessToken:                     "",
-		ArtifactoryUrl:                  mockServer.URL + "/artifactory/",
-		Url:                             platformURL,
-		User:                            "testuser",
-		Password:                        "my-regular-password",
-	}
-
-	err := SaveServersConf([]*ServerDetails{serverDetails})
-	require.NoError(t, err)
-
-	_ = CreateInitialRefreshableTokensIfNeeded(serverDetails)
-
-	mu.Lock()
-	assert.True(t, accessAPIHit,
-		"Token creation should use the Access API endpoint")
-	assert.Equal(t, "basic", authMethod,
-		"Regular password should use basic auth")
-	mu.Unlock()
-}
-
-func TestBasicAuthFailsFallsBackToBearer_MockServer(t *testing.T) {
-	cleanUpTempEnv := configtests.CreateTempEnv(t, false)
-	defer cleanUpTempEnv()
-
-	var mu sync.Mutex
-	var attempts []string
-
-	// #nosec G101 -- mock reference token, not a real credential
-	fakeRefToken := "cmVmdGtuOjAxOjE3NzczNTczOTI6ZmFrZVRva2VuRm9yVGVzdGluZ09ubHk="
-
-	mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		if r.URL.Path == "/access/api/v1/tokens" && r.Method == http.MethodPost {
-			mu.Lock()
-			if _, _, ok := r.BasicAuth(); ok {
-				attempts = append(attempts, "basic")
-				mu.Unlock()
-				w.WriteHeader(http.StatusUnauthorized)
-				return
-			} else if strings.HasPrefix(r.Header.Get("Authorization"), "Bearer ") {
-				attempts = append(attempts, "bearer")
-				mu.Unlock()
-				w.Header().Set("Content-Type", "application/json")
-				// #nosec G101 -- mock test credentials, not real tokens
-				resp := map[string]interface{}{
-					"access_token":  "mock-scoped-jwt",    // #nosec G101
-					"refresh_token": "mock-refresh-token",  // #nosec G101
-					"expires_in":    3600,
-					"scope":         "applied-permissions/user",
-					"token_type":    "Bearer",
-				}
-				_ = json.NewEncoder(w).Encode(resp)
-				return
-			}
-			mu.Unlock()
-		}
-		w.WriteHeader(http.StatusNotFound)
-	}))
-	defer mockServer.Close()
-
-	serverDetails := &ServerDetails{
-		ServerId:                        "test-retry-bearer",
-		ArtifactoryTokenRefreshInterval: 60,
-		ArtifactoryRefreshToken:         "",
-		AccessToken:                     "",
-		ArtifactoryUrl:                  mockServer.URL + "/artifactory/",
-		Url:                             mockServer.URL + "/",
-		User:                            "testuser",
-		Password:                        fakeRefToken,
-	}
-
-	err := SaveServersConf([]*ServerDetails{serverDetails})
-	require.NoError(t, err)
-
-	err = CreateInitialRefreshableTokensIfNeeded(serverDetails)
-	assert.NoError(t, err)
-
-	mu.Lock()
-	assert.Equal(t, []string{"basic", "bearer"}, attempts,
-		"Should first try basic auth, then fall back to bearer when basic fails")
-	mu.Unlock()
-}
-
-func TestAccessAPITokenCreationFails_MockServer(t *testing.T) {
-	cleanUpTempEnv := configtests.CreateTempEnv(t, false)
-	defer cleanUpTempEnv()
-
-	mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		w.WriteHeader(http.StatusUnauthorized)
-	}))
-	defer mockServer.Close()
-
-	serverDetails := &ServerDetails{
-		ServerId:                        "test-api-fail",
-		ArtifactoryTokenRefreshInterval: 60,
-		ArtifactoryRefreshToken:         "",
-		AccessToken:                     "",
-		ArtifactoryUrl:                  mockServer.URL + "/",
-		Url:                             mockServer.URL + "/",
-		User:                            "testuser",
-		Password:                        "my-regular-password",
-	}
-
-	err := SaveServersConf([]*ServerDetails{serverDetails})
-	require.NoError(t, err)
-
-	err = CreateInitialRefreshableTokensIfNeeded(serverDetails)
-	assert.NoError(t, err, "Should not return error when Access API fails — gracefully falls back to basic auth")
-	assert.Equal(t, 0, serverDetails.ArtifactoryTokenRefreshInterval,
-		"Token refresh should be disabled after Access API failure")
-	assert.Empty(t, serverDetails.AccessToken,
-		"No access token should be set when Access API fails")
-}