Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

Fix and refactor rewritten IS url feature. Add sample config docs #40

Merged
merged 5 commits into from
Apr 22, 2020

Conversation

anoadragon453
Copy link
Member

@anoadragon453 anoadragon453 commented Apr 21, 2020

This PR:

  • Changes dinsic config option rewrite_identity_server_urls to include protocol schemes (ex. https://)
    • @michaelkaye seems ok with this
    • tests concerning this are updated
  • Renames every instance of a variable containing an identity server without a protocol scheme to id_server, and likewise with a protocol scheme to url
  • Adds documentation about rewrite_identity_server_urls to the sample config file
  • Consolidates all rewriting and "https://"-prepending into a single helper function

URLs/domain names of identity servers can either come from:

  • Clients sending id_server params (no protocol scheme)
  • account_threepid_delegates (includes scheme)

For the former, we use the add_https parameter to turn them into URLs.

I'm merging this to the release branch instead of dinsic as it concerns account_threepid_delegates stuff which is coming from mainline.

CI is failing as the various linting tools have not been run on this branch yet. Unfortunately we can't get to the rest of the tests without that though.

Not sure about adding a newspaper file...

@anoadragon453 anoadragon453 requested review from michaelkaye and a team April 21, 2020 20:23
@michaelkaye
Copy link
Contributor

Does this mean rewrite_identity_server_urls should be a map of url prefix -> url prefix (no trailing /, i think) rather than host->host as it is currently? This is fine; just need to make sure we update the configuration appropriately.

@anoadragon453
Copy link
Member Author

Does this mean rewrite_identity_server_urls should be a map of url prefix -> url prefix (no trailing /, i think) rather than host->host as it is currently? This is fine; just need to make sure we update the configuration appropriately.

Yes, exactly. No trailing slash.

Copy link
Contributor

@babolivier babolivier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm once these points are addressed. You also probably want to regenerate the sample config for consistency.

synapse/config/registration.py Outdated Show resolved Hide resolved
synapse/config/registration.py Outdated Show resolved Hide resolved
synapse/handlers/identity.py Show resolved Hide resolved
@anoadragon453 anoadragon453 merged commit e66bbf7 into dinsic-release-v1.12.x Apr 22, 2020
@anoadragon453 anoadragon453 deleted the anoa/simplify_is_url_rewriting branch April 22, 2020 16:59
anoadragon453 added a commit that referenced this pull request Apr 23, 2020
…e-dinsic into dinsic-release-v1.12.x

* 'dinsic-release-v1.12.x' of github.com:matrix-org/synapse-dinsic:
  Fix and refactor rewritten IS url feature. Add sample config docs (#40)
anoadragon453 added a commit that referenced this pull request Apr 24, 2020
…e-dinsic into dinsic-release-v1.12.x

* 'dinsic-release-v1.12.x' of github.com:matrix-org/synapse-dinsic:
  Fix and refactor rewritten IS url feature. Add sample config docs (#40)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants