Fix access rights for analyses #181
Conversation
Note that this commit ports over the contents of the previously existing repository. Fixes qiita-spots#59
| 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 |
There was a problem hiding this comment.
Is there a way to currently avoid this?
There was a problem hiding this comment.
Currently no, but it is part of the Analysis object so it shouldn't be too big a problem, should it?
|
Added some comments. |
|
Could you add test to your code? I think that will be the only outstanding issue. |
|
There are currently no tests for anything tornado based/in qiita-pet, so that will go in in a separate pull request once the framework for doing so is figured out. |
|
OK, could you comment what needs to be figured out? |
|
Tornado has its own built-in testing framework that can simulate page calls, asynchronous data pulls from other pages, and a few other key things. We have never used it so it will take probably half a day to a day of messing with it to figure out how it all works. |
|
Got it, is not that we need to figure out how to test but the testing |
| 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.
I don't think the call to set is buying you anything here.
There was a problem hiding this comment.
The added set creation time overwhelms the O(1) set lookup vs O(n) list lookup?
There was a problem hiding this comment.
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.
That's actually really cool to know. Thanks. I'll fix that.
|
Just one comment. Looks good after it's addressed. |
|
Should be ready to merge. |
|
Thanks @squirrelo! For the set creation thing that was in my last comment, note that if you were doing the lookup repeatedly (e.g., in a for loop), the set creation would be well worth it! |
Closes #147
This raises a RuntimeError if a person tries to access an Analysis they are not allowed to. This is for logging purposes.
This will also need to happen for Studies, but we don't have those pages yet.