global: add record/collection permissions #1589

jmartinm · 2016-09-23T08:04:33Z

Signed-off-by: Javier Martin Montull javier.martin.montull@cern.ch

Closes #919

jmartinm · 2016-09-23T08:06:09Z

Addresses #1577

One problem I see at the moment is that when I enable invenio-collections I am getting ES timeouts locally when migrating records (possibly due to some extra computation happening in invenio-collections receivers)

kaplun · 2016-09-23T08:42:44Z

I see from https://github.com/inveniosoftware/invenio-collections/blob/master/invenio_collections/config.py#L24 that by adjusting COLLECTIONS_DELETED_RECORDS we can directly have ES to exclude Deleted records, so then @spirosdelviniotis don't have to explicitly delete records in ES.

jmartinm · 2016-09-23T11:21:31Z

Another side effect I just realised. As invenio-collections listens to before_record_insert signal when creating a new record in the HoldingPen, the receiver runs (which is probably ok) but it is causing an error since in the HoldingPen instead of passing around a Record object we are passing a dict

:15:38 worker.1    | [2016-09-23 13:15:38,553: ERROR/MainProcess] Task invenio_workflows.tasks.start[e8913b38-46fb-487b-a088-4bbf83b73c3d] raised unexpected: AttributeError("'dict' object has no attribute 'dumps'",)
13:15:38 worker.1    | Traceback (most recent call last):
13:15:38 worker.1    |   File "/Users/jmartinm/.virtualenvs/invenio3/lib/python2.7/site-packages/celery/app/trace.py", line 240, in trace_task
13:15:38 worker.1    |     R = retval = fun(*args, **kwargs)
13:15:38 worker.1    |   File "/Users/jmartinm/.virtualenvs/invenio3/lib/python2.7/site-packages/flask_celeryext/app.py", line 43, in __call__
13:15:38 worker.1    |     return Task.__call__(self, *args, **kwargs)
13:15:38 worker.1    |   File "/Users/jmartinm/.virtualenvs/invenio3/lib/python2.7/site-packages/celery/app/trace.py", line 438, in __protected_call__
13:15:38 worker.1    |     return self.run(*args, **kwargs)
13:15:38 worker.1    |   File "/Users/jmartinm/.virtualenvs/invenio3/src/invenio-workflows/invenio_workflows/tasks.py", line 77, in start
13:15:38 worker.1    |     return text_type(run_worker(workflow_name, data, **kwargs).uuid)
13:15:38 worker.1    |   File "/Users/jmartinm/.virtualenvs/invenio3/src/invenio-workflows/invenio_workflows/worker_engine.py", line 53, in run_worker
13:15:38 worker.1    |     engine.process(objects, **kwargs)
13:15:38 worker.1    |   File "/Users/jmartinm/.virtualenvs/invenio3/lib/python2.7/site-packages/workflow/engine.py", line 390, in process
13:15:38 worker.1    |     self._process(objects)
13:15:38 worker.1    |   File "/Users/jmartinm/.virtualenvs/invenio3/lib/python2.7/site-packages/workflow/engine.py", line 547, in _process
13:15:38 worker.1    |     obj, self, callbacks, exc_info
13:15:38 worker.1    |   File "/Users/jmartinm/.virtualenvs/invenio3/src/invenio-workflows/invenio_workflows/engine.py", line 363, in Exception
13:15:38 worker.1    |     obj, eng, callbacks, exc_info
13:15:38 worker.1    |   File "/Users/jmartinm/.virtualenvs/invenio3/lib/python2.7/site-packages/workflow/engine.py", line 970, in Exception
13:15:38 worker.1    |     reraise(*exc_info)
13:15:38 worker.1    |   File "/Users/jmartinm/.virtualenvs/invenio3/lib/python2.7/site-packages/workflow/engine.py", line 529, in _process
13:15:38 worker.1    |     self.run_callbacks(callbacks, objects, obj)
13:15:38 worker.1    |   File "/Users/jmartinm/.virtualenvs/invenio3/lib/python2.7/site-packages/workflow/engine.py", line 481, in run_callbacks
13:15:38 worker.1    |     self.execute_callback(callback_func, obj)
13:15:38 worker.1    |   File "/Users/jmartinm/.virtualenvs/invenio3/lib/python2.7/site-packages/workflow/engine.py", line 564, in execute_callback
13:15:38 worker.1    |     callback(obj, self)
13:15:38 worker.1    |   File "/Users/jmartinm/.virtualenvs/invenio3/src/inspire/inspirehep/modules/workflows/tasks/actions.py", line 175, in emit_record_signals
13:15:38 worker.1    |     before_record_insert.send(obj.data)
13:15:38 worker.1    |   File "/Users/jmartinm/.virtualenvs/invenio3/lib/python2.7/site-packages/blinker/base.py", line 267, in send
13:15:38 worker.1    |     for receiver in self.receivers_for(sender)]
13:15:38 worker.1    |   File "/Users/jmartinm/.virtualenvs/invenio3/src/invenio-collections/invenio_collections/receivers.py", line 112, in __call__
13:15:38 worker.1    |     matcher=self.matcher)
13:15:38 worker.1    |   File "/Users/jmartinm/.virtualenvs/invenio3/src/invenio-collections/invenio_collections/receivers.py", line 88, in get_record_collections
13:15:38 worker.1    |     for collections in matcher(collections, record):
13:15:38 worker.1    |   File "/Users/jmartinm/.virtualenvs/invenio3/src/invenio-collections/invenio_collections/percolator.py", line 99, in _find_matching_collections_externally
13:15:38 worker.1    |     body = {"doc": record.dumps()}
13:15:38 worker.1    | AttributeError: 'dict' object has no attribute 'dumps'

