Merge branch 'master' of https://github.com/Adarnof/allianceauth into sso_registration

# Conflicts: # alliance_auth/__init__.py # corputils/models.py # corputils/views.py # eveonline/tasks.py # fleetactivitytracking/views.py # hrapplications/admin.py # requirements.txt
2026-02-10 00:56:19 +01:00 · 2017-09-17 01:36:05 -04:00
parent c1547cab8b eee6a9132d
commit 02f2968ee5
36 changed files with 386 additions and 299 deletions
--- a/services/admin.py
+++ b/services/admin.py
@@ -1,5 +0,0 @@
-from __future__ import unicode_literals
-from django.contrib import admin
-from services.models import GroupCache
-
-admin.site.register(GroupCache)
--- a/services/migrations/0003_delete_groupcache.py
+++ b/services/migrations/0003_delete_groupcache.py
@@ -0,0 +1,18 @@
+# -*- coding: utf-8 -*-
+# Generated by Django 1.10.5 on 2017-09-02 06:07
+from __future__ import unicode_literals
+
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('services', '0002_auto_20161016_0135'),
+    ]
+
+    operations = [
+        migrations.DeleteModel(
+            name='GroupCache',
+        ),
+    ]
--- a/services/models.py
+++ b/services/models.py
@@ -1,18 +0,0 @@
-from __future__ import unicode_literals
-from django.utils.encoding import python_2_unicode_compatible
-from django.db import models
-
-
-@python_2_unicode_compatible
-class GroupCache(models.Model):
-    SERVICE_CHOICES = (
-        ("discourse", "discourse"),
-        ("discord", "discord"),
-    )
-
-    created = models.DateTimeField(auto_now_add=True)
-    groups = models.TextField(default={})
-    service = models.CharField(max_length=254, choices=SERVICE_CHOICES, unique=True)
-
-    def __str__(self):
-        return self.service
--- a/services/modules/discord/manager.py
+++ b/services/modules/discord/manager.py
@@ -3,14 +3,13 @@ import requests
 import json
 import re
 from django.conf import settings
-from services.models import GroupCache
 from requests_oauthlib import OAuth2Session
 from functools import wraps
 import logging
 import datetime
 import time
-from django.utils import timezone
 from django.core.cache import cache
+from hashlib import md5

 logger = logging.getLogger(__name__)

@@ -20,9 +19,13 @@ EVE_IMAGE_SERVER = "https://image.eveonline.com"
 AUTH_URL = "https://discordapp.com/api/oauth2/authorize"
 TOKEN_URL = "https://discordapp.com/api/oauth2/token"

-# needs administrator, since Discord can't get their permissions system to work
-# was kick members, manage roles, manage nicknames
-#BOT_PERMISSIONS = 0x00000002 + 0x10000000 + 0x08000000
+"""
+Previously all we asked for was permission to kick members, manage roles, and manage nicknames.
+Users have reported weird unauthorized errors we don't understand. So now we ask for full server admin.
+It's almost fixed the problem.
+"""
+# kick members, manage roles, manage nicknames
+# BOT_PERMISSIONS = 0x00000002 + 0x10000000 + 0x08000000
 BOT_PERMISSIONS = 0x00000008

 # get user ID, accept invite
@@ -31,7 +34,7 @@ SCOPES = [
    'guilds.join',
 ]

-GROUP_CACHE_MAX_AGE = datetime.timedelta(minutes=30)
+GROUP_CACHE_MAX_AGE = int(getattr(settings, 'DISCORD_GROUP_CACHE_MAX_AGE', 2 * 60 * 60))  # 2 hours default


 class DiscordApiException(Exception):
@@ -110,16 +113,16 @@ def api_backoff(func):
                    break
                except requests.HTTPError as e:
                    if e.response.status_code == 429:
-                        if 'Retry-After' in e.response.headers:
-                            retry_after = e.response.headers['Retry-After']
-                        else:
+                        try:
+                            retry_after = int(e.response.headers['Retry-After'])
+                        except (TypeError, KeyError):
                            # Pick some random time
                            retry_after = 5

                        logger.info("Received backoff from API of %s seconds, handling" % retry_after)
                        # Store value in redis
                        backoff_until = (datetime.datetime.utcnow() +
-                                         datetime.timedelta(seconds=int(retry_after)))
+                                         datetime.timedelta(seconds=retry_after))
                        global_backoff = bool(e.response.headers.get('X-RateLimit-Global', False))
                        if global_backoff:
                            logger.info("Global backoff!!")
@@ -150,10 +153,14 @@ class DiscordOAuthManager:
    def __init__(self):
        pass

