New listener resource for the federation API "openid/userinfo" endpoint

[jaywink]

Integration managers use the OpenID userinfo endpoint in the federation API to verify that user OpenID access tokens are valid. If the federation resource is disabled, integration managers will not be able to verify the access token, causing a broken experience for users. The OpenID userinfo endpoint has now been split to a separate openid resource, which is enabled by default in newly generated configuration. It is also enabled automatically if the federation resource is enabled.

Requires no actions on homeservers which have federation listener enabled. For homeservers with federation listener disabled, the openid listener resource needs to be manually added to the existing configuration, if the resource is wanted to be enabled. New installations will adopt the default of activating the resource even if federation resource is disabled.

Add also parameterized Python module to be able to easily parameterize test runs.

[codecov-io]

Codecov Report

Merging #4420 into develop will increase coverage by 0.27%.
The diff coverage is 96.87%.

@@             Coverage Diff             @@
##           develop    #4420      +/-   ##
===========================================
+ Coverage     73.7%   73.98%   +0.27%     
===========================================
  Files          300      301       +1     
  Lines        29705    29841     +136     
  Branches      4882     4905      +23     
===========================================
+ Hits         21895    22077     +182     
+ Misses        6385     6335      -50     
- Partials      1425     1429       +4

[erikjohnston]

This looks good! A few nits, but otherwise the main thing are the tests.

Personally, I think we can just do three cases:

1. Neither federation nor openid listeners, and requests to openid and federation apis both fail
2. Only openid listener is added, etc
3. Federation listener is added, etc

That would also mean we wouldn't need to pull in parameterized, which while neat probably just complicates matters in this case

[erikjohnston]

Why is this necessary?

[jaywink]

The tests blow up otherwise due to some internals in the KeyApiV2Resource which would require further tweaking of the test homeserver possibly. Since we're not testing that, it was easier to mock it.

I'll post here what the problem was when refactoring the tests.

[jaywink]

  File "/home/jaywink/workspace/synapse/synapse/app/homeserver.py", line 227, in _configure_named_resource
    resources[SERVER_KEY_V2_PREFIX] = KeyApiV2Resource(self)
  File "/home/jaywink/workspace/synapse/synapse/rest/key/v2/__init__.py", line 25, in __init__
    self.putChild(b"server", LocalKey(hs))
  File "/home/jaywink/workspace/synapse/synapse/rest/key/v2/local_key_resource.py", line 70, in __init__
    self.update_response_body(self.clock.time_msec())
  File "/home/jaywink/workspace/synapse/synapse/rest/key/v2/local_key_resource.py", line 75, in update_response_body
    self.valid_until_ts = int(time_now_msec + refresh_interval)
TypeError: unsupported operand type(s) for +: 'int' and 'Mock'

Just noticed it's only a problem with SynapseHomeserverOpenIDListenerTests though, not the reader. If federation resource is active, key resource is added automatically, thus the mock.

[erikjohnston]

I'm not sure what this is testing? Can't we just test that the below attempt at requesting fails appropriately?

[jaywink]

So, I took how the tests were written pretty much from test_frontend_proxy, added here https://github.com/matrix-org/synapse/pull/3694/files#diff-20dd51850158a57f8dd29117bf46b484R50.

What I noticed is that if I don't call self.resource = site.resource.children[b"_matrix"].children[b"federation"].children[b"v1"] at all, then everything fails with a 400 error. But when there is no federation listener, calling this will fail due to KeyError. I tried using .get() but that didn't have the same effect. I'm not entirely sure how the test make_request works underneath. This was also the same reason I introduced parameterized because otherwise there would have been a lot of duplication of the test code when in fact we just want to run the same test with different parameters.

I'm not entirely sure how to test a failure since we can't call self.resource without that failing, and if it's not called, any request fails, so it's not really a successful test. Maybe @hawkowl has some details on that as the author the frontend proxy test, what do I need to do here to make make_request works without calling self.resource first?

[erikjohnston]

Oh, I hadn't realised we had done this somewhere else. Will poke @hawkowl to see if there's a better way of doing it, but if not I can live with the current stuff

[erikjohnston]

BTW, travis is failing on the isort tests, which is basically just checking the imports are correctly ordered/etc. You can easily fix it by running isort -rc synapse/ tests/.

[erikjohnston]

Oh, oops, I think all the test failures are my fault as I changed the listener to listen on /_matrix/federation/ rather than /_matrix/federation/v1, but hopefully an easy fix by just removing the .children["v1"] bit

[jaywink]

Rebased and fixed the tests.

fixed

[erikjohnston]

I'm going to merge this as the code looks correct and there are tests. I feel like there should be a neater way of doing the tests, but if they're using the same style as other unit tests its fine for now.