kaplun · 2016-09-23T11:36:18Z

As discussed IRL we can drop emit_record_signals since all the use-cases are already taken care during real record insertion/indexing.

jmartinm · 2016-09-23T12:27:04Z

One problem I see at the moment is that when I enable invenio-collections I am getting ES timeouts locally when migrating records (possibly due to some extra computation happening in invenio-collections receivers)

This problem is now solved by not using percolators (COLLECTIONS_USE_PERCOLATOR=False) and instead using 'internal matching'.

jmartinm · 2016-09-23T13:05:54Z

Second commit fixes #1589 (comment)

jmartinm · 2016-09-23T15:08:40Z

TODO:

Fix landing page query. Currently shows the number of records in the index (and once searching we only search with Literature collection) so numbers don't match.
Restrict collections. Collections that are currently restricted should still be restricted.

jmartinm · 2016-09-26T22:01:59Z

inspirehep/modules/search/api.py

+    if request:
+        collection = request.values.get('cc', 'Literature')
+
+        all_restricted_collections = set(


This would be better cached to avoid doing a DB query on every request.

jmartinm · 2016-09-27T15:52:02Z

Ready for comments. Now that Travis is passing, will add some extra tests regarding permissions/collections.

jacquerie · 2016-09-27T16:58:03Z

Last commit (tests: test_record app fix) should go first, otherwise you are breaking git bisect.

kaplun · 2016-09-23T08:41:08Z

inspirehep/config.py

+  {
+    "dbquery": "collections.primary:HEPNAMES",
+    "name": "HepNames"
+  }


I think this list is not up-to-date.

kaplun · 2016-09-27T19:58:50Z

inspirehep/modules/records/permissions.py

+            a.argument for a in ActionUsers.query.filter_by(
+                action='view-restricted-collection').all()
+        ]
+    )


Shouldn't this be computed only once, for performance reason. We could cache it in Redis for a minute or so.

kaplun · 2016-09-27T20:02:35Z

setup.py

+            'view_restricted_collection'
+            ' = inspirehep.modules.records.permissions:'
+            'action_view_restricted_collection',
+        ],


This seems extreme indentation 💃

jmartinm · 2016-10-04T07:59:03Z

This PR is now ready to be reviewed. The functionality added is covered by tests.

kaplun · 2016-10-04T08:27:06Z

inspirehep/config.py

@@ -125,6 +125,114 @@ def _(x):
 USERPROFILES_EXTEND_SECURITY_FORMS = False
 USERPROFILES_SETTINGS_TEMPLATE = 'inspirehep_theme/accounts/settings/profile.html'

+# Collections
+# ===========
+COLLECTIONS_DELETED_RECORDS = '{dbquery} AND NOT collections.primary:"DELETED"'


Currently, after @spirosdelviniotis work the DELETED information corresponds to a flag being set to True in deleted.

