Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added more distinct admin and teacher roles and ability to impersonate #111

Merged
merged 8 commits into from
Jan 2, 2024
3 changes: 3 additions & 0 deletions learning_observer/learning_observer/auth/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,13 @@
# e.g. @admin_ajax @admin_html, or @admin(type=ajax)
from learning_observer.auth.utils import admin
from learning_observer.auth.utils import teacher
from learning_observer.auth.roles import ROLES

# Utility functions
from learning_observer.auth.utils import fernet_key
from learning_observer.auth.utils import google_id_to_user_id
from learning_observer.auth.utils import get_active_user
from learning_observer.auth.events import encode_id

# Utility handlers
from learning_observer.auth.handlers import logout_handler
Expand Down
7 changes: 6 additions & 1 deletion learning_observer/learning_observer/auth/http_basic.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@
import learning_observer.settings
import learning_observer.prestartup

import learning_observer.auth

from learning_observer.log_event import debug_log


Expand Down Expand Up @@ -160,7 +162,10 @@ async def password_auth_handler(request):
'name': "",
'family_name': "",
'picture': "",
'authorized': True
'authorized': True,
# TODO this ought to pull from somewhere else; however, I'm not familiar
# how to test the http_basic code, so I'm setting this as student for now
'role': learning_observer.auth.ROLES.STUDENT
}
)
# This is usually ignored, but just in case...
Expand Down
4 changes: 3 additions & 1 deletion learning_observer/learning_observer/auth/password.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import aiohttp.web

import learning_observer.auth.handlers
import learning_observer.auth
import learning_observer.auth.utils

from learning_observer.log_event import debug_log
Expand Down Expand Up @@ -79,7 +80,8 @@ async def password_auth_handler(request):
'name': user_data.get('name', ''),
'family_name': user_data.get('family_name', ''),
'picture': user_data.get('picture', '/auth/default-avatar.svg'),
'authorized': True
'authorized': True,
'role': user_data.get('role', learning_observer.auth.ROLES.STUDENT)
}
)
return aiohttp.web.json_response({"status": "authorized"})
Expand Down
50 changes: 50 additions & 0 deletions learning_observer/learning_observer/auth/roles.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
'''
Roles defined for the system. We explicitly do not want the complexity
of ACLs, roles, groups, etc. We plan four levels, including later a
school/district-level one.

We create the enum for roles like this to make everything
a string and thus, it is all json serializable. Starting in
Python 3.11, there is an easier way to do this.
`ROLES = enum.StrEnum("ROLES", names=('STUDENT', 'TEACHER', 'ADMIN'))`
bradley-erickson marked this conversation as resolved.
Show resolved Hide resolved
'''
import collections
import os
import shutil

import learning_observer.paths as paths
import learning_observer.prestartup

class ROLES:
pass
possible_roles = ['STUDENT', 'TEACHER', 'ADMIN']
[setattr(ROLES, role, role) for role in possible_roles]

'''
TODO we should include other methods of determining
where to pull the list of users from. In the future we want
to allow text-based and database backends. We should support
multiple methods of retrieving this information at once.
'''
USER_FILES = collections.OrderedDict({
ROLES.ADMIN: 'admins.yaml',
ROLES.TEACHER: 'teachers.yaml',
})


@learning_observer.prestartup.register_startup_check
def validate_user_lists():
'''
Validate the role list files. These are YAML files that contains
a list of users authorized for each role to use the Learning Observer.
'''
for k in USER_FILES.keys():
if not os.path.exists(paths.data(USER_FILES[k])):
shutil.copyfile(
paths.data(f"{USER_FILES[k]}.template"),
paths.data(USER_FILES[k])
)
raise learning_observer.prestartup.StartupCheck(
f"Created a blank {k} file: static_data/{USER_FILES[k]}\n"
f"Populate it with {k} accounts."
)
10 changes: 9 additions & 1 deletion learning_observer/learning_observer/auth/social_sso.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import learning_observer.settings as settings
import learning_observer.auth.handlers as handlers
import learning_observer.auth.utils
import learning_observer.auth.roles

import learning_observer.exceptions

Expand Down Expand Up @@ -143,6 +144,7 @@ async def _google(request):
async with client.get(url, headers=headers) as resp:
profile = await resp.json()

