[sso-admin] Fix issues detected with mypy

While there, refactor thread handling in AppViews since it was not
practical.

Some issues found with mypy and fixed by this commit:

src/admin/views/ApiViews.py:240: error: Name "user_ddid" is not defined
src/admin/lib/nextcloud.py:331: error: Name "group" is not defined
src/admin/lib/nextcloud.py:394: error: Name "ProviderUserNotExists" is not defined
src/admin/lib/admin.py:1604: error: Trying to read deleted variable "se"
src/admin/lib/admin.py:1798: error: Trying to read deleted variable "se"
src/admin/lib/admin.py:1903: error: Name "group" is not defined

dd-sso/admin/src/admin/lib/admin.py

@@ -1553,7 +1553,7 @@ class Admin:
         internaluser : DDUser = [u for u in self.internal["users"] if u["id"] == user_id][0]
         cohorts = self.moodle.get_cohorts()
         for group in mdelete:
-            cohort = [c for c in cohorts if c["name"] == group[0]]
+            cohort = [c for c in cohorts if c["name"] == group[0]][0]
             try:
                 self.moodle.delete_user_in_cohort(
                     internaluser["moodle_id"], cohort["id"]
@@ -1599,12 +1599,12 @@ class Admin:
         try:
             self.moodle.create_user(email, username, password, first_name, last_name)
             ev.update_text(str({"name": "Added to moodle", "data": []}))
-        except UserExists:
+        except UserExists as ex:
             log.error("  -->> User already exists")
-            error = Events(self.app, "User already exists.", str(se), type="error")
+            error = Events(self.app, "User already exists.", str(ex), type="error")
-        except SystemError as se:
+        except SystemError as ex:
-            log.error("Moodle create user error: " + str(se))
+            log.error("Moodle create user error: " + str(ex))
-            error = Events(self.app, "Moodle create user error", str(se), type="error")
+            error = Events(self.app, "Moodle create user error", str(ex), type="error")
         except:
             log.error("  -->> Error creating on moodle the user: " + username)
             print(traceback.format_exc())
@@ -1793,12 +1793,12 @@ class Admin:
                 u["last"],
             )[0]["id"]
             ev.increment({"name": "Added to moodle", "data": []})
-        except UserExists:
+        except UserExists as ex:
             log.error("  -->> User already exists")
-            error = Events(self.app, "User already exists.", str(se), type="error")
+            error = Events(self.app, "User already exists.", str(ex), type="error")
-        except SystemError as se:
+        except SystemError as ex:
-            log.error("Moodle create user error: " + str(se))
+            log.error("Moodle create user error: " + str(ex))
-            error = Events(self.app, "Moodle create user error", str(se), type="error")
+            error = Events(self.app, "Moodle create user error", str(ex), type="error")
         except:
             log.error("  -->> Error creating on moodle the user: " + u["username"])
             print(traceback.format_exc())
@@ -1900,7 +1900,7 @@ class Admin:
         try:
             self.keycloak.delete_group(group_id)
         except:
-            log.error("KEYCLOAK GROUPS: Could no delete group " + group["path"])
+            log.error("KEYCLOAK GROUPS: Could no delete group " + group_id)
             return
 
         cohorts = self.moodle.get_cohorts()

dd-sso/admin/src/admin/lib/events.py

@@ -61,9 +61,9 @@ class Events:
     title : str
     text : str
     total : int
-    table : bool
+    table : str
     type : str
-    def __init__(self, app : "AdminFlaskApp", title : str, text : str="", total : int=0, table : bool=False, type : str="info") -> None:
+    def __init__(self, app : "AdminFlaskApp", title : str, text : str="", total : int=0, table : str="", type : str="info") -> None:
         self.app = app
         # notice, info, success, and error
         self.eid = str(base64.b64encode(os.urandom(32))[:8])
@@ -195,7 +195,7 @@ class Events:
         self.app.socketio.emit("reload", json.dumps({}), namespace="/sio", room="admin")
         sleep(0.0001)
 
-    def table(self, event : str, table : bool, data : Dict[str, Any]={}) -> None:
+    def table(self, event : str, table : str, data : Dict[str, Any]={}) -> None:
         # refresh, add, delete, update
         self.app.socketio.emit(
             "table_" + event,

dd-sso/admin/src/admin/lib/moodle.py

@@ -197,9 +197,9 @@ class Moodle:
         )
         return attempts
 
