Fix saml.DefaultSamlMappingProvider import

[jaywink]

Not sure from when, but trying to import the default configured SAML mapping provider crashes with circular imports.

Removed the imports synapse.state and synapse.storage which were caising the circular, and seem to have only been used for type hints, which were removed in 98aec1c

Signed-off-by: Jason Robinson jasonr@matrix.org

[clokep]

Is this an issue when using SAML under any config or only when sub-classing the default mapping handler or something else?

[jaywink]

I noticed this issue with an EMS host that was configured without a custom mapping handler, so it tried to import the default one when we run the config test before deploying.

We have some custom mapping providers which don't subclass the default one.

[richvdh]

before this lands, it would be good to record the text/stacktrace of the exception, and to track down where the regression got introduced. please.

[richvdh]

if these were really unused... why didn't the linter pick them up? 🤔

[clokep]

I think because they're just importing the module (as opposed to from foo import bar, but I had the same thought...

[jaywink]

Sorry for not posting the stracktrace before, but here it is:

((v1.38.1))$ ipython
Python 3.8.5 (default, Jul 28 2020, 12:59:40) 
Type 'copyright', 'credits' or 'license' for more information
IPython 7.19.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: from synapse.handlers.saml import DefaultSamlMappingProvider
---------------------------------------------------------------------------
ImportError                               Traceback (most recent call last)
<ipython-input-1-28551a889677> in <module>
----> 1 from synapse.handlers.saml import DefaultSamlMappingProvider

~/workspace/synapse/synapse/handlers/saml.py in <module>
     23 from synapse.api.errors import SynapseError
     24 from synapse.config import ConfigError
---> 25 from synapse.handlers._base import BaseHandler
     26 from synapse.handlers.sso import MappingException, UserAttributes
     27 from synapse.http.servlet import parse_string

~/workspace/synapse/synapse/handlers/_base.py in <module>
     16 from typing import TYPE_CHECKING, Optional
     17 
---> 18 import synapse.state
     19 import synapse.storage
     20 import synapse.types

~/workspace/synapse/synapse/state/__init__.py in <module>
     45 from synapse.logging.context import ContextResourceUsage
     46 from synapse.logging.utils import log_function
---> 47 from synapse.state import v1, v2
     48 from synapse.storage.databases.main.events_worker import EventRedactBehaviour
     49 from synapse.storage.roommember import ProfileInfo

~/workspace/synapse/synapse/state/v1.py in <module>
     27 )
     28 
---> 29 from synapse import event_auth
     30 from synapse.api.constants import EventTypes
     31 from synapse.api.errors import AuthError

~/workspace/synapse/synapse/event_auth.py in <module>
     30 )
     31 from synapse.events import EventBase
---> 32 from synapse.events.builder import EventBuilder
     33 from synapse.types import StateMap, UserID, get_domain_from_id
     34 

~/workspace/synapse/synapse/events/builder.py in <module>
     28 from synapse.crypto.event_signing import add_hashes_and_signatures
     29 from synapse.events import EventBase, _EventInternalMetadata, make_event_from_dict
---> 30 from synapse.state import StateHandler
     31 from synapse.storage.databases.main import DataStore
     32 from synapse.types import EventID, JsonDict

ImportError: cannot import name 'StateHandler' from partially initialized module 'synapse.state' (most likely due to a circular import) (/home/jaywink/workspace/synapse/synapse/state/__init__.py)

[jaywink]

AFAICT the regression happened in v1.38.0 and is caused by this line: https://github.com/matrix-org/synapse/pull/10253/files#diff-2f0812c5ff333e8ee94dd38210fc77f98ccd0d8a995f9864d8c839e311b3f1a9R32

Those type definitions could be moved so that they don't require importing EventBase during runtime. But still, these imports in _base.py are unnecessary as far as I can see, as the whole type definitions were removed in 98aec1c

Edit: line ref corrected to event_auth.py.

[richvdh]

lgtm otherwise

[anoadragon453]

We're going to ship this with v1.39.0 as this is a recent enough regression to warrant it.