-
Notifications
You must be signed in to change notification settings - Fork 416
Allow subpaths in MAS endpoints #19186
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
Conversation
sandhose
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good overall, but maybe we should just roll back to the old way we did that instead?
synapse/api/auth/mas.py
Outdated
|
|
||
| @property | ||
| def _metadata_url(self) -> str: | ||
| return str( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we wouldn't be better off just doing what we were doing before? e.g.
@property
def _metadata_url(self) -> str:
return f"{str(self._config.endpoint).rstrip('/')}/.well-known/openid-configuration"
@property
def _introspection_endpoint(self) -> str:
return f"{str(self._config.endpoint).rstrip('/')}/oauth2/introspect"alternatively we could probably use urllib.parse.urljoin, but that might also have weird consequences depending on the trailing slash:
>>> import urllib.parse
>>> urllib.parse.urljoin("https://example.com", ".well-known/openid-configuration")
'https://example.com/.well-known/openid-configuration'
>>> urllib.parse.urljoin("https://example.com/foo", ".well-known/openid-configuration")
'https://example.com/.well-known/openid-configuration'
>>> urllib.parse.urljoin("https://example.com/foo/", ".well-known/openid-configuration")
'https://example.com/foo/.well-known/openid-configuration'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm yeah the old way seems a lot simpler.
Not sure why it was drastically changed in the first place.
I say we go back to the old way.
Fixes #19184 ### Pull Request Checklist <!-- Please read https://element-hq.github.io/synapse/latest/development/contributing_guide.html before submitting your pull request --> * [X] Pull request is based on the develop branch * [X] Pull request includes a [changelog file](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#changelog). The entry should: - Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from `EventStore` to `EventWorkerStore`.". - Use markdown where necessary, mostly for `code blocks`. - End with either a period (.) or an exclamation mark (!). - Start with a capital letter. - Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry. * [X] [Code style](https://element-hq.github.io/synapse/latest/code_style.html) is correct (run the [linters](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#run-the-linters))
Fixes a bug introduced in v1.142.0 preventing subpaths in MAS endpoints from working. Link: element-hq/synapse#19184 Link: element-hq/synapse#19186 Signed-off-by: Petr Vaněk <[email protected]>
Fixes #19184
Pull Request Checklist
EventStoretoEventWorkerStore.".code blocks.