-    def get_cohorts(self) -> Any:
+    def get_cohorts(self) -> List[Dict[str, Any]]:
         cohorts = self.call("core_cohort_get_cohorts")
-        return cohorts
+        return cast(List[Dict[str, Any]], cohorts)
 
     def add_system_cohort(self, name : str, description : str ="", visible : bool=True) -> Any:
         bit_visible = 1 if visible else 0

dd-sso/admin/src/admin/lib/nextcloud.py

@@ -327,9 +327,11 @@ class Nextcloud:
                 return True
             if result["ocs"]["meta"]["statuscode"] == 102:
                 raise ProviderItemExists
-            if result["ocs"]["meta"]["statuscode"] == 104:
+            # TODO: It is unclear what status code 104 is, it certainly
-                self.add_group(group)
+            # shouldn't the group if it doesn't exist
-                # raise ProviderGroupNotExists
+            #if result["ocs"]["meta"]["statuscode"] == 104:
+            #    self.add_group(group)
+            #    # raise ProviderGroupNotExists
             log.error("Get Nextcloud provider user add error: " + str(result))
             raise ProviderOpError
         except:

dd-sso/admin/src/admin/lib/nextcloud_exc.py

@@ -1,5 +1,6 @@
 #
 #   Copyright © 2021,2022 IsardVDI S.L.
+#   Copyright © 2022 Evilham <contact@evilham.com>
 #
 #   This file is part of DD
 #
@@ -46,6 +47,10 @@ class ProviderItemNotExists(Exception):
     pass
 
 
+class ProviderUserNotExists(Exception):
+    pass
+
+
 class ProviderGroupNotExists(Exception):
     pass
 

dd-sso/admin/src/admin/views/ApiViews.py

@@ -188,7 +188,7 @@ def setup_api_views(app : "AdminFlaskApp") -> None:
             if not app.validators["user"].validate(data):
                 raise Error(
                     "bad_request",
-                    "Data validation for user failed: ",
+                    "Data validation for user failed: "
                       + str(app.validators["user"].errors),
                     traceback.format_exc(),
                 )
@@ -237,7 +237,7 @@ def setup_api_views(app : "AdminFlaskApp") -> None:
     @app.json_route("/ddapi/username/<old_user_ddid>/<new_user_did>", methods=["PUT"])
     @has_token
     def ddapi_username(old_user_ddid : str, new_user_did : str) -> OptionalJsonResponse:
-        user = app.admin.get_user_username(user_ddid)
+        user = app.admin.get_user_username(old_user_ddid)
         if not user:
             raise Error("not_found", "User id not found")
         # user = app.admin.update_user_username(old_user_ddid,new_user_did)

dd-sso/admin/src/admin/views/AppViews.py

@@ -21,6 +21,7 @@
 import concurrent.futures
 import json
 import logging as log
+from operator import itemgetter
 import os
 import re
 import sys
@@ -34,14 +35,15 @@ from uuid import uuid4
 from flask import Response, jsonify, redirect, render_template, request, url_for
 from flask_login import current_user, login_required
 
-from typing import TYPE_CHECKING, cast, Any, Dict, Optional
+from typing import TYPE_CHECKING, cast, Any, Callable, Dict, List, Optional, Tuple
 if TYPE_CHECKING:
     from admin.flaskapp import AdminFlaskApp
 
 from ..lib.helpers import system_group
 from .decorators import login_or_token, OptionalJsonResponse
 
-threads = {"external": None}
+# TODO: this is quirky and non-trivial to manage
+threads : Dict[str, threading.Thread] = {}
 # q = Queue.Queue()
 
 from keycloak.exceptions import KeycloakGetError
@@ -52,6 +54,39 @@ from ..lib.exceptions import UserExists, UserNotFound
 
 from ..lib.legal import get_legal, gen_legal_if_not_exists, new_legal
 
