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

bradley-erickson · 2023-12-21T16:38:40Z

No description provided.

pmitros · 2023-12-21T16:45:28Z

learning_observer/learning_observer/auth/password.py

@@ -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': learning_observer.auth.roles.ROLES.ADMIN


auth.ROLES.ADMIN or auth.roles.ADMIN <-- remove one level of indirection

fine just as from roles import ROLES or setattr on the module with a loop over ROLES

pmitros · 2023-12-21T16:47:23Z

learning_observer/learning_observer/auth/social_sso.py

@@ -151,7 +152,9 @@ 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 this still just verify the teacher account?
+        'authorized': await learning_observer.auth.utils.verify_teacher_account(profile['id'], profile['email']),


verify_account(TEACHER, profiles...) or similar.

Same everywhere. Have a dict mapping to files if needed: {TEACHER: {'roster_file': '/foo/bar/teacher.json', ...}}

The key here is DRY.

pmitros · 2023-12-21T16:47:48Z

learning_observer/learning_observer/auth/utils.py

@@ -50,6 +51,28 @@ def fernet_key(secret_string):
    return md5_hash.hexdigest().encode('utf-8')


+async def verify_role(user_id, email):
+    if await verify_admin_account(user_id, email):


pmitros · 2023-12-21T16:50:54Z

learning_observer/learning_observer/auth/http_basic.py

@@ -160,7 +162,8 @@ async def password_auth_handler(request):
                'name': "",
                'family_name': "",
                'picture': "",
-                'authorized': True
+                'authorized': True,
+                'role': learning_observer.auth.roles.ROLES.ADMIN


We do not want to default to ADMIN ever, anywhere, since that will be brittle to security issues

Default to student

Permit a different role in settings (e.g. settings.auth.http_basic_auth.role)

Permit escalation through files enumerating admin and teacher users

The use cases:

Students log in through http basic, and a select number of teacher have dashboards

Teachers log in for demoing dashboards, same as above for admin

Extend the password file to allow an optional role field for escalation beyond default (note the script in util needs to be updated to allow this, and to allow this to be left blank for default behavior).

learning_observer/learning_observer/auth/roles.py

pmitros · 2023-12-21T16:59:33Z

learning_observer/learning_observer/auth/utils.py

-#
-# For now, we don't have seperate teacher and admin accounts.
-teacher = admin
+def role_required(role):


_role_required

pmitros · 2023-12-21T17:02:31Z

learning_observer/learning_observer/google.py

+    session = await aiohttp_session.get_session(request)
+    user = session['user']
+
+    cache_key = "raw_google/" + user['user_id'] + '/' + learning_observer.util.url_pathname(url)


It seems like there ought to be (already, and if not make one) a helper to make keys. If I submit a user_id of /etc/passwd or something, I don't want the universe imploding. This was of constructing the key looks very concerning to me.

In the whole system, we want one way of encoding strings, and one way of making safe keys and similar (or perhaps two if really, really needed)

See also, safe versus unsafe strings.

pmitros · 2023-12-21T17:05:06Z

learning_observer/learning_observer/google.py

@@ -190,7 +194,8 @@ def initialize_and_register_routes(app):
    for key in ['save_google_ajax', 'use_google_ajax', 'save_clean_ajax', 'use_clean_ajax']:
        if key in settings.settings['feature_flags']:
            global cache
-            cache = learning_observer.kvs.FilesystemKVS(path=learning_observer.paths.data('google'), subdirs=True)
+            cache = learning_observer.kvs.KVS()


This should be some kind of specific KVS for this functionality, bubbling up to default. I want to be able to use the filesystem method too, for example.

pmitros · 2023-12-21T17:05:51Z

Before merge (not now): Let's make sure everything has good docstrings.

pmitros · 2023-12-21T17:09:25Z

learning_observer/learning_observer/impersonate.py

+
+    session['original_user_id'] = session['user']['user_id']
+    session['user']['user_id'] = requested_user_id
+    return aiohttp.web.Response(text=f'Masking as a new user: {requested_user_id}')


Two impersonations later, I've lost my original user id.

We're overloading the meaning of user_id in ways I don't like

I'd much rather have a helper to get at the user (e.g. not call into session directly anymore) which gives the appropriate user, and have fields in the session:

user_id and masquerading_as

which the helper uses. That's a better data representation, and we want a helper in either case. As much as possible, we want to use decorators and helpers for specific purposes, rather than diving into session directly.

learning_observer/learning_observer/impersonate.py

pmitros · 2023-12-21T17:10:27Z

learning_observer/learning_observer/impersonate.py

+    if 'original_user_id' in session:
+        session['user']['user_id'] = session['original_user_id']
+        del session['original_user_id']
+        return aiohttp.web.Response(text='Done masking user')


Use same consistent terminology :)

pmitros · 2023-12-21T17:10:47Z