role = await learning_observer.auth.utils.verify_role(profile['id'], profile['email'])
return {
'user_id': profile['id'],
'email': profile['email'],
Expand All @@ -151,7 +153,13 @@ async def _google(request):
'back_to': request.query.get('state'),
'picture': profile['picture'],
# TODO: Should this be immediate?
'authorized': await learning_observer.auth.utils.verify_teacher_account(profile['id'], profile['email'])
# TODO: Should authorized just take over the role?
# the old code relis on authorized being set to True
# to allow users access to various dashboards. Should
# we modify this behavior to check for a role or True
# instead of using the role attribute.
'authorized': role in [learning_observer.auth.ROLES.ADMIN, learning_observer.auth.ROLES.TEACHER],
'role': role
}


Expand Down
123 changes: 73 additions & 50 deletions learning_observer/learning_observer/auth/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,12 @@
import learning_observer.paths

from learning_observer.log_event import debug_log
from . import roles

# TODO this is originally defined in a file that
# imports this one, so we are unable to import it.
# We should move the constant and import this instead.
IMPERSONATING_AS = 'impersonating_as'


def google_id_to_user_id(google_id):
Expand All @@ -50,27 +56,27 @@ def fernet_key(secret_string):
return md5_hash.hexdigest().encode('utf-8')


async def verify_teacher_account(user_id, email):
async def verify_role(user_id, email):
'''
Confirm the teacher is registered with the system. Eventually, we will want
Confirm the user is registered with the system. Eventually, we will want
3 versions of this:
* Always true (open system)
* Text file backed (pilots, small deploys)
* Database-backed (large-scale deploys)

For now, we have the file-backed version
'''
teachers = yaml.safe_load(open(learning_observer.paths.data("teachers.yaml")))
if email not in teachers:
# email is untrusted; repr prevents injection of newlines
debug_log("Email not found in teachers", repr(email))
return False
if teachers[email]["google_id"] != user_id:
# user_id is untrusted; repr prevents injection of newlines
debug_log("Non-matching Google ID", teachers[email]["google_id"], repr(user_id))
return False
debug_log("Teacher account verified")
return True
for k in roles.USER_FILES.keys():
users = yaml.safe_load(open(learning_observer.paths.data(roles.USER_FILES[k])))
if email not in users:
# email is untrusted; repr prevents injection of newlines
debug_log(f"Email not found in {roles.USER_FILES[k]}", repr(email))
continue
if users[email]["google_id"] != user_id:
# user_id is untrusted; repr prevents injection of newlines
debug_log(f"Non-matching Google ID in {roles.USER_FILES[k]}", users[email]["google_id"], repr(user_id))
continue
debug_log(f"{k} account verified")
return k
return roles.ROLES.STUDENT


async def update_session_user_info(request, user):
Expand All @@ -87,13 +93,24 @@ async def update_session_user_info(request, user):
session["user"] = user


async def get_active_user(request):
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good. FYI: Maybe get_effective_user and get_true_user? Not sure.

'''
Fetch current impersonated user or self from session
'''
session = await aiohttp_session.get_session(request)
if IMPERSONATING_AS in session:
return session[IMPERSONATING_AS]
return session['user']


async def logout(request):
'''
Log the user out
'''
session = await aiohttp_session.get_session(request)
session.pop("user", None)
session.pop("auth_headers", None)
session.pop(IMPERSONATING_AS, None)
request['user'] = None


Expand Down Expand Up @@ -138,38 +155,44 @@ async def verify_password(filename, username, password):
# we plan to have teacher, student, and admin accounts.
#
# In the long term, we will probably want a little more, but not full ACLs.


def admin(func):
'''
Decorator to mark a view as an admin view.

This should be moved to the auth/auth framework, and have more
granular levels (e.g. teachers and sys-admins). We probably don't
want a full ACL scheme (which overcomplicates things), but we will
want to think through auth/auth.
'''
@functools.wraps(func)
def wrapper(request):
if learning_observer.settings.settings['auth'].get("test_case_insecure", False):
return func(request)
if 'user' in request and \
request['user'] is not None and \
'authorized' in request['user'] and \
request['user']['authorized']:
return func(request)
# Else, if unauthorized
# send user to login page /
# there may be a slight oddball with the url hash being
# included after the location updates
response = aiohttp.web.Response(status=302)
redirect_url = '/'
response.headers['Location'] = redirect_url
raise aiohttp.web.HTTPFound(location=redirect_url, headers=response.headers)
return wrapper


