Skip to content

Commit 0be014a

Browse files
max-zillamax-zilla
authored
Fix several search result bugs (#100)
* Honor superadmin in additional spots * Fix autoscrolling bug and small stepsize * Consistent permissions with metadata search * Update CHANGELOG.md * Update CHANGELOG.md * One-liner for imports * Improve public user case handling * remove total size from Results
1 parent c14557f commit 0be014a

File tree

6 files changed

+109
-33
lines changed

6 files changed

+109
-33
lines changed

‎CHANGELOG.md‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,11 @@ All notable changes to this project will be documented in this file.
44
The format is based on [Keep a Changelog](http://keepachangelog.com/)
55
and this project adheres to [Semantic Versioning](http://semver.org/).
66

7+
## Unreleased
8+
9+
### Fixed
10+
- Fixed permissions checks on search results for search interfaces that would cause misleading counts. [#60](https://github.com/clowder-framework/clowder/issues/60)
11+
712
## 1.12.0 - 2020-10-19
813
**_Warning:_**
914
- This update modifies the MongoDB schema. Make sure to start the application with `-DMONGOUPDATE=1`.
@@ -24,7 +29,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
2429
- Ignore the `update` field when posting to `/api/extractors`. [#89](https://github.com/clowder-framework/clowder/issues/89)
2530
- Search results were hardcoded to be in batches of 2.
2631

27-
# 1.11.2 - 2020-10-13
32+
## 1.11.2 - 2020-10-13
2833

2934
### Fixed
3035
- Clowder healthcheck was not correct, resulting in docker-compose never thinking it was healthy. This could also result

‎app/api/Search.scala‎

Lines changed: 21 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,7 @@
11
package api
22

33
import api.Permission._
4-
import services.{RdfSPARQLService, DatasetService, FileService, CollectionService, PreviewService, SpaceService,
5-
MultimediaQueryService, ElasticsearchPlugin}
4+
import services.{RdfSPARQLService, PreviewService, SpaceService, MultimediaQueryService, ElasticsearchPlugin}
65
import play.Logger
76
import scala.collection.mutable.{ListBuffer, HashMap}
87
import util.{SearchUtils, SearchResult}
@@ -15,9 +14,6 @@ import models._
1514

1615
@Singleton
1716
class Search @Inject() (
18-
files: FileService,
19-
datasets: DatasetService,
20-
collections: CollectionService,
2117
previews: PreviewService,
2218
queries: MultimediaQueryService,
2319
spaces: SpaceService,
@@ -49,10 +45,16 @@ class Search @Inject() (
4945
(tag match {case Some(x) => s"&tag=$x" case None => ""})
5046

5147
// Add space filter to search here as a simple permissions check
52-
val permitted = spaces.listAccess(0, Set[Permission](Permission.ViewSpace), request.user, true, true, false, false).map(sp => sp.id)
48+
val superAdmin = request.user match {
49+
case Some(u) => u.superAdminMode
50+
case None => false
51+
}
52+
val permitted = if (superAdmin)
53+
List[UUID]()
54+
else
55+
spaces.listAccess(0, Set[Permission](Permission.ViewSpace), request.user, true, true, false, false).map(sp => sp.id)
5356

5457
val response = plugin.search(query, resource_type, datasetid, collectionid, spaceid, folderid, field, tag, from_index, size, permitted, request.user)
55-
5658
val result = SearchUtils.prepareSearchResponse(response, source_url, request.user)
5759
Ok(toJson(result))
5860
}
@@ -70,8 +72,19 @@ class Search @Inject() (
7072

7173
current.plugin[ElasticsearchPlugin] match {
7274
case Some(plugin) => {
75+
// Add space filter to search here as a simple permissions check
76+
val superAdmin = request.user match {
77+
case Some(u) => u.superAdminMode
78+
case None => false
79+
}
80+
val permitted = if (superAdmin)
81+
List[UUID]()
82+
else
83+
spaces.listAccess(0, Set[Permission](Permission.ViewSpace), request.user, true, true, false, false).map(sp => sp.id)
84+
85+
7386
val queryList = Json.parse(query).as[List[JsValue]]
74-
val response = plugin.search(queryList, grouping, from, size, user)
87+
val response = plugin.search(queryList, grouping, from, size, permitted, user)
7588

7689
// TODO: Better way to build a URL?
7790
val source_url = s"/api/search?query=$query&grouping=$grouping"

‎app/services/ElasticsearchPlugin.scala‎

Lines changed: 45 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ class ElasticsearchPlugin(application: Application) extends Plugin {
4141
val folders: FolderService = DI.injector.getInstance(classOf[FolderService])
4242
val datasets: DatasetService = DI.injector.getInstance(classOf[DatasetService])
4343
val collections: CollectionService = DI.injector.getInstance(classOf[CollectionService])
44+
val spaces: SpaceService = DI.injector.getInstance(classOf[SpaceService])
4445
val queue: ElasticsearchQueue = DI.injector.getInstance(classOf[ElasticsearchQueue])
4546
var client: Option[TransportClient] = None
4647
val nameOfCluster = play.api.Play.configuration.getString("elasticsearchSettings.clusterName").getOrElse("clowder")
@@ -119,15 +120,16 @@ class ElasticsearchPlugin(application: Application) extends Plugin {
119120
}
120121

121122
/** Prepare and execute Elasticsearch query, and return list of matching ResourceRefs */
122-
def search(query: List[JsValue], grouping: String, from: Option[Int], size: Option[Int], user: Option[User]): ElasticsearchResult = {
123+
def search(query: List[JsValue], grouping: String, from: Option[Int], size: Option[Int],
124+
permitted: List[UUID], user: Option[User]): ElasticsearchResult = {
123125
/** Each item in query list has properties:
124126
* "field_key": full name of field to query, e.g. 'extractors.wordCount.lines'
125127
* "operator": type of query for this term, e.g. '=='
126128
* "field_value": value to search for using specified field & operator
127129
* "extractor_key": name of extractor component only, e.g. 'extractors.wordCount'
128130
* "field_leaf_key": name of immediate field only, e.g. 'lines'
129131
*/
130-
val queryObj = prepareElasticJsonQuery(query, grouping)
132+
val queryObj = prepareElasticJsonQuery(query, grouping, permitted, user)
131133
accumulatePageResult(queryObj, user, from.getOrElse(0), size.getOrElse(maxResults))
132134
}
133135

@@ -759,7 +761,7 @@ class ElasticsearchPlugin(application: Application) extends Plugin {
759761
}
760762

761763
/** Convert list of search term JsValues into an Elasticsearch-ready JSON query object **/
762-
def prepareElasticJsonQuery(query: List[JsValue], grouping: String): XContentBuilder = {
764+
def prepareElasticJsonQuery(query: List[JsValue], grouping: String, permitted: List[UUID], user: Option[User]): XContentBuilder = {
763765
/** OPERATORS
764766
* : contains (partial match)
765767
* == equals (exact match)
@@ -805,6 +807,36 @@ class ElasticsearchPlugin(application: Application) extends Plugin {
805807
builder.startObject().startObject("exists").field("field", key).endObject().endObject()
806808
})
807809

810+
// Apply appropriate permissions filters based on user/superadmin
811+
user match {
812+
case Some(u) => {
813+
if (!u.superAdminMode) {
814+
builder.startObject.startObject("bool").startArray("should")
815+
816+
// Restrict to spaces the user is permitted access to
817+
permitted.foreach(ps => {
818+
builder.startObject().startObject("match").field("child_of", ps.stringify).endObject().endObject()
819+
})
820+
821+
// Also include anything the user owns
822+
builder.startObject().startObject("match").field("creator", u.id.stringify).endObject().endObject()
823+
824+
builder.endArray().endObject().endObject()
825+
}
826+
}
827+
case None => {
828+
// Metadata search is not publicly accessible so this shouldn't happen, public filter
829+
builder.startObject.startObject("bool").startArray("should")
830+
831+
// TODO: Does this behave properly with public spaces?
832+
spaces.list.foreach(ps => {
833+
builder.startObject().startObject("match").field("child_of", ps.id.stringify).endObject().endObject()
834+
})
835+
836+
builder.endArray().endObject().endObject()
837+
}
838+
}
839+
808840
builder.endArray()
809841
}
810842

@@ -943,7 +975,7 @@ class ElasticsearchPlugin(application: Application) extends Plugin {
943975
}
944976
})
945977

946-
// If user is superadmin or there is no user, no filters applied
978+
// Apply appropriate permissions filters based on user/superadmin
947979
user match {
948980
case Some(u) => {
949981
if (!u.superAdminMode) {
@@ -967,7 +999,15 @@ class ElasticsearchPlugin(application: Application) extends Plugin {
967999
}
9681000
}
9691001
case None => {
970-
// Calling this with no user should only happen internally (e.g. listTags) so no filter
1002+
// Metadata search is not publicly accessible so this shouldn't happen, public filter
1003+
builder.startObject.startObject("bool").startArray("should")
1004+
1005+
// TODO: Does this behave properly with public spaces?
1006+
spaces.list.foreach(ps => {
1007+
builder.startObject().startObject("match").field("child_of", ps.id.stringify).endObject().endObject()
1008+
})
1009+
1010+
builder.endArray().endObject().endObject()
9711011
}
9721012
}
9731013

‎app/util/SearchUtils.scala‎

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -291,12 +291,28 @@ object SearchUtils {
291291
var results = ListBuffer.empty[JsValue]
292292

293293
// Use bulk Mongo queries to get many resources at once
294-
val filesList = files.get(Permission.checkPermissions(user, Permission.ViewFile,
295-
response.results.filter(_.resourceType == 'file)).approved.map(_.id)).found
296-
val datasetsList = datasets.get(Permission.checkPermissions(user, Permission.ViewDataset,
297-
response.results.filter(_.resourceType == 'dataset)).approved.map(_.id)).found
298-
val collectionsList = collections.get(Permission.checkPermissions(user, Permission.ViewCollection,
299-
response.results.filter(_.resourceType == 'collection)).approved.map(_.id)).found
294+
val superAdmin = user match {
295+
case Some(u) => u.superAdminMode
296+
case None => false
297+
}
298+
299+
val filesList = if (superAdmin)
300+
files.get(response.results.filter(_.resourceType == 'file).map(_.id)).found
301+
else
302+
files.get(Permission.checkPermissions(user, Permission.ViewFile,
303+
response.results.filter(_.resourceType == 'file)).approved.map(_.id)).found
304+
305+
val datasetsList = if (superAdmin)
306+
datasets.get(response.results.filter(_.resourceType == 'dataset).map(_.id)).found
307+
else
308+
datasets.get(Permission.checkPermissions(user, Permission.ViewDataset,
309+
response.results.filter(_.resourceType == 'dataset)).approved.map(_.id)).found
310+
311+
val collectionsList = if (superAdmin)
312+
collections.get(response.results.filter(_.resourceType == 'collection).map(_.id)).found
313+
else
314+
collections.get(Permission.checkPermissions(user, Permission.ViewCollection,
315+
response.results.filter(_.resourceType == 'collection)).approved.map(_.id)).found
300316

301317
// Now reorganize the separate lists back into Elasticsearch score order
302318
for (resource <- response.results) {

‎app/views/metadatald/search.scala.html‎

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -464,34 +464,36 @@ <h1>Search Metadata within Space: "@space.name"</h1>
464464
count += resp.scanned_size;
465465
searching = false;
466466

467-
// Correctly increment skipped count
467+
// Correctly increment skipped count if we are missing results but have no more pages
468468
if (resp.count != resp.total_size) {
469469
if (resp.size > 0) {
470470
if (resp.total_size < resp.scanned_size)
471471
skipped_count += (resp.total_size - resp.count);
472-
else
472+
else if (resp.total_size > (resp.from+resp.count)) // last page case
473473
skipped_count += (resp.scanned_size - resp.count);
474474
} else if (resp.hasOwnProperty('next') || resp.total_size == resp.scanned_size) {
475+
// We got size zero result, but ES reports there should be some in total_size
475476
skipped_count += resp.total_size
476477
}
477478
}
478479

480+
$('#resultstitle').html('<h3>Results</h3>');
481+
482+
// Convert results into displayed list
483+
if (from_count > 0) {
484+
parseSearchResults(resp, status, err, false)
485+
} else {
486+
parseSearchResults(resp, status, err)
487+
}
488+
479489
if (resp.count == 0) {
480490
found_all = true;
481491
if (skipped_count>0)
482492
$('#resultsfooter').append("<i><b>All results displayed (omitted "+skipped_count+" results due to insufficient permissions).</b></i>");
483493
else
484494
$('#resultsfooter').append("<i><b>All results displayed.</b></i>");
485-
$('#resultsfooter').append("<br/>&nbsp;<br/>")
486-
}
487-
488-
$('#resultstitle').html('<h3>Results ('+resp.total_size+')</h3>');
489495

490-
// Convert results into displayed list
491-
if (from_count > 0) {
492-
parseSearchResults(resp, status, err, false)
493-
} else {
494-
parseSearchResults(resp, status, err)
496+
$('#resultsfooter').append("<br/>&nbsp;<br/>")
495497
}
496498
});
497499

‎app/views/searchResults.scala.html‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -173,7 +173,7 @@ <h1>Search</h1>
173173
if (resp.size > 0) {
174174
if (resp.total_size < resp.scanned_size)
175175
skipped_count += (resp.total_size - resp.count);
176-
else
176+
else if (resp.total_size > (resp.from+resp.count))
177177
skipped_count += (resp.scanned_size - resp.count);
178178
} else if (resp.hasOwnProperty('next') || resp.total_size == resp.scanned_size) {
179179
skipped_count += resp.total_size
@@ -189,7 +189,7 @@ <h1>Search</h1>
189189
$('#resultsfooter').append("<br/>&nbsp;<br/>")
190190
}
191191

192-
$("#resultheader").html("<h3>Results ("+resp.total_size+")</h3>");
192+
$("#resultheader").html("<h3>Results</h3>");
193193

194194
updateResults(resp, empty);
195195
// On initial page load, call this again just in case we have room to show more

0 commit comments

Comments
 (0)