+    @staticmethod
+    def _sanitize_name(name):
+        return re.sub('[^\w.-]', '', name)[:32]
+
    @staticmethod
    def _sanitize_groupname(name):
        name = name.strip(' _')
-        return re.sub('[^\w.-]', '', name)
+        return DiscordOAuthManager._sanitize_name(name)

    @staticmethod
    def generate_bot_add_url():
@@ -198,8 +205,9 @@ class DiscordOAuthManager:
    @staticmethod
    def update_nickname(user_id, nickname):
        try:
+            nickname = DiscordOAuthManager._sanitize_name(nickname)
            custom_headers = {'content-type': 'application/json', 'authorization': 'Bot ' + settings.DISCORD_BOT_TOKEN}
-            data = {'nick': nickname, }
+            data = {'nick': nickname}
            path = DISCORD_URL + "/guilds/" + str(settings.DISCORD_GUILD_ID) + "/members/" + str(user_id)
            r = requests.patch(path, headers=custom_headers, json=data)
            logger.debug("Got status code %s after setting nickname for Discord user ID %s (%s)" % (
@@ -230,7 +238,7 @@ class DiscordOAuthManager:
            return False

    @staticmethod
-    def __get_groups():
+    def _get_groups():
        custom_headers = {'accept': 'application/json', 'authorization': 'Bot ' + settings.DISCORD_BOT_TOKEN}
        path = DISCORD_URL + "/guilds/" + str(settings.DISCORD_GUILD_ID) + "/roles"
        r = requests.get(path, headers=custom_headers)
@@ -239,41 +247,20 @@ class DiscordOAuthManager:
        return r.json()

    @staticmethod
-    def __update_group_cache():
-        GroupCache.objects.filter(service="discord").delete()
-        cache = GroupCache.objects.create(service="discord")
-        cache.groups = json.dumps(DiscordOAuthManager.__get_groups())
-        cache.save()
-        return cache
+    def _generate_cache_role_key(name):
+        return 'DISCORD_ROLE_NAME__%s' % md5(str(name).encode('utf-8')).hexdigest()

    @staticmethod
-    def __get_group_cache():
-        if not GroupCache.objects.filter(service="discord").exists():
-            DiscordOAuthManager.__update_group_cache()
-        cache = GroupCache.objects.get(service="discord")
-        age = timezone.now() - cache.created
-        if age > GROUP_CACHE_MAX_AGE:
-            logger.debug("Group cache has expired. Triggering update.")
-            cache = DiscordOAuthManager.__update_group_cache()
-        return json.loads(cache.groups)
+    def _group_name_to_id(name):
+        name = DiscordOAuthManager._sanitize_groupname(name)

-    @staticmethod
-    def __group_name_to_id(name):
-        cache = DiscordOAuthManager.__get_group_cache()
-        for g in cache:
-            if g['name'] == name:
-                return g['id']
-        logger.debug("Group %s not found on Discord. Creating" % name)
-        DiscordOAuthManager.__create_group(name)
-        return DiscordOAuthManager.__group_name_to_id(name)
-
-    @staticmethod
-    def __group_id_to_name(id):
-        cache = DiscordOAuthManager.__get_group_cache()
-        for g in cache:
-            if g['id'] == id:
-                return g['name']
-        raise KeyError("Group ID %s not found on Discord" % id)
+        def get_or_make_role():
+            groups = DiscordOAuthManager._get_groups()
+            for g in groups:
+                if g['name'] == name:
+                    return g['id']
+            return DiscordOAuthManager._create_group(name)['id']
+        return cache.get_or_set(DiscordOAuthManager._generate_cache_role_key(name), get_or_make_role, GROUP_CACHE_MAX_AGE)

    @staticmethod
    def __generate_role():
@@ -300,16 +287,15 @@ class DiscordOAuthManager:
        return r.json()

    @staticmethod
-    def __create_group(name):
+    def _create_group(name):
        role = DiscordOAuthManager.__generate_role()
-        DiscordOAuthManager.__edit_role(role['id'], name)
-        DiscordOAuthManager.__update_group_cache()
+        return DiscordOAuthManager.__edit_role(role['id'], name)

    @staticmethod
    @api_backoff
    def update_groups(user_id, groups):
        custom_headers = {'content-type': 'application/json', 'authorization': 'Bot ' + settings.DISCORD_BOT_TOKEN}
-        group_ids = [DiscordOAuthManager.__group_name_to_id(DiscordOAuthManager._sanitize_groupname(g)) for g in groups]
+        group_ids = [DiscordOAuthManager._group_name_to_id(DiscordOAuthManager._sanitize_groupname(g)) for g in groups]
        path = DISCORD_URL + "/guilds/" + str(settings.DISCORD_GUILD_ID) + "/members/" + str(user_id)
        data = {'roles': group_ids}
        r = requests.patch(path, headers=custom_headers, json=data)
--- a/services/modules/discord/tasks.py
+++ b/services/modules/discord/tasks.py
@@ -9,7 +9,6 @@ from django.core.exceptions import ObjectDoesNotExist

 from notifications import notify
 from services.modules.discord.manager import DiscordOAuthManager, DiscordApiBackoff
-from services.tasks import only_one
 from .models import DiscordUser

 logger = logging.getLogger(__name__)
--- a/services/modules/discord/tests.py
+++ b/services/modules/discord/tests.py
@@ -346,7 +346,7 @@ class DiscordManagerTestCase(TestCase):
        # Assert
        self.assertTrue(result)

-    @mock.patch(MODULE_PATH + '.manager.DiscordOAuthManager._DiscordOAuthManager__get_group_cache')
+    @mock.patch(MODULE_PATH + '.manager.DiscordOAuthManager._get_groups')
    @requests_mock.Mocker()
    def test_update_groups(self, group_cache, m):
        from . import manager
@@ -380,7 +380,7 @@ class DiscordManagerTestCase(TestCase):
        self.assertNotIn(444, history['roles'], 'The group id 444 must NOT be added to the request')

    @mock.patch(MODULE_PATH + '.manager.cache')
-    @mock.patch(MODULE_PATH + '.manager.DiscordOAuthManager._DiscordOAuthManager__get_group_cache')
+    @mock.patch(MODULE_PATH + '.manager.DiscordOAuthManager._get_groups')
    @requests_mock.Mocker()
    def test_update_groups_backoff(self, group_cache, djcache, m):
        from . import manager
@@ -415,7 +415,7 @@ class DiscordManagerTestCase(TestCase):
        self.assertTrue(datetime.datetime.strptime(args[1], manager.cache_time_format) > datetime.datetime.now())

    @mock.patch(MODULE_PATH + '.manager.cache')
-    @mock.patch(MODULE_PATH + '.manager.DiscordOAuthManager._DiscordOAuthManager__get_group_cache')
+    @mock.patch(MODULE_PATH + '.manager.DiscordOAuthManager._get_groups')
    @requests_mock.Mocker()
    def test_update_groups_global_backoff(self, group_cache, djcache, m):
        from . import manager
--- a/services/modules/discourse/manager.py
+++ b/services/modules/discourse/manager.py
@@ -1,17 +1,15 @@
 from __future__ import unicode_literals
 import logging
 import requests
-import random
-import string
-import datetime
-import json
 import re
 from django.conf import settings
-from django.utils import timezone
-from services.models import GroupCache
+from django.core.cache import cache
+from hashlib import md5

 logger = logging.getLogger(__name__)

+GROUP_CACHE_MAX_AGE = int(getattr(settings, 'DISCOURSE_GROUP_CACHE_MAX_AGE', 2 * 60 * 60))  # default 2 hours
+

 class DiscourseError(Exception):
    def __init__(self, endpoint, errors):
@@ -21,12 +19,13 @@ class DiscourseError(Exception):
    def __str__(self):
        return "API execution failed.\nErrors: %s\nEndpoint: %s" % (self.errors, self.endpoint)

+
 # not exhaustive, only the ones we need
 ENDPOINTS = {
    'groups': {
        'list': {
            'path': "/admin/groups.json",
-            'method': requests.get,
+            'method': 'get',
            'args': {
                'required': [],
                'optional': [],
@@ -34,7 +33,7 @@ ENDPOINTS = {
        },
        'create': {
            'path': "/admin/groups",
-            'method': requests.post,
+            'method': 'post',
            'args': {
                'required': ['name'],
                'optional': ['visible'],
@@ -42,7 +41,7 @@ ENDPOINTS = {
        },
        'add_user': {
            'path': "/admin/groups/%s/members.json",
-            'method': requests.put,
+            'method': 'put',
            'args': {
                'required': ['usernames'],
                'optional': [],
@@ -50,7 +49,7 @@ ENDPOINTS = {
        },
        'remove_user': {
            'path': "/admin/groups/%s/members.json",
-            'method': requests.delete,
+            'method': 'delete',
            'args': {
                'required': ['username'],
                'optional': [],
@@ -58,7 +57,7 @@ ENDPOINTS = {
        },
        'delete': {
            'path': "/admin/groups/%s.json",
-            'method': requests.delete,
+            'method': 'delete',
            'args': {
                'required': [],
                'optional': [],
@@ -68,7 +67,7 @@ ENDPOINTS = {
    'users': {
        'create': {
            'path': "/users",
-            'method': requests.post,
+            'method': 'post',
            'args': {
                'required': ['name', 'email', 'password', 'username'],
                'optional': ['active'],
@@ -76,7 +75,7 @@ ENDPOINTS = {
        },
        'update': {
            'path': "/users/%s.json",
-            'method': requests.put,
+            'method': 'put',
            'args': {
                'required': ['params'],
                'optional': [],
@@ -84,7 +83,7 @@ ENDPOINTS = {
        },
        'get': {
            'path': "/users/%s.json",
-            'method': requests.get,
+            'method': 'get',
            'args': {
                'required': [],
                'optional': [],
@@ -92,7 +91,7 @@ ENDPOINTS = {
        },
        'activate': {
            'path': "/admin/users/%s/activate",
-            'method': requests.put,
+            'method': 'put',
            'args': {
                'required': [],
                'optional': [],
@@ -100,7 +99,7 @@ ENDPOINTS = {
        },
        'set_email': {
            'path': "/users/%s/preferences/email",
-            'method': requests.put,
+            'method': 'put',
            'args': {
                'required': ['email'],
                'optional': [],
@@ -108,7 +107,7 @@ ENDPOINTS = {
        },
        'suspend': {
            'path': "/admin/users/%s/suspend",
-            'method': requests.put,
+            'method': 'put',
            'args': {
                'required': ['duration', 'reason'],
                'optional': [],
@@ -116,7 +115,7 @@ ENDPOINTS = {
        },
        'unsuspend': {
            'path': "/admin/users/%s/unsuspend",
-            'method': requests.put,
+            'method': 'put',
            'args': {
                'required': [],
                'optional': [],
@@ -124,7 +123,7 @@ ENDPOINTS = {
        },
        'logout': {
            'path': "/admin/users/%s/log_out",
-            'method': requests.post,
+            'method': 'post',
            'args': {
                'required': [],
                'optional': [],
@@ -132,7 +131,7 @@ ENDPOINTS = {
        },
        'external': {
            'path': "/users/by-external/%s.json",
-            'method': requests.get,
+            'method': 'get',
            'args': {
                'required': [],
                'optional': [],
@@ -146,8 +145,7 @@ class DiscourseManager:
    def __init__(self):
        pass

-    GROUP_CACHE_MAX_AGE = datetime.timedelta(minutes=30)
-    REVOKED_EMAIL = 'revoked@' + settings.DOMAIN
+    REVOKED_EMAIL = 'revoked@localhost'
    SUSPEND_DAYS = 99999
    SUSPEND_REASON = "Disabled by auth."

@@ -171,7 +169,8 @@ class DiscourseManager:
        for arg in kwargs:
            if arg not in endpoint['args']['required'] and arg not in endpoint['args']['optional'] and not silent:
                logger.warn("Received unrecognized kwarg %s for endpoint %s" % (arg, endpoint))
-        r = endpoint['method'](settings.DISCOURSE_URL + endpoint['parsed_url'], params=params, json=data)
+        r = getattr(requests, endpoint['method'])(settings.DISCOURSE_URL + endpoint['parsed_url'], params=params,
+                                                  json=data)
        try:
            if 'errors' in r.json() and not silent:
                logger.error("Discourse execution failed.\nEndpoint: %s\nErrors: %s" % (endpoint, r.json()['errors']))
@@ -190,67 +189,59 @@ class DiscourseManager:
        return out

    @staticmethod
-    def __generate_random_pass():
-        return ''.join([random.choice(string.ascii_letters + string.digits) for n in range(16)])
-
-    @staticmethod
-    def __get_groups():
+    def _get_groups():
        endpoint = ENDPOINTS['groups']['list']
        data = DiscourseManager.__exc(endpoint)
        return [g for g in data if not g['automatic']]

    @staticmethod
-    def __update_group_cache():
-        GroupCache.objects.filter(service="discourse").delete()
-        cache = GroupCache.objects.create(service="discourse")
-        cache.groups = json.dumps(DiscourseManager.__get_groups())
-        cache.save()
-        return cache
-
-    @staticmethod
-    def __get_group_cache():
-        if not GroupCache.objects.filter(service="discourse").exists():
-            DiscourseManager.__update_group_cache()
-        cache = GroupCache.objects.get(service="discourse")
-        age = timezone.now() - cache.created
-        if age > DiscourseManager.GROUP_CACHE_MAX_AGE:
-            logger.debug("Group cache has expired. Triggering update.")
-            cache = DiscourseManager.__update_group_cache()
-        return json.loads(cache.groups)
-
-    @staticmethod
-    def __create_group(name):
+    def _create_group(name):
        endpoint = ENDPOINTS['groups']['create']
-        DiscourseManager.__exc(endpoint, name=name[:20], visible=True)
-        DiscourseManager.__update_group_cache()
+        return DiscourseManager.__exc(endpoint, name=name[:20], visible=True)['basic_group']
+
+    @staticmethod
+    def _generate_cache_group_name_key(name):
+        return 'DISCOURSE_GROUP_NAME__%s' % md5(name.encode('utf-8')).hexdigest()
+
+    @staticmethod
+    def _generate_cache_group_id_key(g_id):
+        return 'DISCOURSE_GROUP_ID__%s' % g_id

    @staticmethod
    def __group_name_to_id(name):
-        cache = DiscourseManager.__get_group_cache()
-        for g in cache:
-            if g['name'] == name[0:20]:
-                return g['id']
-        logger.debug("Group %s not found on Discourse. Creating" % name)
-        DiscourseManager.__create_group(name)
-        return DiscourseManager.__group_name_to_id(name)
+        name = DiscourseManager._sanitize_groupname(name)
+
+        def get_or_create_group():
+            groups = DiscourseManager._get_groups()
+            for g in groups:
+                if g['name'] == name:
+                    return g['id']
+            return DiscourseManager._create_group(name)['id']
+
+        return cache.get_or_set(DiscourseManager._generate_cache_group_name_key(name), get_or_create_group,
+                                GROUP_CACHE_MAX_AGE)

    @staticmethod
-    def __group_id_to_name(id):
-        cache = DiscourseManager.__get_group_cache()
-        for g in cache:
-            if g['id'] == id:
-                return g['name']
-        raise KeyError("Group ID %s not found on Discourse" % id)
+    def __group_id_to_name(g_id):
+        def get_group_name():
+            groups = DiscourseManager._get_groups()
+            for g in groups:
+                if g['id'] == g_id:
+                    return g['name']
+            raise KeyError("Group ID %s not found on Discourse" % g_id)
+
+        return cache.get_or_set(DiscourseManager._generate_cache_group_id_key(g_id), get_group_name,
+                                GROUP_CACHE_MAX_AGE)

    @staticmethod
-    def __add_user_to_group(id, username):
+    def __add_user_to_group(g_id, username):
        endpoint = ENDPOINTS['groups']['add_user']
-        DiscourseManager.__exc(endpoint, id, usernames=[username])
+        DiscourseManager.__exc(endpoint, g_id, usernames=[username])

    @staticmethod
-    def __remove_user_from_group(id, username):
+    def __remove_user_from_group(g_id, username):
        endpoint = ENDPOINTS['groups']['remove_user']
-        DiscourseManager.__exc(endpoint, id, username=username)
+        DiscourseManager.__exc(endpoint, g_id, username=username)

    @staticmethod
    def __generate_group_dict(names):
@@ -269,10 +260,6 @@ class DiscourseManager:
        data = DiscourseManager.__get_user(name, silent=silent)
        return data['user']['id']

-    @staticmethod
-    def __user_id_to_name(id):
-        raise NotImplementedError
-
    @staticmethod
    def __get_user(username, silent=False):
        endpoint = ENDPOINTS['users']['get']
@@ -281,14 +268,14 @@ class DiscourseManager:
    @staticmethod
    def __activate_user(username):
        endpoint = ENDPOINTS['users']['activate']
-        id = DiscourseManager.__user_name_to_id(username)
-        DiscourseManager.__exc(endpoint, id)
+        u_id = DiscourseManager.__user_name_to_id(username)
+        DiscourseManager.__exc(endpoint, u_id)

    @staticmethod
    def __update_user(username, **kwargs):
        endpoint = ENDPOINTS['users']['update']
-        id = DiscourseManager.__user_name_to_id(username)
-        DiscourseManager.__exc(endpoint, id, params=kwargs)
+        u_id = DiscourseManager.__user_name_to_id(username)
+        DiscourseManager.__exc(endpoint, u_id, params=kwargs)

    @staticmethod
    def __create_user(username, email, password):
@@ -300,21 +287,21 @@ class DiscourseManager:
        try:
            DiscourseManager.__user_name_to_id(username, silent=True)
            return True
-        except:
+        except DiscourseError:
            return False

    @staticmethod
    def __suspend_user(username):
-        id = DiscourseManager.__user_name_to_id(username)
+        u_id = DiscourseManager.__user_name_to_id(username)
        endpoint = ENDPOINTS['users']['suspend']
-        return DiscourseManager.__exc(endpoint, id, duration=DiscourseManager.SUSPEND_DAYS,
+        return DiscourseManager.__exc(endpoint, u_id, duration=DiscourseManager.SUSPEND_DAYS,
                                      reason=DiscourseManager.SUSPEND_REASON)

    @staticmethod
    def __unsuspend(username):
-        id = DiscourseManager.__user_name_to_id(username)
+        u_id = DiscourseManager.__user_name_to_id(username)
        endpoint = ENDPOINTS['users']['unsuspend']
-        return DiscourseManager.__exc(endpoint, id)
+        return DiscourseManager.__exc(endpoint, u_id)

    @staticmethod
    def __set_email(username, email):
@@ -322,47 +309,53 @@ class DiscourseManager:
        return DiscourseManager.__exc(endpoint, username, email=email)

    @staticmethod
-    def __logout(id):
+    def __logout(u_id):
        endpoint = ENDPOINTS['users']['logout']
-        return DiscourseManager.__exc(endpoint, id)
+        return DiscourseManager.__exc(endpoint, u_id)

    @staticmethod
-    def __get_user_by_external(id):
+    def __get_user_by_external(u_id):
        endpoint = ENDPOINTS['users']['external']
-        return DiscourseManager.__exc(endpoint, id)
+        return DiscourseManager.__exc(endpoint, u_id)

    @staticmethod
-    def __user_id_by_external_id(id):
-        data = DiscourseManager.__get_user_by_external(id)
+    def __user_id_by_external_id(u_id):
+        data = DiscourseManager.__get_user_by_external(u_id)
        return data['user']['id']

+    @staticmethod
+    def _sanitize_name(name):
+        name = name.replace(' ', '_')
+        name = name.replace("'", '')
+        name = name.lstrip(' _')
+        name = name[:20]
+        name = name.rstrip(' _')
+        return name
+
    @staticmethod
    def _sanitize_username(username):
-        sanitized = username.replace(" ", "_")
-        sanitized = sanitized.strip(' _')
-        sanitized = sanitized.replace("'", "")
-        return sanitized
+        return DiscourseManager._sanitize_name(username)

    @staticmethod
    def _sanitize_groupname(name):
-        name = name.strip(' _')
        name = re.sub('[^\w]', '', name)
+        name = DiscourseManager._sanitize_name(name)
        if len(name) < 3:
-            name = name + "".join('_' for i in range(3-len(name)))
-        return name[:20]
+            name = "Group " + name
+        return name

    @staticmethod
    def update_groups(user):
        groups = []
        for g in user.groups.all():
-            groups.append(DiscourseManager._sanitize_groupname(str(g)[:20]))
+            groups.append(DiscourseManager._sanitize_groupname(str(g)))
        logger.debug("Updating discourse user %s groups to %s" % (user, groups))
        group_dict = DiscourseManager.__generate_group_dict(groups)
        inv_group_dict = {v: k for k, v in group_dict.items()}
        username = DiscourseManager.__get_user_by_external(user.pk)['user']['username']
        user_groups = DiscourseManager.__get_user_groups(username)
        add_groups = [group_dict[x] for x in group_dict if not group_dict[x] in user_groups]
-        rem_groups = [x for x in user_groups if not x in inv_group_dict]
+        rem_groups = [x for x in user_groups if x not in inv_group_dict]
        if add_groups or rem_groups:
            logger.info(
                "Updating discourse user %s groups: adding %s, removing %s" % (username, add_groups, rem_groups))
--- a/services/modules/discourse/tasks.py
+++ b/services/modules/discourse/tasks.py
@@ -5,8 +5,6 @@ from django.contrib.auth.models import User
 from django.core.exceptions import ObjectDoesNotExist
 from notifications import notify

-from services.tasks import only_one
-
 from .manager import DiscourseManager
 from .models import DiscourseUser

--- a/services/modules/openfire/manager.py
+++ b/services/modules/openfire/manager.py
@@ -61,7 +61,7 @@ class OpenfireManager:

    @staticmethod
    def _sanitize_groupname(name):
-        name = name.strip(' _')
+        name = name.strip(' _').lower()
        return re.sub('[^\w.-]', '', name)

    @staticmethod
@@ -120,9 +120,10 @@ class OpenfireManager:
            logger.error("Unable to update openfire user %s password - user not found on server." % username)
            return ""

-    @staticmethod
-    def update_user_groups(username, groups):
+    @classmethod
+    def update_user_groups(cls, username, groups):
        logger.debug("Updating openfire user %s groups %s" % (username, groups))
+        s_groups = list(map(cls._sanitize_groupname, groups))  # Sanitized group names
        api = ofUsers(settings.OPENFIRE_ADDRESS, settings.OPENFIRE_SECRET_KEY)
        response = api.get_user_groups(username)
        remote_groups = []
@@ -130,16 +131,15 @@ class OpenfireManager:
            remote_groups = response['groupname']
            if isinstance(remote_groups, six.string_types):
                remote_groups = [remote_groups]
+        remote_groups = list(map(cls._sanitize_groupname, remote_groups))
        logger.debug("Openfire user %s has groups %s" % (username, remote_groups))
        add_groups = []
        del_groups = []
-        for g in groups:
-            g = OpenfireManager._sanitize_groupname(g)
+        for g in s_groups:
            if g not in remote_groups:
                add_groups.append(g)
        for g in remote_groups:
-            g = OpenfireManager._sanitize_groupname(g)
-            if g not in groups:
+            if g not in s_groups:
                del_groups.append(g)
        logger.info(
            "Updating openfire groups for user %s - adding %s, removing %s" % (username, add_groups, del_groups))
@@ -155,10 +155,11 @@ class OpenfireManager:
        api.delete_user_groups(username, groups)
        logger.info("Deleted groups %s from openfire user %s" % (groups, username))

-    @staticmethod
-    def send_broadcast_message(group_name, broadcast_message):
-        logger.debug("Sending jabber ping to group %s with message %s" % (group_name, broadcast_message))
-        to_address = group_name + '@' + settings.BROADCAST_SERVICE_NAME + '.' + settings.JABBER_URL
+    @classmethod
+    def send_broadcast_message(cls, group_name, broadcast_message):
+        s_group_name = cls._sanitize_groupname(group_name)
+        logger.debug("Sending jabber ping to group %s with message %s" % (s_group_name, broadcast_message))
+        to_address = s_group_name + '@' + settings.BROADCAST_SERVICE_NAME + '.' + settings.JABBER_URL
        xmpp = PingBot(settings.BROADCAST_USER, settings.BROADCAST_USER_PASSWORD, to_address, broadcast_message)
        xmpp.register_plugin('xep_0030')  # Service Discovery
        xmpp.register_plugin('xep_0199')  # XMPP Ping
--- a/services/modules/openfire/tests.py
+++ b/services/modules/openfire/tests.py
@@ -211,3 +211,28 @@ class OpenfireManagerTestCase(TestCase):
        result_username = self.manager._OpenfireManager__sanitize_username(test_username)

        self.assertEqual(result_username, 'My_Test\\20User\\22\\27\\26\\2f\\3a\\3c\\3e\\40name\\5c20name')
+
+    def test__sanitize_groupname(self):
+        test_groupname = " My_Test Groupname"
+
+        result_groupname = self.manager._sanitize_groupname(test_groupname)
+
+        self.assertEqual(result_groupname, "my_testgroupname")
+
+    @mock.patch(MODULE_PATH + '.manager.ofUsers')
+    def test_update_user_groups(self, api):
+        groups = ["AddGroup", "othergroup", "Guest Group"]
+        server_groups = ["othergroup", "Guest Group", "REMOVE group"]
+        username = "testuser"
+        api_instance = api.return_value
+        api_instance.get_user_groups.return_value = {'groupname': server_groups}
+
+        self.manager.update_user_groups(username, groups)
+
+        self.assertTrue(api_instance.add_user_groups.called)
+        args, kwargs = api_instance.add_user_groups.call_args
+        self.assertEqual(args[1], ["addgroup"])
+
+        self.assertTrue(api_instance.delete_user_groups.called)
+        args, kwargs = api_instance.delete_user_groups.call_args
+        self.assertEqual(args[1], ["removegroup"])
--- a/services/modules/seat/manager.py
+++ b/services/modules/seat/manager.py
@@ -63,7 +63,7 @@ class SeatManager:
            logger.info("Added SeAT user with username %s" % sanitized)
            return sanitized, password
        logger.info("Failed to add SeAT user with username %s" % sanitized)
-        return None
+        return None, None

    @classmethod
    def delete_user(cls, username):
@@ -75,25 +75,6 @@ class SeatManager:
            return username
        return None

-    @classmethod
-    def disable_user(cls, username):
-        """ Disable user """
-        ret = cls.exec_request('user/{}'.format(username), 'put', active=0)
-        logger.debug(ret)
-        ret = cls.exec_request('user/{}'.format(username), 'put', email="")
-        logger.debug(ret)
-        if cls._response_ok(ret):
-            try:
-                cls.update_roles(username, [])
-                logger.info("Disabled SeAT user with username %s" % username)
-                return username
-            except KeyError:
-                # if something goes wrong, delete user from seat instead of disabling
-                if cls.delete_user(username):
-                    return username
-        logger.info("Failed to disabled SeAT user with username %s" % username)
-        return None
-
    @classmethod
    def enable_user(cls, username):
        """ Enable user """
@@ -105,14 +86,22 @@ class SeatManager:
        logger.info("Failed to enabled SeAT user with username %s" % username)
        return None

+    @classmethod
+    def _check_email_changed(cls, username, email):
+        """Compares email to one set on SeAT"""
+        ret = cls.exec_request('user/{}'.format(username), 'get', raise_for_status=True)
+        return ret['email'] != email
+
    @classmethod
    def update_user(cls, username, email, password):
        """ Edit user info """
-        logger.debug("Updating SeAT username %s with email %s and password" % (username, email))
-        ret = cls.exec_request('user/{}'.format(username), 'put', email=email)
-        logger.debug(ret)
-        if not cls._response_ok(ret):
-            logger.warn("Failed to update email for username {}".format(username))
+        if cls._check_email_changed(username, email):
+            # if we try to set the email to whatever it is already on SeAT, we get a HTTP422 error
+            logger.debug("Updating SeAT username %s with email %s and password" % (username, email))
+            ret = cls.exec_request('user/{}'.format(username), 'put', email=email)
+            logger.debug(ret)
+            if not cls._response_ok(ret):
+                logger.warn("Failed to update email for username {}".format(username))
        ret = cls.exec_request('user/{}'.format(username), 'put', password=password)
        logger.debug(ret)
        if not cls._response_ok(ret):
@@ -275,5 +264,5 @@ class SeatManager:
    @staticmethod
    def username_hash(username):
        m = hashlib.sha1()
-        m.update(username)
+        m.update(username.encode('utf-8'))
        return m.hexdigest()
--- a/services/modules/seat/tasks.py
+++ b/services/modules/seat/tasks.py
@@ -28,7 +28,7 @@ class SeatTasks:

    @classmethod
    def delete_user(cls, user, notify_user=False):
-        if cls.has_account(user) and SeatManager.disable_user(user.seat.username):
+        if cls.has_account(user) and SeatManager.delete_user(user.seat.username):
            user.seat.delete()
            logger.info("Successfully deactivated SeAT for user %s" % user)
            if notify_user:
--- a/services/modules/seat/tests.py
+++ b/services/modules/seat/tests.py
@@ -92,10 +92,10 @@ class SeatHooksTestCase(TestCase):

        # Test none user is deleted
        none_user = User.objects.get(username=self.none_user)
-        manager.disable_user.return_value = 'abc123'
+        manager.delete_user.return_value = 'abc123'
        SeatUser.objects.create(user=none_user, username='abc123')
        service.validate_user(none_user)
-        self.assertTrue(manager.disable_user.called)
+        self.assertTrue(manager.delete_user.called)
        with self.assertRaises(ObjectDoesNotExist):
            none_seat = User.objects.get(username=self.none_user).seat

@@ -107,7 +107,7 @@ class SeatHooksTestCase(TestCase):
        result = service.delete_user(member)

        self.assertTrue(result)
-        self.assertTrue(manager.disable_user.called)
+        self.assertTrue(manager.delete_user.called)
        with self.assertRaises(ObjectDoesNotExist):
            seat_user = User.objects.get(username=self.member).seat

@@ -169,7 +169,7 @@ class SeatViewsTestCase(TestCase):

        response = self.client.get(urls.reverse('auth_deactivate_seat'))

-        self.assertTrue(manager.disable_user.called)
+        self.assertTrue(manager.delete_user.called)
        self.assertRedirects(response, expected_url=urls.reverse('auth_services'), target_status_code=200)
        with self.assertRaises(ObjectDoesNotExist):
            seat_user = User.objects.get(pk=self.member.pk).seat
--- a/services/modules/teamspeak3/manager.py
+++ b/services/modules/teamspeak3/manager.py
@@ -217,6 +217,10 @@ class Teamspeak3Manager:
        logger.debug("Deleting user %s with id %s from TS3 server." % (user, uid))
        if user:
            clients = self.server.send_command('clientlist')
+            if isinstance(clients, dict):
+                # Rewrap list
+                clients = [clients]
+                
            for client in clients:
                try:
                    if client['keys']['client_database_id'] == user: