Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 14 commits into from Feb 11, 2019

Conversation

Projects
None yet
4 participants
@jaywink
Copy link
Member

jaywink commented Jan 20, 2019

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.

@jaywink jaywink requested a review from matrix-org/synapse-core Jan 20, 2019

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 20, 2019

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
Copy link
Member

erikjohnston left a comment

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

Show resolved Hide resolved changelog.d/4420.feature Outdated
Show resolved Hide resolved synapse/config/server.py Outdated
Show resolved Hide resolved synapse/federation/transport/server.py
Show resolved Hide resolved synapse/federation/transport/server.py
from tests.unittest import HomeserverTestCase


@patch("synapse.app.homeserver.KeyApiV2Resource", new=Mock())

This comment has been minimized.

Copy link
@erikjohnston

erikjohnston Jan 21, 2019

Member

Why is this necessary?

This comment has been minimized.

Copy link
@jaywink

jaywink Jan 22, 2019

Author Member

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.

This comment has been minimized.

Copy link
@jaywink

jaywink Jan 22, 2019

Author Member
  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.

except KeyError:
if expectation == "no_resource":
return
raise

This comment has been minimized.

Copy link
@erikjohnston

erikjohnston Jan 21, 2019

Member

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

This comment has been minimized.

Copy link
@jaywink

jaywink Jan 22, 2019

Author Member

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?

This comment has been minimized.

Copy link
@erikjohnston

erikjohnston Jan 22, 2019

Member

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

This comment has been minimized.

Copy link
Member

erikjohnston commented Jan 21, 2019

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

This comment has been minimized.

Copy link
Member

erikjohnston commented Jan 22, 2019

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 added some commits Jan 17, 2019

Fix a test docstring in frontend proxy tests
Signed-off-by: Jason Robinson <jasonr@matrix.org>
Add parameterized Python module to test dependencies
Allows running parameterized tests. BSD license.

Signed-off-by: Jason Robinson <jasonr@matrix.org>
Make FederationReaderServer _http_listen use self.get_reactor()
For all the homeserver classes, only the FrontendProxyServer passes
its reactor when doing the http listen. Looking at previous PR's looks
like this was introduced to make it possible to write a test, otherwise
when you try to run a test with the test homeserver it tries to
do a real bind to a port. Passing the reactor that the homeserver
is instantiated with should probably be the right thing to do anyway?

Signed-off-by: Jason Robinson <jasonr@matrix.org>
Add tests for the openid lister for FederationReaderServer
Check all possible variants of openid and federation listener on/off
possibilities.

Signed-off-by: Jason Robinson <jasonr@matrix.org>
Make SynapseHomeServer _http_listener use self.get_reactor()
For all the homeserver classes, only the FrontendProxyServer passes
its reactor when doing the http listen. Looking at previous PR's looks
like this was introduced to make it possible to write a test, otherwise
when you try to run a test with the test homeserver it tries to
do a real bind to a port. Passing the reactor that the homeserver
is instantiated with should probably be the right thing to do anyway?

Signed-off-by: Jason Robinson <jasonr@matrix.org>
Add tests for the openid lister for SynapseHomeServer
Check all possible variants of openid and federation listener on/off
possibilities.

Signed-off-by: Jason Robinson <jasonr@matrix.org>
Split federation OpenID userinfo endpoint out of the federation resource
This allows the OpenID userinfo endpoint to be active even if the
federation resource is not active. The OpenID userinfo endpoint
is called by integration managers to verify user actions using the
client API OpenID access token. Without this verification, the
integration manager cannot know that the access token is valid.

The OpenID userinfo endpoint will be loaded in the case that either
"federation" or "openid" resource is defined. The new "openid"
resource is defaulted to active in default configuration.

Signed-off-by: Jason Robinson <jasonr@matrix.org>
Add changelog for openid resource addition
Signed-off-by: Jason Robinson <jasonr@matrix.org>
Document `servlet_groups` parameters
Signed-off-by: Jason Robinson <jasonr@matrix.org>
Remove openid resource from default config
Instead document it commented out.

