Add jobs and reservations privacy settings #149

yencli · 2017-01-06T11:03:27Z

The "PrivateData" parameter from Slurm configuration is now partially introduced to slurm-web. However, this change is only compatible with a modified version of pyslurm that contains an extra output (see edf-hpc/pyslurm@634ed69). Also, the mocks for testing are modified along with.

Jobs and Reservations views are affected in this change. Regular users may only see their own jobs or reservations if defined in PrivateData previously.

rezib

Additionally to all other comments, can you please also move mocks mods into a separate commit to avoid confusions between the feature and all the stuff related to tests.

rezib · 2017-01-09T07:55:20Z

doc/usage.rst


 .. figure:: img/screenshot_job_close_details.*

+The Jobs view starts taking Slurm's Private Data parameter into account partially


This should not be mentioned only in the usage guide but also in the installation (§auth) and architecture guides (§auth). It is useless to mention slurm-web version since the documentation is versioned along with the software. A link should be added to related slurm documentation. The possible values with their impact on slurm-web behaviour should be explained explicitely. The mention "partially taken into account" is definitely too vague.

Please also explain what guest user still has access to in presence of privatedata.

rezib · 2017-01-09T07:55:39Z

doc/usage.rst

 #. The start time of this reservation
 #. The end time of this reservation

+The Reservation view starts taking Slurm's Private Data parameter into account partially


Please avoid copy/paste.

rezib · 2017-01-09T07:59:27Z

rest/slurmrestapi.py

 @authentication_verify()
 @cache()
 def get_reservations():
+    if auth_enabled:


What happens here if auth is disabled?

rezib · 2017-01-09T08:02:23Z

rest/auth.py

+            to be enabled to retrieve current user")
+
+    token = request.headers['Authorization'].split()[1]
+    return User.verify_auth_token(token)


Explain in comment why you don't catch the auth verification exceptions here.

rezib · 2017-01-09T08:04:16Z

rest/auth.py

+        return abort(403, "Authentication has \
+            to be enabled to retrieve current user")
+
+    token = request.headers['Authorization'].split()[1]


The token extraction logic should be factorized to avoid loosely access to request headers dict from anywhere in the code.

rezib · 2017-01-09T08:07:56Z

rest/slurmrestapi.py

            )
-    return jobs
+
+    if onlyUsersJobs:


This if should be removed. We should have only one dict of jobs at this point, whatever the value of onlyUsersJobs is true or false.

rezib · 2017-01-09T08:08:21Z

rest/slurmrestapi.py

-    # pyslurm >= 16.05 expects a string in parameter of job.find_id()
-    job = pyslurm.job().find_id(str(job_id))
-    fill_job_user(job)
+    config = pyslurm.config().get()


What if auth is disabled?

rezib · 2017-01-09T08:10:22Z

tests/mocks/pyslurm.py

    def find_id(self, jobid):
        for job in context.CTLD.jobs:
-            if job.jobid == jobid:
+            if str(job.jobid) == str(jobid):


This is probably useless since all jobids should already be strings at this point.

rezib · 2017-01-09T08:12:03Z

rest/slurmrestapi.py

+    result = {}
+    if auth_enabled:
+        # Fetch the attributs of private_data
+        config = pyslurm.config().get()


The logic to extract the configuration, parse the privatedata and set a boolean should done in a factorized function since this code is very similar in the various routes.

rezib · 2017-01-09T08:14:19Z

2 additional notes:

Your commit message does not follow the contribution guidelines. It also mentions a commit hash that has to be reverted so it will disappear in the long term. You can eventually add a reference to a commit hash but it must be a commit on upstream repository of PySLURM.
Can you please also remove this weird merge commit from the commit list?

Updates following "Add jobs and reservations privacy settings rackslab#149". Elements built previously for data privacy settings are refactored and documentation has been added.

This reverts commit 27a47fd since it has several issues. Notably, it does not compile on python 2.6: $ python -m py_compile rest/slurmrestapi.py SyntaxError: ('invalid syntax', ('rest/slurmrestapi.py', 241, 24, ' \ return {k: v for k, v in reservations.iteritems()\n')) The feature is reviewed in PR #149.

rezib · 2017-01-17T08:43:03Z

Can you please rebase your PR on current master branch 431e189?

