From bdd603fb179fccfc56abdbe166e4e223300c5b96 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= Date: Tue, 29 Aug 2017 11:34:39 +0200 Subject: [PATCH] Using new UserClass instances everywhere: - No more direct access to g.current_user, unless unavoidable. - Using pillar.auth.current_user instead of g.current_user or flask_login.current_user. - p.a.current_user is never checked against None. - p.a.current_user.is_authenticated or is_anonymous is used, and never together with a negation (instead of 'not is_anon' use 'is_auth'). - No more accessing current_user a a dict. - No more checks for admin role, use capability check instead. --- pillar/api/activities.py | 7 +-- pillar/api/blender_cloud/home_project.py | 8 +-- pillar/api/blender_cloud/texture_libs.py | 12 +++-- pillar/api/file_storage/__init__.py | 15 +++--- pillar/api/nodes/__init__.py | 6 ++- pillar/api/projects/routes.py | 10 ++-- pillar/api/users/hooks.py | 29 +++++------ pillar/api/users/routes.py | 6 ++- pillar/api/utils/authentication.py | 8 +-- pillar/api/utils/authorization.py | 64 ++++++++++++------------ pillar/auth/__init__.py | 25 +++++++-- pillar/web/users/routes.py | 10 ++-- 12 files changed, 113 insertions(+), 87 deletions(-) diff --git a/pillar/api/activities.py b/pillar/api/activities.py index 5bc7c910..b4bffa34 100644 --- a/pillar/api/activities.py +++ b/pillar/api/activities.py @@ -1,7 +1,8 @@ import logging -from flask import g, request, current_app +from flask import request, current_app from pillar.api.utils import gravatar +from pillar.auth import current_user log = logging.getLogger(__name__) @@ -30,7 +31,7 @@ def notification_parse(notification): object_name = '' object_id = activity['object'] - if node['parent']['user'] == g.current_user['user_id']: + if node['parent']['user'] == current_user.user_id: owner = "your {0}".format(node['parent']['node_type']) else: parent_comment_user = users_collection.find_one( @@ -52,7 +53,7 @@ def notification_parse(notification): action = activity['verb'] lookup = { - 'user': g.current_user['user_id'], + 'user': current_user.user_id, 'context_object_type': 'node', 'context_object': context_object_id, } diff --git a/pillar/api/blender_cloud/home_project.py b/pillar/api/blender_cloud/home_project.py index 4f764144..67c0e45b 100644 --- a/pillar/api/blender_cloud/home_project.py +++ b/pillar/api/blender_cloud/home_project.py @@ -4,7 +4,7 @@ import logging import datetime from bson import ObjectId, tz_util from eve.methods.get import get -from flask import Blueprint, g, current_app, request +from flask import Blueprint, current_app, request from pillar.api import utils from pillar.api.utils import authentication, authorization from werkzeug import exceptions as wz_exceptions @@ -201,8 +201,10 @@ def home_project(): Eve projections are supported, but at least the following fields must be present: 'permissions', 'category', 'user' """ - user_id = g.current_user['user_id'] - roles = g.current_user.get('roles', ()) + from pillar.auth import current_user + + user_id = current_user.user_id + roles = current_user.roles log.debug('Possibly creating home project for user %s with roles %s', user_id, roles) if HOME_PROJECT_USERS and not HOME_PROJECT_USERS.intersection(roles): diff --git a/pillar/api/blender_cloud/texture_libs.py b/pillar/api/blender_cloud/texture_libs.py index 206ee8d9..34ce67f9 100644 --- a/pillar/api/blender_cloud/texture_libs.py +++ b/pillar/api/blender_cloud/texture_libs.py @@ -3,12 +3,14 @@ import logging from eve.methods.get import get from eve.utils import config as eve_config -from flask import Blueprint, request, current_app, g +from flask import Blueprint, request, current_app +from werkzeug.datastructures import MultiDict +from werkzeug.exceptions import InternalServerError + from pillar.api import utils from pillar.api.utils.authentication import current_user_id from pillar.api.utils.authorization import require_login -from werkzeug.datastructures import MultiDict -from werkzeug.exceptions import InternalServerError +from pillar.auth import current_user FIRST_ADDON_VERSION_WITH_HDRI = (1, 4, 0) TL_PROJECTION = utils.dumps({'name': 1, 'url': 1, 'permissions': 1,}) @@ -25,8 +27,8 @@ log = logging.getLogger(__name__) def keep_fetching_texture_libraries(proj_filter): - groups = g.current_user['groups'] - user_id = g.current_user['user_id'] + groups = current_user.group_ids + user_id = current_user.user_id page = 1 max_page = float('inf') diff --git a/pillar/api/file_storage/__init__.py b/pillar/api/file_storage/__init__.py index f929c7f5..e94b563b 100644 --- a/pillar/api/file_storage/__init__.py +++ b/pillar/api/file_storage/__init__.py @@ -18,7 +18,6 @@ import werkzeug.datastructures from bson import ObjectId from flask import Blueprint from flask import current_app -from flask import g from flask import jsonify from flask import request from flask import send_from_directory @@ -34,6 +33,7 @@ from pillar.api.utils.cdn import hash_file_path from pillar.api.utils.encoding import Encoder from pillar.api.utils.imaging import generate_local_thumbnails from pillar.api.file_storage_backends import default_storage_backend, Bucket +from pillar.auth import current_user log = logging.getLogger(__name__) @@ -387,10 +387,11 @@ def before_returning_file(response): def strip_link_and_variations(response): # Check the access level of the user. - if g.current_user is None: + if current_user.is_anonymous: has_full_access = False else: - user_roles = g.current_user['roles'] + user_roles = current_user.roles + # TODO: convert to a capability and check for that. access_roles = current_app.config['FULL_FILE_ACCESS_ROLES'] has_full_access = bool(user_roles.intersection(access_roles)) @@ -598,12 +599,10 @@ def create_file_doc(name, filename, content_type, length, project, if backend is None: backend = current_app.config['STORAGE_BACKEND'] - current_user = g.get('current_user') - file_doc = {'name': name, 'filename': filename, 'file_path': '', - 'user': current_user['user_id'], + 'user': current_user.user_id, 'backend': backend, 'md5': '', 'content_type': content_type, @@ -666,7 +665,7 @@ def assert_file_size_allowed(file_size: int): filesize_limit_mb = filesize_limit / 2.0 ** 20 log.info('User %s tried to upload a %.3f MiB file, but is only allowed ' '%.3f MiB.', - authentication.current_user_id(), file_size / 2.0 ** 20, + current_user.user_id, file_size / 2.0 ** 20, filesize_limit_mb) raise wz_exceptions.RequestEntityTooLarge( 'To upload files larger than %i MiB, subscribe to Blender Cloud' % @@ -685,7 +684,7 @@ def stream_to_storage(project_id: str): raise wz_exceptions.NotFound('Project %s does not exist' % project_id) log.info('Streaming file to bucket for project=%s user_id=%s', project_id, - authentication.current_user_id()) + current_user.user_id) log.info('request.headers[Origin] = %r', request.headers.get('Origin')) log.info('request.content_length = %r', request.content_length) diff --git a/pillar/api/nodes/__init__.py b/pillar/api/nodes/__init__.py index cddf1f8d..27437ce6 100644 --- a/pillar/api/nodes/__init__.py +++ b/pillar/api/nodes/__init__.py @@ -6,7 +6,7 @@ import urllib.parse import pymongo.errors import werkzeug.exceptions as wz_exceptions from bson import ObjectId -from flask import current_app, g, Blueprint, request +from flask import current_app, Blueprint, request import pillar.markdown from pillar.api.activities import activity_subscribe, activity_object_add @@ -205,6 +205,8 @@ def before_inserting_nodes(items): """Before inserting a node in the collection we check if the user is allowed and we append the project id to it. """ + from pillar.auth import current_user + nodes_collection = current_app.data.driver.db['nodes'] def find_parent_project(node): @@ -226,7 +228,7 @@ def before_inserting_nodes(items): item['project'] = project['_id'] # Default the 'user' property to the current user. - item.setdefault('user', g.current_user['user_id']) + item.setdefault('user', current_user.user_id) def after_inserting_nodes(items): diff --git a/pillar/api/projects/routes.py b/pillar/api/projects/routes.py index 1433fbfc..432f1c72 100644 --- a/pillar/api/projects/routes.py +++ b/pillar/api/projects/routes.py @@ -2,11 +2,13 @@ import json import logging from bson import ObjectId -from flask import Blueprint, g, request, current_app, make_response, url_for +from flask import Blueprint, request, current_app, make_response, url_for +from werkzeug import exceptions as wz_exceptions + from pillar.api.utils import authorization, jsonify, str2id from pillar.api.utils import mongo from pillar.api.utils.authorization import require_login, check_permissions -from werkzeug import exceptions as wz_exceptions +from pillar.auth import current_user from . import utils @@ -24,7 +26,7 @@ def create_project(overrides=None): project_name = request.json['name'] else: project_name = request.form['project_name'] - user_id = g.current_user['user_id'] + user_id = current_user.user_id project = utils.create_new_project(project_name, user_id, overrides) @@ -62,7 +64,7 @@ def project_manage_users(): project_id = str2id(data['project_id']) target_user_id = str2id(data['user_id']) action = data['action'] - current_user_id = g.current_user['user_id'] + current_user_id = current_user.user_id project = projects_collection.find_one({'_id': project_id}) diff --git a/pillar/api/users/hooks.py b/pillar/api/users/hooks.py index 3ac21d61..356eb329 100644 --- a/pillar/api/users/hooks.py +++ b/pillar/api/users/hooks.py @@ -3,12 +3,12 @@ import json import bson from eve.utils import parse_request -from flask import g from werkzeug import exceptions as wz_exceptions from pillar import current_app from pillar.api.users.routes import log from pillar.api.utils.authorization import user_has_role +import pillar.auth USER_EDITABLE_FIELDS = {'full_name', 'username', 'email', 'settings'} @@ -33,7 +33,7 @@ def before_replacing_user(request, lookup): # Reset fields that shouldn't be edited to their original values. This is only # needed when users are editing themselves; admins are allowed to edit much more. - if not user_has_role('admin'): + if not pillar.auth.current_user.has_cap('admin'): for db_key, db_value in original.items(): if db_key[0] == '_' or db_key in USER_EDITABLE_FIELDS: continue @@ -89,33 +89,31 @@ def send_blinker_signal_roles_changed(user, original): def check_user_access(request, lookup): """Modifies the lookup dict to limit returned user info.""" - # No access when not logged in. - current_user = g.get('current_user') - current_user_id = current_user['user_id'] if current_user else None + user = pillar.auth.get_current_user() # Admins can do anything and get everything, except the 'auth' block. - if user_has_role('admin'): + if user.has_cap('admin'): return - if not lookup and not current_user: + if not lookup and user.is_anonymous: raise wz_exceptions.Forbidden() # Add a filter to only return the current user. if '_id' not in lookup: - lookup['_id'] = current_user['user_id'] + lookup['_id'] = user.user_id def check_put_access(request, lookup): """Only allow PUT to the current user, or all users if admin.""" - if user_has_role('admin'): + user = pillar.auth.get_current_user() + if user.has_cap('admin'): return - current_user = g.get('current_user') - if not current_user: + if user.is_anonymous: raise wz_exceptions.Forbidden() - if str(lookup['_id']) != str(current_user['user_id']): + if str(lookup['_id']) != str(user.user_id): raise wz_exceptions.Forbidden() @@ -124,15 +122,14 @@ def after_fetching_user(user): # custom end-points. user.pop('auth', None) - current_user = g.get('current_user') - current_user_id = current_user['user_id'] if current_user else None + current_user = pillar.auth.get_current_user() # Admins can do anything and get everything, except the 'auth' block. - if user_has_role('admin'): + if current_user.has_cap('admin'): return # Only allow full access to the current user. - if str(user['_id']) == str(current_user_id): + if current_user.is_authenticated and str(user['_id']) == str(current_user.user_id): return # Remove all fields except public ones. diff --git a/pillar/api/users/routes.py b/pillar/api/users/routes.py index 11ad0a14..7f0e79c2 100644 --- a/pillar/api/users/routes.py +++ b/pillar/api/users/routes.py @@ -1,9 +1,11 @@ import logging from eve.methods.get import get -from flask import g, Blueprint +from flask import Blueprint + from pillar.api.utils import jsonify from pillar.api.utils.authorization import require_login +from pillar.auth import current_user log = logging.getLogger(__name__) blueprint_api = Blueprint('users_api', __name__) @@ -12,7 +14,7 @@ blueprint_api = Blueprint('users_api', __name__) @blueprint_api.route('/me') @require_login() def my_info(): - eve_resp, _, _, status, _ = get('users', {'_id': g.current_user['user_id']}) + eve_resp, _, _, status, _ = get('users', {'_id': current_user.user_id}) resp = jsonify(eve_resp['_items'][0], status=status) return resp diff --git a/pillar/api/utils/authentication.py b/pillar/api/utils/authentication.py index 35486111..fb83f125 100644 --- a/pillar/api/utils/authentication.py +++ b/pillar/api/utils/authentication.py @@ -113,6 +113,8 @@ def validate_token(): @returns True iff the user is logged in with a valid Blender ID token. """ + from pillar.auth import force_logout_user + if request.authorization: token = request.authorization.username oauth_subclient = request.authorization.password @@ -129,7 +131,7 @@ def validate_token(): # If no authorization headers are provided, we are getting a request # from a non logged in user. Proceed accordingly. log.debug('No authentication headers, so not logged in.') - g.current_user = None + force_logout_user() return False return validate_this_token(token, oauth_subclient) is not None @@ -142,9 +144,9 @@ def validate_this_token(token, oauth_subclient=None): :rtype: dict """ - from pillar.auth import UserClass + from pillar.auth import UserClass, force_logout_user - g.current_user = None + force_logout_user() _delete_expired_tokens() # Check the users to see if there is one with this Blender ID token. diff --git a/pillar/api/utils/authorization.py b/pillar/api/utils/authorization.py index c24cd832..b7016e4e 100644 --- a/pillar/api/utils/authorization.py +++ b/pillar/api/utils/authorization.py @@ -45,6 +45,8 @@ def compute_allowed_methods(collection_name, resource, check_node_type=None): :rtype: set """ + import pillar.auth + # Check some input values. if collection_name not in CHECK_PERMISSIONS_IMPLEMENTED_FOR: raise ValueError('compute_allowed_methods only implemented for %s, not for %s', @@ -62,18 +64,18 @@ def compute_allowed_methods(collection_name, resource, check_node_type=None): # Accumulate allowed methods from the user, group and world level. allowed_methods = set() - current_user = getattr(g, 'current_user', None) + user = pillar.auth.get_current_user() - if current_user: - user_is_admin = is_admin(current_user) + if user.is_authenticated: + user_is_admin = is_admin(user) # If the user is authenticated, proceed to compare the group permissions for permission in computed_permissions.get('groups', ()): - if user_is_admin or permission['group'] in current_user['groups']: + if user_is_admin or permission['group'] in user.group_ids: allowed_methods.update(permission['methods']) for permission in computed_permissions.get('users', ()): - if user_is_admin or current_user['user_id'] == permission['user']: + if user_is_admin or user.user_id == permission['user']: allowed_methods.update(permission['methods']) # Check if the node is public or private. This must be set for non logged @@ -298,21 +300,24 @@ def require_login(require_roles=set(), def decorator(func): @functools.wraps(func) def wrapper(*args, **kwargs): - if g.current_user is None: + import pillar.auth + + current_user = pillar.auth.get_current_user() + if current_user.is_anonymous: # We don't need to log at a higher level, as this is very common. # Many browsers first try to see whether authentication is needed # at all, before sending the password. log.debug('Unauthenticated access to %s attempted.', func) abort(403) - if require_roles and not g.current_user.matches_roles(require_roles, require_all): + if require_roles and not current_user.matches_roles(require_roles, require_all): log.warning('User %s is authenticated, but does not have required roles %s to ' - 'access %s', g.current_user['user_id'], require_roles, func) + 'access %s', current_user.user_id, require_roles, func) abort(403) - if require_cap and not g.current_user.has_cap(require_cap): + if require_cap and not current_user.has_cap(require_cap): log.warning('User %s is authenticated, but does not have required capability %s to ' - 'access %s', g.current_user.user_id, require_cap, func) + 'access %s', current_user.user_id, require_cap, func) abort(403) return func(*args, **kwargs) @@ -356,16 +361,17 @@ def ab_testing(require_roles=set(), def user_has_role(role, user=None): """Returns True iff the user is logged in and has the given role.""" - from pillar.auth import UserClass + import pillar.auth if user is None: - user = g.get('current_user') - if user is not None and not isinstance(user, UserClass): - raise TypeError(f'g.current_user should be instance of UserClass, not {type(user)}') - elif not isinstance(user, UserClass): + user = pillar.auth.get_current_user() + if user is not None and not isinstance(user, pillar.auth.UserClass): + raise TypeError(f'pillar.auth.current_user should be instance of UserClass, ' + f'not {type(user)}') + elif not isinstance(user, pillar.auth.UserClass): raise TypeError(f'user should be instance of UserClass, not {type(user)}') - if user is None: + if user.is_anonymous: return False return user.has_role(role) @@ -374,17 +380,14 @@ def user_has_role(role, user=None): def user_has_cap(capability: str, user=None) -> bool: """Returns True iff the user is logged in and has the given capability.""" - from pillar.auth import UserClass + import pillar.auth assert capability if user is None: - user = g.get('current_user') + user = pillar.auth.get_current_user() - if user is None: - return False - - if not isinstance(user, UserClass): + if not isinstance(user, pillar.auth.UserClass): raise TypeError(f'user should be instance of UserClass, not {type(user)}') return user.has_cap(capability) @@ -402,19 +405,16 @@ def user_matches_roles(require_roles=set(), returning True. """ - from pillar.auth import UserClass + import pillar.auth - current_user: UserClass = g.get('current_user') - if current_user is None: - return False + user = pillar.auth.get_current_user() + if not isinstance(user, pillar.auth.UserClass): + raise TypeError(f'user should be instance of UserClass, not {type(user)}') - if not isinstance(current_user, UserClass): - raise TypeError(f'g.current_user should be instance of UserClass, not {type(current_user)}') - - return current_user.matches_roles(require_roles, require_all) + return user.matches_roles(require_roles, require_all) def is_admin(user): - """Returns True iff the given user has the admin role.""" + """Returns True iff the given user has the admin capability.""" - return user_has_role('admin', user) + return user_has_cap('admin', user) diff --git a/pillar/auth/__init__.py b/pillar/auth/__init__.py index d10962fc..5ea1b25d 100644 --- a/pillar/auth/__init__.py +++ b/pillar/auth/__init__.py @@ -12,9 +12,6 @@ from pillar import current_app import bson -from ..api import utils -from ..api.utils import authentication - log = logging.getLogger(__name__) # Mapping from user role to capabilities obtained by users with that role. @@ -48,6 +45,8 @@ class UserClass(flask_login.UserMixin): def construct(cls, token: str, db_user: dict) -> 'UserClass': """Constructs a new UserClass instance from a Mongo user document.""" + from ..api import utils + user = cls(token) user.user_id = db_user['_id'] @@ -170,6 +169,8 @@ def _load_user(token) -> typing.Union[UserClass, AnonymousUser]: :returns: returns a UserClass instance if logged in, or an AnonymousUser() if not. """ + from ..api.utils import authentication + db_user = authentication.validate_this_token(token) if not db_user: return AnonymousUser() @@ -206,6 +207,15 @@ def login_user(oauth_token: str, *, load_from_db=False): g.current_user = user +def force_logout_user(): + """Resets the current user to an AnonymousUser instance.""" + + from flask import g + + flask_login.logout_user() + g.current_user = flask_login.current_user._get_current_object() + + def get_blender_id_oauth_token(): """Returns a tuple (token, ''), for use with flask_oauthlib.""" @@ -221,10 +231,15 @@ def get_blender_id_oauth_token(): return None -def _get_current_user() -> UserClass: +def get_current_user() -> UserClass: """Returns the current user as a UserClass instance. Never returns None; returns an AnonymousUser() instance instead. + + This function is intended to be used when pillar.auth.current_user is + accessed many times in the same scope. Calling this function is then + more efficient, since it doesn't have to resolve the LocalProxy for + each access to the returned object. """ from ..api.utils.authentication import current_user @@ -232,5 +247,5 @@ def _get_current_user() -> UserClass: return current_user() -current_user: UserClass = LocalProxy(_get_current_user) +current_user: UserClass = LocalProxy(get_current_user) """The current user.""" diff --git a/pillar/web/users/routes.py b/pillar/web/users/routes.py index 66ca0441..e14fa0de 100644 --- a/pillar/web/users/routes.py +++ b/pillar/web/users/routes.py @@ -2,9 +2,9 @@ import json import logging from werkzeug import exceptions as wz_exceptions -from flask import abort, Blueprint, current_app, flash, redirect, render_template, request, session,\ +from flask import abort, Blueprint, flash, redirect, render_template, request, session,\ url_for -from flask_login import login_required, logout_user, current_user +from flask_login import login_required, logout_user from pillarsdk import exceptions as sdk_exceptions from pillarsdk.users import User @@ -15,6 +15,7 @@ from pillar.web import system_util from pillar.api.local_auth import generate_and_store_token, get_local_user from pillar.api.utils.authentication import find_user_in_db, upsert_user from pillar.api.blender_cloud.subscription import update_subscription +from pillar.auth import current_user from pillar.auth.oauth import OAuthSignIn, ProviderConfigurationMissing, ProviderNotImplemented, \ OAuthCodeNotProvided from . import forms @@ -47,8 +48,9 @@ def oauth_authorize(provider): @blueprint.route('/oauth//authorized') def oauth_callback(provider): - if not current_user.is_anonymous: + if current_user.is_authenticated: return redirect(url_for('main.homepage')) + oauth = OAuthSignIn.get_provider(provider) try: oauth_user = oauth.callback() @@ -68,7 +70,7 @@ def oauth_callback(provider): # Login user pillar.auth.login_user(token['token'], load_from_db=True) - if provider == 'blender-id' and current_user is not None: + if provider == 'blender-id' and current_user.is_authenticated: # Check with the store for user roles. If the user has an active subscription, we apply # the 'subscriber' role update_subscription()