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

SAML2 Improvements and redirect stuff #5316

Closed
wants to merge 1 commit into from
Closed

SAML2 Improvements and redirect stuff #5316

wants to merge 1 commit into from

Conversation

galexrt
Copy link
Contributor

@galexrt galexrt commented Jun 2, 2019

This is a "draft" right now and doesn't really comply with the guidelines of this repository / project (will work on the PR changelog file when I refined my changes). I just want to put out my changes to get SAML2 SSO working for me, so that other people can enjoy SAML2 with Matrix.

There are still things to fix:

  • Make SAML2 Client global in some way to prevent the "unsolicited" SAML2 requests / response error(s).
  • Improve logging for SAML2 for better debugging issues.
  • Add some documentation for using SAML2. I have it running with Keycloak working fine after a lot of trial and error.

NOTE This basically takes code from older PRs #200 and #4267 to get it working.

Please let me know what would needs to be improved to get this PR ready for review and / or merge.
Thanks!

Could potentially close #5130 when a SSO logout endpoint has been proposed and implemented?

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file
  • Pull request includes a sign off

@richvdh
Copy link
Member

richvdh commented Jun 10, 2019

This is awesome. Thank you so much for doing and sharing this work. It turns out that I need to get this going in a hurry, so I'm going to make a branch based on it to fix up the global SAML2Client and logging issues you've already identified.

@@ -75,6 +75,7 @@ def default_config(self, config_dir_path, server_name, **kwargs):
# override them.
#
#saml2_config:
# enabled: true
Copy link
Member

Choose a reason for hiding this comment

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

tbh I think this is redundant. The default is for 'enabled' to be considered True: the only reason that the config parser checks it is to avoid breaking people who have enabled: false in their config files.

@@ -727,6 +727,9 @@ def validate_login(self, username, login_submission):
if canonical_user_id:
defer.returnValue((canonical_user_id, None))

if login_type == LoginType.SSO:
Copy link
Member

Choose a reason for hiding this comment

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

can you share the thinking behind this change? it looks like it's turning a 400 into a 403, but which endpoint is this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I remember correctly, because of this

canonical_user_id, callback = yield self.auth_handler.validate_login(
identifier["user"],
login_submission,
)

Due to no if condition being met it would fail because SSO is not a valid login type.

@@ -56,6 +56,7 @@ var show_login = function() {
}

if (matrixLogin.serverAcceptsSso) {
$("#sso_form").attr("action", "/_matrix/client/r0/login/sso/redirect");
Copy link
Member

Choose a reason for hiding this comment

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

isn't this the default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In which point the default? This change was needed as otherwise no SSO redirect would have happened.

Copy link
Member

Choose a reason for hiding this comment

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

The default action for #sso_form is /_matrix/client/r0/login/sso/redirect: https://github.com/matrix-org/synapse/blob/master/synapse/static/client/login/index.html#L22

@richvdh
Copy link
Member

richvdh commented Jun 10, 2019

Ok I have taken this work and based a separate PR on it over at #5422. If you'd like to keep working on it, I suggest you start by merging my branch. Otherwise we'll probably land #5422 in the next few days.

@galexrt
Copy link
Contributor Author

galexrt commented Jun 11, 2019

@richvdh Thanks for the feedback! I'm fine with you picking up the PR in #5422. 🙂

I'm going ahead and closing this in favor of #5422.

@galexrt galexrt closed this Jun 11, 2019
@localguru
Copy link
Contributor

@galexrt nice, thank you. I will test and give feedback!

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