From 449f0d0ac3fc746763225f0ab70c21c09c2ca7dc Mon Sep 17 00:00:00 2001 From: LaurentiuNiculae Date: Thu, 4 May 2023 19:51:21 +0300 Subject: [PATCH] fix(repoinfo): fix userprefs values for repos returned by expanded repo info (#1413) - now isBookmarked and isStarred are updated correctly Signed-off-by: Laurentiu Niculae --- pkg/extensions/search/resolver.go | 2 +- pkg/extensions/search/resolver_test.go | 2 +- pkg/extensions/search/search_test.go | 2 +- pkg/extensions/search/userprefs_test.go | 192 ++++++++++++++++++ .../repodb/boltdb-wrapper/boltdb_wrapper.go | 30 +++ .../boltdb-wrapper/boltdb_wrapper_test.go | 24 +++ .../repodb/dynamodb-wrapper/dynamo_test.go | 49 +++++ .../repodb/dynamodb-wrapper/dynamo_wrapper.go | 39 +++- pkg/meta/repodb/repodb.go | 4 + pkg/meta/repodb/repodb_test.go | 29 +++ pkg/test/mocks/repo_db_mock.go | 10 + 11 files changed, 377 insertions(+), 6 deletions(-) diff --git a/pkg/extensions/search/resolver.go b/pkg/extensions/search/resolver.go index 510b35b7..f3add25e 100644 --- a/pkg/extensions/search/resolver.go +++ b/pkg/extensions/search/resolver.go @@ -1096,7 +1096,7 @@ func expandedRepoInfo(ctx context.Context, repo string, repoDB repodb.RepoDB, cv return &gql_generated.RepoInfo{}, nil //nolint:nilerr // don't give details to a potential attacker } - repoMeta, err := repoDB.GetRepoMeta(repo) + repoMeta, err := repoDB.GetUserRepoMeta(ctx, repo) if err != nil { log.Error().Err(err).Str("repository", repo).Msg("resolver: can't retrieve repoMeta for repo") diff --git a/pkg/extensions/search/resolver_test.go b/pkg/extensions/search/resolver_test.go index 003cc965..fb631317 100644 --- a/pkg/extensions/search/resolver_test.go +++ b/pkg/extensions/search/resolver_test.go @@ -3274,7 +3274,7 @@ func TestExpandedRepoInfo(t *testing.T) { graphql.DefaultRecover) repoDB := mocks.RepoDBMock{ - GetRepoMetaFn: func(repo string) (repodb.RepoMetadata, error) { + GetUserRepoMetaFn: func(ctx context.Context, repo string) (repodb.RepoMetadata, error) { return repodb.RepoMetadata{ Tags: map[string]repodb.Descriptor{ "tagManifest": { diff --git a/pkg/extensions/search/search_test.go b/pkg/extensions/search/search_test.go index edae8955..cea9d648 100644 --- a/pkg/extensions/search/search_test.go +++ b/pkg/extensions/search/search_test.go @@ -4699,7 +4699,7 @@ func TestRepoDBIndexOperations(t *testing.T) { func RunRepoDBIndexTests(baseURL, port string) { Convey("Push test index", func() { - repo := "repo" + const repo = "repo" multiarchImage, err := GetRandomMultiarchImage("tag1") So(err, ShouldBeNil) diff --git a/pkg/extensions/search/userprefs_test.go b/pkg/extensions/search/userprefs_test.go index d61a710c..754a8e0e 100644 --- a/pkg/extensions/search/userprefs_test.go +++ b/pkg/extensions/search/userprefs_test.go @@ -806,6 +806,198 @@ func TestGlobalSearchWithUserPrefFiltering(t *testing.T) { }) } +func TestExpandedRepoInfoWithUserPrefs(t *testing.T) { + Convey("ExpandedRepoInfo with User Prefs", t, func() { + dir := t.TempDir() + port := GetFreePort() + baseURL := GetBaseURL(port) + conf := config.New() + conf.HTTP.Port = port + conf.Storage.RootDirectory = dir + + simpleUser := "simpleUser" + simpleUserPassword := "simpleUserPass" + credTests := fmt.Sprintf("%s\n\n", getCredString(simpleUser, simpleUserPassword)) + + htpasswdPath := MakeHtpasswdFileFromString(credTests) + defer os.Remove(htpasswdPath) + + conf.HTTP.Auth = &config.AuthConfig{ + HTPasswd: config.AuthHTPasswd{ + Path: htpasswdPath, + }, + } + + conf.HTTP.AccessControl = &config.AccessControlConfig{ + Repositories: config.Repositories{ + "**": config.PolicyGroup{ + Policies: []config.Policy{ + { + Users: []string{simpleUser}, + Actions: []string{"read", "create"}, + }, + }, + }, + }, + } + + defaultVal := true + conf.Extensions = &extconf.ExtensionConfig{ + Search: &extconf.SearchConfig{BaseConfig: extconf.BaseConfig{Enable: &defaultVal}}, + } + + ctlr := api.NewController(conf) + + ctlrManager := NewControllerManager(ctlr) + ctlrManager.StartAndWait(port) + defer ctlrManager.StopServer() + + preferencesBaseURL := baseURL + constants.FullUserPreferencesPrefix + simpleUserClient := resty.R().SetBasicAuth(simpleUser, simpleUserPassword) + + // ------ Add sbrepo and star/bookmark it + sbrepo := "sbrepo" + img, err := GetRandomImage("tag") + So(err, ShouldBeNil) + err = UploadImageWithBasicAuth(img, baseURL, sbrepo, simpleUser, simpleUserPassword) + So(err, ShouldBeNil) + + resp, err := simpleUserClient.Put(preferencesBaseURL + PutRepoStarURL(sbrepo)) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + So(err, ShouldBeNil) + + resp, err = simpleUserClient.Put(preferencesBaseURL + PutRepoBookmarkURL(sbrepo)) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + So(err, ShouldBeNil) + + // ExpandedRepoinfo + + query := ` + { + ExpandedRepoInfo(repo:"sbrepo"){ + Summary { + Name IsStarred IsBookmarked + } + } + }` + + resp, err = simpleUserClient.Get(baseURL + graphqlQueryPrefix + "?query=" + url.QueryEscape(query)) + So(resp, ShouldNotBeNil) + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, 200) + + responseStruct := ExpandedRepoInfoResp{} + + err = json.Unmarshal(resp.Body(), &responseStruct) + So(err, ShouldBeNil) + + repoInfo := responseStruct.ExpandedRepoInfo.RepoInfo + So(repoInfo.Summary.IsBookmarked, ShouldBeTrue) + So(repoInfo.Summary.IsStarred, ShouldBeTrue) + + // ------ Add srepo and star it + srepo := "srepo" + img, err = GetRandomImage("tag") + So(err, ShouldBeNil) + err = UploadImageWithBasicAuth(img, baseURL, srepo, simpleUser, simpleUserPassword) + So(err, ShouldBeNil) + + resp, err = simpleUserClient.Put(preferencesBaseURL + PutRepoStarURL(srepo)) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + So(err, ShouldBeNil) + + // ExpandedRepoinfo + query = ` + { + ExpandedRepoInfo(repo:"srepo"){ + Summary { + Name IsStarred IsBookmarked + } + } + }` + + resp, err = simpleUserClient.Get(baseURL + graphqlQueryPrefix + "?query=" + url.QueryEscape(query)) + So(resp, ShouldNotBeNil) + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, 200) + + responseStruct = ExpandedRepoInfoResp{} + + err = json.Unmarshal(resp.Body(), &responseStruct) + So(err, ShouldBeNil) + + repoInfo = responseStruct.ExpandedRepoInfo.RepoInfo + So(repoInfo.Summary.IsBookmarked, ShouldBeFalse) + So(repoInfo.Summary.IsStarred, ShouldBeTrue) + + // ------ Add brepo and bookmark it + brepo := "brepo" + img, err = GetRandomImage("tag") + So(err, ShouldBeNil) + err = UploadImageWithBasicAuth(img, baseURL, brepo, simpleUser, simpleUserPassword) + So(err, ShouldBeNil) + + resp, err = simpleUserClient.Put(preferencesBaseURL + PutRepoBookmarkURL(brepo)) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + So(err, ShouldBeNil) + + // ExpandedRepoinfo + query = ` + { + ExpandedRepoInfo(repo:"brepo"){ + Summary { + Name IsStarred IsBookmarked + } + } + }` + + resp, err = simpleUserClient.Get(baseURL + graphqlQueryPrefix + "?query=" + url.QueryEscape(query)) + So(resp, ShouldNotBeNil) + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, 200) + + responseStruct = ExpandedRepoInfoResp{} + + err = json.Unmarshal(resp.Body(), &responseStruct) + So(err, ShouldBeNil) + + repoInfo = responseStruct.ExpandedRepoInfo.RepoInfo + So(repoInfo.Summary.IsBookmarked, ShouldBeTrue) + So(repoInfo.Summary.IsStarred, ShouldBeFalse) + + // ------ Add repo without star/bookmark + repo := "repo" + img, err = GetRandomImage("tag") + So(err, ShouldBeNil) + err = UploadImageWithBasicAuth(img, baseURL, repo, simpleUser, simpleUserPassword) + So(err, ShouldBeNil) + + // ExpandedRepoinfo + query = ` + { + ExpandedRepoInfo(repo:"repo"){ + Summary { + Name IsStarred IsBookmarked + } + } + }` + + resp, err = simpleUserClient.Get(baseURL + graphqlQueryPrefix + "?query=" + url.QueryEscape(query)) + So(resp, ShouldNotBeNil) + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, 200) + + responseStruct = ExpandedRepoInfoResp{} + + err = json.Unmarshal(resp.Body(), &responseStruct) + So(err, ShouldBeNil) + + repoInfo = responseStruct.ExpandedRepoInfo.RepoInfo + So(repoInfo.Summary.IsBookmarked, ShouldBeFalse) + So(repoInfo.Summary.IsStarred, ShouldBeFalse) + }) +} + func PutRepoStarURL(repo string) string { return fmt.Sprintf("?repo=%s&action=toggleStar", repo) } diff --git a/pkg/meta/repodb/boltdb-wrapper/boltdb_wrapper.go b/pkg/meta/repodb/boltdb-wrapper/boltdb_wrapper.go index cbb37dc0..1963d1f6 100644 --- a/pkg/meta/repodb/boltdb-wrapper/boltdb_wrapper.go +++ b/pkg/meta/repodb/boltdb-wrapper/boltdb_wrapper.go @@ -533,6 +533,36 @@ func (bdw *DBWrapper) GetRepoMeta(repo string) (repodb.RepoMetadata, error) { return repoMeta, err } +func (bdw *DBWrapper) GetUserRepoMeta(ctx context.Context, repo string) (repodb.RepoMetadata, error) { + var repoMeta repodb.RepoMetadata + + err := bdw.DB.Update(func(tx *bbolt.Tx) error { + buck := tx.Bucket([]byte(bolt.RepoMetadataBucket)) + userBookmarks := getUserBookmarks(ctx, tx) + userStars := getUserStars(ctx, tx) + + repoMetaBlob := buck.Get([]byte(repo)) + + // object not found + if repoMetaBlob == nil { + return zerr.ErrRepoMetaNotFound + } + + // object found + err := json.Unmarshal(repoMetaBlob, &repoMeta) + if err != nil { + return err + } + + repoMeta.IsBookmarked = zcommon.Contains(userBookmarks, repo) + repoMeta.IsStarred = zcommon.Contains(userStars, repo) + + return nil + }) + + return repoMeta, err +} + func (bdw *DBWrapper) SetRepoMeta(repo string, repoMeta repodb.RepoMetadata) error { err := bdw.DB.Update(func(tx *bbolt.Tx) error { buck := tx.Bucket([]byte(bolt.RepoMetadataBucket)) diff --git a/pkg/meta/repodb/boltdb-wrapper/boltdb_wrapper_test.go b/pkg/meta/repodb/boltdb-wrapper/boltdb_wrapper_test.go index 44b90b05..4b03d1b4 100644 --- a/pkg/meta/repodb/boltdb-wrapper/boltdb_wrapper_test.go +++ b/pkg/meta/repodb/boltdb-wrapper/boltdb_wrapper_test.go @@ -922,6 +922,30 @@ func TestWrapperErrors(t *testing.T) { ) So(err, ShouldBeNil) }) + + Convey("GetUserRepoMeta unmarshal error", func() { + acCtx := localCtx.AccessControlContext{ + ReadGlobPatterns: map[string]bool{ + "repo": true, + }, + Username: "username", + } + authzCtxKey := localCtx.GetContextKey() + ctx := context.WithValue(context.Background(), authzCtxKey, acCtx) + + err = boltdbWrapper.DB.Update(func(tx *bbolt.Tx) error { + repoBuck := tx.Bucket([]byte(bolt.RepoMetadataBucket)) + + err := repoBuck.Put([]byte("repo"), []byte("bad repo")) + So(err, ShouldBeNil) + + return nil + }) + So(err, ShouldBeNil) + + _, err := boltdbWrapper.GetUserRepoMeta(ctx, "repo") + So(err, ShouldNotBeNil) + }) }) } diff --git a/pkg/meta/repodb/dynamodb-wrapper/dynamo_test.go b/pkg/meta/repodb/dynamodb-wrapper/dynamo_test.go index bfbe1b51..349713b4 100644 --- a/pkg/meta/repodb/dynamodb-wrapper/dynamo_test.go +++ b/pkg/meta/repodb/dynamodb-wrapper/dynamo_test.go @@ -1037,6 +1037,55 @@ func TestWrapperErrors(t *testing.T) { err := dynamoWrapper.PatchDB() So(err, ShouldNotBeNil) }) + + Convey("GetUserRepoMeta client.GetItem error", func() { + dynamoWrapper.RepoMetaTablename = badTablename + + _, err = dynamoWrapper.GetUserRepoMeta(ctx, "repo") + So(err, ShouldNotBeNil) + }) + + Convey("GetUserRepoMeta repoMeta not found", func() { + _, err = dynamoWrapper.GetUserRepoMeta(ctx, "unknown-repo-meta") + So(err, ShouldNotBeNil) + }) + + Convey("GetUserRepoMeta userMeta not found", func() { + err := dynamoWrapper.SetRepoReference("repo", "tag", digest.FromString("1"), ispec.MediaTypeImageManifest) + So(err, ShouldBeNil) + dynamoWrapper.UserDataTablename = badTablename + + acCtx := localCtx.AccessControlContext{ + ReadGlobPatterns: map[string]bool{ + "repo": true, + }, + Username: "username", + } + + authzCtxKey := localCtx.GetContextKey() + ctx := context.WithValue(context.Background(), authzCtxKey, acCtx) + + _, err = dynamoWrapper.GetUserRepoMeta(ctx, "repo") + So(err, ShouldNotBeNil) + }) + + Convey("GetUserRepoMeta unmarshal error", func() { + err := setBadRepoMeta(dynamoWrapper.Client, repoMetaTablename, "repo") + So(err, ShouldBeNil) + + acCtx := localCtx.AccessControlContext{ + ReadGlobPatterns: map[string]bool{ + "repo": true, + }, + Username: "username", + } + + authzCtxKey := localCtx.GetContextKey() + ctx := context.WithValue(context.Background(), authzCtxKey, acCtx) + + _, err = dynamoWrapper.GetUserRepoMeta(ctx, "repo") + So(err, ShouldNotBeNil) + }) }) Convey("NewDynamoDBWrapper errors", t, func() { diff --git a/pkg/meta/repodb/dynamodb-wrapper/dynamo_wrapper.go b/pkg/meta/repodb/dynamodb-wrapper/dynamo_wrapper.go index 4c623339..82267ae0 100644 --- a/pkg/meta/repodb/dynamodb-wrapper/dynamo_wrapper.go +++ b/pkg/meta/repodb/dynamodb-wrapper/dynamo_wrapper.go @@ -616,6 +616,39 @@ func (dwr *DBWrapper) GetRepoMeta(repo string) (repodb.RepoMetadata, error) { return repoMeta, nil } +func (dwr *DBWrapper) GetUserRepoMeta(ctx context.Context, repo string) (repodb.RepoMetadata, error) { + resp, err := dwr.Client.GetItem(ctx, &dynamodb.GetItemInput{ + TableName: aws.String(dwr.RepoMetaTablename), + Key: map[string]types.AttributeValue{ + "RepoName": &types.AttributeValueMemberS{Value: repo}, + }, + }) + if err != nil { + return repodb.RepoMetadata{}, err + } + + if resp.Item == nil { + return repodb.RepoMetadata{}, zerr.ErrRepoMetaNotFound + } + + var repoMeta repodb.RepoMetadata + + err = attributevalue.Unmarshal(resp.Item["RepoMetadata"], &repoMeta) + if err != nil { + return repodb.RepoMetadata{}, err + } + + userMeta, err := dwr.GetUserMeta(ctx) + if err != nil { + return repodb.RepoMetadata{}, err + } + + repoMeta.IsBookmarked = zcommon.Contains(userMeta.BookmarkedRepos, repo) + repoMeta.IsStarred = zcommon.Contains(userMeta.StarredRepos, repo) + + return repoMeta, nil +} + func (dwr *DBWrapper) IncrementImageDownloads(repo string, reference string) error { repoMeta, err := dwr.GetRepoMeta(repo) if err != nil { @@ -1926,7 +1959,7 @@ func (dwr *DBWrapper) GetStarredRepos(ctx context.Context) ([]string, error) { return userMeta.StarredRepos, err } -func (dwr DBWrapper) GetUserMeta(ctx context.Context) (repodb.UserData, error) { +func (dwr *DBWrapper) GetUserMeta(ctx context.Context) (repodb.UserData, error) { acCtx, err := localCtx.GetAccessControlContext(ctx) if err != nil { return repodb.UserData{}, err @@ -1963,7 +1996,7 @@ func (dwr DBWrapper) GetUserMeta(ctx context.Context) (repodb.UserData, error) { return userMeta, nil } -func (dwr DBWrapper) createUserDataTable() error { +func (dwr *DBWrapper) createUserDataTable() error { _, err := dwr.Client.CreateTable(context.Background(), &dynamodb.CreateTableInput{ TableName: aws.String(dwr.UserDataTablename), AttributeDefinitions: []types.AttributeDefinition{ @@ -1988,7 +2021,7 @@ func (dwr DBWrapper) createUserDataTable() error { return dwr.waitTableToBeCreated(dwr.UserDataTablename) } -func (dwr DBWrapper) SetUserMeta(ctx context.Context, userMeta repodb.UserData) error { +func (dwr *DBWrapper) SetUserMeta(ctx context.Context, userMeta repodb.UserData) error { acCtx, err := localCtx.GetAccessControlContext(ctx) if err != nil { return err diff --git a/pkg/meta/repodb/repodb.go b/pkg/meta/repodb/repodb.go index 0bf0c304..d8d3be76 100644 --- a/pkg/meta/repodb/repodb.go +++ b/pkg/meta/repodb/repodb.go @@ -47,6 +47,10 @@ type RepoDB interface { //nolint:interfacebloat // GetRepoMeta returns RepoMetadata of a repo from the database GetRepoMeta(repo string) (RepoMetadata, error) + // GetUserRepometa return RepoMetadata of a repo from the database along side specific information about the + // user + GetUserRepoMeta(ctx context.Context, repo string) (RepoMetadata, error) + // GetRepoMeta returns RepoMetadata of a repo from the database SetRepoMeta(repo string, repoMeta RepoMetadata) error diff --git a/pkg/meta/repodb/repodb_test.go b/pkg/meta/repodb/repodb_test.go index 6b856925..aac0e012 100644 --- a/pkg/meta/repodb/repodb_test.go +++ b/pkg/meta/repodb/repodb_test.go @@ -2511,6 +2511,35 @@ func RunRepoDBTests(repoDB repodb.RepoDB, preparationFuncs ...func() error) { So(repoMetas[0].IsBookmarked, ShouldBeTrue) So(repoMetas[0].IsStarred, ShouldBeTrue) }) + + Convey("Test GetUserRepoMeta", func() { + authzCtxKey := localCtx.GetContextKey() + + acCtx := localCtx.AccessControlContext{ + ReadGlobPatterns: map[string]bool{ + "repo": true, + }, + Username: "user1", + } + ctx := context.WithValue(context.Background(), authzCtxKey, acCtx) + + digest := godigest.FromString("1") + + err := repoDB.SetRepoReference("repo", "tag", digest, ispec.MediaTypeImageManifest) + So(err, ShouldBeNil) + + _, err = repoDB.ToggleBookmarkRepo(ctx, "repo") + So(err, ShouldBeNil) + + _, err = repoDB.ToggleStarRepo(ctx, "repo") + So(err, ShouldBeNil) + + repoMeta, err := repoDB.GetUserRepoMeta(ctx, "repo") + So(err, ShouldBeNil) + So(repoMeta.IsBookmarked, ShouldBeTrue) + So(repoMeta.IsStarred, ShouldBeTrue) + So(repoMeta.Tags, ShouldContainKey, "tag") + }) }) } diff --git a/pkg/test/mocks/repo_db_mock.go b/pkg/test/mocks/repo_db_mock.go index bb2e08a5..a83ebd37 100644 --- a/pkg/test/mocks/repo_db_mock.go +++ b/pkg/test/mocks/repo_db_mock.go @@ -25,6 +25,8 @@ type RepoDBMock struct { GetRepoMetaFn func(repo string) (repodb.RepoMetadata, error) + GetUserRepoMetaFn func(ctx context.Context, repo string) (repodb.RepoMetadata, error) + SetRepoMetaFn func(repo string, repoMeta repodb.RepoMetadata) error GetMultipleRepoMetaFn func(ctx context.Context, filter func(repoMeta repodb.RepoMetadata) bool, @@ -155,6 +157,14 @@ func (sdm RepoDBMock) GetRepoMeta(repo string) (repodb.RepoMetadata, error) { return repodb.RepoMetadata{}, nil } +func (sdm RepoDBMock) GetUserRepoMeta(ctx context.Context, repo string) (repodb.RepoMetadata, error) { + if sdm.GetUserRepoMetaFn != nil { + return sdm.GetUserRepoMetaFn(ctx, repo) + } + + return repodb.RepoMetadata{}, nil +} + func (sdm RepoDBMock) SetRepoMeta(repo string, repoMeta repodb.RepoMetadata) error { if sdm.SetRepoMetaFn != nil { return sdm.SetRepoMetaFn(repo, repoMeta)