Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix access rights for analyses #181

Merged
merged 10 commits into from
Jul 2, 2014
15 changes: 15 additions & 0 deletions qiita_db/analysis.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,21 @@ def _status_setter_checks(self, conn_handler):
"""
self._lock_check(conn_handler)

@classmethod
def get_public(cls):
"""Returns analysis id for all public Analyses

Returns
-------
list of int
All public analysses in the database
"""
conn_handler = SQLConnectionHandler()
sql = ("SELECT analysis_id FROM qiita.{0} WHERE "
"{0}_analysis_id = %s".format(cls._table))
# MAGIC NUMBER 6: status id for a public study
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to currently avoid this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently no, but it is part of the Analysis object so it shouldn't be too big a problem, should it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, just wanted to be sure.

return [x[0] for x in conn_handler.execute_fetchall(sql, (6,))]

@classmethod
def create(cls, owner, name, description, parent=None):
"""Creates a new analysis on the database
Expand Down
5 changes: 2 additions & 3 deletions qiita_db/study.py
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ def _status_setter_checks(self, conn_handler):

@classmethod
def get_public(cls):
"""Returns study for all public Studies
"""Returns study id for all public Studies
Returns
-------
Expand All @@ -160,8 +160,7 @@ def get_public(cls):
sql = ("SELECT study_id FROM qiita.{0} WHERE "
"{0}_status_id = %s".format(cls._table))
# MAGIC NUMBER 2: status id for a public study
return [cls(x[0]) for x in
conn_handler.execute_fetchall(sql, (2,))]
return [x[0] for x in conn_handler.execute_fetchall(sql, (2, ))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, just with the Study object.


@classmethod
def create(cls, owner, title, efo, info, investigation=None):
Expand Down
48 changes: 37 additions & 11 deletions qiita_pet/handlers/analysis_handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,26 @@
from qiita_db.metadata_template import SampleTemplate
from qiita_db.job import Job
from qiita_db.util import get_db_files_base_dir
# login code modified from https://gist.github.com/guillaumevincent/4771570


def check_access(user, analysis_id):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be check_analysis_access? My guess is that there is going to be another ones, right? Or even better, leave as check_access but add a parameter that tells you if is analysis, study, or whatever.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's part of the analysis object, so it will be called as Analysis.check_access(#)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed that, it makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woops, thought this was something else. Yeah, that can be done.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK thanks.

"""Checks whether user has access to an analysis

Parameters
----------
user : User object
User to check
analysis_id : int
Analysis to check access for

Raises
------
RuntimeError
Tried to access analysis that user does not have access to
"""
if analysis_id not in set(Analysis.get_public() + user.shared_analyses +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the call to set is buying you anything here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The added set creation time overwhelms the O(1) set lookup vs O(n) list lookup?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likely, especially since these sets are likely to share few entries (private and public are disjoint, and shared is likely very small by comparison). A small test (which is not perfect, but meh):

>>> from timeit import Timer
>>> as_set = Timer('3 not in set(range(1000) + range(1001,1020) + range(1021,1061))')
>>> as_list = Timer('3 not in range(1000) + range(1001,1020) + range(1021,1061)') 
>>> as_set.repeat()
[51.94052219390869, 50.028679847717285, 50.24499797821045]
>>> as_list.repeat()
[17.484385013580322, 18.724167108535767, 18.87842297554016]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's actually really cool to know. Thanks. I'll fix that.

user.private_analyses):
raise RuntimeError("Analysis access denied to %s" % (analysis_id))


class CreateAnalysisHandler(BaseHandler):
Expand All @@ -43,11 +62,10 @@ def post(self):
name = self.get_argument('name')
description = self.get_argument('description')
user = self.get_current_user()
# create list of studies
study_ids = {s.id for s in Study.get_public()}
userobj = User(user)
[study_ids.add(x) for x in userobj.private_studies]
[study_ids.add(x) for x in userobj.shared_studies]
# create list of studies
study_ids = set(Study.get_public() + userobj.private_studies +
userobj.shared_studies)

studies = [Study(i) for i in study_ids]
analysis = Analysis.create(User(user), name, description)
Expand Down Expand Up @@ -101,22 +119,28 @@ def post(self):
class AnalysisWaitHandler(BaseHandler):
@authenticated
def get(self, analysis_id):
user = self.get_current_user()
check_access(User(user), analysis_id)

analysis = Analysis(analysis_id)
commands = []
for job in analysis.jobs:
jobject = Job(job)
commands.append("%s:%s" % (jobject.datatype, jobject.command[0]))

self.render("analysis_waiting.html", user=self.get_current_user(),
self.render("analysis_waiting.html", user=user,
aid=analysis_id, aname=analysis.name,
commands=commands)

@authenticated
@asynchronous
def post(self, analysis_id):
def post(self, aid):
user = self.get_current_user()
check_access(User(user), aid)

command_args = self.get_arguments("commands")
split = [x.split("#") for x in command_args]
analysis = Analysis(analysis_id)
analysis = Analysis(aid)

commands = []
# HARD CODED HACKY THING FOR DEMO, FIX Issue #164
Expand All @@ -141,9 +165,8 @@ def post(self, analysis_id):
Job.create(data_type, command, opts, analysis)
commands.append("%s: %s" % (data_type, command))
user = self.get_current_user()
self.render("analysis_waiting.html", user=user,
aid=analysis_id, aname=analysis.name,
commands=commands)
self.render("analysis_waiting.html", user=user, aid=aid,
aname=analysis.name, commands=commands)
# fire off analysis run here
# currently synch run so redirect done here. Will remove after demo
run_analysis(user, analysis)
Expand All @@ -152,6 +175,9 @@ def post(self, analysis_id):
class AnalysisResultsHandler(BaseHandler):
@authenticated
def get(self, aid):
user = self.get_current_user()
check_access(User(user), aid)

analysis = Analysis(aid)
jobres = defaultdict(list)
for job in analysis.jobs:
Expand Down
4 changes: 4 additions & 0 deletions test.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
#SampleID center_name center_project_name data_type_id ebi_study_accession ebi_submission_accession emp_status_id str_column
SKB7.640196 ANL Test Project 2 None None 1 Value for sample 3
SKB8.640193 ANL Test Project 2 None None 1 Value for sample 1
SKD8.640184 ANL Test Project 2 None None 1 Value for sample 2