workspaces: add validation function for the workflows #286
Conversation
Codecov Report
@@ Coverage Diff @@
## master #286 +/- ##
==========================================
- Coverage 33.03% 32.48% -0.55%
==========================================
Files 19 20 +1
Lines 890 905 +15
==========================================
Hits 294 294
- Misses 596 611 +15
|
reana_commons/config.py
Outdated
| DEFAULT_WORKSPACE_PATH = os.getenv("DEFAULT_WORKSPACE_PATH", "/var/reana") | ||
| """Default workspace path defined by the admin.""" | ||
|
|
||
| ADMIN_ALLOWED_WORKSPACES = os.getenv("ADMIN_ALLOWED_WORKSPACES", None) |
There was a problem hiding this comment.
Minor: docstring is missing.
Perhaps we could split the string and append SHARED_VOLUME_PATH here to have the final list. In that case code will be simplified reana_commons/workspaces.py and this operation will be done only once
There was a problem hiding this comment.
I agree, good change, as I wont have to make that operation in the validation or showing the workspaces
reana_commons/workspaces.py
Outdated
| :param workspace_option: A string of the workspace. | ||
| :returns: A string of the validated workspace. | ||
| """ | ||
| if ADMIN_ALLOWED_WORKSPACES: |
There was a problem hiding this comment.
Please see my comment, I think this part could be simplified if we would store the final list in reana-commons
reana_commons/config.py
Outdated
| DEFAULT_WORKSPACE_PATH = os.getenv("DEFAULT_WORKSPACE_PATH", "/var/reana") | ||
| """Default workspace path defined by the admin.""" | ||
|
|
||
| ADMIN_ALLOWED_WORKSPACES = os.getenv("ADMIN_ALLOWED_WORKSPACES", None) |
There was a problem hiding this comment.
Also, please see my comment about the name
marcdiazsan
force-pushed
the
workspace-validation
branch
from
6c72f04
to
e7385e1
Compare
reana_commons/config.py
Outdated
| workspaces_list = workspaces_list.split(",") | ||
| workspaces_list.append(SHARED_VOLUME_PATH) | ||
| else: | ||
| workspaces_list = [SHARED_VOLUME_PATH] |
There was a problem hiding this comment.
Very minor: could be simplified like this
workspaces_list = workspaces_list.split(",") if workspaces_list else []
workspaces_list.append(SHARED_VOLUME_PATH)
reana_commons/workspaces.py
Outdated
| :returns: A string of the validated workspace. | ||
| """ | ||
| if workspace_option: | ||
| if workspace_option not in WORKSPACE_PATHS: |
There was a problem hiding this comment.
We need to take into account that one of the WORKSPACE_PATHS could be for example /eos,
but the user defined workspace would be /eos/user/a/amecioni/workflows/my_super_workflow. So direct comparison would not allow this workspace definition even though it's perfectly valid. I guess for now we could iterate through the WORKSPACE_PATHS list and check if workspace_option startswith() at least one path?
Later some more complex validation could be done (better in a separate PR in the future) to catch all the possible ways to abuse the system, like to catch "../", etc. (thanks @mvidalgarcia for noticing this possibility)
marcdiazsan
force-pushed
the
workspace-validation
branch
from
0d4710b
to
f632742
Compare
marcdiazsan
force-pushed
the
workspace-validation
branch
from
f632742
to
e3faede
Compare
marcdiazsan
force-pushed
the
workspace-validation
branch
from
e3faede
to
2fbbac4
Compare
Add default workspace as an environmental variable coming from the cluster deployment choice. The validation is done centralized in the validate_workspace function Closes reanahub/reana-client#546
marcdiazsan
force-pushed
the
workspace-validation
branch
from
2fbbac4
to
f5789b0
Compare
Goes on top of reanahub/reana-client#545 PR set, and reanahub/reana#532 PR set
Closes reanahub/reana-client#546