+def run_in_thread(
+        op : Callable[..., Any],
+        args : Tuple = tuple(),
+        err_msg : str = "Something went wrong",
+        err_code : int = 500,
+        busy_err_msg : str ="Precondition failed: already operating users"
+    ) -> OptionalJsonResponse:
+    if threads.get("external", None) is not None:
+        if threads["external"].is_alive():
+            return (
+                json.dumps(
+                    {"msg": busy_err_msg}
+                ),
+                412,
+                {"Content-Type": "application/json"},
+            )
+        else:
+            del threads["external"]
+    try:
+        threads["external"] = threading.Thread(
+            target=op, args=args
+        )
+        # TODO: this probably returns immediately and client gets no real feedback
+        threads["external"].start()
+        return json.dumps({}), 200, {"Content-Type": "application/json"}
+    except:
+        log.error(traceback.format_exc())
+        return (
+            json.dumps({"msg": err_msg}),
+            err_code,
+            {"Content-Type": "application/json"},
+        )
+
 def setup_app_views(app : "AdminFlaskApp") -> None:
     dashboard = Dashboard(app)
     @app.json_route("/sysadmin/api/resync")
@@ -109,32 +144,7 @@ def setup_app_views(app : "AdminFlaskApp") -> None:
             if current_user.role != "admin":
                 return json.dumps({}), 301, {"Content-Type": "application/json"}
 
-            if "external" in threads.keys():
+            return run_in_thread(app.admin.update_users_from_keycloak, err_msg="Add user error.")
-                if threads["external"] is not None and threads["external"].is_alive():
-                    return (
-                        json.dumps(
-                            {"msg": "Precondition failed: already working with users"}
-                        ),
-                        412,
-                        {"Content-Type": "application/json"},
-                    )
-                else:
-                    threads["external"] = None
-            try:
-                threads["external"] = threading.Thread(
-                    target=app.admin.update_users_from_keycloak, args=()
-                )
-                threads["external"].start()
-                return json.dumps({}), 200, {"Content-Type": "application/json"}
-            except:
-                log.error(traceback.format_exc())
-                return (
-                    json.dumps({"msg": "Add user error."}),
-                    500,
-                    {"Content-Type": "application/json"},
-                )
-
-            # return json.dumps(app.admin.update_users_from_keycloak()), 200, {'Content-Type': 'application/json'}
 
         users = app.admin.get_mix_users()
         if current_user.role != "admin":
@@ -151,80 +161,12 @@ def setup_app_views(app : "AdminFlaskApp") -> None:
         data = request.get_json(force=True)
         if request.method == "PUT":
             if action == "enable":
-                if "external" in threads.keys():
+                return run_in_thread(app.admin.enable_users, args=(data,), err_msg="Enable users error.")
-                    if threads["external"] is not None and threads["external"].is_alive():
-                        return (
-                            json.dumps(
-                                {"msg": "Precondition failed: already operating users"}
-                            ),
-                            412,
-                            {"Content-Type": "application/json"},
-                        )
-                    else:
-                        threads["external"] = None
-                try:
-                    threads["external"] = threading.Thread(
-                        target=app.admin.enable_users, args=(data,)
-                    )
-                    threads["external"].start()
-                    return json.dumps({}), 200, {"Content-Type": "application/json"}
-                except:
-                    log.error(traceback.format_exc())
-                    return (
-                        json.dumps({"msg": "Enable users error."}),
-                        500,
-                        {"Content-Type": "application/json"},
-                    )
             if action == "disable":
-                if "external" in threads.keys():
+                return run_in_thread(app.admin.disable_users, args=(data,), err_msg="Disabling users error.")
-                    if threads["external"] is not None and threads["external"].is_alive():
-                        return (
-                            json.dumps(
-                                {"msg": "Precondition failed: already operating users"}
-                            ),
-                            412,
-                            {"Content-Type": "application/json"},
-                        )
-                    else:
-                        threads["external"] = None
-                try:
-                    threads["external"] = threading.Thread(
-                        target=app.admin.disable_users, args=(data,)
-                    )
-                    threads["external"].start()
-                    return json.dumps({}), 200, {"Content-Type": "application/json"}
-                except:
-                    log.error(traceback.format_exc())
-                    return (
-                        json.dumps({"msg": "Disabling users error."}),
-                        500,
-                        {"Content-Type": "application/json"},
-                    )
             if action == "delete":
