-
Notifications
You must be signed in to change notification settings - Fork 80
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
Changes from 9 commits
d0ab982
2d196f6
9435939
f93b97c
1831d6d
6817cd4
e1dca81
cf5ffe3
01a8622
0b95cbe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
------- | ||
|
@@ -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, ))] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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_analysis_access(user, analysis_id): | ||
"""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 + | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think the call to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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] There was a problem hiding this comment. Choose a reason for hiding this commentThe 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): | ||
|
@@ -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 = Study.get_public() + userobj.private_studies + \ | ||
userobj.shared_studies | ||
|
||
studies = [Study(i) for i in study_ids] | ||
analysis = Analysis.create(User(user), name, description) | ||
|
@@ -101,22 +119,28 @@ def post(self): | |
class AnalysisWaitHandler(BaseHandler): | ||
@authenticated | ||
def get(self, analysis_id): | ||
user = self.get_current_user() | ||
check_analysis_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_analysis_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 | ||
|
@@ -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) | ||
|
@@ -152,6 +175,9 @@ def post(self, analysis_id): | |
class AnalysisResultsHandler(BaseHandler): | ||
@authenticated | ||
def get(self, aid): | ||
user = self.get_current_user() | ||
check_analysis_access(User(user), aid) | ||
|
||
analysis = Analysis(aid) | ||
jobres = defaultdict(list) | ||
for job in analysis.jobs: | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.