Can you also please respect contribution guidelines (https://github.com/edf-hpc/slurm-web/blob/master/CONTRIBUTING.md) in your commit messages?

rezib · 2017-01-18T09:22:07Z

doc/architecture.rst

+
+Slurm-web also takes into account of the Private data parameter from
+`Slurm <https://slurm.schedmd.com/slurm.conf.html>`_. The python wrapper
+edf-hpc/`pyslurm <https://github.com/edf-hpc/pyslurm/>`_ has been


The ling to Slurm configuration man is not properly formatted in generated documentation. The fact that PySLURM has been modified is not a useful information for slurm-web users and administrators. Please fix this.

rezib · 2017-01-18T09:22:13Z

doc/usage.rst

+parameter in edf-hpc/`pyslurm <https://github.com/edf-hpc/pyslurm/>`_. The original
+description can be found in`Slurm <https://slurm.schedmd.com/slurm.conf.html>`_.
+If ``jobs`` has been defined previously in Slurm configuration, regular users can
+only see their own jobs whereas the superusers see all the jobs and guests can see


Slurm-web has no notion of superusers. There are admins, users and guests but none superusers.

rezib · 2017-01-18T09:26:01Z

rest/auth.py


+
+# Retrive current user role and login
+def get_current_user():


Please add docstring to explain what this function does, notably in which cases it aborts.

rezib · 2017-01-18T09:26:04Z

rest/auth.py

+
+
+def retrieve_token():
+    if 'Authorization' not in request.headers:


Please add docstring to explain what this function does, notably in which case it aborts.

The file has been updated.

rezib · 2017-01-18T09:37:24Z

rest/auth.py

+            to be enabled to retrieve current user")
+
+    token = retrieve_token()
+    # No exception is thrown from this method but return None when failed


The comment is wrong, it does not return None.

rezib · 2017-01-18T09:38:20Z

rest/auth.py

+
+    token = retrieve_token()
+    # No exception is thrown from this method but return None when failed
+    try:


This is a copy/paste of the code in the decorator, please factorize.

rezib · 2017-01-18T09:39:28Z

rest/slurmrestapi.py

    jobs = pyslurm.job().get()
+    result = {}
+    if auth_enabled:
+        """


PEP-8 says that block comment should use #[1], not docstrings, please fix it.

[1] https://www.python.org/dev/peps/pep-0008/#block-comments

rezib · 2017-01-18T09:40:39Z

rest/slurmrestapi.py


+
+def check_private_data_for_entity(user, entity):
+    onlyUsersEntities = False


Please add docstring to explain what this function does.

The user argument is not used, please fix it.

The user argument is used as well as entity.

The code sets currentUser with user but currentUser then receives the result of get_current_user() before being actually used if the if check. If you remove currentUser = user the function is the same. Then, no, the argument is definitely not used.

rezib

PEP8 says variable names must be in lowercase separated by underscore. For instance, you should replace currentUser by current_user. Please fix this.

rezib · 2017-02-06T09:24:45Z

rest/slurmrestapi.py

+    return onlyUsersEntities
+
+
+def filter_entities(entity, entitiesList):


Please add missing docstring here.

rezib · 2017-02-06T09:37:56Z

rest/cache.py



-def cache():
+def cache(userSpecific=False):


This is not a good solution as the cache is not shared between users. It implies that n different users accessing the jobs will generate n RPC to slurmctld and this is not what we want. N users accessing the jobs in a short period of time (< jobs cache expiration) should generate only 1 RPC to slurmctld.

Do you prefer to have the job filter inside the cache?

Clearly, no. This cache function is not about jobs so there is no reason to filter jobs here. Caching and filtering are 2 different purposes that must be treated separately.

Any suggestion? Userspecific only when privateData is on?

Userspecific only when privateData is on?

This doesn't solve the mentioned problem neither.

For me, the right solution is to manage the cache at pyslurm calls level, not at API route level. This is what I suggested last friday.

rezib · 2017-02-06T09:40:28Z

rest/slurmrestapi.py

+    Private Data settings.
+    """
+    onlyUsersEntities = False
+    currentUser = user


Please avoid this useless additional variable. The rest of the function code can simply use the user argument directly.

rezib · 2017-02-06T09:42:07Z

rest/slurmrestapi.py

+            currentUser, entity)
+
+    if onlyUsersEntities:
+        # if private data is applied and the jobs' owner is different from


The comment is wrong as the logic here is not only about jobs.

rezib · 2017-02-06T09:51:31Z

rest/slurmrestapi.py

+        # show if auth disabled, onlyUsersJobs becomes always False and all
+        # the jobs are added to the job list to show
+
+        return dict((k, v) for k, v in entitiesList.iteritems()


Please fix the indentation here:

C:556, 0: Wrong continued indentation. v['users']) or (entity == 'jobs' and ^| (bad-continuation) C:557, 0: Wrong continued indentation. currentUser.login == v['login']))) ^ | (bad-continuation)

The readability of the of this nested logic is not that great. I think it would be relevantly replaced by an additional if statement before the return and its iteration...

rezib · 2017-02-07T09:27:44Z