-                if "external" in threads.keys():
+                return run_in_thread(app.admin.delete_users, args=(data,), err_msg="Deleting users error.")
-                    if threads["external"] is not None and threads["external"].is_alive():
+
-                        return (
-                            json.dumps(
-                                {"msg": "Precondition failed: already operating users"}
-                            ),
-                            412,
-                            {"Content-Type": "application/json"},
-                        )
-                    else:
-                        threads["external"] = None
-                try:
-                    threads["external"] = threading.Thread(
-                        target=app.admin.delete_users, args=(data,)
-                    )
-                    threads["external"].start()
-                    return json.dumps({}), 200, {"Content-Type": "application/json"}
-                except:
-                    log.error(traceback.format_exc())
-                    return (
-                        json.dumps({"msg": "Deleting users error."}),
-                        500,
-                        {"Content-Type": "application/json"},
-                    )
         return json.dumps({}), 405, {"Content-Type": "application/json"}
 
 
@@ -278,28 +220,7 @@ def setup_app_views(app : "AdminFlaskApp") -> None:
             data["enabled"] = data.get("enabled", False) in [True, "on"]
             data["quota"] = data["quota"] if data["quota"] != "false" else False
             data["groups"] = data["groups"] if data.get("groups", False) else []
-            if "external" in threads.keys():
+            return run_in_thread(app.admin.add_user, args=(data,), err_msg="Add user error")
-                if threads["external"] is not None and threads["external"].is_alive():
-                    return (
-                        json.dumps({"msg": "Precondition failed: already adding users"}),
-                        412,
-                        {"Content-Type": "application/json"},
-                    )
-                else:
-                    threads["external"] = None
-            try:
-                threads["external"] = threading.Thread(
-                    target=app.admin.add_user, args=(data,)
-                )
-                threads["external"].start()
-                return json.dumps({}), 200, {"Content-Type": "application/json"}
-            except:
-                log.error(traceback.format_exc())
-                return (
-                    json.dumps({"msg": "Add user error."}),
-                    500,
-                    {"Content-Type": "application/json"},
-                )
 
         if request.method == "PUT":
             data = request.get_json(force=True)
@@ -331,7 +252,7 @@ def setup_app_views(app : "AdminFlaskApp") -> None:
     @app.json_route("/api/roles")
     @login_required
     def roles() -> OptionalJsonResponse:
-        sorted_roles = sorted(app.admin.get_roles(), key=lambda k: k["name"])
+        sorted_roles = sorted(app.admin.get_roles(), key=itemgetter("name"))
         if current_user.role != "admin":
             sorted_roles = [sr for sr in sorted_roles if sr["name"] != "admin"]
         return json.dumps(sorted_roles), 200, {"Content-Type": "application/json"}
@@ -396,37 +317,19 @@ def setup_app_views(app : "AdminFlaskApp") -> None:
     @app.json_route("/api/external", methods=["POST", "PUT", "GET", "DELETE"])
     @login_required
     def external() -> OptionalJsonResponse:
-        if "external" in threads.keys():
-            if threads["external"] is not None and threads["external"].is_alive():
-                return json.dumps({}), 301, {"Content-Type": "application/json"}
-            else:
-                threads["external"] = None
-
         if request.method == "POST":
             data = request.get_json(force=True)
             if data["format"] == "json-ga":
-                threads["external"] = threading.Thread(
+                return run_in_thread(app.admin.upload_json_ga, args=(data,))
-                    target=app.admin.upload_json_ga, args=(data,)
-                )
-                threads["external"].start()
-                return json.dumps({}), 200, {"Content-Type": "application/json"}
             if data["format"] == "csv-ug":
                 valid = check_upload_errors(data)
                 if valid["pass"]:
-                    threads["external"] = threading.Thread(
+                    return run_in_thread(app.admin.upload_csv_ug, args=(data,))
-                        target=app.admin.upload_csv_ug, args=(data,)
-                    )
-                    threads["external"].start()
-                    return json.dumps({}), 200, {"Content-Type": "application/json"}
                 else:
                     return json.dumps(valid), 422, {"Content-Type": "application/json"}
         if request.method == "PUT":
             data = request.get_json(force=True)
-            threads["external"] = threading.Thread(
+            return run_in_thread(app.admin.sync_external, args=(data,))
-                target=app.admin.sync_external, args=(data,)
-            )
-            threads["external"].start()
-            return json.dumps({}), 200, {"Content-Type": "application/json"}
         if request.method == "DELETE":
             print("RESET")
             app.admin.reset_external()