Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions augur/api/routes/__init__.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
AUGUR_API_VERSION = 'api/unstable'

from .application import *
from .admin import *
from .batch import *
from .collection_status import *
from .config import *
Expand Down
27 changes: 27 additions & 0 deletions augur/api/routes/admin.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
from augur.api.routes import AUGUR_API_VERSION
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

Consider possible security implications associated with the subprocess module.
from flask import Response, current_app, jsonify

from augur.application.db.lib import get_value
from augur.application.logs import AugurLogger
from augur.api.util import admin_required

logger = AugurLogger("augur").get_logger()

@app.route(f"/{AUGUR_API_VERSION}/admin/shutdown")
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Defaults to GET requests. Shouldn't this be POST only? Would prevent CSRF.

@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

subprocess call - check for execution of untrusted input.
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

Starting a process with a partial executable path

Check notice

Code scanning / Bandit

subprocess call with shell=True seems safe, but may be changed in the future, consider rewriting without shell Note

subprocess call with shell=True seems safe, but may be changed in the future, consider rewriting without shell

return jsonify({"status": "shutdown"})

@app.route(f"/{AUGUR_API_VERSION}/admin/restart")
@admin_required
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same as admin/shudown. I think this should also be POST only.

def restart_system():
Popen("python scripts/control/restart.py", shell=True)

Check notice

Code scanning / Bandit

Starting a process with a partial executable path Note

Starting a process with a partial executable path

Check notice

Code scanning / Bandit

subprocess call with shell=True seems safe, but may be changed in the future, consider rewriting without shell Note

subprocess call with shell=True seems safe, but may be changed in the future, consider rewriting without shell
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can we use absolute path derived from __file__ or the project root instead of relative path?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

also is this script going to work for docker installs?


return jsonify({"status": "restart"})
43 changes: 25 additions & 18 deletions augur/api/routes/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,43 +7,52 @@
import sqlalchemy as s

# Disable the requirement for SSL by setting env["AUGUR_DEV"] = True
from augur.application.config import get_development_flag
from augur.api.util import ssl_required, admin_required
from augur.application.db.lib import get_session
from augur.application.db.models import Config
from augur.application.config import AugurConfig
from augur.application.db.session import DatabaseSession
from ..server import app

logger = logging.getLogger(__name__)
development = get_development_flag()

from augur.api.routes import AUGUR_API_VERSION

def generate_upgrade_request():
# https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/426
response = jsonify({"status": "SSL Required"})
response.headers["Upgrade"] = "TLS"
response.headers["Connection"] = "Upgrade"

return response, 426

@app.route(f"/{AUGUR_API_VERSION}/config/get", methods=['GET', 'POST'])
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why are we allowing POST here?

@ssl_required
def get_config():
if not development and not request.is_secure:
return generate_upgrade_request()

with DatabaseSession(logger, engine=current_app.engine) as session:

config_dict = AugurConfig(logger, session).config.load_config()

return jsonify(config_dict), 200

@app.route(f"/{AUGUR_API_VERSION}/config/set", methods=['GET', 'POST'])
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

@ssl_required
@admin_required
def set_config_item():
setting = request.args.get("setting")
section = request.args.get("section")
value = request.values.get("value")

result = {
"section_name": section,
"setting_name": setting,
"value": value
}

if not setting or not section or not value:
return jsonify({"status": "Missing argument"}), 400

with get_session() as session:
config = AugurConfig(logger, session)
config.add_or_update_settings([result])

return jsonify({"status": "success"})

@app.route(f"/{AUGUR_API_VERSION}/config/update", methods=['POST'])
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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:

Suggested change
@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():

@ssl_required
def update_config():
if not development and not request.is_secure:
return generate_upgrade_request()

update_dict = request.get_json()

with get_session() as session:
Expand All @@ -64,5 +73,3 @@ def update_config():
session.commit()

return jsonify({"status": "success"}), 200


25 changes: 22 additions & 3 deletions augur/api/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,16 @@
import re
import beaker

from flask import request, jsonify, current_app
from flask import request, jsonify, current_app, abort
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[pylint] reported by reviewdog 🐶
W0611: Unused current_app imported from flask (unused-import)


from augur.application.db import get_session
from functools import wraps
from sqlalchemy.orm.exc import NoResultFound
from augur.application.config import get_development_flag
from augur.application.db.models import ClientApplication

from flask_login import login_required, current_user

development = get_development_flag()

__ROOT = os.path.abspath(os.path.dirname(__file__))
Expand Down Expand Up @@ -112,7 +114,6 @@ def get_client_token():

return token