Please split commit with cache mechanism change and privatedata introduction. Even though the later requires the former, they are 2 distinct and complex things that really deserve 2 separate commits.

rezib · 2017-02-07T11:04:03Z

Cache change is a requirement for privatedata so please put cache change before privatedata introduction.

rezib · 2017-02-07T14:00:06Z

Please remove the @cache decorator everywhere as it is confusing to have 2 distinct ways of using the cache.

rezib · 2017-02-08T08:01:30Z

Please remove the code of the cache decorator itself as it is useless as of now.

yencli · 2017-02-08T09:12:51Z

OK, I removed the decorator.

rezib · 2017-02-08T09:55:30Z

rest/cache.py

-                ''.join("%s-%r" % (key, val) for (key,
-                                                  val) in kwargs.iteritems())
-                )
+def get_from_cache(f, overrideName=None, *args, **kwargs):


Please add a docstring on this new function.

rezib · 2017-02-08T09:56:01Z

rest/slurmrestapi.py

    reservations = get_from_cache(
        pyslurm.reservation().get, 'get_reservations')
-    return reservations
+    return filter_entities('reservations', reservations)


Lot of changes are missing in this file.

I put it back.

This modification prevents regular users from seeing other users' previous requests before the caches expired.

The application of Slurm's Private Data settings for jobs and reservations has been introduced to Slurm-web. Descriptions of Slurm's configuration can be found in https://slurm.schedmd.com/slurm.conf.html (see "PrivateData").This implies that regular users may only see their own jobs or reservations if PrivateData is set previously in Slurm's configuration. Associated documentations has been updated accordingly.

Update mock pyslurm and define private_data in 2 mock clusters.

rezib · 2017-02-08T12:19:49Z

OK, it's been merged.

rezib requested changes Jan 9, 2017

View reviewed changes

yencli force-pushed the master branch from 4a9100d to 209fa52 Compare January 16, 2017 17:09

yencli force-pushed the master branch from 209fa52 to 79ec650 Compare January 16, 2017 17:42

yencli force-pushed the master branch 5 times, most recently from 3c12ccb to d8407da Compare January 17, 2017 13:41

rezib approved these changes Jan 18, 2017

View reviewed changes

rezib requested changes Jan 18, 2017

View reviewed changes

yencli force-pushed the master branch 4 times, most recently from 0cab2fe to ecb078e Compare January 23, 2017 08:39

yencli force-pushed the master branch 5 times, most recently from acfec8f to 28af211 Compare February 6, 2017 08:45

rezib approved these changes Feb 6, 2017

View reviewed changes

rezib reviewed Feb 6, 2017

View reviewed changes

yencli force-pushed the master branch 2 times, most recently from da04f2f to 1f4aff9 Compare February 7, 2017 08:57

yencli force-pushed the master branch from 1f4aff9 to 69e8ec0 Compare February 7, 2017 09:56

yencli force-pushed the master branch 5 times, most recently from ce1038b to c555876 Compare February 7, 2017 12:41

yencli force-pushed the master branch from c555876 to c3068f1 Compare February 7, 2017 16:29

yencli force-pushed the master branch from c3068f1 to 01f9a77 Compare February 8, 2017 09:11

rezib reviewed Feb 8, 2017

View reviewed changes

Privacy issue when cache is on

f8ce7ff

This modification prevents regular users from seeing other users' previous requests before the caches expired.

yencli force-pushed the master branch from 01f9a77 to a125fd3 Compare February 8, 2017 10:30

yencli added 2 commits February 8, 2017 11:52

Add PrivateData to mocks

8005c48

Update mock pyslurm and define private_data in 2 mock clusters.

yencli force-pushed the master branch from a125fd3 to 8005c48 Compare February 8, 2017 10:52

rezib closed this Feb 8, 2017


		.. figure:: img/screenshot_job_close_details.*

		The Jobs view starts taking Slurm's Private Data parameter into account partially



		# Retrive current user role and login
		def get_current_user():



		def retrieve_token():
		if 'Authorization' not in request.headers:



		def check_private_data_for_entity(user, entity):
		onlyUsersEntities = False

		return onlyUsersEntities


		def filter_entities(entity, entitiesList):

Add jobs and reservations privacy settings #149

Add jobs and reservations privacy settings #149

Uh oh!

Conversation

yencli commented Jan 6, 2017

Uh oh!

rezib left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rezib commented Jan 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rezib commented Jan 17, 2017

Uh oh!

rezib Jan 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rezib left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

rezib commented Jan 9, 2017 •

edited

Loading

rezib Jan 18, 2017 •

edited

Loading

rezib commented Feb 8, 2017 •

edited

Loading