From 98109a28cd834f7093bba69a21b54f59b6b9bba8 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Fri, 24 Jan 2014 14:12:04 -0500 Subject: [PATCH 01/20] Better error messages when using the API, index and registry --- endpoints/api.py | 119 +++++++++++++++++---------------------- endpoints/index.py | 13 +++++ endpoints/registry.py | 110 ++++++++++++++++++++++++------------ static/js/controllers.js | 2 +- 4 files changed, 142 insertions(+), 102 deletions(-) diff --git a/endpoints/api.py b/endpoints/api.py index 21054b38b..90b0140d9 100644 --- a/endpoints/api.py +++ b/endpoints/api.py @@ -48,6 +48,38 @@ def csrf_protect(): abort(403) +@api.errorhandler(404) +def endpoint_not_found(e): + return jsonify({ + 'error_code': 404, + 'message': 'Resource not found' + }) + + +@api.errorhandler(403) +def endpoint_forbidden(e): + return jsonify({ + 'error_code': 403, + 'message': 'Permission Denied' + }) + + +@api.errorhandler(400) +def endpoint_invalid_request(e): + return jsonify({ + 'error_code': 400, + 'message': 'Invalid Request' + }) + + +def request_error(exception = None, **kwargs): + data = kwargs.copy() + if exception: + data['message'] = ex.message + + return make_response(jsonify(data), 400) + + def get_route_data(): global route_data if route_data: @@ -132,7 +164,7 @@ def discovery(): @api.route('/') @internal_api_call def welcome(): - return make_response('welcome', 200) + return jsonify({'version': '0.5'}) @api.route('/plans/') @@ -222,20 +254,12 @@ def convert_user_to_organization(): # Ensure that the new admin user is the not user being converted. admin_username = convert_data['adminUser'] if admin_username == user.username: - error_resp = jsonify({ - 'reason': 'invaliduser' - }) - error_resp.status_code = 400 - return error_resp + return request_error(reason = 'invaliduser', message = 'The admin user is not valid') # Ensure that the sign in credentials work. admin_password = convert_data['adminPassword'] if not model.verify_user(admin_username, admin_password): - error_resp = jsonify({ - 'reason': 'invaliduser' - }) - error_resp.status_code = 400 - return error_resp + return request_error(reason = 'invaliduser', message = 'The admin user credentials are not valid') # Subscribe the organization to the new plan. plan = convert_data['plan'] @@ -271,22 +295,14 @@ def change_user_details(): new_email = user_data['email'] if model.find_user_by_email(new_email): # Email already used. - error_resp = jsonify({ - 'message': 'E-mail address already used' - }) - error_resp.status_code = 400 - return error_resp + return request_error(message = 'E-mail address already used') logger.debug('Sending email to change email address for user: %s', user.username) code = model.create_confirm_email_code(user, new_email=new_email) send_change_email(user.username, user_data['email'], code.code) except model.InvalidPasswordException, ex: - error_resp = jsonify({ - 'message': ex.message, - }) - error_resp.status_code = 400 - return error_resp + return request_error(exception = ex) return jsonify(user_view(user)) @@ -298,11 +314,7 @@ def create_new_user(): existing_user = model.get_user(user_data['username']) if existing_user: - error_resp = jsonify({ - 'message': 'The username already exists' - }) - error_resp.status_code = 400 - return error_resp + return request_error(message = 'The username already exists') try: new_user = model.create_user(user_data['username'], user_data['password'], @@ -311,11 +323,7 @@ def create_new_user(): send_confirmation_email(new_user.username, new_user.email, code.code) return make_response('Created', 201) except model.DataModelException as ex: - error_resp = jsonify({ - 'message': ex.message, - }) - error_resp.status_code = 400 - return error_resp + return request_error(exception = ex) @api.route('/signin', methods=['POST']) @@ -336,7 +344,7 @@ def conduct_signin(username_or_email, password): verified = model.verify_user(username_or_email, password) if verified: if common_login(verified): - return make_response('Success', 200) + return jsonify({'success': True}) else: needs_email_verification = True @@ -357,7 +365,7 @@ def conduct_signin(username_or_email, password): def logout(): logout_user() identity_changed.send(app, identity=AnonymousIdentity()) - return make_response('Success', 200) + return jsonify({'success': True}) @api.route("/recovery", methods=['POST']) @@ -459,22 +467,14 @@ def create_organization(): pass if existing: - error_resp = jsonify({ - 'message': 'A user or organization with this name already exists' - }) - error_resp.status_code = 400 - return error_resp + return request_error(message = 'A user or organization with this name already exists') try: model.create_organization(org_data['name'], org_data['email'], current_user.db_user()) return make_response('Created', 201) except model.DataModelException as ex: - error_resp = jsonify({ - 'message': ex.message, - }) - error_resp.status_code = 400 - return error_resp + return request_error(exception = ex) def org_view(o, teams): @@ -529,12 +529,7 @@ def change_organization_details(orgname): if 'email' in org_data and org_data['email'] != org.email: new_email = org_data['email'] if model.find_user_by_email(new_email): - # Email already used. - error_resp = jsonify({ - 'message': 'E-mail address already used' - }) - error_resp.status_code = 400 - return error_resp + return request_error(message = 'E-mail address already used') logger.debug('Changing email address for organization: %s', org.username) model.update_email(org, new_email) @@ -637,10 +632,10 @@ def create_organization_prototype_permission(orgname): if delegate_teamname else None) if activating_username and not activating_user: - abort(404) + return request_error(message = 'Unknown activating user') if not delegate_user and not delegate_team: - abort(400) + return request_error(message = 'Missing delagate user or team') role_name = details['role'] @@ -898,7 +893,7 @@ def update_organization_team_member(orgname, teamname, membername): # Find the user. user = model.get_user(membername) if not user: - abort(400) + return request_error(message = 'Unknown user') # Add the user to the team. model.add_user_to_team(user, team) @@ -939,7 +934,7 @@ def create_repo(): existing = model.get_repository(namespace_name, repository_name) if existing: - return make_response('Repository already exists', 400) + return request_error(message = 'Repository already exists') visibility = req['visibility'] @@ -1542,11 +1537,7 @@ def change_user_permissions(namespace, repository, username): # This repository is not part of an organization pass except model.DataModelException as ex: - error_resp = jsonify({ - 'message': ex.message, - }) - error_resp.status_code = 400 - return error_resp + return request_error(exception = ex) log_action('change_repo_permission', namespace, {'username': username, 'repo': repository, @@ -1599,11 +1590,7 @@ def delete_user_permissions(namespace, repository, username): try: model.delete_user_permission(username, namespace, repository) except model.DataModelException as ex: - error_resp = jsonify({ - 'message': ex.message, - }) - error_resp.status_code = 400 - return error_resp + return request_error(exception = ex) log_action('delete_repo_permission', namespace, {'username': username, 'repo': repository}, @@ -1859,7 +1846,7 @@ def subscribe(user, plan, token, require_business_plan): plan_found['price'] == 0): logger.warning('Business attempting to subscribe to personal plan: %s', user.username) - abort(400) + return request_error(message = 'No matching plan found') private_repos = model.get_private_repo_count(user.username) @@ -2089,7 +2076,7 @@ def delete_user_robot(robot_shortname): parent = current_user.db_user() model.delete_robot(format_robot_username(parent.username, robot_shortname)) log_action('delete_robot', parent.username, {'robot': robot_shortname}) - return make_response('No Content', 204) + return make_response('Deleted', 204) @api.route('/organization//robots/', @@ -2101,7 +2088,7 @@ def delete_org_robot(orgname, robot_shortname): if permission.can(): model.delete_robot(format_robot_username(orgname, robot_shortname)) log_action('delete_robot', orgname, {'robot': robot_shortname}) - return make_response('No Content', 204) + return make_response('Deleted', 204) abort(403) diff --git a/endpoints/index.py b/endpoints/index.py index b3896bf91..122036c7a 100644 --- a/endpoints/index.py +++ b/endpoints/index.py @@ -52,6 +52,19 @@ def generate_headers(role='read'): return decorator_method +@index.errorhandler(404) +def fallback_not_found(e): + return make_response('Not Found', 404) + +@index.errorhandler(403) +def fallback_forbidden(e): + return make_response('Forbidden', 403) + +@index.errorhandler(400) +def fallback_invalid_request(e): + return make_response('Invalid Request', 400) + + @index.route('/users', methods=['POST']) @index.route('/users/', methods=['POST']) def create_user(): diff --git a/endpoints/registry.py b/endpoints/registry.py index 57895aebe..5e8a83ced 100644 --- a/endpoints/registry.py +++ b/endpoints/registry.py @@ -1,7 +1,7 @@ import logging import json -from flask import (make_response, request, session, Response, abort, +from flask import (make_response, request, session, Response, abort as flask_abort, redirect, Blueprint) from functools import wraps from datetime import datetime @@ -10,7 +10,7 @@ from time import time from data.queue import image_diff_queue from app import app -from auth.auth import process_auth, extract_namespace_repo_from_session +from auth.auth import process_auth, extract_namespace_repo_from_session, get_authenticated_user, get_validated_token from util import checksums, changes from auth.permissions import (ReadRepositoryPermission, ModifyRepositoryPermission) @@ -21,6 +21,38 @@ registry = Blueprint('registry', __name__) store = app.config['STORAGE'] logger = logging.getLogger(__name__) +DEFAULT_MESSAGE = {} +DEFAULT_MESSAGE[400] = 'Invalid Request' +DEFAULT_MESSAGE[403] = 'Forbidden' +DEFAULT_MESSAGE[404] = 'Not Found' + +@registry.errorhandler(404) +def fallback_not_found(e): + return make_response('Not Found', 404) + +@registry.errorhandler(403) +def fallback_forbidden(e): + return make_response('Forbidden', 403) + +@registry.errorhandler(400) +def fallback_invalid_request(e): + return make_response('Invalid Request', 400) + +def abort(status_code, message=None, **kwargs): + if status_code == 403 and not message: + # Create a default error message for auth failure. + message = 'Forbidden. ' + auth_user = get_authenticated_user() + auth_token = get_validated_token() + if auth_user: + message = message + 'Current user: ' + auth_user + elif auth_token: + message = message + 'Current token: ' + auth_token + + message = message % kwargs if message else DEFAULT_MESSAGE[status_code] + log.error('Error %s: %s' % (status_code, message)) + flask_abort(make_response(HTTPException(message), status_code, headers)) + class SocketReader(object): def __init__(self, fp): @@ -45,8 +77,8 @@ def require_completion(f): def wrapper(namespace, repository, *args, **kwargs): if store.exists(store.image_mark_path(namespace, repository, kwargs['image_id'])): - logger.warning('Image is already being uploaded: %s', kwargs['image_id']) - abort(400) # 'Image is being uploaded, retry later') + abort(400, 'Image %(image_id)s is being uploaded, retry later', image_id=kwargs['image_id']) + return f(namespace, repository, *args, **kwargs) return wrapper @@ -90,9 +122,8 @@ def get_image_layer(namespace, repository, image_id, headers): try: return Response(store.stream_read(path), headers=headers) except IOError: - logger.warning('Image not found: %s', image_id) - abort(404) # 'Image not found', 404) - + abort(404, 'Image %(image_id)s not found', image_id=image_id) + abort(403) @@ -108,11 +139,11 @@ def put_image_layer(namespace, repository, image_id): json_data = store.get_content(store.image_json_path(namespace, repository, image_id)) except IOError: - abort(404) # 'Image not found', 404) + abort(404, 'Image not found') layer_path = store.image_layer_path(namespace, repository, image_id) mark_path = store.image_mark_path(namespace, repository, image_id) if store.exists(layer_path) and not store.exists(mark_path): - abort(409) # 'Image already exists', 409) + abort(409, 'Image already exists') input_stream = request.stream if request.headers.get('transfer-encoding') == 'chunked': # Careful, might work only with WSGI servers supporting chunked @@ -151,7 +182,8 @@ def put_image_layer(namespace, repository, image_id): # We check if the checksums provided matches one the one we computed if checksum not in csums: logger.warning('put_image_layer: Wrong checksum') - abort(400) # 'Checksum mismatch, ignoring the layer') + abort(400, 'Checksum mismatch; ignoring the layer') + # Checksum is ok, we remove the marker store.remove(mark_path) @@ -177,24 +209,28 @@ def put_image_checksum(namespace, repository, image_id): checksum = request.headers.get('X-Docker-Checksum') if not checksum: - logger.warning('Missing Image\'s checksum: %s', image_id) - abort(400) # 'Missing Image\'s checksum') + abort(400, "Missing checksum for image %(image_id)s", image_id=image_id) + if not session.get('checksum'): - logger.warning('Checksum not found in Cookie for image: %s', image_id) - abort(400) # 'Checksum not found in Cookie') + abort(400, 'Checksum not found in Cookie for image %(imaage_id)s', image_id=image_id) + if not store.exists(store.image_json_path(namespace, repository, image_id)): - abort(404) # 'Image not found', 404) + abort(404, 'Image not found: %(image_id)s', image_id=image_id) + mark_path = store.image_mark_path(namespace, repository, image_id) if not store.exists(mark_path): - abort(409) # 'Cannot set this image checksum', 409) + abort(409, 'Cannot set checksum for image %(image_id)s', image_id=image_id) + err = store_checksum(namespace, repository, image_id, checksum) if err: - abort(err) + abort(400, err) + if checksum not in session.get('checksum', []): logger.debug('session checksums: %s' % session.get('checksum', [])) logger.debug('client supplied checksum: %s' % checksum) logger.debug('put_image_layer: Wrong checksum') - abort(400) # 'Checksum mismatch') + abort(400, 'Checksum mismatch for image: %(image_id)s', image_id=image_id) + # Checksum is ok, we remove the marker store.remove(mark_path) @@ -225,16 +261,19 @@ def get_image_json(namespace, repository, image_id, headers): data = store.get_content(store.image_json_path(namespace, repository, image_id)) except IOError: - abort(404) # 'Image not found', 404) + abort(404, 'Image %(image_id)%s not found', image_id=image_id) + try: size = store.get_size(store.image_layer_path(namespace, repository, image_id)) headers['X-Docker-Size'] = str(size) except OSError: pass + checksum_path = store.image_checksum_path(namespace, repository, image_id) if store.exists(checksum_path): headers['X-Docker-Checksum'] = store.get_content(checksum_path) + response = make_response(data, 200) response.headers.extend(headers) return response @@ -255,7 +294,8 @@ def get_image_ancestry(namespace, repository, image_id, headers): data = store.get_content(store.image_ancestry_path(namespace, repository, image_id)) except IOError: - abort(404) # 'Image not found', 404) + abort(404, 'Image %(image_id)s not found', image_id=image_id) + response = make_response(json.dumps(json.loads(data)), 200) response.headers.extend(headers) return response @@ -280,6 +320,7 @@ def store_checksum(namespace, repository, image_id, checksum): checksum_parts = checksum.split(':') if len(checksum_parts) != 2: return 'Invalid checksum format' + # We store the checksum checksum_path = store.image_checksum_path(namespace, repository, image_id) store.put_content(checksum_path, checksum) @@ -298,36 +339,35 @@ def put_image_json(namespace, repository, image_id): except json.JSONDecodeError: pass if not data or not isinstance(data, dict): - logger.warning('Invalid JSON for image: %s json: %s', image_id, - request.data) - abort(400) # 'Invalid JSON') + abort(400, 'Invalid JSON for image: %(image_id)s\nJSON: %(json)s', image_id=image_id, json=request.data) + if 'id' not in data: - logger.warning('Missing key `id\' in JSON for image: %s', image_id) - abort(400) # 'Missing key `id\' in JSON') + abort(400, 'Missing key `id` in JSON for image: %(image_id)s', image_id=image_id) + # Read the checksum checksum = request.headers.get('X-Docker-Checksum') if checksum: # Storing the checksum is optional at this stage err = store_checksum(namespace, repository, image_id, checksum) if err: - abort(err) + abort(400, err) + else: # We cleanup any old checksum in case it's a retry after a fail store.remove(store.image_checksum_path(namespace, repository, image_id)) if image_id != data['id']: - logger.warning('JSON data contains invalid id for image: %s', image_id) - abort(400) # 'JSON data contains invalid id') + abort(400, 'JSON data contains invalid id for image: %(image_id)s', image_id=image_id) + parent_id = data.get('parent') - if parent_id and not store.exists(store.image_json_path(namespace, - repository, - data['parent'])): - logger.warning('Image depends on a non existing parent image: %s', - image_id) - abort(400) # 'Image depends on a non existing parent') + if parent_id and not store.exists(store.image_json_path(namespace, repository, parent_id)): + abort(400, 'Image %(image_id)s depends on non existing parent image %(parent_id)s', + image_id=image_id, parent_id=parent_id) + json_path = store.image_json_path(namespace, repository, image_id) mark_path = store.image_mark_path(namespace, repository, image_id) if store.exists(json_path) and not store.exists(mark_path): - abort(409) # 'Image already exists', 409) + abort(409, 'Image %(image_id)s already exists', image_id=image_id) + # If we reach that point, it means that this is a new image or a retry # on a failed push # save the metadata diff --git a/static/js/controllers.js b/static/js/controllers.js index 8f10ca5ef..fc79963d1 100644 --- a/static/js/controllers.js +++ b/static/js/controllers.js @@ -1242,7 +1242,7 @@ function NewRepoCtrl($scope, $location, $http, $timeout, UserService, ApiService $location.path('/repository/' + created.namespace + '/' + created.name); }, function(result) { $scope.creating = false; - $scope.createError = result.data; + $scope.createError = result.data ? result.data.message : 'Cannot create repository'; $timeout(function() { $('#repoName').popover('show'); }); From aa5f669e690d791956da233eb7031b7b14fea7a2 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Fri, 24 Jan 2014 14:25:41 -0500 Subject: [PATCH 02/20] Add the request args --- endpoints/registry.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/endpoints/registry.py b/endpoints/registry.py index 5e8a83ced..5ec1e02fa 100644 --- a/endpoints/registry.py +++ b/endpoints/registry.py @@ -50,7 +50,11 @@ def abort(status_code, message=None, **kwargs): message = message + 'Current token: ' + auth_token message = message % kwargs if message else DEFAULT_MESSAGE[status_code] - log.error('Error %s: %s' % (status_code, message)) + + # Log the abort. + log.error('Error %s: %s. Arguments: %s' % (status_code, message, request.view_args)) + + # Report the abort to the user. flask_abort(make_response(HTTPException(message), status_code, headers)) From 30a26d099f7945eaca4832e4f76bda901cd8ec32 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Fri, 24 Jan 2014 15:01:40 -0500 Subject: [PATCH 03/20] Have the index use the same abort method --- endpoints/index.py | 4 +++- endpoints/registry.py | 26 ++------------------------ util/http.py | 27 +++++++++++++++++++++++++++ 3 files changed, 32 insertions(+), 25 deletions(-) create mode 100644 util/http.py diff --git a/endpoints/index.py b/endpoints/index.py index 122036c7a..9cb6f61f5 100644 --- a/endpoints/index.py +++ b/endpoints/index.py @@ -2,7 +2,7 @@ import json import logging import urlparse -from flask import request, make_response, jsonify, abort, session, Blueprint +from flask import request, make_response, jsonify, abort as flask_abort, session, Blueprint from functools import wraps from data import model @@ -16,6 +16,8 @@ from auth.permissions import (ModifyRepositoryPermission, UserPermission, ReadRepositoryPermission, CreateRepositoryPermission) +from util.http import abort + logger = logging.getLogger(__name__) index = Blueprint('index', __name__) diff --git a/endpoints/registry.py b/endpoints/registry.py index 5ec1e02fa..3218e38bf 100644 --- a/endpoints/registry.py +++ b/endpoints/registry.py @@ -10,8 +10,9 @@ from time import time from data.queue import image_diff_queue from app import app -from auth.auth import process_auth, extract_namespace_repo_from_session, get_authenticated_user, get_validated_token +from auth.auth import process_auth, extract_namespace_repo_from_session from util import checksums, changes +from util.http import abort from auth.permissions import (ReadRepositoryPermission, ModifyRepositoryPermission) from data import model @@ -21,10 +22,6 @@ registry = Blueprint('registry', __name__) store = app.config['STORAGE'] logger = logging.getLogger(__name__) -DEFAULT_MESSAGE = {} -DEFAULT_MESSAGE[400] = 'Invalid Request' -DEFAULT_MESSAGE[403] = 'Forbidden' -DEFAULT_MESSAGE[404] = 'Not Found' @registry.errorhandler(404) def fallback_not_found(e): @@ -38,25 +35,6 @@ def fallback_forbidden(e): def fallback_invalid_request(e): return make_response('Invalid Request', 400) -def abort(status_code, message=None, **kwargs): - if status_code == 403 and not message: - # Create a default error message for auth failure. - message = 'Forbidden. ' - auth_user = get_authenticated_user() - auth_token = get_validated_token() - if auth_user: - message = message + 'Current user: ' + auth_user - elif auth_token: - message = message + 'Current token: ' + auth_token - - message = message % kwargs if message else DEFAULT_MESSAGE[status_code] - - # Log the abort. - log.error('Error %s: %s. Arguments: %s' % (status_code, message, request.view_args)) - - # Report the abort to the user. - flask_abort(make_response(HTTPException(message), status_code, headers)) - class SocketReader(object): def __init__(self, fp): diff --git a/util/http.py b/util/http.py new file mode 100644 index 000000000..fe2f09172 --- /dev/null +++ b/util/http.py @@ -0,0 +1,27 @@ +from flask import request, abort as flask_abort +from auth.auth import process_auth, extract_namespace_repo_from_session, get_authenticated_user, get_validated_token + +DEFAULT_MESSAGE = {} +DEFAULT_MESSAGE[400] = 'Invalid Request' +DEFAULT_MESSAGE[403] = 'Forbidden' +DEFAULT_MESSAGE[404] = 'Not Found' + +def abort(status_code, message=None, **kwargs): + if status_code == 403 and not message: + # Create a default error message for auth failure. + message = 'Forbidden. ' + auth_user = get_authenticated_user() + auth_token = get_validated_token() + if auth_user: + message = message + 'Current user: ' + auth_user + elif auth_token: + message = message + 'Current token: ' + auth_token + + message = message % kwargs if message else DEFAULT_MESSAGE[status_code] + + # Log the abort. + log.error('Error %s: %s. Arguments: %s' % (status_code, message, request.view_args)) + + # Report the abort to the user. + flask_abort(make_response(HTTPException(message), status_code, headers)) + From 77f2706a195b2ed485d8a4403ce8a23ee38bda87 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Fri, 24 Jan 2014 15:06:35 -0500 Subject: [PATCH 04/20] =?UTF-8?q?Properly=20use=20the=20auth=E2=80=99ed=20?= =?UTF-8?q?objects?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- util/http.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/util/http.py b/util/http.py index fe2f09172..8cad8a209 100644 --- a/util/http.py +++ b/util/http.py @@ -13,9 +13,9 @@ def abort(status_code, message=None, **kwargs): auth_user = get_authenticated_user() auth_token = get_validated_token() if auth_user: - message = message + 'Current user: ' + auth_user + message = message + 'Current user: ' + auth_user.username elif auth_token: - message = message + 'Current token: ' + auth_token + message = message + 'Current token: ' + auth_token.friendly_name or auth_token.code message = message % kwargs if message else DEFAULT_MESSAGE[status_code] From efcbe4421d2c0959366c7d1db087904e467c3f2e Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Fri, 24 Jan 2014 15:09:35 -0500 Subject: [PATCH 05/20] Fix logger --- util/http.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/util/http.py b/util/http.py index 8cad8a209..b607fc01b 100644 --- a/util/http.py +++ b/util/http.py @@ -1,6 +1,10 @@ +import logging + from flask import request, abort as flask_abort from auth.auth import process_auth, extract_namespace_repo_from_session, get_authenticated_user, get_validated_token +logger = logging.getLogger(__name__) + DEFAULT_MESSAGE = {} DEFAULT_MESSAGE[400] = 'Invalid Request' DEFAULT_MESSAGE[403] = 'Forbidden' @@ -20,7 +24,7 @@ def abort(status_code, message=None, **kwargs): message = message % kwargs if message else DEFAULT_MESSAGE[status_code] # Log the abort. - log.error('Error %s: %s. Arguments: %s' % (status_code, message, request.view_args)) + logger.error('Error %s: %s. Arguments: %s' % (status_code, message, request.view_args)) # Report the abort to the user. flask_abort(make_response(HTTPException(message), status_code, headers)) From fd4ac04b5a4fe74a7561e4b6ad0ece79aefaa72e Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Fri, 24 Jan 2014 15:10:51 -0500 Subject: [PATCH 06/20] HTTPException import --- util/http.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/util/http.py b/util/http.py index b607fc01b..bb43bd7f8 100644 --- a/util/http.py +++ b/util/http.py @@ -1,7 +1,8 @@ import logging -from flask import request, abort as flask_abort +from flask import request, abort as flask_abort, make_response from auth.auth import process_auth, extract_namespace_repo_from_session, get_authenticated_user, get_validated_token +from werkzeug.exceptions import HTTPException logger = logging.getLogger(__name__) From 229eeec1be396cc945c0916e2d98767905b07c40 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Fri, 24 Jan 2014 15:12:11 -0500 Subject: [PATCH 07/20] =?UTF-8?q?Don=E2=80=99t=20have=20a=20headers=20var?= =?UTF-8?q?=20anymore?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- util/http.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/util/http.py b/util/http.py index bb43bd7f8..764a8e01d 100644 --- a/util/http.py +++ b/util/http.py @@ -28,5 +28,5 @@ def abort(status_code, message=None, **kwargs): logger.error('Error %s: %s. Arguments: %s' % (status_code, message, request.view_args)) # Report the abort to the user. - flask_abort(make_response(HTTPException(message), status_code, headers)) + flask_abort(make_response(HTTPException(message), status_code, {})) From 4cea2a6449294837de12174b39b2c1cbf901dc89 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Fri, 24 Jan 2014 15:17:00 -0500 Subject: [PATCH 08/20] Nicer error formatting --- util/http.py | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/util/http.py b/util/http.py index 764a8e01d..26451e9dc 100644 --- a/util/http.py +++ b/util/http.py @@ -2,31 +2,29 @@ import logging from flask import request, abort as flask_abort, make_response from auth.auth import process_auth, extract_namespace_repo_from_session, get_authenticated_user, get_validated_token -from werkzeug.exceptions import HTTPException logger = logging.getLogger(__name__) DEFAULT_MESSAGE = {} DEFAULT_MESSAGE[400] = 'Invalid Request' -DEFAULT_MESSAGE[403] = 'Forbidden' +DEFAULT_MESSAGE[403] = 'Permission Denied' DEFAULT_MESSAGE[404] = 'Not Found' def abort(status_code, message=None, **kwargs): - if status_code == 403 and not message: - # Create a default error message for auth failure. - message = 'Forbidden. ' + message = message % kwargs if message else DEFAULT_MESSAGE[status_code] + + if status_code == 403: + # Add the user information. auth_user = get_authenticated_user() auth_token = get_validated_token() if auth_user: - message = message + 'Current user: ' + auth_user.username + message = '%s (user: %s)' % (message, auth_user.username) elif auth_token: - message = message + 'Current token: ' + auth_token.friendly_name or auth_token.code - - message = message % kwargs if message else DEFAULT_MESSAGE[status_code] + message = '%s (token: %s)' % (message, auth_token.friendly_name or auth_token.code) # Log the abort. logger.error('Error %s: %s. Arguments: %s' % (status_code, message, request.view_args)) # Report the abort to the user. - flask_abort(make_response(HTTPException(message), status_code, {})) + flask_abort(make_response(message, status_code, {})) From 3b62d883f4ed5fb0a729952d6635e601f40a4bf0 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Fri, 24 Jan 2014 15:20:46 -0500 Subject: [PATCH 09/20] Add missing make_response --- endpoints/common.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/endpoints/common.py b/endpoints/common.py index 2648f5e9c..ec4727edb 100644 --- a/endpoints/common.py +++ b/endpoints/common.py @@ -2,7 +2,7 @@ import logging import os import base64 -from flask import request, abort, session +from flask import request, abort, session, make_response from flask.ext.login import login_user, UserMixin from flask.ext.principal import identity_changed From 374754c9c9c6bd44b9ef29751b352e5173cb41ad Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Fri, 24 Jan 2014 15:24:26 -0500 Subject: [PATCH 10/20] Handle if message is a non-string --- util/http.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/util/http.py b/util/http.py index 26451e9dc..dd1df29d8 100644 --- a/util/http.py +++ b/util/http.py @@ -11,7 +11,7 @@ DEFAULT_MESSAGE[403] = 'Permission Denied' DEFAULT_MESSAGE[404] = 'Not Found' def abort(status_code, message=None, **kwargs): - message = message % kwargs if message else DEFAULT_MESSAGE[status_code] + message = str(message) % kwargs if message else DEFAULT_MESSAGE[status_code] if status_code == 403: # Add the user information. From 0d84cfdf1732739bc372acaca154f5ac2f33ec51 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Fri, 24 Jan 2014 15:26:32 -0500 Subject: [PATCH 11/20] Add more default messages --- util/http.py | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/util/http.py b/util/http.py index dd1df29d8..05f27970e 100644 --- a/util/http.py +++ b/util/http.py @@ -7,20 +7,21 @@ logger = logging.getLogger(__name__) DEFAULT_MESSAGE = {} DEFAULT_MESSAGE[400] = 'Invalid Request' +DEFAULT_MESSAGE[401] = 'Unauthorized' DEFAULT_MESSAGE[403] = 'Permission Denied' DEFAULT_MESSAGE[404] = 'Not Found' +DEFAULT_MESSAGE[409] = 'Conflict' def abort(status_code, message=None, **kwargs): - message = str(message) % kwargs if message else DEFAULT_MESSAGE[status_code] + message = str(message) % kwargs if message else DEFAULT_MESSAGE.get(status_code, '') - if status_code == 403: - # Add the user information. - auth_user = get_authenticated_user() - auth_token = get_validated_token() - if auth_user: - message = '%s (user: %s)' % (message, auth_user.username) - elif auth_token: - message = '%s (token: %s)' % (message, auth_token.friendly_name or auth_token.code) + # Add the user information. + auth_user = get_authenticated_user() + auth_token = get_validated_token() + if auth_user: + message = '%s (user: %s)' % (message, auth_user.username) + elif auth_token: + message = '%s (token: %s)' % (message, auth_token.friendly_name or auth_token.code) # Log the abort. logger.error('Error %s: %s. Arguments: %s' % (status_code, message, request.view_args)) From f5854303998a04ce19b6d1d0586fc838d2e4bc70 Mon Sep 17 00:00:00 2001 From: yackob03 Date: Fri, 24 Jan 2014 17:00:42 -0500 Subject: [PATCH 12/20] Make abort return a json block, which is apparently what the client expects. Remove unused imports. Fix line length and kwarg problems. --- endpoints/api.py | 47 ++++++++++++++++++++++++------------------- endpoints/index.py | 6 +++--- endpoints/registry.py | 22 ++++++++++++-------- util/http.py | 22 +++++++++++++------- 4 files changed, 58 insertions(+), 39 deletions(-) diff --git a/endpoints/api.py b/endpoints/api.py index 90b0140d9..273736910 100644 --- a/endpoints/api.py +++ b/endpoints/api.py @@ -72,10 +72,10 @@ def endpoint_invalid_request(e): }) -def request_error(exception = None, **kwargs): +def request_error(exception=None, **kwargs): data = kwargs.copy() if exception: - data['message'] = ex.message + data['message'] = exception.message return make_response(jsonify(data), 400) @@ -254,12 +254,14 @@ def convert_user_to_organization(): # Ensure that the new admin user is the not user being converted. admin_username = convert_data['adminUser'] if admin_username == user.username: - return request_error(reason = 'invaliduser', message = 'The admin user is not valid') + return request_error(reason='invaliduser', + message='The admin user is not valid') # Ensure that the sign in credentials work. admin_password = convert_data['adminPassword'] if not model.verify_user(admin_username, admin_password): - return request_error(reason = 'invaliduser', message = 'The admin user credentials are not valid') + return request_error(reason='invaliduser', + message='The admin user credentials are not valid') # Subscribe the organization to the new plan. plan = convert_data['plan'] @@ -295,14 +297,15 @@ def change_user_details(): new_email = user_data['email'] if model.find_user_by_email(new_email): # Email already used. - return request_error(message = 'E-mail address already used') + return request_error(message='E-mail address already used') - logger.debug('Sending email to change email address for user: %s', user.username) + logger.debug('Sending email to change email address for user: %s', + user.username) code = model.create_confirm_email_code(user, new_email=new_email) send_change_email(user.username, user_data['email'], code.code) except model.InvalidPasswordException, ex: - return request_error(exception = ex) + return request_error(exception=ex) return jsonify(user_view(user)) @@ -314,7 +317,7 @@ def create_new_user(): existing_user = model.get_user(user_data['username']) if existing_user: - return request_error(message = 'The username already exists') + return request_error(message='The username already exists') try: new_user = model.create_user(user_data['username'], user_data['password'], @@ -323,7 +326,7 @@ def create_new_user(): send_confirmation_email(new_user.username, new_user.email, code.code) return make_response('Created', 201) except model.DataModelException as ex: - return request_error(exception = ex) + return request_error(exception=ex) @api.route('/signin', methods=['POST']) @@ -467,14 +470,15 @@ def create_organization(): pass if existing: - return request_error(message = 'A user or organization with this name already exists') + msg = 'A user or organization with this name already exists' + return request_error(message=msg) try: model.create_organization(org_data['name'], org_data['email'], current_user.db_user()) return make_response('Created', 201) except model.DataModelException as ex: - return request_error(exception = ex) + return request_error(exception=ex) def org_view(o, teams): @@ -529,7 +533,7 @@ def change_organization_details(orgname): if 'email' in org_data and org_data['email'] != org.email: new_email = org_data['email'] if model.find_user_by_email(new_email): - return request_error(message = 'E-mail address already used') + return request_error(message='E-mail address already used') logger.debug('Changing email address for organization: %s', org.username) model.update_email(org, new_email) @@ -614,7 +618,8 @@ def create_organization_prototype_permission(orgname): details = request.get_json() activating_username = None - if 'activating_user' in details and details['activating_user'] and 'name' in details['activating_user']: + if ('activating_user' in details and details['activating_user'] and + 'name' in details['activating_user']): activating_username = details['activating_user']['name'] delegate = details['delegate'] @@ -632,10 +637,10 @@ def create_organization_prototype_permission(orgname): if delegate_teamname else None) if activating_username and not activating_user: - return request_error(message = 'Unknown activating user') + return request_error(message='Unknown activating user') if not delegate_user and not delegate_team: - return request_error(message = 'Missing delagate user or team') + return request_error(message='Missing delagate user or team') role_name = details['role'] @@ -893,7 +898,7 @@ def update_organization_team_member(orgname, teamname, membername): # Find the user. user = model.get_user(membername) if not user: - return request_error(message = 'Unknown user') + return request_error(message='Unknown user') # Add the user to the team. model.add_user_to_team(user, team) @@ -934,7 +939,7 @@ def create_repo(): existing = model.get_repository(namespace_name, repository_name) if existing: - return request_error(message = 'Repository already exists') + return request_error(message='Repository already exists') visibility = req['visibility'] @@ -1007,7 +1012,7 @@ def list_repos(): if page: try: page = int(page) - except: + except Exception: page = None username = None @@ -1537,7 +1542,7 @@ def change_user_permissions(namespace, repository, username): # This repository is not part of an organization pass except model.DataModelException as ex: - return request_error(exception = ex) + return request_error(exception=ex) log_action('change_repo_permission', namespace, {'username': username, 'repo': repository, @@ -1590,7 +1595,7 @@ def delete_user_permissions(namespace, repository, username): try: model.delete_user_permission(username, namespace, repository) except model.DataModelException as ex: - return request_error(exception = ex) + return request_error(exception=ex) log_action('delete_repo_permission', namespace, {'username': username, 'repo': repository}, @@ -1846,7 +1851,7 @@ def subscribe(user, plan, token, require_business_plan): plan_found['price'] == 0): logger.warning('Business attempting to subscribe to personal plan: %s', user.username) - return request_error(message = 'No matching plan found') + return request_error(message='No matching plan found') private_repos = model.get_private_repo_count(user.username) diff --git a/endpoints/index.py b/endpoints/index.py index 9cb6f61f5..b740bf998 100644 --- a/endpoints/index.py +++ b/endpoints/index.py @@ -2,12 +2,12 @@ import json import logging import urlparse -from flask import request, make_response, jsonify, abort as flask_abort, session, Blueprint +from flask import request, make_response, jsonify, session, Blueprint from functools import wraps from data import model from data.queue import webhook_queue -from app import app, mixpanel +from app import mixpanel from auth.auth import (process_auth, get_authenticated_user, get_validated_token) from util.names import parse_repository_name @@ -15,9 +15,9 @@ from util.email import send_confirmation_email from auth.permissions import (ModifyRepositoryPermission, UserPermission, ReadRepositoryPermission, CreateRepositoryPermission) - from util.http import abort + logger = logging.getLogger(__name__) index = Blueprint('index', __name__) diff --git a/endpoints/registry.py b/endpoints/registry.py index 3218e38bf..4d02dfd14 100644 --- a/endpoints/registry.py +++ b/endpoints/registry.py @@ -1,8 +1,8 @@ import logging import json -from flask import (make_response, request, session, Response, abort as flask_abort, - redirect, Blueprint) +from flask import (make_response, request, session, Response, redirect, + Blueprint) from functools import wraps from datetime import datetime from time import time @@ -59,7 +59,8 @@ def require_completion(f): def wrapper(namespace, repository, *args, **kwargs): if store.exists(store.image_mark_path(namespace, repository, kwargs['image_id'])): - abort(400, 'Image %(image_id)s is being uploaded, retry later', image_id=kwargs['image_id']) + abort(400, 'Image %(image_id)s is being uploaded, retry later', + image_id=kwargs['image_id']) return f(namespace, repository, *args, **kwargs) return wrapper @@ -194,7 +195,8 @@ def put_image_checksum(namespace, repository, image_id): abort(400, "Missing checksum for image %(image_id)s", image_id=image_id) if not session.get('checksum'): - abort(400, 'Checksum not found in Cookie for image %(imaage_id)s', image_id=image_id) + abort(400, 'Checksum not found in Cookie for image %(imaage_id)s', + image_id=image_id) if not store.exists(store.image_json_path(namespace, repository, image_id)): abort(404, 'Image not found: %(image_id)s', image_id=image_id) @@ -321,10 +323,12 @@ def put_image_json(namespace, repository, image_id): except json.JSONDecodeError: pass if not data or not isinstance(data, dict): - abort(400, 'Invalid JSON for image: %(image_id)s\nJSON: %(json)s', image_id=image_id, json=request.data) + abort(400, 'Invalid JSON for image: %(image_id)s\nJSON: %(json)s', + image_id=image_id, json=request.data) if 'id' not in data: - abort(400, 'Missing key `id` in JSON for image: %(image_id)s', image_id=image_id) + abort(400, 'Missing key `id` in JSON for image: %(image_id)s', + image_id=image_id) # Read the checksum checksum = request.headers.get('X-Docker-Checksum') @@ -338,10 +342,12 @@ def put_image_json(namespace, repository, image_id): # We cleanup any old checksum in case it's a retry after a fail store.remove(store.image_checksum_path(namespace, repository, image_id)) if image_id != data['id']: - abort(400, 'JSON data contains invalid id for image: %(image_id)s', image_id=image_id) + abort(400, 'JSON data contains invalid id for image: %(image_id)s', + image_id=image_id) parent_id = data.get('parent') - if parent_id and not store.exists(store.image_json_path(namespace, repository, parent_id)): + if (parent_id and not + store.exists(store.image_json_path(namespace, repository, parent_id))): abort(400, 'Image %(image_id)s depends on non existing parent image %(parent_id)s', image_id=image_id, parent_id=parent_id) diff --git a/util/http.py b/util/http.py index 05f27970e..2f8c50236 100644 --- a/util/http.py +++ b/util/http.py @@ -1,10 +1,12 @@ import logging -from flask import request, abort as flask_abort, make_response -from auth.auth import process_auth, extract_namespace_repo_from_session, get_authenticated_user, get_validated_token +from flask import request, abort as flask_abort, jsonify +from auth.auth import get_authenticated_user, get_validated_token + logger = logging.getLogger(__name__) + DEFAULT_MESSAGE = {} DEFAULT_MESSAGE[400] = 'Invalid Request' DEFAULT_MESSAGE[401] = 'Unauthorized' @@ -13,7 +15,8 @@ DEFAULT_MESSAGE[404] = 'Not Found' DEFAULT_MESSAGE[409] = 'Conflict' def abort(status_code, message=None, **kwargs): - message = str(message) % kwargs if message else DEFAULT_MESSAGE.get(status_code, '') + message = (str(message) % kwargs if message else + DEFAULT_MESSAGE.get(status_code, '')) # Add the user information. auth_user = get_authenticated_user() @@ -21,11 +24,16 @@ def abort(status_code, message=None, **kwargs): if auth_user: message = '%s (user: %s)' % (message, auth_user.username) elif auth_token: - message = '%s (token: %s)' % (message, auth_token.friendly_name or auth_token.code) + message = '%s (token: %s)' % (message, + auth_token.friendly_name or auth_token.code) # Log the abort. - logger.error('Error %s: %s. Arguments: %s' % (status_code, message, request.view_args)) + logger.warning('Error %s: %s. Arguments: %s' % (status_code, message, + request.view_args)) + + resp = jsonify({'error': message}) + resp.status_code = status_code # Report the abort to the user. - flask_abort(make_response(message, status_code, {})) - + + flask_abort(resp) From 8d074d8f3ad4e8f822296ba8d88a13a122985578 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Fri, 24 Jan 2014 20:29:25 -0500 Subject: [PATCH 13/20] Add mix panel reporting to http error codes --- util/http.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/util/http.py b/util/http.py index 05f27970e..16cc517c3 100644 --- a/util/http.py +++ b/util/http.py @@ -1,5 +1,6 @@ import logging +from app import mixpanel from flask import request, abort as flask_abort, make_response from auth.auth import process_auth, extract_namespace_repo_from_session, get_authenticated_user, get_validated_token @@ -15,16 +16,21 @@ DEFAULT_MESSAGE[409] = 'Conflict' def abort(status_code, message=None, **kwargs): message = str(message) % kwargs if message else DEFAULT_MESSAGE.get(status_code, '') + params = dict(request.view_args) + params.copy(kwargs) + # Add the user information. auth_user = get_authenticated_user() auth_token = get_validated_token() if auth_user: + mixpanel.track(auth_user.username, 'http_error', params) message = '%s (user: %s)' % (message, auth_user.username) elif auth_token: + mixpanel.track(auth_token.core, 'http_error', params) message = '%s (token: %s)' % (message, auth_token.friendly_name or auth_token.code) # Log the abort. - logger.error('Error %s: %s. Arguments: %s' % (status_code, message, request.view_args)) + logger.error('Error %s: %s. Arguments: %s' % (status_code, message, params)) # Report the abort to the user. flask_abort(make_response(message, status_code, {})) From c18212f4771560c8cd4fa5e09b463ad9aeb9c76f Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Fri, 24 Jan 2014 20:33:42 -0500 Subject: [PATCH 14/20] Meant update, not copy --- util/http.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/util/http.py b/util/http.py index 95b39e4f9..f91f60df5 100644 --- a/util/http.py +++ b/util/http.py @@ -19,7 +19,7 @@ def abort(status_code, message=None, **kwargs): DEFAULT_MESSAGE.get(status_code, '')) params = dict(request.view_args) - params.copy(kwargs) + params.update(kwargs) # Add the user information. auth_user = get_authenticated_user() From b1ec9c4469e256cdbe225e649049d6ef64edee2c Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Fri, 24 Jan 2014 20:34:18 -0500 Subject: [PATCH 15/20] Typo fix --- util/http.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/util/http.py b/util/http.py index f91f60df5..5e390cce2 100644 --- a/util/http.py +++ b/util/http.py @@ -28,7 +28,7 @@ def abort(status_code, message=None, **kwargs): mixpanel.track(auth_user.username, 'http_error', params) message = '%s (user: %s)' % (message, auth_user.username) elif auth_token: - mixpanel.track(auth_token.core, 'http_error', params) + mixpanel.track(auth_token.code, 'http_error', params) message = '%s (token: %s)' % (message, auth_token.friendly_name or auth_token.code) From aef719c71a715a8cf9aee87041654bb9ca3bc267 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Fri, 24 Jan 2014 20:37:44 -0500 Subject: [PATCH 16/20] Add the status code and message to the mix panel reported error --- util/http.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/util/http.py b/util/http.py index 5e390cce2..e4e1aa89f 100644 --- a/util/http.py +++ b/util/http.py @@ -21,6 +21,9 @@ def abort(status_code, message=None, **kwargs): params = dict(request.view_args) params.update(kwargs) + params['status_code'] = status_code + params['message'] = message + # Add the user information. auth_user = get_authenticated_user() auth_token = get_validated_token() From c7e616edb9f0e55e1cb4794f00b2fd79575a1dae Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Fri, 24 Jan 2014 20:40:22 -0500 Subject: [PATCH 17/20] Add the request URL to the reported error info --- util/http.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/util/http.py b/util/http.py index e4e1aa89f..a07b1e8a7 100644 --- a/util/http.py +++ b/util/http.py @@ -21,6 +21,7 @@ def abort(status_code, message=None, **kwargs): params = dict(request.view_args) params.update(kwargs) + params['url'] = request.url params['status_code'] = status_code params['message'] = message @@ -36,7 +37,7 @@ def abort(status_code, message=None, **kwargs): auth_token.friendly_name or auth_token.code) # Log the abort. - logger.error('Error %s: %s. Arguments: %s' % (status_code, message, params)) + logger.error('Error %s: %s; Arguments: %s' % (status_code, message, params)) resp = jsonify({'error': message}) resp.status_code = status_code From 2b134158f504a040d9b3da74a246bcc7e4869e02 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Tue, 28 Jan 2014 18:29:45 -0500 Subject: [PATCH 18/20] Add issue URLs to most errors. The corresponding issue pages will be checked into the public docs repo --- endpoints/index.py | 33 ++++++++++++++++++++----------- endpoints/registry.py | 46 ++++++++++++++++++++++++++----------------- util/http.py | 19 ++++++++++++++++-- 3 files changed, 66 insertions(+), 32 deletions(-) diff --git a/endpoints/index.py b/endpoints/index.py index b740bf998..94a5d4fef 100644 --- a/endpoints/index.py +++ b/endpoints/index.py @@ -15,6 +15,7 @@ from util.email import send_confirmation_email from auth.permissions import (ModifyRepositoryPermission, UserPermission, ReadRepositoryPermission, CreateRepositoryPermission) + from util.http import abort @@ -79,14 +80,14 @@ def create_user(): model.load_token_data(password) return make_response('Verified', 201) except model.InvalidTokenException: - return make_response('Invalid access token.', 400) + abort(400, 'Invalid access token.', issue='invalid-access-token') elif '+' in username: try: model.verify_robot(username, password) return make_response('Verified', 201) except model.InvalidRobotException: - return make_response('Invalid robot account or password.', 400) + abort(400, 'Invalid robot account or password.', issue='robot-login-failure') existing_user = model.get_user(username) if existing_user: @@ -94,7 +95,8 @@ def create_user(): if verified: return make_response('Verified', 201) else: - return make_response('Invalid password.', 400) + abort(400, 'Invalid password.', issue='login-failure') + else: # New user case new_user = model.create_user(username, password, user_data['email']) @@ -146,23 +148,30 @@ def update_user(username): @generate_headers(role='write') def create_repository(namespace, repository): image_descriptions = json.loads(request.data) - repo = model.get_repository(namespace, repository) if not repo and get_authenticated_user() is None: logger.debug('Attempt to create new repository without user auth.') - abort(401) + abort(401, + message='Cannot create a repository as a guest. Please login via "docker login" first.', + issue='no-login') elif repo: permission = ModifyRepositoryPermission(namespace, repository) if not permission.can(): - abort(403) + abort(403, + message='You do not have permission to modify repository %(namespace)s/%(repository)s', + issue='no-repo-write-permission', + namespace=namespace, repository=repository) else: permission = CreateRepositoryPermission(namespace) if not permission.can(): logger.info('Attempt to create a new repo with insufficient perms.') - abort(403) + abort(403, + message='You do not have permission to create repositories in namespace "%(namespace)s"', + issue='no-create-permission', + namespace=namespace) logger.debug('Creaing repository with owner: %s' % get_authenticated_user().username) @@ -215,7 +224,7 @@ def update_images(namespace, repository): repo = model.get_repository(namespace, repository) if not repo: # Make sure the repo actually exists. - abort(404) + abort(404, message='Unknown repository', issue='unknown-repo') image_with_checksums = json.loads(request.data) @@ -263,7 +272,7 @@ def get_repository_images(namespace, repository): # We can't rely on permissions to tell us if a repo exists anymore repo = model.get_repository(namespace, repository) if not repo: - abort(404) + abort(404, message='Unknown repository', issue='unknown-repo') all_images = [] for image in model.get_repository_images(namespace, repository): @@ -311,18 +320,18 @@ def get_repository_images(namespace, repository): @parse_repository_name @generate_headers(role='write') def delete_repository_images(namespace, repository): - return make_response('Not Implemented', 501) + abort(501, 'Not Implemented', issue='not-implemented') @index.route('/repositories//auth', methods=['PUT']) @parse_repository_name def put_repository_auth(namespace, repository): - return make_response('Not Implemented', 501) + abort(501, 'Not Implemented', issue='not-implemented') @index.route('/search', methods=['GET']) def get_search(): - return make_response('Not Implemented', 501) + abort(501, 'Not Implemented', issue='not-implemented') @index.route('/_ping') diff --git a/endpoints/registry.py b/endpoints/registry.py index 4d02dfd14..fc1575c1c 100644 --- a/endpoints/registry.py +++ b/endpoints/registry.py @@ -60,7 +60,7 @@ def require_completion(f): if store.exists(store.image_mark_path(namespace, repository, kwargs['image_id'])): abort(400, 'Image %(image_id)s is being uploaded, retry later', - image_id=kwargs['image_id']) + issue='upload-in-progress', image_id=kwargs['image_id']) return f(namespace, repository, *args, **kwargs) return wrapper @@ -105,7 +105,7 @@ def get_image_layer(namespace, repository, image_id, headers): try: return Response(store.stream_read(path), headers=headers) except IOError: - abort(404, 'Image %(image_id)s not found', image_id=image_id) + abort(404, 'Image %(image_id)s not found', issue='unknown-image', image_id=image_id) abort(403) @@ -122,16 +122,20 @@ def put_image_layer(namespace, repository, image_id): json_data = store.get_content(store.image_json_path(namespace, repository, image_id)) except IOError: - abort(404, 'Image not found') + abort(404, 'Image %(image_id)s not found', issue='unknown-image', image_id=image_id) + layer_path = store.image_layer_path(namespace, repository, image_id) mark_path = store.image_mark_path(namespace, repository, image_id) + if store.exists(layer_path) and not store.exists(mark_path): - abort(409, 'Image already exists') + abort(409, 'Image %(image_id)s already exists', issue='image-exists', image_id=image_id) + input_stream = request.stream if request.headers.get('transfer-encoding') == 'chunked': # Careful, might work only with WSGI servers supporting chunked # encoding (Gunicorn) input_stream = request.environ['wsgi.input'] + # compute checksums csums = [] sr = SocketReader(input_stream) @@ -141,6 +145,7 @@ def put_image_layer(namespace, repository, image_id): sr.add_handler(sum_hndlr) store.stream_write(layer_path, sr) csums.append('sha256:{0}'.format(h.hexdigest())) + try: image_size = tmp.tell() @@ -153,6 +158,7 @@ def put_image_layer(namespace, repository, image_id): except (IOError, checksums.TarError) as e: logger.debug('put_image_layer: Error when computing tarsum ' '{0}'.format(e)) + try: checksum = store.get_content(store.image_checksum_path(namespace, repository, @@ -162,10 +168,12 @@ def put_image_layer(namespace, repository, image_id): # Not removing the mark though, image is not downloadable yet. session['checksum'] = csums return make_response('true', 200) + # We check if the checksums provided matches one the one we computed if checksum not in csums: logger.warning('put_image_layer: Wrong checksum') - abort(400, 'Checksum mismatch; ignoring the layer') + abort(400, 'Checksum mismatch; ignoring the layer for image %(image_id)s', + issue='checksum-mismatch', image_id=image_id) # Checksum is ok, we remove the marker store.remove(mark_path) @@ -192,18 +200,19 @@ def put_image_checksum(namespace, repository, image_id): checksum = request.headers.get('X-Docker-Checksum') if not checksum: - abort(400, "Missing checksum for image %(image_id)s", image_id=image_id) + abort(400, "Missing checksum for image %(image_id)s", issue='missing-checksum', image_id=image_id) if not session.get('checksum'): abort(400, 'Checksum not found in Cookie for image %(imaage_id)s', - image_id=image_id) + issue='missing-checksum-cookie', image_id=image_id) if not store.exists(store.image_json_path(namespace, repository, image_id)): - abort(404, 'Image not found: %(image_id)s', image_id=image_id) + abort(404, 'Image not found: %(image_id)s', issue='unknown-image', image_id=image_id) mark_path = store.image_mark_path(namespace, repository, image_id) if not store.exists(mark_path): - abort(409, 'Cannot set checksum for image %(image_id)s', image_id=image_id) + abort(409, 'Cannot set checksum for image %(image_id)s', + issue='image-write-error', image_id=image_id) err = store_checksum(namespace, repository, image_id, checksum) if err: @@ -213,7 +222,8 @@ def put_image_checksum(namespace, repository, image_id): logger.debug('session checksums: %s' % session.get('checksum', [])) logger.debug('client supplied checksum: %s' % checksum) logger.debug('put_image_layer: Wrong checksum') - abort(400, 'Checksum mismatch for image: %(image_id)s', image_id=image_id) + abort(400, 'Checksum mismatch for image: %(image_id)s', + issue='checksum-mismatch', image_id=image_id) # Checksum is ok, we remove the marker store.remove(mark_path) @@ -245,7 +255,7 @@ def get_image_json(namespace, repository, image_id, headers): data = store.get_content(store.image_json_path(namespace, repository, image_id)) except IOError: - abort(404, 'Image %(image_id)%s not found', image_id=image_id) + abort(404, 'Image %(image_id)%s not found', issue='unknown-image', image_id=image_id) try: size = store.get_size(store.image_layer_path(namespace, repository, @@ -278,7 +288,7 @@ def get_image_ancestry(namespace, repository, image_id, headers): data = store.get_content(store.image_ancestry_path(namespace, repository, image_id)) except IOError: - abort(404, 'Image %(image_id)s not found', image_id=image_id) + abort(404, 'Image %(image_id)s not found', issue='unknown-image', image_id=image_id) response = make_response(json.dumps(json.loads(data)), 200) response.headers.extend(headers) @@ -324,11 +334,11 @@ def put_image_json(namespace, repository, image_id): pass if not data or not isinstance(data, dict): abort(400, 'Invalid JSON for image: %(image_id)s\nJSON: %(json)s', - image_id=image_id, json=request.data) + issue='invalid-request', image_id=image_id, json=request.data) if 'id' not in data: abort(400, 'Missing key `id` in JSON for image: %(image_id)s', - image_id=image_id) + issue='invalid-request', image_id=image_id) # Read the checksum checksum = request.headers.get('X-Docker-Checksum') @@ -336,25 +346,25 @@ def put_image_json(namespace, repository, image_id): # Storing the checksum is optional at this stage err = store_checksum(namespace, repository, image_id, checksum) if err: - abort(400, err) + abort(400, err, issue='write-error') else: # We cleanup any old checksum in case it's a retry after a fail store.remove(store.image_checksum_path(namespace, repository, image_id)) if image_id != data['id']: abort(400, 'JSON data contains invalid id for image: %(image_id)s', - image_id=image_id) + issue='invalid-request', image_id=image_id) parent_id = data.get('parent') if (parent_id and not store.exists(store.image_json_path(namespace, repository, parent_id))): abort(400, 'Image %(image_id)s depends on non existing parent image %(parent_id)s', - image_id=image_id, parent_id=parent_id) + issue='invalid-request', image_id=image_id, parent_id=parent_id) json_path = store.image_json_path(namespace, repository, image_id) mark_path = store.image_mark_path(namespace, repository, image_id) if store.exists(json_path) and not store.exists(mark_path): - abort(409, 'Image %(image_id)s already exists', image_id=image_id) + abort(409, 'Image %(image_id)s already exists', issue='image-exists', image_id=image_id) # If we reach that point, it means that this is a new image or a retry # on a failed push diff --git a/util/http.py b/util/http.py index a07b1e8a7..123029960 100644 --- a/util/http.py +++ b/util/http.py @@ -13,8 +13,9 @@ DEFAULT_MESSAGE[401] = 'Unauthorized' DEFAULT_MESSAGE[403] = 'Permission Denied' DEFAULT_MESSAGE[404] = 'Not Found' DEFAULT_MESSAGE[409] = 'Conflict' +DEFAULT_MESSAGE[501] = 'Not Implemented' -def abort(status_code, message=None, **kwargs): +def abort(status_code, message=None, issue=None, **kwargs): message = (str(message) % kwargs if message else DEFAULT_MESSAGE.get(status_code, '')) @@ -38,7 +39,21 @@ def abort(status_code, message=None, **kwargs): # Log the abort. logger.error('Error %s: %s; Arguments: %s' % (status_code, message, params)) - resp = jsonify({'error': message}) + + # Calculate the issue URL (if the issue ID was supplied). + issue_url = None + if issue: + issue_url = 'http://devtable.github.io/quaydocs/issues/%s.html' % (issue) + + # Create the final response data and message. + data = {} + if issue_url: + data['info_url'] = issue_url + message = message + '. For more information: ' + issue_url + + data['error'] = message + + resp = jsonify(data) resp.status_code = status_code # Report the abort to the user. From 3dc3af9eb209659d1ab1a9de4b711796b1cd4e33 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Wed, 29 Jan 2014 14:08:14 -0500 Subject: [PATCH 19/20] =?UTF-8?q?The=20CLI=20can=20be=E2=80=A6=20odd?= =?UTF-8?q?=E2=80=A6=20when=20handling=20errors.=20Make=20them=20nicer=20f?= =?UTF-8?q?or=20the=20CLI?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- endpoints/registry.py | 4 ++-- util/http.py | 5 ++--- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/endpoints/registry.py b/endpoints/registry.py index fc1575c1c..27c98d44e 100644 --- a/endpoints/registry.py +++ b/endpoints/registry.py @@ -128,7 +128,7 @@ def put_image_layer(namespace, repository, image_id): mark_path = store.image_mark_path(namespace, repository, image_id) if store.exists(layer_path) and not store.exists(mark_path): - abort(409, 'Image %(image_id)s already exists', issue='image-exists', image_id=image_id) + abort(409, 'Image already exists', issue='image-exists', image_id=image_id) input_stream = request.stream if request.headers.get('transfer-encoding') == 'chunked': @@ -364,7 +364,7 @@ def put_image_json(namespace, repository, image_id): json_path = store.image_json_path(namespace, repository, image_id) mark_path = store.image_mark_path(namespace, repository, image_id) if store.exists(json_path) and not store.exists(mark_path): - abort(409, 'Image %(image_id)s already exists', issue='image-exists', image_id=image_id) + abort(409, 'Image already exists', issue='image-exists', image_id=image_id) # If we reach that point, it means that this is a new image or a retry # on a failed push diff --git a/util/http.py b/util/http.py index 123029960..02b07c410 100644 --- a/util/http.py +++ b/util/http.py @@ -47,11 +47,10 @@ def abort(status_code, message=None, issue=None, **kwargs): # Create the final response data and message. data = {} + data['error'] = message + if issue_url: data['info_url'] = issue_url - message = message + '. For more information: ' + issue_url - - data['error'] = message resp = jsonify(data) resp.status_code = status_code From 392e0d7c589bfd3155dc133051bb67c916a43226 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Wed, 29 Jan 2014 15:38:25 -0500 Subject: [PATCH 20/20] Change docs url --- util/http.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/util/http.py b/util/http.py index 02b07c410..6592768cc 100644 --- a/util/http.py +++ b/util/http.py @@ -43,7 +43,7 @@ def abort(status_code, message=None, issue=None, **kwargs): # Calculate the issue URL (if the issue ID was supplied). issue_url = None if issue: - issue_url = 'http://devtable.github.io/quaydocs/issues/%s.html' % (issue) + issue_url = 'http://docs.quay.io/issues/%s.html' % (issue) # Create the final response data and message. data = {}