# Decorator
#
# For now, we don't have seperate teacher and admin accounts.
teacher = admin
def _role_required(role):
'''Returns a decorator for viewing pages that require the passed
in `role`. The role is stored in the user object under `user.role`.
'''
def decorator(func):
@functools.wraps(func)
def wrapper(request):
if learning_observer.settings.settings['auth'].get("test_case_insecure", False):
return func(request)
'''TODO evaluate how we should be using `role` with the
`authorized` key.

`authorized` is how the auth workflow used to work. This
was set to True for teachers/admins and false otherwise.
With the new inclusion of `role`, I'm not sure we need to
use `authorized` anymore.

When this is resolved, we need to update each source of
auth in our code (e.g. password, http_basic, google, etc.)
'''
if 'user' in request and \
request['user'] is not None and \
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI: Formatting here is wonky. Line continuations should be indented.

This is also a train wreck (MY train wreck, looking at original code). Should be lines more like: authorized = request.get(USER, {}).get(AUTHORIZED, False), and ditto for role.

And then: if session_authorized and session_role in [role, role.ROLES.ADMIN]

That's a lot more readable to break it down like that. A 5 line continuation is a flag that something is wrong.

This is actually pretty good code circa-eighties/nineties coding standards :) I sometimes revert to my childhood, apparently.

'authorized' in request['user'] and \
request['user']['authorized'] and \
'role' in request['user'] and \
(request['user']['role'] == role or request['user']['role'] == roles.ROLES.ADMIN):
return func(request)
# Else, if unauthorized
# send user to login page /
# there may be a slight oddball with the url hash being
# included after the location updates
response = aiohttp.web.Response(status=302)
redirect_url = '/'
response.headers['Location'] = redirect_url
raise aiohttp.web.HTTPFound(location=redirect_url, headers=response.headers)
return wrapper
return decorator


teacher = _role_required(roles.ROLES.TEACHER)
admin = _role_required(roles.ROLES.ADMIN)
36 changes: 34 additions & 2 deletions learning_observer/learning_observer/dash_integration.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,18 @@

from pkg_resources import resource_filename, resource_listdir


import aiohttp_session
import asyncio
import dash
from dash import Dash, html, clientside_callback, Output, Input
from dash import Dash, html, clientside_callback, callback, Output, Input
import dash_bootstrap_components as dbc
from flask import request

from lo_dash_react_components import LOConnection

import learning_observer.prestartup
import learning_observer.paths
import learning_observer.impersonate


app = None
Expand All @@ -41,6 +45,8 @@ def get_app():
# paths.
PATH_TEMPLATE = "/{module}/dash/{subpath}/"

impersonation_header_id = '_impersonation_header'


def local_register_page(
module,
Expand Down Expand Up @@ -205,6 +211,8 @@ def load_dash_pages():
update_title=None
)

app.layout = html.Div([html.Div(id=impersonation_header_id), dash.page_container])

dash.register_page(
__name__,
path="/dash/test",
Expand All @@ -230,3 +238,27 @@ def load_dash_pages():
description=page['DESCRIPTION'],
path=path
)

@callback(
Output(impersonation_header_id, 'children'),
Input(impersonation_header_id, 'id') # triggers on page load
)
def update_impersonation_header(id):
'''Add impersonation header if we are impersonating
a user. This includes a 'Stop' button

HACK the impersonation information is stored in the aiohttp request.
Since we are working in the dash/flask environment, we do not
pass the aiohttp request around. Instead, we need to fetch it
from the `flask.request` object.
This does not feel like the optimal workflow for this, but it
does achieve the goal of wrapping a page in a header.
'''
session = asyncio.run(aiohttp_session.get_session(request.environ['aiohttp.request']))
if learning_observer.impersonate.IMPERSONATING_AS in session:
return html.Div([
# TODO clean up text for who we are impersonating
html.Span(f'Impersonating as {session[learning_observer.impersonate.IMPERSONATING_AS]}'),
dbc.Button('Stop', href='/stop-impersonation', external_link=True, color='danger', className='float-end')
], className='m-1')
return []
Loading
Loading