I am not sure how to formulate this filter in Invenio query syntax though. (i.e. deleted != True)

@spirosdelviniotis will test this out.

Maybe something like:

COLLECTIONS_DELETED_RECORDS = '{dbquery} AND NOT deleted:True'

works.

kaplun · 2016-10-04T08:30:23Z

inspirehep/modules/records/mappings/records/journals.json

+                },
+                "lowercase_analyzer": {
+                    "filter": "lowercase",
+                    "tokenizer": "keyword"


Question: since you ignore case in matching collection in ES, are you anyway then presenting them in the HTML and URLs in their canonical form or you are reusing the case that was provided as input by the user?

We use them in the UI in the canonical form, like Authors. But then you can do /search?cc=Authors or /search?cc=authors

kaplun · 2016-10-04T08:31:30Z

inspirehep/modules/records/permissions.py

+        if restricted_collections:
+            cache.set(
+                'restricted_collections',
+                pickle.dumps(restricted_collections),


Please use protocol -1 to have the most efficient protocol being chosen.

I will use #1622 so will not do pickling in the end myself.

kaplun · 2016-10-04T08:36:35Z

inspirehep/modules/search/api.py

+        query = Q('match', _collections=collection)
+
+        for collection in list(all_restricted_collections - user_coll):
+            query = query & ~Q('match', _collections=collection)


Have you tried using actually ES filtering?

The query returned here is added to a ES filter query. You can see the output in this test https://github.com/inspirehep/inspire-next/pull/1589/files#diff-8eedb14f28e016fe0e021789e25878abR336

jmartinm · 2016-10-04T09:10:28Z

After #1622 is merged I will change the way I read/write into Redis as Flask-cache allows for writing sets into Redis (I guess with some internal magic)

jacquerie

👍, just some nits.

jacquerie · 2016-10-04T08:37:40Z

inspirehep/config.py

+COLLECTIONS_QUERY_WALKERS = [
+    'inspirehep.modules.search.walkers.pypeg_to_ast:PypegConverter',
+]
+"""Modules to create the query AST."""


Nit: space here.

jacquerie · 2016-10-04T08:38:55Z

inspirehep/config.py

+COLLECTIONS_USE_PERCOLATOR = False
+"""Define which percolator you want to use.
+
+Default value is `False` to use the internal percolator.


That's confusing! Maybe we should change upstream to have COLLECTIONS_USE_ES_PERCOLATOR instead? Of course, not something to be fixed in this PR.

This docstring is copy/pasted from invenio-collections. Maybe the phrase is a bit confusing since they called it 'internal percolator'. If we think of percolator just as the ES one, then the config makes sense COLLECTIONS_USE_PERCOLATOR True or False

jacquerie · 2016-10-04T08:40:42Z

inspirehep/config.py

 RECORDS_UI_ENDPOINTS = dict(
    literature=dict(
        pid_type='literature',
        route='/literature/<pid_value>',
        template='inspirehep_theme/format/record/'
                 'Inspire_Default_HTML_detailed.tpl',
-        record_class='inspirehep.modules.records.wrappers:LiteratureRecord',
-        permission_factory_imp='invenio_records_rest.utils:allow_all',
+        record_class='inspirehep.modules.records.wrappers:LiteratureRecord'


Nit: keep the trailing commas here, the diffs will look nicer now and in the future.

jacquerie · 2016-10-04T08:41:14Z

inspirehep/config.py

@@ -585,63 +694,59 @@ def _(x):
    ),
 )

+
+RECORDS_UI_DEFAULT_PERMISSION_FACTORY = \
+    "inspirehep.modules.records.permissions:record_read_permission_factory"


Good to know, I was the one who added the permission_factory_imp everywhere...

jacquerie · 2016-10-04T08:53:28Z

inspirehep/modules/records/mappings/records/authors.json

@@ -20,7 +20,7 @@
            "numeric_detection": false,
            "properties": {
                "_collections": {
-                    "index": "not_analyzed",
+                    "analyzer": "lowercase_analyzer",


What is the rationale for using this particular one for _collections? Should we prefer the lowercase_analyzer to not_analyzed in all cases?

This is to be forgiving to the user. Legacy INSPIRE/Invenio has always been case-insensitive for any field.

I wonder maybe if for e.g. DOI we have to be careful though.

jacquerie · 2016-10-04T09:14:43Z