Fix release repo path construction

utils/paths.go

@@ -301,25 +301,24 @@ func ToURI(path string) string {
 	return u.String()
 }
 
-func GetReleasesRemoteDetails(artifact, downloadPath, remoteRepo string, remoteServerDetails *config.ServerDetails) (server *config.ServerDetails, fullRemotePath string, err error) {
+// For a configured remote repository, the resource is expected under '<repo>/artifactory/' (a remote that proxies https://releases.jfrog.io).
+// fallbackRemotePath is '<repo>/' directly (a remote that proxies https://releases.jfrog.io/artifactory, or a repository holding the resources in its root). It is empty when there is no fallback.
+func GetReleasesRemoteDetails(artifact, downloadPath, remoteRepo string, remoteServerDetails *config.ServerDetails) (server *config.ServerDetails, remotePath, fallbackRemotePath string, err error) {
 	if remoteServerDetails != nil && remoteRepo != "" {
 		// Config profile server and repo details are provided
 		server = remoteServerDetails
-		fullRemotePath = path.Join(remoteRepo, downloadPath)
-		return
-	}
-	// Try to get releases remote details from environment variable
-	server, remoteRepo, err = dependencies.GetRemoteDetails(coreutils.ReleasesRemoteEnv)
-	if err != nil {
-		return
+	} else {
+		// Try to get releases remote details from environment variable
+		if server, remoteRepo, err = dependencies.GetRemoteDetails(coreutils.ReleasesRemoteEnv); err != nil {
+			return
+		}
 	}
-	if remoteRepo != "" {
-		fullRemotePath = path.Join(remoteRepo, "artifactory", downloadPath)
-		return
+	if remoteRepo == "" {
+		log.Debug(fmt.Sprintf("'"+coreutils.ReleasesRemoteEnv+"' environment variable is not configured. The '%s' will be downloaded directly from releases.jfrog.io if needed.", artifact))
+		// If not configured to download through a remote repository in Artifactory, download from releases.jfrog.io.
+		return &config.ServerDetails{ArtifactoryUrl: coreutils.JfrogReleasesUrl}, downloadPath, "", nil
 	}
-	log.Debug(fmt.Sprintf("'"+coreutils.ReleasesRemoteEnv+"' environment variable is not configured. The '%s' will be downloaded directly from releases.jfrog.io if needed.", artifact))
-	// If not configured to download through a remote repository in Artifactory, download from releases.jfrog.io.
-	return &config.ServerDetails{ArtifactoryUrl: coreutils.JfrogReleasesUrl}, downloadPath, nil
+	return server, path.Join(remoteRepo, "artifactory", downloadPath), path.Join(remoteRepo, downloadPath), nil
 }
 
 // RepositoryScansListLink is a deep link into Xray's Scans List for one artifact.

utils/paths_test.go

@@ -5,8 +5,17 @@ import (
 	"path/filepath"
 	"testing"
 
+	commonCommands "github.com/jfrog/jfrog-cli-core/v2/common/commands"
+	"github.com/jfrog/jfrog-cli-core/v2/utils/config"
+	"github.com/jfrog/jfrog-cli-core/v2/utils/coreutils"
 	"github.com/jfrog/jfrog-client-go/utils/log"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+const (
+	downloadPath = "xray-scan-lib/xray-scan-lib-1.6.0-linux-amd64.tar.gz"
+	envServerId  = "envServer"
 )
 
 func TestIsPathExcludedFromRoot(t *testing.T) {
@@ -163,3 +172,50 @@ func TestGetFullPathsWorkingDirs(t *testing.T) {
 		})
 	}
 }
+
+func TestGetReleasesRemoteDetails(t *testing.T) {
+	profileServer := &config.ServerDetails{ArtifactoryUrl: "https://profile.example.com/artifactory/"}
+	envServer := &config.ServerDetails{ServerId: envServerId, Url: "https://env.example.com/", ArtifactoryUrl: "https://env.example.com/artifactory/"}
+	t.Setenv(coreutils.HomeDir, t.TempDir())
+	require.NoError(t, commonCommands.NewConfigCommand(commonCommands.AddOrEdit, envServerId).SetDetails(envServer).SetInteractive(false).SetEncPassword(false).Run())
+	testCases := []struct {
+		name                       string
+		remoteRepo                 string
+		remoteServer               *config.ServerDetails
+		envValue                   string
+		expectedServerUrl          string
+		expectedRemotePath         string
+		expectedFallbackRemotePath string
+	}{
+		{
+			name:                       "Config profile remote repository",
+			remoteRepo:                 "releases-remote",
+			remoteServer:               profileServer,
+			expectedServerUrl:          profileServer.ArtifactoryUrl,
+			expectedRemotePath:         "releases-remote/artifactory/" + downloadPath,
+			expectedFallbackRemotePath: "releases-remote/" + downloadPath,
+		},
+		{
+			name:                       "Env var remote repository",
+			envValue:                   envServerId + "/releases-remote",
+			expectedServerUrl:          envServer.ArtifactoryUrl,
+			expectedRemotePath:         "releases-remote/artifactory/" + downloadPath,
+			expectedFallbackRemotePath: "releases-remote/" + downloadPath,
+		},
+		{
+			name:               "Not configured, download from releases.jfrog.io",
+			expectedServerUrl:  coreutils.JfrogReleasesUrl,
+			expectedRemotePath: downloadPath,
+		},
+	}
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			t.Setenv(coreutils.ReleasesRemoteEnv, testCase.envValue)
+			server, remotePath, fallbackRemotePath, err := GetReleasesRemoteDetails("Resource", downloadPath, testCase.remoteRepo, testCase.remoteServer)
+			require.NoError(t, err)
+			assert.Equal(t, testCase.expectedServerUrl, server.ArtifactoryUrl)
+			assert.Equal(t, testCase.expectedRemotePath, remotePath)
+			assert.Equal(t, testCase.expectedFallbackRemotePath, fallbackRemotePath)
+		})
+	}
+}

utils/utils.go

@@ -8,6 +8,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"net/http"
 	"os"
 	"path/filepath"
 	"regexp"
@@ -456,7 +457,7 @@ type RemoteResource struct {
 
 // Get the details of the resource from the JPD / Releases remote it is downloaded from.
 func GetRemoteResource(resourceName, downloadPath, remoteRepo string, remoteServerDetails *config.ServerDetails) (*RemoteResource, error) {
-	rtDetails, remotePath, err := GetReleasesRemoteDetails(resourceName, downloadPath, remoteRepo, remoteServerDetails)
+	rtDetails, remotePath, fallbackRemotePath, err := GetReleasesRemoteDetails(resourceName, downloadPath, remoteRepo, remoteServerDetails)
 	if err != nil {
 		return nil, err
 	}
@@ -466,12 +467,25 @@ func GetRemoteResource(resourceName, downloadPath, remoteRepo string, remoteServ
 	}
 	downloadUrl := rtDetails.ArtifactoryUrl + remotePath
 	remoteFileDetails, _, err := client.GetRemoteFileDetails(downloadUrl, &httpClientDetails)
-	if err != nil {
+	if err != nil && fallbackRemotePath != "" && isLayoutMismatchError(err) {
+		fallbackUrl := rtDetails.ArtifactoryUrl + fallbackRemotePath
+		var fallbackErr error
+		if remoteFileDetails, _, fallbackErr = client.GetRemoteFileDetails(fallbackUrl, &httpClientDetails); fallbackErr != nil {
+			return nil, fmt.Errorf("couldn't get remote file details for %s: %w; and for %s: %w", downloadUrl, err, fallbackUrl, fallbackErr)
+		}
+		remotePath = fallbackRemotePath
+	} else if err != nil {
 		return nil, fmt.Errorf("couldn't get remote file details for %s: %s", downloadUrl, err.Error())
 	}
 	return &RemoteResource{name: resourceName, serverDetails: rtDetails, path: remotePath, details: remoteFileDetails}, nil
 }
 
+// A wrong repository layout returns 404, or 403 when the user's permissions cover only specific folders of the repository.
+func isLayoutMismatchError(err error) bool {
+	var httpErr *errorutils.HttpResponseError
+	return errors.As(err, &httpErr) && (httpErr.StatusCode == http.StatusNotFound || httpErr.StatusCode == http.StatusForbidden)
+}
+
 // Returns true when targetDir already holds the latest version of the resource.
 func (rr *RemoteResource) IsCached(targetDir string) (bool, error) {
 	return isArtifactChecksumsMatch(rr.details, filepath.Join(targetDir, dependencies.ChecksumFileName), true)

utils/utils_test.go

@@ -11,8 +11,11 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"os"
+	"path"
 	"path/filepath"
 	"slices"
+	"strings"
+	"sync"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -299,6 +302,97 @@ func TestRemoteResourceDownloadEncrypted(t *testing.T) {
 	}
 }
 
+func TestGetRemoteResourceFallsBackToPathWithoutArtifactory(t *testing.T) {
+	const (
+		repoPrefix         = "/artifactory/" + resourcesRemoteRepo + "/"
+		withArtifactory    = repoPrefix + "artifactory/" + encryptedArchivePath
+		withoutArtifactory = repoPrefix + encryptedArchivePath
+	)
+	testCases := []struct {
+		name             string
+		statusByPath     map[string]int
+		expectedPath     string
+		expectedRequests []string
+		expectedErrUrls  []string
+		expectedErrCodes []string
+	}{
+		{
+			name:             "Found under artifactory",
+			statusByPath:     map[string]int{withArtifactory: http.StatusOK},
+			expectedPath:     path.Join(resourcesRemoteRepo, "artifactory", encryptedArchivePath),
+			expectedRequests: []string{withArtifactory},
+		},
+		{
+			name:             "Not found under artifactory, found in root",
+			statusByPath:     map[string]int{withoutArtifactory: http.StatusOK},
+			expectedPath:     path.Join(resourcesRemoteRepo, encryptedArchivePath),
+			expectedRequests: []string{withArtifactory, withoutArtifactory},
+		},
+		{
+			name:             "Not found in both",
+			statusByPath:     map[string]int{},
+			expectedRequests: []string{withArtifactory, withoutArtifactory},
+			expectedErrUrls:  []string{withArtifactory, withoutArtifactory},
+			expectedErrCodes: []string{"404", "404"},
+		},
+		{
+			name:             "Forbidden under artifactory, found in root",
+			statusByPath:     map[string]int{withArtifactory: http.StatusForbidden, withoutArtifactory: http.StatusOK},
+			expectedPath:     path.Join(resourcesRemoteRepo, encryptedArchivePath),
+			expectedRequests: []string{withArtifactory, withoutArtifactory},
+		},
+		{
+			name:             "Unauthorized under artifactory is not retried",
+			statusByPath:     map[string]int{withArtifactory: http.StatusUnauthorized, withoutArtifactory: http.StatusOK},
+			expectedRequests: []string{withArtifactory},
+			expectedErrUrls:  []string{withArtifactory},
+			expectedErrCodes: []string{"401"},
+		},
+		{
+			name:             "Both errors are reported when the fallback fails differently",
+			statusByPath:     map[string]int{withoutArtifactory: http.StatusForbidden},
+			expectedRequests: []string{withArtifactory, withoutArtifactory},
+			expectedErrUrls:  []string{withArtifactory, withoutArtifactory},
+			expectedErrCodes: []string{"404", "403"},
+		},
+	}
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			var mutex sync.Mutex
+			var requests []string
+			mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				mutex.Lock()
+				requests = append(requests, r.URL.Path)
+				mutex.Unlock()
+				status, exists := testCase.statusByPath[r.URL.Path]
+				if !exists {
+					status = http.StatusNotFound
+				}
+				w.Header().Set("X-Checksum-Sha256", "checksum")
+				w.WriteHeader(status)
+			}))
+			defer mockServer.Close()
+
+			resource, err := GetRemoteResource("Resource", encryptedArchivePath, resourcesRemoteRepo, &config.ServerDetails{ArtifactoryUrl: mockServer.URL + "/artifactory/"})
+
+			assert.Equal(t, testCase.expectedRequests, requests)
+			if len(testCase.expectedErrCodes) > 0 {
+				require.Error(t, err)
+				for _, expectedCode := range testCase.expectedErrCodes {
+					assert.Contains(t, err.Error(), expectedCode)
+				}
+				for _, expectedUrl := range testCase.expectedErrUrls {
+					assert.Contains(t, err.Error(), mockServer.URL+expectedUrl)
+				}
+				assert.Equal(t, len(testCase.expectedErrUrls), strings.Count(err.Error(), mockServer.URL))
+				return
+			}
+			require.NoError(t, err)
+			assert.Equal(t, testCase.expectedPath, resource.path)
+		})
+	}
+}
+
 func createZip(t *testing.T, fileName, content string) []byte {
 	var buffer bytes.Buffer
 	zipWriter := zip.NewWriter(&buffer)