Signed-off-by: Jason Robinson <jasonr@matrix.org>
Collapse changelog to one line
Signed-off-by: Jason Robinson <jasonr@matrix.org>
Fix sorting of imports in tests. Remove an unnecessary mock
Signed-off-by: Jason Robinson <jasonr@matrix.org>
Fix openid tests after rebase
Signed-off-by: Jason Robinson <jasonr@matrix.org>

@jaywink jaywink force-pushed the jaywink/openid-listener branch from 882cf92 to 1838ef1 Jan 23, 2019

@jaywink

This comment has been minimized.

Copy link
Member Author

jaywink commented Jan 23, 2019

Rebased and fixed the tests.

Fix flake8 issues
Signed-off-by: Jason Robinson <jasonr@matrix.org>

@richvdh richvdh requested a review from matrix-org/synapse-core Feb 6, 2019

@erikjohnston

This comment has been minimized.

Copy link
Member

erikjohnston commented Feb 11, 2019

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.

@erikjohnston erikjohnston merged commit b201149 into develop Feb 11, 2019

5 checks passed

ci/circleci: sytestpy2merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy2postgresmerged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3postgresmerged Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

richvdh added a commit that referenced this pull request Feb 14, 2019

Merge tag 'v0.99.1'
Synapse 0.99.1 (2019-02-14)
===========================

Features
--------

- Include m.room.encryption on invites by default ([\#3902](#3902))
- Federation OpenID listener resource can now be activated even if federation is disabled ([\#4420](#4420))
- Synapse's ACME support will now correctly reprovision a certificate that approaches its expiry while Synapse is running. ([\#4522](#4522))
- Add ability to update backup versions ([\#4580](#4580))
- Allow the "unavailable" presence status for /sync.
  This change makes Synapse compliant with r0.4.0 of the Client-Server specification. ([\#4592](#4592))
- There is no longer any need to specify `no_tls`: it is inferred from the absence of TLS listeners ([\#4613](#4613), [\#4615](#4615), [\#4617](#4617), [\#4636](#4636))
- The default configuration no longer requires TLS certificates. ([\#4614](#4614))

Bugfixes
--------

- Copy over room federation ability on room upgrade. ([\#4530](#4530))
- Fix noisy "twisted.internet.task.TaskStopped" errors in logs ([\#4546](#4546))
- Synapse is now tolerant of the `tls_fingerprints` option being None or not specified. ([\#4589](#4589))
- Fix 'no unique or exclusion constraint' error ([\#4591](#4591))
- Transfer Server ACLs on room upgrade. ([\#4608](#4608))
- Fix failure to start when not TLS certificate was given even if TLS was disabled. ([\#4618](#4618))
- Fix self-signed cert notice from generate-config. ([\#4625](#4625))
- Fix performance of `user_ips` table deduplication background update ([\#4626](#4626), [\#4627](#4627))

Internal Changes
----------------

- Change the user directory state query to use a filtered call to the db instead of a generic one. ([\#4462](#4462))
- Reject federation transactions if they include more than 50 PDUs or 100 EDUs. ([\#4513](#4513))
- Reduce duplication of ``synapse.app`` code. ([\#4567](#4567))
- Fix docker upload job to push -py2 images. ([\#4576](#4576))
- Add port configuration information to ACME instructions. ([\#4578](#4578))
- Update MSC1711 FAQ to calrify .well-known usage ([\#4584](#4584))
- Clean up default listener configuration ([\#4586](#4586))
- Clarifications for reverse proxy docs ([\#4607](#4607))
- Move ClientTLSOptionsFactory init out of `refresh_certificates` ([\#4611](#4611))
- Fail cleanly if listener config lacks a 'port' ([\#4616](#4616))
- Remove redundant entries from docker config ([\#4619](#4619))
- README updates ([\#4621](#4621))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.