# usage:
"""
@app.route("/path")
Expand Down Expand Up @@ -155,4 +156,22 @@ def wrapper(*args, **kwargs):
return generate_upgrade_request()
return fun(*args, **kwargs)

return wrapper
return wrapper

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
Comment on lines +161 to +169
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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:

Suggested change
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


def development_required(func):
@wraps(func)
def inner_function(*args, **kwargs):
if not development:
abort(403)
return func(*args, **kwargs)
return inner_function
35 changes: 27 additions & 8 deletions augur/api/view/augur_view.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
from flask import render_template, redirect, url_for, session, request, jsonify
from flask_login import LoginManager
from flask_login import LoginManager, current_user, login_required
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[pylint] reported by reviewdog 🐶
W0611: Unused current_user imported from flask_login (unused-import)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[pylint] reported by reviewdog 🐶
W0611: Unused login_required imported from flask_login (unused-import)

from io import StringIO
from .utils import *
from .init import logger
from .url_converters import *

from functools import wraps
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[pylint] reported by reviewdog 🐶
W0611: Unused wraps imported from functools (unused-import)


# from .server import User
from ..server import app, db_session
from augur.application.db.models import User, UserSessionToken
Expand Down Expand Up @@ -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
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[pylint] reported by reviewdog 🐶
E0602: Undefined variable 'render_message' (undefined-variable)


@app.errorhandler(403)
def forbidden(error):
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
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[pylint] reported by reviewdog 🐶
E0602: Undefined variable 'render_message' (undefined-variable)


@app.errorhandler(500)
def internal_server_error(error):
if AUGUR_API_VERSION in str(request.path):
Expand All @@ -52,8 +61,21 @@ def internal_server_error(error):
errout.close()
except Exception as e:
logger.error(e)
raise e
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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?


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.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[pylint] reported by reviewdog 🐶
E0602: Undefined variable 'render_message' (undefined-variable)

Please try again, and if the error persists, please file a GitHub issue with a description
of what you were doing before this error occurred accompanied by the below error message:""", error=stacktrace), 500

@app.template_filter("escape_ID")
def escape_HTML_ID(data: str) -> str:
# Done this way in case we want to add more replacements in the future
data = data.replace(".", "\\.")
return data

@app.template_filter("quoted")
def quote_surrounded(data: str) -> str:
return '"' + data + '"'

@login_manager.unauthorized_handler
def unauthorized():
Expand Down Expand Up @@ -98,19 +120,16 @@ def load_user(user_id):
@login_manager.request_loader
def load_user_request(request):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[pylint] reported by reviewdog 🐶
W0621: Redefining name 'request' from outer scope (line 1) (redefined-outer-name)

token = get_bearer_token()

current_time = int(time.time())
token = db_session.query(UserSessionToken).filter(UserSessionToken.token == token, UserSessionToken.expiration >= current_time).first()
if token:

print("Valid user")
token = db_session.query(UserSessionToken).filter(UserSessionToken.token == token, UserSessionToken.expiration >= current_time).first()

if token:
user = token.user
user._is_authenticated = True
user._is_active = True

return user

return None

@app.template_filter('as_datetime')
Expand Down
25 changes: 23 additions & 2 deletions augur/api/view/routes.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,23 @@
"""
import logging
import math
import os
import signal
from flask import render_template, request, redirect, url_for, session, flash
from .utils import *
from augur.api.util import admin_required, development_required
from flask_login import login_user, logout_user, current_user, login_required
from sqlalchemy.exc import OperationalError

from augur.application.db.models import User, Repo, ClientApplication
from .server import LoginException
from augur.application.util import *
from augur.application.db.lib import get_value
from augur.application.config import AugurConfig
from ..server import app, db_session

from augur.application.db.lib import get_session

logger = logging.getLogger(__name__)


Expand Down Expand Up @@ -112,6 +119,10 @@ def repo_card_view():
@app.route('/collection/status')
def status_view():
return render_module("status", title="Status")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[pylint] reported by reviewdog 🐶
E0602: Undefined variable 'render_module' (undefined-variable)


@app.route('/connection_status')
def server_ping_frontend():
return render_module("ping")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[pylint] reported by reviewdog 🐶
E0602: Undefined variable 'render_module' (undefined-variable)


""" ----------------------------------------------------------------
login:
Expand Down Expand Up @@ -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)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[pylint] reported by reviewdog 🐶
E0602: Undefined variable 'render_module' (undefined-variable)


@app.route('/error')
@development_required
def throw_exception():
raise Exception("This Exception intentionally raised")

Expand All @@ -326,6 +338,7 @@ def throw_exception():
View the admin dashboard.
"""
@app.route('/dashboard')
@admin_required
def dashboard_view():
empty = [
{ "title": "Placeholder", "settings": [
Expand All @@ -337,6 +350,14 @@ def dashboard_view():
]}
]

backend_config = requestJson("config/get", False)
backend_config = AugurConfig(logger, db_session).load_config()

with get_session() as session:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[pylint] reported by reviewdog 🐶
W0621: Redefining name 'session' from outer scope (line 8) (redefined-outer-name)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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?

try:
users = session.query(User).all()
except OperationalError as e:
# Instruct Gunicorn to reboot workers to resolve database connection instability
os.kill(os.getpid(), signal.SIGTERM)
return "reloading"

return render_template('admin-dashboard.j2', sections = empty, config = backend_config)
return render_template('admin-dashboard.j2', sections = empty, config = backend_config, users = users)
Loading
Loading