From 98230d0ee33bdd46013a26d7b03d7efbae975a6d Mon Sep 17 00:00:00 2001 From: Adarnof Date: Fri, 20 Apr 2018 14:15:19 -0400 Subject: [PATCH] Log but don't deal with problems refreshing tokens. --- allianceauth/authentication/tasks.py | 18 +++++--- allianceauth/authentication/tests.py | 66 ++++++++++++++++++---------- setup.py | 2 +- 3 files changed, 57 insertions(+), 29 deletions(-) diff --git a/allianceauth/authentication/tasks.py b/allianceauth/authentication/tasks.py index 61cc49a5..3a9064f1 100644 --- a/allianceauth/authentication/tasks.py +++ b/allianceauth/authentication/tasks.py @@ -1,6 +1,6 @@ import logging -from esi.errors import TokenExpiredError, TokenInvalidError +from esi.errors import TokenExpiredError, TokenInvalidError, IncompleteResponseError from esi.models import Token from celery import shared_task @@ -20,13 +20,19 @@ def check_character_ownership(owner_hash): except (TokenExpiredError, TokenInvalidError): t.delete() continue - - if t.character_owner_hash == old_hash: + except (KeyError, IncompleteResponseError): + # We can't validate the hash hasn't changed but also can't assume it has. Abort for now. + logger.warning("Failed to validate owner hash of {0} due to problems contacting SSO servers.".format( + tokens[0].character_name)) break - else: - logger.info('Character %s has changed ownership. Revoking %s tokens.' % (t.character_name, tokens.count())) + + if not t.character_owner_hash == old_hash: + logger.info( + 'Character %s has changed ownership. Revoking %s tokens.' % (t.character_name, tokens.count())) tokens.delete() - else: + break + + if not Token.objects.filter(character_owner_hash=owner_hash).exists(): logger.info('No tokens found with owner hash %s. Revoking ownership.' % owner_hash) CharacterOwnership.objects.filter(owner_hash=owner_hash).delete() diff --git a/allianceauth/authentication/tests.py b/allianceauth/authentication/tests.py index fb63f52e..2e235455 100644 --- a/allianceauth/authentication/tests.py +++ b/allianceauth/authentication/tests.py @@ -8,6 +8,7 @@ from .backends import StateBackend from .tasks import check_character_ownership from allianceauth.eveonline.models import EveCharacter, EveCorporationInfo, EveAllianceInfo from esi.models import Token +from esi.errors import IncompleteResponseError from allianceauth.authentication.decorators import main_character_required from django.test.client import RequestFactory from django.http.response import HttpResponse @@ -194,28 +195,6 @@ class CharacterOwnershipTestCase(TestCase): self.user = User.objects.get(pk=self.user.pk) self.assertIsNone(self.user.profile.main_character) - @mock.patch('esi.models.Token.update_token_data') - def test_character_ownership_check(self, update_token_data): - t = Token.objects.create( - user=self.user, - character_id=self.character.character_id, - character_name=self.character.character_name, - character_owner_hash='1', - ) - co = CharacterOwnership.objects.get(owner_hash='1') - check_character_ownership(co.owner_hash) - self.assertTrue(CharacterOwnership.objects.filter(owner_hash='1').exists()) - - t.character_owner_hash = '2' - t.save() - check_character_ownership(co.owner_hash) - self.assertFalse(CharacterOwnership.objects.filter(owner_hash='1').exists()) - - t.delete() - co = CharacterOwnership.objects.create(user=self.user, character=self.character, owner_hash='3') - check_character_ownership(co.owner_hash) - self.assertFalse(CharacterOwnership.objects.filter(owner_hash='3').exists()) - class StateTestCase(TestCase): @classmethod @@ -350,3 +329,46 @@ class StateTestCase(TestCase): self.user.save() self._refresh_user() self.assertEquals(self.user.profile.state, self.member_state) + + +class CharacterOwnershipCheckTestCase(TestCase): + @classmethod + def setUpTestData(cls): + cls.user = AuthUtils.create_user('test_user', disconnect_signals=True) + AuthUtils.add_main_character(cls.user, 'Test Character', '1', corp_id='1', alliance_id='1', + corp_name='Test Corp', alliance_name='Test Alliance') + cls.character = EveCharacter.objects.get(character_id='1') + cls.token = Token.objects.create( + user=cls.user, + character_id='1', + character_name='Test', + character_owner_hash='1', + ) + cls.ownership = CharacterOwnership.objects.get(character=cls.character) + + @mock.patch(MODULE_PATH + '.tasks.Token.update_token_data') + def test_no_change_owner_hash(self, update_token_data): + # makes sure the ownership isn't delete if owner hash hasn't changed + check_character_ownership(self.ownership) + self.assertTrue(CharacterOwnership.objects.filter(user=self.user).filter(character=self.character).exists()) + + @mock.patch(MODULE_PATH + '.tasks.Token.update_token_data') + def test_unable_to_update_token_data(self, update_token_data): + # makes sure ownerships and tokens aren't hellpurged when there's problems with the SSO servers + update_token_data.side_effect = IncompleteResponseError() + check_character_ownership(self.ownership) + self.assertTrue(CharacterOwnership.objects.filter(user=self.user).filter(character=self.character).exists()) + + update_token_data.side_effect = KeyError() + check_character_ownership(self.ownership) + self.assertTrue(CharacterOwnership.objects.filter(user=self.user).filter(character=self.character).exists()) + + @mock.patch(MODULE_PATH + '.tasks.Token.update_token_data') + @mock.patch(MODULE_PATH + '.tasks.Token.delete') + @mock.patch(MODULE_PATH + '.tasks.Token.objects.exists') + @mock.patch(MODULE_PATH + '.tasks.CharacterOwnership.objects.filter') + def test_owner_hash_changed(self, filter, exists, delete, update_token_data): + # makes sure the ownership is revoked when owner hash changes + filter.return_value.exists.return_value = False + check_character_ownership(self.ownership) + self.assertTrue(filter.return_value.delete.called) diff --git a/setup.py b/setup.py index 049a34ca..91198ff7 100644 --- a/setup.py +++ b/setup.py @@ -26,7 +26,7 @@ install_requires = [ 'openfire-restapi', 'sleekxmpp', - 'adarnauth-esi>=1.4,<2.0', + 'adarnauth-esi>=1.4.10,<2.0', ] testing_extras = [