diff --git a/services/proxy/pkg/command/server.go b/services/proxy/pkg/command/server.go index 2a9a5cc0ba..12adf8c6ce 100644 --- a/services/proxy/pkg/command/server.go +++ b/services/proxy/pkg/command/server.go @@ -104,6 +104,7 @@ func Server(cfg *config.Config) *cobra.Command { InsecureSkipVerify: cfg.OIDC.Insecure, //nolint:gosec }, DisableKeepAlives: true, + Proxy: http.ProxyFromEnvironment, }, Timeout: time.Second * 10, } @@ -279,6 +280,7 @@ func loadMiddlewares(logger log.Logger, cfg *config.Config, InsecureSkipVerify: cfg.OIDC.Insecure, //nolint:gosec }, DisableKeepAlives: true, + Proxy: http.ProxyFromEnvironment, }, Timeout: time.Second * 10, } diff --git a/services/proxy/pkg/middleware/authentication.go b/services/proxy/pkg/middleware/authentication.go index 4c7b879699..b314b0c670 100644 --- a/services/proxy/pkg/middleware/authentication.go +++ b/services/proxy/pkg/middleware/authentication.go @@ -50,6 +50,10 @@ type Authenticator interface { Authenticate(*http.Request) (*http.Request, bool) } +type authenticationChallengeSuppressor interface { + SuppressAuthenticationChallenge(*http.Request) bool +} + // Authentication is a higher order authentication middleware. func Authentication(auths []Authenticator, opts ...Option) func(next http.Handler) http.Handler { options := newOptions(opts...) @@ -76,15 +80,19 @@ func Authentication(auths []Authenticator, opts ...Option) func(next http.Handle return } + suppressAuthenticationChallenge := false for _, a := range auths { if req, ok := a.Authenticate(r); ok { span.End() next.ServeHTTP(w, req) return } + if suppressor, ok := a.(authenticationChallengeSuppressor); ok && suppressor.SuppressAuthenticationChallenge(r) { + suppressAuthenticationChallenge = true + } } - if !isPublicPath(r.URL.Path) { + if !suppressAuthenticationChallenge && !isPublicPath(r.URL.Path) { // Failed basic authentication attempts receive the Www-Authenticate header in the response var touch bool caser := cases.Title(language.Und) @@ -103,8 +111,10 @@ func Authentication(auths []Authenticator, opts ...Option) func(next http.Handle } } - for _, s := range SupportedAuthStrategies { - userAgentAuthenticateLockIn(w, r, options.CredentialsByUserAgent, s) + if !suppressAuthenticationChallenge { + for _, s := range SupportedAuthStrategies { + userAgentAuthenticateLockIn(w, r, options.CredentialsByUserAgent, s) + } } w.WriteHeader(http.StatusUnauthorized) // if the request is a PROPFIND return a WebDAV error code. diff --git a/services/proxy/pkg/middleware/signed_url_auth.go b/services/proxy/pkg/middleware/signed_url_auth.go index 1c62202360..1acf93d369 100644 --- a/services/proxy/pkg/middleware/signed_url_auth.go +++ b/services/proxy/pkg/middleware/signed_url_auth.go @@ -65,6 +65,16 @@ func (m SignedURLAuthenticator) shouldServe(req *http.Request) bool { return req.URL.Query().Get(_paramOCJWTSig) != "" } +// SuppressAuthenticationChallenge prevents other authentication mechanisms from challenging signed URL clients. +// Requests that carry an Authorization header are not suppressed, so clients that additionally sent +// (possibly stale) credentials still receive a challenge and can re-authenticate. +func (m SignedURLAuthenticator) SuppressAuthenticationChallenge(req *http.Request) bool { + if req.Header.Get("Authorization") != "" { + return false + } + return m.shouldServeLegacy(req) || m.shouldServe(req) +} + func (m SignedURLAuthenticator) validate(req *http.Request) (err error) { query := req.URL.Query() diff --git a/services/proxy/pkg/middleware/signed_url_auth_test.go b/services/proxy/pkg/middleware/signed_url_auth_test.go index bdaecd8072..f16be89d5f 100644 --- a/services/proxy/pkg/middleware/signed_url_auth_test.go +++ b/services/proxy/pkg/middleware/signed_url_auth_test.go @@ -10,9 +10,13 @@ import ( userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" "github.com/opencloud-eu/opencloud/pkg/log" "github.com/opencloud-eu/opencloud/services/proxy/pkg/config" + "github.com/opencloud-eu/opencloud/services/proxy/pkg/router" + "github.com/opencloud-eu/opencloud/services/proxy/pkg/user/backend" + backendmocks "github.com/opencloud-eu/opencloud/services/proxy/pkg/user/backend/mocks" revactx "github.com/opencloud-eu/reva/v2/pkg/ctx" "github.com/opencloud-eu/reva/v2/pkg/signedurl" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "go-micro.dev/v4/store" ) @@ -94,6 +98,88 @@ func TestSignedURLAuth_authenticateRejectsDisallowedMethods(t *testing.T) { } } +func TestSignedURLAuthFailureSuppressesAuthenticationChallenge(t *testing.T) { + oldStrategies := SupportedAuthStrategies + SupportedAuthStrategies = nil + t.Cleanup(func() { SupportedAuthStrategies = oldStrategies }) + + verifier, err := signedurl.NewJWTSignedURL(signedurl.WithSecret("secret")) + if err != nil { + t.Fatalf("failed to create signed URL verifier: %v", err) + } + userProvider := &backendmocks.UserBackend{} + userProvider.On("GetUserByClaims", mock.Anything, "username", "").Return(nil, "", backend.ErrAccountNotFound) + + tests := []struct { + name string + url string + authenticator SignedURLAuthenticator + expectedChallenge bool + }{ + { + name: "invalid signed URL", + url: "https://example.com/file?oc-jwt-sig=invalid", + authenticator: SignedURLAuthenticator{ + Logger: log.NewLogger(), + PreSignedURLConfig: config.PreSignedURL{AllowedHTTPMethods: []string{http.MethodGet}}, + URLVerifier: verifier, + }, + expectedChallenge: false, + }, + { + name: "invalid legacy signed URL", + url: "https://example.com/file?OC-Signature=invalid", + authenticator: SignedURLAuthenticator{ + Logger: log.NewLogger(), + PreSignedURLConfig: config.PreSignedURL{Enabled: true}, + UserProvider: userProvider, + }, + expectedChallenge: false, + }, + { + name: "legacy signed URLs disabled", + url: "https://example.com/file?OC-Signature=invalid", + authenticator: SignedURLAuthenticator{}, + expectedChallenge: true, + }, + { + name: "signed URL verifier disabled", + url: "https://example.com/file?oc-jwt-sig=invalid", + authenticator: SignedURLAuthenticator{}, + expectedChallenge: true, + }, + { + name: "unsigned URL", + url: "https://example.com/file", + authenticator: SignedURLAuthenticator{URLVerifier: verifier}, + expectedChallenge: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + SupportedAuthStrategies = nil + req := httptest.NewRequest(http.MethodGet, tt.url, nil) + req = req.WithContext(router.SetRoutingInfo(req.Context(), router.RoutingInfo{})) + rr := httptest.NewRecorder() + nextCalled := false + handler := Authentication([]Authenticator{tt.authenticator}, EnableBasicAuth(true))( + http.HandlerFunc(func(http.ResponseWriter, *http.Request) { nextCalled = true }), + ) + + handler.ServeHTTP(rr, req) + + assert.Equal(t, http.StatusUnauthorized, rr.Code) + assert.False(t, nextCalled) + if tt.expectedChallenge { + assert.NotEmpty(t, rr.Header().Values(WwwAuthenticate)) + } else { + assert.Empty(t, rr.Header().Values(WwwAuthenticate)) + } + }) + } +} + func TestSignedURLAuth_allRequiredParametersPresent(t *testing.T) { pua := SignedURLAuthenticator{} baseURL := "https://example.com/example.jpg?" diff --git a/services/search/MIGRATION.md b/services/search/MIGRATION.md index 5bd8c48c3b..332ad9d893 100644 --- a/services/search/MIGRATION.md +++ b/services/search/MIGRATION.md @@ -13,7 +13,7 @@ Fill the new index by indexing all spaces again: ```shell # the service keeps running while it happens -opencloud search index --all-spaces +opencloud search index --all-spaces --insecure ``` Once the new index is filled, every index but the one with the highest @@ -31,7 +31,7 @@ The new index is a directory next to the old `bleve` one, both in bleve index cannot be copied, index all spaces again: ```shell -opencloud search index --all-spaces +opencloud search index --all-spaces --insecure ``` Once the new index is filled, every directory but the one with the highest diff --git a/services/search/README.md b/services/search/README.md index 96bdc00c21..d0f650e528 100644 --- a/services/search/README.md +++ b/services/search/README.md @@ -124,14 +124,14 @@ opencloud search index --space $SPACE_ID It can also be used to re-index all spaces: ```shell -opencloud search index --all-spaces +opencloud search index --all-spaces --insecure ``` Please note that a reindex only picks up new or changed files. Files that have already been indexed are not scanned again, even if the configuration or the whole extractor has been changed. To force a full rescan (re-running the extractor on every file) you need to use the `force-rescan` flag: ```shell -opencloud search index --all-spaces --force-rescan +opencloud search index --all-spaces --force-rescan --insecure ``` ## Metrics diff --git a/services/search/pkg/bleve/backend.go b/services/search/pkg/bleve/backend.go index 7fc6fe5bfa..d8426e5435 100644 --- a/services/search/pkg/bleve/backend.go +++ b/services/search/pkg/bleve/backend.go @@ -75,14 +75,10 @@ func (b *Backend) Search(_ context.Context, sir *searchService.SearchIndexReques }, ) // Scope below the space root: restrict at query level so totals and - // paging respect the path too. Path is a case-preserving keyword - // (paths act as references, /Foo and /foo are distinct), so the exact - // folder or the folder prefix matches all of, and only, the scope. + // paging respect the path too. The folder term matches the folder and + // its descendants (see PathAnalyzer). if requestedPath := utils.MakeRelativePath(sir.Ref.Path); requestedPath != "." { - q.Conjuncts = append(q.Conjuncts, query.NewDisjunctionQuery([]query.Query{ - &query.TermQuery{FieldVal: "Path", Term: requestedPath}, - &query.PrefixQuery{FieldVal: "Path", Prefix: requestedPath + "/"}, - })) + q.Conjuncts = append(q.Conjuncts, &query.TermQuery{FieldVal: "Path", Term: requestedPath}) } } diff --git a/services/search/pkg/bleve/bleve.go b/services/search/pkg/bleve/bleve.go index 1bbf53857d..6c244a542a 100644 --- a/services/search/pkg/bleve/bleve.go +++ b/services/search/pkg/bleve/bleve.go @@ -1,8 +1,6 @@ package bleve import ( - "regexp" - bleveSearch "github.com/blevesearch/bleve/v2/search" storageProvider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" @@ -11,8 +9,6 @@ import ( "github.com/opencloud-eu/opencloud/services/search/pkg/search" ) -var queryEscape = regexp.MustCompile(`([` + regexp.QuoteMeta(`+=&|> ".", "./a", "./a/b" with +// delimiter "/", one level per byte without. tag_depth prepends "/". +const Name = "hierarchy" + +type Tokenizer struct { + delimiter []byte + tagDepth bool +} + +func (t *Tokenizer) Tokenize(input []byte) analysis.TokenStream { + if len(input) == 0 { + return nil + } + var out analysis.TokenStream + emit := func(depth, end int) { + term := input[:end] + if t.tagDepth { + term = strconv.AppendInt(make([]byte, 0, end+4), int64(depth), 10) + term = append(term, '/') + term = append(term, input[:end]...) + } + out = append(out, &analysis.Token{ + Term: term, + Position: depth, + Start: 0, + End: end, + Type: analysis.AlphaNumeric, + }) + } + + if len(t.delimiter) == 0 { + for i := range input { + emit(i+1, i+1) + } + return out + } + + depth := 0 + for start := 0; start <= len(input); { + i := bytes.Index(input[start:], t.delimiter) + if i < 0 { + if start < len(input) { + depth++ + emit(depth, len(input)) + } + break + } + if i > 0 { + depth++ + emit(depth, start+i) + } + start += i + len(t.delimiter) + } + return out +} + +func Constructor(config map[string]interface{}, _ *registry.Cache) (analysis.Tokenizer, error) { + t := &Tokenizer{} + if d, ok := config["delimiter"].(string); ok { + t.delimiter = []byte(d) + } + if v, ok := config["tag_depth"].(bool); ok { + t.tagDepth = v + } + return t, nil +} + +func init() { + if err := registry.RegisterTokenizer(Name, Constructor); err != nil { + panic(err) + } +} diff --git a/services/search/pkg/bleve/hierarchy/hierarchy_suite_test.go b/services/search/pkg/bleve/hierarchy/hierarchy_suite_test.go new file mode 100644 index 0000000000..a74f37fdd0 --- /dev/null +++ b/services/search/pkg/bleve/hierarchy/hierarchy_suite_test.go @@ -0,0 +1,13 @@ +package hierarchy_test + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestHierarchy(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "hierarchy tokenizer") +} diff --git a/services/search/pkg/bleve/hierarchy/hierarchy_test.go b/services/search/pkg/bleve/hierarchy/hierarchy_test.go new file mode 100644 index 0000000000..6bc8eaec31 --- /dev/null +++ b/services/search/pkg/bleve/hierarchy/hierarchy_test.go @@ -0,0 +1,56 @@ +package hierarchy_test + +import ( + "github.com/blevesearch/bleve/v2/analysis" + "github.com/blevesearch/bleve/v2/registry" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "github.com/opencloud-eu/opencloud/services/search/pkg/bleve/hierarchy" +) + +func terms(ts analysis.TokenStream) []string { + out := make([]string, 0, len(ts)) + for _, t := range ts { + out = append(out, string(t.Term)) + } + return out +} + +func tokenize(config map[string]any, input string) []string { + tok, err := hierarchy.Constructor(config, registry.NewCache()) + Expect(err).ToNot(HaveOccurred()) + return terms(tok.Tokenize([]byte(input))) +} + +var _ = Describe("hierarchy tokenizer", func() { + path := map[string]any{"delimiter": "/"} + geohash := map[string]any{"tag_depth": true} + + DescribeTable("emits every prefix up to a level boundary", + func(config map[string]any, input string, want []string) { + Expect(tokenize(config, input)).To(Equal(want)) + }, + Entry("relative path", path, "./a/b.txt", []string{".", "./a", "./a/b.txt"}), + Entry("space root", path, ".", []string{"."}), + Entry("trailing delimiter is not a level", path, "./a/", []string{".", "./a"}), + Entry("delimiter only", path, "/", []string{}), + Entry("leading delimiter", path, "/abs/x", []string{"/abs", "/abs/x"}), + Entry("double delimiter", path, "./a//b", []string{".", "./a", "./a//b"}), + Entry("spaces and special characters stay literal", path, "./odd name*[1]/f:x?.txt", + []string{".", "./odd name*[1]", "./odd name*[1]/f:x?.txt"}), + Entry("empty input", path, "", []string{}), + Entry("geohash, one level per byte, depth tagged", geohash, "u4pru", + []string{"1/u", "2/u4", "3/u4p", "4/u4pr", "5/u4pru"}), + ) + + It("keeps byte offsets on the source value", func() { + tok, err := hierarchy.Constructor(path, registry.NewCache()) + Expect(err).ToNot(HaveOccurred()) + ts := tok.Tokenize([]byte("./a/b")) + Expect(ts).To(HaveLen(3)) + Expect(ts[2].Start).To(Equal(0)) + Expect(ts[2].End).To(Equal(5)) + Expect(ts[2].Position).To(Equal(3)) + }) +}) diff --git a/services/search/pkg/bleve/index.go b/services/search/pkg/bleve/index.go index fd4d50e0d6..e72754b814 100644 --- a/services/search/pkg/bleve/index.go +++ b/services/search/pkg/bleve/index.go @@ -19,6 +19,7 @@ import ( storageProvider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" "github.com/opencloud-eu/opencloud/pkg/log" + "github.com/opencloud-eu/opencloud/services/search/pkg/bleve/hierarchy" searchmapping "github.com/opencloud-eu/opencloud/services/search/pkg/mapping" "github.com/opencloud-eu/opencloud/services/search/pkg/search" ) @@ -208,6 +209,42 @@ func NewMapping() (mapping.IndexMapping, error) { if err != nil { return nil, err } + // path: every ancestor prefix is a term, so one term query matches a folder + // and all of its descendants + err = indexMapping.AddCustomTokenizer("path_hierarchy", map[string]any{ + "type": hierarchy.Name, + "delimiter": "/", + }) + if err != nil { + return nil, err + } + err = indexMapping.AddCustomAnalyzer(searchmapping.PathAnalyzer, map[string]any{ + "type": custom.Name, + "tokenizer": "path_hierarchy", + }) + if err != nil { + return nil, err + } + // geohash: every prefix is a depth-tagged term (1/u, 2/u4, ...), so a terms + // facet with TermPrefix "/" is a geohash grid at that precision. + // No field uses it yet. It is part of the v5 schema so that #3272 can add + // its geohash field additively: new fields reconcile at startup, a changed + // analysis block does not (classifyStoredMapping), so the names and the + // config below must not change. + err = indexMapping.AddCustomTokenizer("geohash_hierarchy", map[string]any{ + "type": hierarchy.Name, + "tag_depth": true, + }) + if err != nil { + return nil, err + } + err = indexMapping.AddCustomAnalyzer("geohash", map[string]any{ + "type": custom.Name, + "tokenizer": "geohash_hierarchy", + }) + if err != nil { + return nil, err + } return indexMapping, nil } @@ -226,11 +263,15 @@ func searchResourceByID(id string, index bleve.Index) (*search.Resource, error) return matchToResource(res.Hits[0]), nil } +// searchResourcesByPath returns the descendants of the folder at lookupPath. +// The folder term matches the folder and everything below it in one term +// query (see PathAnalyzer); the folder itself is dropped from the result. func searchResourcesByPath(rootID string, lookupPath string, index bleve.Index) ([]*search.Resource, error) { - q := bleve.NewConjunctionQuery( - bleve.NewQueryStringQuery("RootID:"+rootID), - bleve.NewQueryStringQuery("Path:"+escapeQuery(lookupPath+"/*")), - ) + rootQuery := bleve.NewTermQuery(rootID) + rootQuery.SetField("RootID") + pathQuery := bleve.NewTermQuery(lookupPath) + pathQuery.SetField("Path") + q := bleve.NewConjunctionQuery(rootQuery, pathQuery) bleveReq := bleve.NewSearchRequest(q) bleveReq.Size = math.MaxInt bleveReq.Fields = []string{"*"} @@ -241,7 +282,11 @@ func searchResourcesByPath(rootID string, lookupPath string, index bleve.Index) resources := make([]*search.Resource, 0, res.Hits.Len()) for _, match := range res.Hits { - resources = append(resources, matchToResource(match)) + resource := matchToResource(match) + if resource.Path == lookupPath { + continue + } + resources = append(resources, resource) } return resources, nil diff --git a/services/search/pkg/bleve/testdata/mapping.golden.json b/services/search/pkg/bleve/testdata/mapping.golden.json index 61ea6474f1..73f6baa748 100644 --- a/services/search/pkg/bleve/testdata/mapping.golden.json +++ b/services/search/pkg/bleve/testdata/mapping.golden.json @@ -160,7 +160,7 @@ "fields": [ { "type": "text", - "analyzer": "keyword", + "analyzer": "path_hierarchy", "store": true, "index": true, "include_term_vectors": true, @@ -1259,7 +1259,25 @@ "type": "regexp" } }, + "tokenizers": { + "geohash_hierarchy": { + "tag_depth": true, + "type": "hierarchy" + }, + "path_hierarchy": { + "delimiter": "/", + "type": "hierarchy" + } + }, "analyzers": { + "geohash": { + "tokenizer": "geohash_hierarchy", + "type": "custom" + }, + "path_hierarchy": { + "tokenizer": "path_hierarchy", + "type": "custom" + }, "words": { "char_filters": [ "dot_to_space" diff --git a/services/search/pkg/mapping/bleve.go b/services/search/pkg/mapping/bleve.go index 0a3a55c45b..5c45faa2ef 100644 --- a/services/search/pkg/mapping/bleve.go +++ b/services/search/pkg/mapping/bleve.go @@ -60,7 +60,6 @@ func buildBleveDocMapping(t reflect.Type, overrides map[string]FieldOpts, prefix } if fieldType == TypeKeyword || fieldType == TypePath { - // bleve has no path tokenizer, so a path is a plain keyword here. base := bleveKeywordMapping(fieldType, opts) doc.AddFieldMappingsAt(fi.Name, base) if opts.caseInsensitive() { @@ -84,8 +83,9 @@ func buildBleveDocMapping(t reflect.Type, overrides map[string]FieldOpts, prefix return doc, err } -// bleveKeywordMapping is a case-preserving keyword field; path fields stay out -// of _all by default. +// bleveKeywordMapping is a case-preserving keyword field; path fields are +// analyzed into their ancestor prefixes (see PathAnalyzer) and stay out of +// _all by default. func bleveKeywordMapping(fieldType string, opts FieldOpts) *bleveMapping.FieldMapping { fm := bleve.NewKeywordFieldMapping() switch { @@ -94,6 +94,9 @@ func bleveKeywordMapping(fieldType string, opts FieldOpts) *bleveMapping.FieldMa case fieldType == TypePath: fm.IncludeInAll = false } + if fieldType == TypePath { + fm.Analyzer = PathAnalyzer + } return fm } diff --git a/services/search/pkg/mapping/opensearch.go b/services/search/pkg/mapping/opensearch.go index ddd6249b6f..30c358a482 100644 --- a/services/search/pkg/mapping/opensearch.go +++ b/services/search/pkg/mapping/opensearch.go @@ -61,7 +61,7 @@ func buildOpenSearchProperties(t reflect.Type, overrides map[string]FieldOpts, p // path_hierarchy is case-preserving here; casing lives in the value. m := map[string]any{"type": "keyword"} if fieldType == TypePath { - m = map[string]any{"type": "text", "analyzer": "path_hierarchy"} + m = map[string]any{"type": "text", "analyzer": PathAnalyzer} } props[fi.Name] = m if opts.caseInsensitive() { diff --git a/services/search/pkg/mapping/opensearch_test.go b/services/search/pkg/mapping/opensearch_test.go index cfe1dea8a6..b8369a5976 100644 --- a/services/search/pkg/mapping/opensearch_test.go +++ b/services/search/pkg/mapping/opensearch_test.go @@ -100,8 +100,8 @@ var _ = Describe("OpenSearchBuildMapping", func() { Expect(content["term_vector"]).To(Equal("with_positions_offsets"), "Content: %#v", content) Expect(content["analyzer"]).To(Equal(WordsAnalyzer), "Content uses the words analyzer, like bleve") // Path: path_hierarchy base + lowercased sibling, both case-preserving. - Expect(props["Path"]).To(Equal(map[string]any{"type": "text", "analyzer": "path_hierarchy"})) - Expect(props["Path_lowercase"]).To(Equal(map[string]any{"type": "text", "analyzer": "path_hierarchy"})) + Expect(props["Path"]).To(Equal(map[string]any{"type": "text", "analyzer": PathAnalyzer})) + Expect(props["Path_lowercase"]).To(Equal(map[string]any{"type": "text", "analyzer": PathAnalyzer})) mime := props["MimeType"].(map[string]any) Expect(mime["type"]).To(Equal("wildcard"), "MimeType: %#v", mime) }) diff --git a/services/search/pkg/mapping/opts.go b/services/search/pkg/mapping/opts.go index d8af5f17bf..07ae324b04 100644 --- a/services/search/pkg/mapping/opts.go +++ b/services/search/pkg/mapping/opts.go @@ -26,6 +26,11 @@ const WordsSuffix = "_words" // WordsAnalyzer names the analyzer both engines register for the words sibling. const WordsAnalyzer = "words" +// PathAnalyzer names the analyzer both engines register for TypePath fields: +// every ancestor prefix of a path is a term, so one term query matches a +// folder and its descendants. +const PathAnalyzer = "path_hierarchy" + // FieldOpts overrides the default type inference for a struct field. Keys in // the override map are json-tag names (e.g. "Name", "location", "audio.artist"), // not Go field names. diff --git a/services/search/pkg/mapping/reconcile.go b/services/search/pkg/mapping/reconcile.go index 82cc46feb6..7490998a67 100644 --- a/services/search/pkg/mapping/reconcile.go +++ b/services/search/pkg/mapping/reconcile.go @@ -27,7 +27,7 @@ func Reconcile(index string, r SchemaReconciler, logger log.Logger) (Classificat case VerdictAdditive: persisted, err := r.ApplyAdditive() if persisted { - logger.Warn().Strs("fields", classification.NewFields).Str("index", index).Msg("extended the search index mapping with new fields; documents indexed before the upgrade do not contain them and queries on these fields will miss those documents until they are re-indexed; to re-index everything run: opencloud search index --all-spaces --force-rescan") + logger.Warn().Strs("fields", classification.NewFields).Str("index", index).Msg("extended the search index mapping with new fields; documents indexed before the upgrade do not contain them and queries on these fields will miss those documents until they are re-indexed; to re-index everything run: opencloud search index --all-spaces --force-rescan --insecure") } if err != nil { return classification, err @@ -40,5 +40,5 @@ func Reconcile(index string, r SchemaReconciler, logger log.Logger) (Classificat // LogNewIndexCreated logs that a fresh, empty index was created and how to // backfill it. The create path does not run through Reconcile. func LogNewIndexCreated(logger log.Logger, index string) { - logger.Info().Str("index", index).Msg("created a new empty search index; if this OpenCloud instance already held files, they are not in it yet, index them by running: opencloud search index --all-spaces --force-rescan") + logger.Info().Str("index", index).Msg("created a new empty search index; if this OpenCloud instance already held files, they are not in it yet, index them by running: opencloud search index --all-spaces --force-rescan --insecure") } diff --git a/services/search/pkg/opensearch/index.go b/services/search/pkg/opensearch/index.go index e79e053aa0..b68b71f302 100644 --- a/services/search/pkg/opensearch/index.go +++ b/services/search/pkg/opensearch/index.go @@ -82,7 +82,7 @@ func buildResourceMapping() ([]byte, error) { "analysis": map[string]any{ // path_hierarchy is case-preserving; casing lives in the value. "analyzer": map[string]any{ - "path_hierarchy": map[string]any{ + searchmapping.PathAnalyzer: map[string]any{ "type": "custom", "tokenizer": "path_hierarchy", }, diff --git a/services/search/pkg/parity/README.md b/services/search/pkg/parity/README.md index 8b3691c710..6558e96847 100644 --- a/services/search/pkg/parity/README.md +++ b/services/search/pkg/parity/README.md @@ -530,12 +530,22 @@ Fixtures: - `parent`, ID = 1$1!2, folder - `child.pdf`, ID = 1$1!3, Path = ./parent/child.pdf +- `big`, ID = 1$1!4, folder +- `f1.txt`, ID = 1$1!5, Path = ./big/f1.txt +- `x.txt`, ID = 1$1!6, Path = ./big2/x.txt +- `odd name (1)`, ID = 1$1!7, folder +- `f:x+y.txt`, ID = 1$1!8, Path = ./odd name (1)/f:x+y.txt | Case | Query | expected | bleve | OpenSearch | same? | |---|---|---|---|---|---| | MOVE-01 | carries the descendants to the new path, then `path:"./my/newname/child.pdf"` | child.pdf | child.pdf | child.pdf | ✅ | | MOVE-01 | carries the descendants to the new path, then `path:"./parent/child.pdf"` | no match | no match | no match | ✅ | | MOVE-02 | through the trash and back leaves the flag behind, then `hidden:true` | no match | no match | no match | ✅ | +| MOVE-03 | leaves a sibling folder that shares the prefix alone, then `path:"./moved"` | f1.txt, moved | f1.txt, moved | f1.txt, moved | ✅ | +| MOVE-03 | leaves a sibling folder that shares the prefix alone, then `path:"./big"` | no match | no match | no match | ✅ | +| MOVE-03 | leaves a sibling folder that shares the prefix alone, then `path:"./big2"` | x.txt | x.txt | x.txt | ✅ | +| MOVE-04 | carries the descendants of a path with special characters, then `path:"./odd name (2)"` | f:x+y.txt, odd name (2) | f:x+y.txt, odd name (2) | f:x+y.txt, odd name (2) | ✅ | +| MOVE-04 | carries the descendants of a path with special characters, then `path:"./odd name (1)"` | no match | no match | no match | ✅ | ### rootscope diff --git a/services/search/pkg/parity/lifecycle_move_test.go b/services/search/pkg/parity/lifecycle_move_test.go index eb54fd16b6..3a27d91b12 100644 --- a/services/search/pkg/parity/lifecycle_move_test.go +++ b/services/search/pkg/parity/lifecycle_move_test.go @@ -6,6 +6,11 @@ import ( func moveLifecycle() lifecycleGroup { parent, child := fixtureTree() + big := fixtureFolder("big", withID("1$1!4")) + inBig := fixtureDoc("f1.txt", withID("1$1!5"), withParent(big.ID), withPath("./big/f1.txt")) + inSibling := fixtureDoc("x.txt", withID("1$1!6"), withParent("1$1!big2"), withPath("./big2/x.txt")) + odd := fixtureFolder("odd name (1)", withID("1$1!7")) + inOdd := fixtureDoc("f:x+y.txt", withID("1$1!8"), withParent(odd.ID), withPath("./odd name (1)/f:x+y.txt")) return lifecycleGroup{ name: "move", @@ -32,6 +37,25 @@ func moveLifecycle() lifecycleGroup { }, expect: []expectation{{`hidden:true`, nil}}, }, + { + id: 3, title: "leaves a sibling folder that shares the prefix alone", + fixtures: []search.Resource{big, inBig, inSibling}, + do: func(e search.Engine) error { return e.Move(big.ID, big.ParentID, "./moved") }, + expect: []expectation{ + {`path:"./moved"`, []string{"moved", "f1.txt"}}, + {`path:"./big"`, nil}, + {`path:"./big2"`, []string{"x.txt"}}, + }, + }, + { + id: 4, title: "carries the descendants of a path with special characters", + fixtures: []search.Resource{odd, inOdd}, + do: func(e search.Engine) error { return e.Move(odd.ID, odd.ParentID, "./odd name (2)") }, + expect: []expectation{ + {`path:"./odd name (2)"`, []string{"odd name (2)", "f:x+y.txt"}}, + {`path:"./odd name (1)"`, nil}, + }, + }, }, } } diff --git a/services/search/pkg/query/bleve/compiler.go b/services/search/pkg/query/bleve/compiler.go index 1ca00dbe7e..a1dba9a65a 100644 --- a/services/search/pkg/query/bleve/compiler.go +++ b/services/search/pkg/query/bleve/compiler.go @@ -123,6 +123,13 @@ func walk(offset int, nodes []ast.Node) (bleveQuery.Query, int, error) { var q bleveQuery.Query = bleveQuery.NewQueryStringQuery(k + ":" + v) switch { + case searchQuery.FieldIsPath(n.Key) && !isWildcard: + // the folder term matches the folder itself and its descendants + // (see PathAnalyzer); a query string would analyze the value into + // its prefixes and match everything under the root + tq := bleveQuery.NewTermQuery(val) + tq.SetField(k) + q = tq case n.Exact && !isWildcard: // = matches the whole value, on the lowercased sibling for // case-insensitive fields @@ -140,17 +147,6 @@ func walk(offset int, nodes []ast.Node) (bleveQuery.Query, int, error) { bq.SetMinShould(1) q = bq } - if searchQuery.FieldIsPath(n.Key) { - // bleve has no path hierarchy analyzer, unlike OpenSearch: match the - // folder itself and its descendants (`\/*`). A BooleanQuery keeps - // this atomic; a DisjunctionQuery would be redistributed by an - // enclosing AND (mapBinary treats a left disjunction as an OR-chain). - bq := bleve.NewBooleanQuery() - bq.AddShould(q, bleveQuery.NewQueryStringQuery(k+":"+v+`\/*`)) - bq.SetMinShould(1) - q = bq - } - if prev == nil { prev = q } else { diff --git a/services/search/pkg/query/bleve/compiler_test.go b/services/search/pkg/query/bleve/compiler_test.go index c4becff38f..632a29b387 100644 --- a/services/search/pkg/query/bleve/compiler_test.go +++ b/services/search/pkg/query/bleve/compiler_test.go @@ -51,23 +51,17 @@ func Test_compile(t *testing.T) { wantErr: false, }, { - // path fields expand to match the folder itself and its descendants, - // since bleve has no path hierarchy analyzer. + // one term matches the folder itself and its descendants name: `path:/Foo`, args: &ast.Ast{ Nodes: []ast.Node{ &ast.StringNode{Key: "path", Value: "/Foo"}, }, }, - // a BooleanQuery (should: exact OR descendants), not a DisjunctionQuery, - // so an enclosing AND does not redistribute the folder-itself clause. want: func() query.Query { - bq := query.NewBooleanQuery(nil, []query.Query{ - query.NewQueryStringQuery(`Path:\/Foo`), - query.NewQueryStringQuery(`Path:\/Foo\/*`), - }, nil) - bq.SetMinShould(1) - return query.NewConjunctionQuery([]query.Query{bq}) + tq := query.NewTermQuery("/Foo") + tq.SetField("Path") + return query.NewConjunctionQuery([]query.Query{tq}) }(), wantErr: false, }, diff --git a/services/search/pkg/search/search.go b/services/search/pkg/search/search.go index 009c8afa87..fb81135f2f 100644 --- a/services/search/pkg/search/search.go +++ b/services/search/pkg/search/search.go @@ -29,7 +29,7 @@ import ( // on a breaking mapping change: each version gets its own index (OpenSearch name // suffix, bleve path suffix), so the service builds a fresh index instead of // colliding with the old one. No migration; reindex to populate. -const SchemaVersion = 4 +const SchemaVersion = 5 var scopeRegex = regexp.MustCompile(`scope:\s*([^" "\n\r]*)`) diff --git a/tests/acceptance/bootstrap/SharingNgContext.php b/tests/acceptance/bootstrap/SharingNgContext.php index 1d395d6ef5..ffc7b31450 100644 --- a/tests/acceptance/bootstrap/SharingNgContext.php +++ b/tests/acceptance/bootstrap/SharingNgContext.php @@ -1028,8 +1028,11 @@ class SharingNgContext implements Context { // if recipient is not provided, it means user tries to remove own access, then we need to get the user permission id if ($shareType == 'user' && !isset($recipient)) { - $this->featureContext->shareNgAddToCreatedUserGroupShares($this->getDrivePermissionsList($sharer, $space)); - $permissionID = $this->featureContext->shareNgGetLastCreatedUserGroupShareID(); + $response = $this->getDrivePermissionsList($sharer, $space); + $permissionID = $this->getPermissionIdForUser( + $response, + $this->featureContext->getAttributeOfCreatedUser($sharer, 'id') + ); } elseif ($shareType == 'group' && !isset($recipient)) { $response = $this->getDrivePermissionsList($sharer, $space); $permissionID = $this->featureContext->getJsonDecodedResponse($response)['value'][0]['id']; @@ -2174,6 +2177,22 @@ class SharingNgContext implements Context { return $grantees; } + /** + * @param ResponseInterface $response + * @param string $userId + * + * @return string + * @throws Exception + */ + private function getPermissionIdForUser(ResponseInterface $response, string $userId): string { + foreach ($this->featureContext->getJsonDecodedResponse($response)['value'] as $permission) { + if (($permission['grantedToV2']['user']['id'] ?? null) === $userId) { + return $permission['id']; + } + } + throw new Exception("No permission found for user '$userId'"); + } + /** * @param string $grantee * diff --git a/tests/acceptance/features/apiSharingNg1/removeAccessToDrive.feature b/tests/acceptance/features/apiSharingNg1/removeAccessToDrive.feature index 6e09004cce..312a410125 100644 --- a/tests/acceptance/features/apiSharingNg1/removeAccessToDrive.feature +++ b/tests/acceptance/features/apiSharingNg1/removeAccessToDrive.feature @@ -141,7 +141,7 @@ Feature: Remove access to a drive Then the HTTP status code should be "403" And the user "Alice" should have a space called "NewSpace" - @flaky @issue-3193 + Scenario: user of a group cannot remove own group from project space if it is the last manager using root endpoint Given the administrator has assigned the role "Space Admin" to user "Alice" using the Graph API And group "group1" has been created