learning_observer/learning_observer/kvs.py

@@ -278,6 +278,7 @@ async def keys(self):
    'stub': InMemoryKVS,
    'redis_ephemeral': EphemeralRedisKVS,
    'redis': PersistentRedisKVS
+    # TODO add filesystem KVS to this


Do this TODO :)

pmitros · 2023-12-21T17:11:12Z

learning_observer/learning_observer/prestartup.py

@@ -105,6 +105,23 @@ def validate_teacher_list():
        )


+@register_startup_check
+def validate_admins_list():


Loop. One call. DRY

TEACHER, ADMIN

pmitros · 2023-12-21T17:12:03Z

learning_observer/learning_observer/routes.py

+            '/stop-impersonation',
+            learning_observer.impersonate.stop_impersonation
+        )
+    ])


We'll also want a view, UX, or similar at some point.

Header on every page, warning "viewing as" as well as UX for changing who / where

Fine do do UX just for react pages and assume we'll add it in as we migrate the rest of the system.

…r passing impersonated user around

pmitros · 2023-12-23T18:12:09Z

learning_observer/learning_observer/auth/utils.py

@@ -88,6 +88,16 @@ async def update_session_user_info(request, user):
    session["user"] = user


+async def get_active_user(request):


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

pmitros · 2023-12-23T20:00:14Z

learning_observer/learning_observer/impersonate.py

-    session['original_user_id'] = session['user']['user_id']
-    session['user']['user_id'] = requested_user_id
+    # TODO we should pull more of the users information from somewhere
+    session[IMPERSONATING_AS] = {'user_id': requested_user_id}


Good idea! Come to think of it, we should have similar variables for user_id and more globally as well, if you are making further passes. :)

pmitros · 2024-01-02T17:46:20Z

learning_observer/learning_observer/auth/utils.py

+            auth in our code (e.g. password, http_basic, google, etc.)
+            '''
+            if 'user' in request and \
+            request['user'] is not None and \


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.

pmitros · 2024-01-02T17:49:17Z

learning_observer/learning_observer/google.py

+@learning_observer.prestartup.register_startup_check
+def connect_to_google_cache():
+    '''Setup cache for request to the Google API if.
+    The cache is currenlty only used with the `use_google_ajax`


Typos: "if." "currenlty"

pmitros · 2024-01-02T17:52:20Z

learning_observer/learning_observer/impersonate.py

+    session = await aiohttp_session.get_session(request)
+    if IMPERSONATING_AS in session:
+        del session[IMPERSONATING_AS]
+        return aiohttp.web.Response(text='Done impersonating user.')


Perhaps json_response so it's expandable?

added more distinct admin and teacher roles

90b5804

pmitros reviewed Dec 21, 2023

View reviewed changes

learning_observer/learning_observer/auth/roles.py Show resolved Hide resolved

pmitros reviewed Dec 21, 2023

View reviewed changes

learning_observer/learning_observer/impersonate.py Show resolved Hide resolved

pmitros reviewed Dec 21, 2023

View reviewed changes

bradley-erickson added 4 commits December 22, 2023 09:56

updated comments and password auth role handling

0a0d96b

updated encoding and added file system to kvs options

c3e3db4

followed DRY on startup and when verifying roles

a197d61

updated impersonation workflow, still needs work finding best spot fo…

222c383

…r passing impersonated user around

pmitros reviewed Dec 23, 2023

View reviewed changes

bradley-erickson added 2 commits December 26, 2023 10:06

add more docstrings and comments

3cb36e6

added impersonation header

8f2e5f0

pmitros reviewed Jan 2, 2024

View reviewed changes

cleaned up conditional and fixed typos

e2d384a

bradley-erickson merged commit 8735c98 into master Jan 2, 2024
1 check failed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bradley-erickson commented Dec 21, 2023

pmitros Dec 21, 2023

pmitros Dec 21, 2023

pmitros Dec 21, 2023

pmitros Dec 21, 2023

pmitros Dec 21, 2023

pmitros Dec 21, 2023

pmitros Dec 21, 2023

pmitros Dec 21, 2023

pmitros commented Dec 21, 2023

pmitros Dec 21, 2023

pmitros Dec 21, 2023

pmitros Dec 21, 2023

pmitros Dec 21, 2023

pmitros Dec 21, 2023

pmitros Dec 21, 2023

pmitros Dec 21, 2023

pmitros Dec 21, 2023

pmitros Dec 23, 2023

pmitros Dec 23, 2023

pmitros Jan 2, 2024

pmitros Jan 2, 2024

pmitros Jan 2, 2024

		@@ -88,6 +88,16 @@ async def update_session_user_info(request, user):
		session["user"] = user


		async def get_active_user(request):

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

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

Conversation

bradley-erickson commented Dec 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmitros commented Dec 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment