Add dependency injection of remote store for NotaryRepository initial…

…ization

Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>

client/client.go

@@ -17,7 +17,6 @@ import (
 	"github.com/docker/notary/client/changelist"
 	"github.com/docker/notary/cryptoservice"
 	store "github.com/docker/notary/storage"
-	"github.com/docker/notary/trustmanager"
 	"github.com/docker/notary/trustpinning"
 	"github.com/docker/notary/tuf"
 	"github.com/docker/notary/tuf/data"
@@ -44,6 +43,7 @@ type NotaryRepository struct {
 	baseURL        string
 	tufRepoPath    string
 	cache          store.MetadataStore
+	remoteStore    store.RemoteStore
 	CryptoService  signed.CryptoService
 	tufRepo        *tuf.Repo
 	invalid        *tuf.Repo // known data that was parsable but deemed invalid
@@ -53,7 +53,9 @@ type NotaryRepository struct {
 }
 
 // NewFileCachedNotaryRepository is a wrapper for NewNotaryRepository that initializes
-// a file cache from the provided repository and local config information
+// a file cache from the provided repository, local config information and a crypto service.
+// It also retrieves the remote store associated to the base directory under where all the
+// trust files will be stored and the specified GUN.
 func NewFileCachedNotaryRepository(baseDir, gun, baseURL string, rt http.RoundTripper,
 	retriever notary.PassRetriever, trustPinning trustpinning.TrustPinConfig) (
 	*NotaryRepository, error) {
@@ -65,47 +67,44 @@ func NewFileCachedNotaryRepository(baseDir, gun, baseURL string, rt http.RoundTr
 	if err != nil {
 		return nil, err
 	}
-	return NewNotaryRepository(baseDir, gun, baseURL, rt, cache, retriever, trustPinning)
-}
-
-// NewNotaryRepository is a helper method that returns a new notary repository.
-// It takes the base directory under where all the trust files will be stored
-// (This is normally defaults to "~/.notary" or "~/.docker/trust" when enabling
-// docker content trust).
-func NewNotaryRepository(baseDir, gun, baseURL string, rt http.RoundTripper, cache store.MetadataStore,
-	retriever notary.PassRetriever, trustPinning trustpinning.TrustPinConfig) (
-	*NotaryRepository, error) {
 
 	keyStores, err := getKeyStores(baseDir, retriever)
 	if err != nil {
 		return nil, err
 	}
 
-	return repositoryFromKeystores(baseDir, gun, baseURL, rt, cache,
-		keyStores, trustPinning)
-}
+	cryptoService := cryptoservice.NewCryptoService(keyStores...)
 
-// repositoryFromKeystores is a helper function for NewNotaryRepository that
-// takes some basic NotaryRepository parameters as well as keystores (in order
-// of usage preference), and returns a NotaryRepository.
-func repositoryFromKeystores(baseDir, gun, baseURL string, rt http.RoundTripper, cache store.MetadataStore,
-	keyStores []trustmanager.KeyStore, trustPin trustpinning.TrustPinConfig) (*NotaryRepository, error) {
+	remoteStore, err := getRemoteStore(baseURL, gun, rt)
+	if err != nil {
+		return nil, err
+	}
 
-	cryptoService := cryptoservice.NewCryptoService(keyStores...)
+	return NewNotaryRepository(baseDir, gun, baseURL, remoteStore, rt, cache, trustPinning, cryptoService)
+}
+
+// NewNotaryRepository is the base method that returns a new notary repository.
+// It takes the base directory under where all the trust files will be stored
+// (This is normally defaults to "~/.notary" or "~/.docker/trust" when enabling
+// docker content trust).
+// It expects initiated remote store and cache.
+func NewNotaryRepository(baseDir, gun, baseURL string, remoteStore store.RemoteStore, rt http.RoundTripper, cache store.MetadataStore,
+	trustPinning trustpinning.TrustPinConfig, cryptoService signed.CryptoService) (
+	*NotaryRepository, error) {
 
 	nRepo := &NotaryRepository{
 		gun:            gun,
 		baseDir:        baseDir,
 		baseURL:        baseURL,
 		tufRepoPath:    filepath.Join(baseDir, tufDir, filepath.FromSlash(gun)),
+		cache:          cache,
+		remoteStore:    remoteStore,
 		CryptoService:  cryptoService,
 		roundTrip:      rt,
-		trustPinning:   trustPin,
+		trustPinning:   trustPinning,
 		LegacyVersions: 0, // By default, don't sign with legacy roles
 	}
 
-	nRepo.cache = cache
-
 	return nRepo, nil
 }
 
@@ -686,10 +685,7 @@ func (r *NotaryRepository) publish(cl changelist.Changelist) error {
 		return err
 	}
 
-	remote, err := getRemoteStore(r.baseURL, r.gun, r.roundTrip)
-	if err != nil {
-		return err
-	}
+	remote := r.remoteStore
 
 	return remote.SetMulti(updatedFiles)
 }
@@ -970,10 +966,8 @@ func (r *NotaryRepository) bootstrapClient(checkInitialized bool) (*TUFClient, e
 		}
 	}
 
-	remote, remoteErr := getRemoteStore(r.baseURL, r.gun, r.roundTrip)
-	if remoteErr != nil {
-		logrus.Error(remoteErr)
-	} else if !newBuilder.IsLoaded(data.CanonicalRootRole) || checkInitialized {
+	remote := r.remoteStore
+	if !newBuilder.IsLoaded(data.CanonicalRootRole) || checkInitialized {
 		// remoteErr was nil and we were not able to load a root from cache or
 		// are specifically checking for initialization of the repo.
 
@@ -1146,13 +1140,25 @@ func (r *NotaryRepository) DeleteTrustData(deleteRemote bool) error {
 	}
 	// Note that this will require admin permission in this NotaryRepository's roundtripper
 	if deleteRemote {
-		remote, err := getRemoteStore(r.baseURL, r.gun, r.roundTrip)
-		if err != nil {
-			return err
-		}
+		remote := r.remoteStore
 		if err := remote.RemoveAll(); err != nil {
 			return err
 		}
 	}
 	return nil
 }
+
+// SetBaseURL updates the repository's base URL and its associated remote store
+func (r *NotaryRepository) setBaseURL(baseURL string) error {
+	// We try to retrieve the remote store before updating the repository
+	remoteStore, err := getRemoteStore(baseURL, r.gun, r.roundTrip)
+	if err != nil {
+		return err
+	}
+
+	// Update the repository
+	r.baseURL = baseURL
+	r.remoteStore = remoteStore
+
+	return nil
+}

client/client_test.go

@@ -3509,10 +3509,10 @@ func TestBootstrapClientBadURL(t *testing.T) {
 	require.EqualError(t, err, err2.Error())
 }
 
-// TestBootstrapClientInvalidURL checks that bootstrapClient correctly
-// returns an error when the URL is valid but does not point to
+// TestClientInvalidURL checks that instantiating a new NotaryRepository
+// correctly returns an error when the URL is valid but does not point to
 // a TUF server
-func TestBootstrapClientInvalidURL(t *testing.T) {
+func TestClientInvalidURL(t *testing.T) {
 	tempBaseDir, err := ioutil.TempDir("", "notary-test-")
 	require.NoError(t, err, "failed to create a temporary directory: %s", err)
 	repo, err := NewFileCachedNotaryRepository(
@@ -3523,20 +3523,11 @@ func TestBootstrapClientInvalidURL(t *testing.T) {
 		passphraseRetriever,
 		trustpinning.TrustPinConfig{},
 	)
-	require.NoError(t, err, "error creating repo: %s", err)
-
-	c, err := repo.bootstrapClient(false)
-	require.Nil(t, c)
+	// NewFileCachedNotaryRepository should fail and return an error
+	// since it initializes the cache but also the remote repository
+	// from the baseURL and the GUN
+	require.Nil(t, repo)
 	require.Error(t, err)
-
-	c, err2 := repo.bootstrapClient(true)
-	require.Nil(t, c)
-	require.Error(t, err2)
-
-	// same error should be returned because we don't have local data
-	// and are requesting remote root regardless of checkInitialized
-	// value
-	require.EqualError(t, err, err2.Error())
 }
 
 func TestPublishTargetsDelegationCanUseUserKeyWithArbitraryRole(t *testing.T) {
@@ -3705,8 +3696,7 @@ func TestDeleteRemoteRepo(t *testing.T) {
 	requireRepoHasExpectedKeys(t, repo, rootKeyID, true)
 
 	// Try connecting to the remote store directly and make sure that no metadata exists for this gun
-	remoteStore, err := getRemoteStore(repo.baseURL, repo.gun, repo.roundTrip)
-	require.NoError(t, err)
+	remoteStore := repo.remoteStore
 	meta, err := remoteStore.GetSized(data.CanonicalRootRole, store.NoSizeLimit)
 	require.Error(t, err)
 	require.IsType(t, store.ErrMetaNotFound{}, err)
@@ -3731,8 +3721,7 @@ func TestDeleteRemoteRepo(t *testing.T) {
 	require.Len(t, longLivingCl.List(), 1)
 
 	// Check that the other repo's remote data is unaffected
-	remoteStore, err = getRemoteStore(longLivingRepo.baseURL, longLivingRepo.gun, longLivingRepo.roundTrip)
-	require.NoError(t, err)
+	remoteStore = longLivingRepo.remoteStore
 	meta, err = remoteStore.GetSized(data.CanonicalRootRole, store.NoSizeLimit)
 	require.NoError(t, err)
 	require.NotNil(t, meta)
@@ -3746,9 +3735,11 @@ func TestDeleteRemoteRepo(t *testing.T) {
 	require.NoError(t, err)
 	require.NotNil(t, meta)
 
-	// Try deleting again with an invalid server URL
-	repo.baseURL = "invalid"
-	require.Error(t, repo.DeleteTrustData(true))
+	// Try setting an invalid server URL before deleting
+	require.Error(t, repo.setBaseURL("invalid"))
+
+	// Try deleting again the remote
+	require.NoError(t, repo.DeleteTrustData(true))
 }
 
 // Test that we get a correct list of roles with keys and signatures