Conversation
Add @requires_admin decorator Add clarity to backend start error status Signed-off-by: Ulincsys <ulincsys@gmail.com>
- Add ssl decorator to config endpoints - Fix syntax error in admin_required decorator - Update dashboard endpoint to use config class directly - Update dashboard styles with more consistent colors - Implement config update functionality in admin dashboard Signed-off-by: Ulincsys <28362836a@gmail.com>
Signed-off-by: Ulincsys <ulincsys@gmail.com>
Signed-off-by: Ulincsys <ulincsys@gmail.com>
Signed-off-by: Ulincsys <ulincsys@gmail.com>
Signed-off-by: Ulincsys <ulincsys@gmail.com>
Signed-off-by: Ulincsys <ulincsys@gmail.com>
Signed-off-by: Ulincsys <ulincsys@gmail.com>
Signed-off-by: Ulincsys <ulincsys@gmail.com>
Signed-off-by: Ulincsys <ulincsys@gmail.com>
Signed-off-by: Ulincsys <ulincsys@gmail.com>
Signed-off-by: Ulincsys <ulincsys@gmail.com>
Signed-off-by: Ulincsys <ulincsys@gmail.com>
Signed-off-by: Ulincsys <ulincsys@gmail.com>
Signed-off-by: Ulincsys <ulincsys@gmail.com>
Signed-off-by: Ulincsys <ulincsys@gmail.com>
Signed-off-by: Ulincsys <ulincsys@gmail.com>
Signed-off-by: Ulincsys <ulincsys@gmail.com>
| if signal.getsignal(signal.SIGHUP) != signal.SIG_DFL: | ||
| cmd = "nohup " + cmd | ||
|
|
||
| Popen(cmd, shell=True) |
Check failure
Code scanning / Bandit
subprocess call with shell=True identified, security issue. Error
| from ..server import app | ||
| import sqlalchemy as s | ||
| import json | ||
| from subprocess import run, PIPE, Popen |
Check notice
Code scanning / Bandit
Consider possible security implications associated with the subprocess module. Note
| @app.route(f"/{AUGUR_API_VERSION}/admin/shutdown") | ||
| @admin_required | ||
| def shutdown_system(): | ||
| run("augur backend stop-collection-blocking".split(), stdin=PIPE, stdout=PIPE, stderr=PIPE) |
Check notice
Code scanning / Bandit
subprocess call - check for execution of untrusted input. Note
| @admin_required | ||
| def shutdown_system(): | ||
| run("augur backend stop-collection-blocking".split(), stdin=PIPE, stdout=PIPE, stderr=PIPE) | ||
| Popen("augur backend stop", shell=True, stdin=PIPE, stdout=PIPE, stderr=PIPE) |
Check notice
Code scanning / Bandit
Starting a process with a partial executable path Note
| @admin_required | ||
| def shutdown_system(): | ||
| run("augur backend stop-collection-blocking".split(), stdin=PIPE, stdout=PIPE, stderr=PIPE) | ||
| Popen("augur backend stop", shell=True, stdin=PIPE, stdout=PIPE, stderr=PIPE) |
Check notice
Code scanning / Bandit
subprocess call with shell=True seems safe, but may be changed in the future, consider rewriting without shell Note
| @@ -0,0 +1,29 @@ | |||
| from subprocess import run, PIPE, Popen | |||
Check notice
Code scanning / Bandit
Consider possible security implications associated with the subprocess module. Note
| # Ignore SIGTERM from parent process (since we're terminating our parent) | ||
| signal.signal(signal.SIGTERM, lambda signum, frame: None) | ||
|
|
||
| run("augur backend stop-collection-blocking", shell=True, stderr=PIPE, stdout=PIPE, stdin=PIPE) |
Check notice
Code scanning / Bandit
Starting a process with a partial executable path Note
| # Ignore SIGTERM from parent process (since we're terminating our parent) | ||
| signal.signal(signal.SIGTERM, lambda signum, frame: None) | ||
|
|
||
| run("augur backend stop-collection-blocking", shell=True, stderr=PIPE, stdout=PIPE, stdin=PIPE) |
Check notice
Code scanning / Bandit
subprocess call with shell=True seems safe, but may be changed in the future, consider rewriting without shell Note
| signal.signal(signal.SIGTERM, lambda signum, frame: None) | ||
|
|
||
| run("augur backend stop-collection-blocking", shell=True, stderr=PIPE, stdout=PIPE, stdin=PIPE) | ||
| Popen("augur backend stop", shell=True, stderr=PIPE, stdout=PIPE, stdin=PIPE).wait() |
Check notice
Code scanning / Bandit
Starting a process with a partial executable path Note
| signal.signal(signal.SIGTERM, lambda signum, frame: None) | ||
|
|
||
| run("augur backend stop-collection-blocking", shell=True, stderr=PIPE, stdout=PIPE, stdin=PIPE) | ||
| Popen("augur backend stop", shell=True, stderr=PIPE, stdout=PIPE, stdin=PIPE).wait() |
Check notice
Code scanning / Bandit
subprocess call with shell=True seems safe, but may be changed in the future, consider rewriting without shell Note
| import beaker | ||
|
|
||
| from flask import request, jsonify, current_app | ||
| from flask import request, jsonify, current_app, abort |
There was a problem hiding this comment.
[pylint] reported by reviewdog 🐶
W0611: Unused current_app imported from flask (unused-import)
| @@ -38,6 +40,13 @@ def unsupported_method(error): | |||
|
|
|||
| return render_message("405 - Method not supported", "The resource you are trying to access does not support the request method used"), 405 | |||
There was a problem hiding this comment.
[pylint] reported by reviewdog 🐶
E0602: Undefined variable 'render_message' (undefined-variable)
| if AUGUR_API_VERSION in str(request.url_rule): | ||
| return jsonify({"status": "Forbidden"}), 403 | ||
|
|
||
| return render_message("403 - Forbidden", "You do not have permission to view this page"), 403 |
There was a problem hiding this comment.
[pylint] reported by reviewdog 🐶
E0602: Undefined variable 'render_message' (undefined-variable)
| raise e | ||
|
|
||
| return render_message("500 - Internal Server Error", "An error occurred while trying to service your request. Please try again, and if the issue persists, please file a GitHub issue with the below error message:", error=stacktrace), 500 | ||
| return render_message("500 - Internal Server Error", """An error occurred while trying to service your request. |
There was a problem hiding this comment.
[pylint] reported by reviewdog 🐶
E0602: Undefined variable 'render_message' (undefined-variable)
| @@ -98,19 +120,16 @@ def load_user(user_id): | |||
| @login_manager.request_loader | |||
| def load_user_request(request): | |||
There was a problem hiding this comment.
[pylint] reported by reviewdog 🐶
W0621: Redefining name 'request' from outer scope (line 1) (redefined-outer-name)
| from .init import logger | ||
| from .url_converters import * | ||
|
|
||
| from functools import wraps |
There was a problem hiding this comment.
[pylint] reported by reviewdog 🐶
W0611: Unused wraps imported from functools (unused-import)
| @@ -112,6 +119,10 @@ def repo_card_view(): | |||
| @app.route('/collection/status') | |||
| def status_view(): | |||
| return render_module("status", title="Status") | |||
There was a problem hiding this comment.
[pylint] reported by reviewdog 🐶
E0602: Undefined variable 'render_module' (undefined-variable)
|
|
||
| @app.route('/connection_status') | ||
| def server_ping_frontend(): | ||
| return render_module("ping") |
There was a problem hiding this comment.
[pylint] reported by reviewdog 🐶
E0602: Undefined variable 'render_module' (undefined-variable)
| @@ -318,6 +329,7 @@ def user_group_view(group = None): | |||
| return render_module("user-group-repos-table", title="Repos", repos=data, query_key=query, activePage=params["page"], pages=page_count, offset=pagination_offset, PS="user_group_view", reverse = rev, sorting = params.get("sort"), group=group) | |||
There was a problem hiding this comment.
[pylint] reported by reviewdog 🐶
E0602: Undefined variable 'render_module' (undefined-variable)
| backend_config = requestJson("config/get", False) | ||
| backend_config = AugurConfig(logger, db_session).load_config() | ||
|
|
||
| with get_session() as session: |
There was a problem hiding this comment.
[pylint] reported by reviewdog 🐶
W0621: Redefining name 'session' from outer scope (line 8) (redefined-outer-name)
|
|
||
| logger = AugurLogger("augur").get_logger() | ||
|
|
||
| @app.route(f"/{AUGUR_API_VERSION}/admin/shutdown") |
There was a problem hiding this comment.
Defaults to GET requests. Shouldn't this be POST only? Would prevent CSRF.
| return jsonify({"status": "shutdown"}) | ||
|
|
||
| @app.route(f"/{AUGUR_API_VERSION}/admin/restart") | ||
| @admin_required |
There was a problem hiding this comment.
Same as admin/shudown. I think this should also be POST only.
| @app.route(f"/{AUGUR_API_VERSION}/admin/restart") | ||
| @admin_required | ||
| def restart_system(): | ||
| Popen("python scripts/control/restart.py", shell=True) |
There was a problem hiding this comment.
Can we use absolute path derived from __file__ or the project root instead of relative path?
There was a problem hiding this comment.
also is this script going to work for docker installs?
|
|
||
| return response, 426 | ||
|
|
||
| @app.route(f"/{AUGUR_API_VERSION}/config/get", methods=['GET', 'POST']) |
There was a problem hiding this comment.
Why are we allowing POST here?
|
|
||
| return jsonify(config_dict), 200 | ||
|
|
||
| @app.route(f"/{AUGUR_API_VERSION}/config/set", methods=['GET', 'POST']) |
There was a problem hiding this comment.
Similar to above. Config mutations via GET are a CSRF risk and leak values into server logs, browser history, and referer headers. This should be POST-only with a JSON body.
|
|
||
| return jsonify({"status": "success"}) | ||
|
|
||
| @app.route(f"/{AUGUR_API_VERSION}/config/update", methods=['POST']) |
There was a problem hiding this comment.
update_config is missing @admin_required. The new set_config_item above it has admin protection, but this existing endpoint that can overwrite the entire config is unprotected. Should be:
| @app.route(f"/{AUGUR_API_VERSION}/config/update", methods=['POST']) | |
| @app.route(f"/{AUGUR_API_VERSION}/config/update", methods=['POST']) | |
| @ssl_required | |
| @admin_required | |
| def update_config(): |
| def admin_required(func): | ||
| @login_required | ||
| @wraps(func) | ||
| def inner_function(*args, **kwargs): | ||
| if current_user.admin: | ||
| return func(*args, **kwargs) | ||
| else: | ||
| abort(403) | ||
| return inner_function |
There was a problem hiding this comment.
The admin_required decorator has a subtle ordering issue. @login_required is applied to inner_function after @wraps(func), but login_required returns its own wrapper that doesn't preserve the metadata set by @wraps. This means the final returned function loses func's __name__ and __module__. Maybe we can use this approach:
| def admin_required(func): | |
| @login_required | |
| @wraps(func) | |
| def inner_function(*args, **kwargs): | |
| if current_user.admin: | |
| return func(*args, **kwargs) | |
| else: | |
| abort(403) | |
| return inner_function | |
| def admin_required(func): | |
| @wraps(func) | |
| def inner_function(*args, **kwargs): | |
| if not current_user.is_authenticated: | |
| return current_app.login_manager.unauthorized() | |
| if current_user.admin: | |
| return func(*args, **kwargs) | |
| else: | |
| abort(403) | |
| return inner_function |
| errout.close() | ||
| except Exception as e: | ||
| logger.error(e) | ||
| raise e |
There was a problem hiding this comment.
raise e inside the 500 error handler will crash the handler itself, meaning the user never sees the friendly error page. Is that the intended idea?
| backend_config = requestJson("config/get", False) | ||
| backend_config = AugurConfig(logger, db_session).load_config() | ||
|
|
||
| with get_session() as session: |
There was a problem hiding this comment.
session here shadows Flask's session import at the top of the file. Not a bug right now but could cause confusion. Consider db_session or db_sess instead?
There was a problem hiding this comment.
The IDs stats-section is reused on three different <h1> elements (Stats, User List, Config). Should be stats-section, user-section, and config-section to avoid issues down the line?
There was a problem hiding this comment.
The fetch() calls for restart/shutdown don't specify { method: 'POST' }. They default to GET. If the backend endpoints get fixed to POST-only (as suggested above), these need updating too:
fetch(url, { method: 'POST' }).then(response => { ... });|
@shlokgilda some context: i think the point of this PR is mainly to bring the admin page back (it used to be merged but somehow got unmerged) definitely agree with a lot of the feedback though im very hesitant about including "start and stop your instance" controls in our mixed manual/docker setup environment |
|
As discussed in the March 10 maintainers meeting: Most of these API endpoints, especially for restarting the Instance, assume a non-docker install. Thus this restarting only works on bare metal Many of these endpoints are likely disabled anyway and Augur’s core functionality has changed since this was merged Id like to avoid merging endpoints that are disabled and dont have a future so we dont need a followup PR to remove them later |
I will wait for @Ulincsys to address these points. Have we verified the restart does not work on Docker? I may have missed that discussion point at the last meeting. |
|
@sgoggins I'm planning to address the comments on this tonight after I get home. We did discuss the restart endpoints during the last meeting. They make the assumption that they are not running in Docker, and so should are not suitable for attempting a restart from within Docker |
|
I see potentially two forward paths:
It seems like the benefit of this UI was for admin convenience when augur was not yet dockerized and startup/shutdown was a special sequence of commands that needed remembering/referring to. with docker, it can be as simple as running I worry that, if we have endpoints that allow instances to be restarted in the UI, that could present a much larger DoS attack surface, or general user inconvenience in multi-admin/multi-tenancy scenarios, than the convenience it provides. I can see a benefit to controlling augur tasks within the admin dashboard, but im not sure we need shutdown and restart endpoints |
|
Jumpstart does appear to still be in the repo. last touched 2 years ago. Im very tempted to exclude it from the docker builds. maybe we can have the code check if jumpstart is available before trying to use it and make these endpoints no-ops on docker? |
Description
admin_requireddecorator to protect admin routesNotes for Reviewers
augur user addcommand with the--adminflagSigned commits