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

Consolidate SSO redirects through /_matrix/client/v3/login/sso/redirect(/{idpId}) #17972

Merged
merged 11 commits into from
Nov 29, 2024

Conversation

MadLittleMods
Copy link
Contributor

@MadLittleMods MadLittleMods commented Nov 27, 2024

Consolidate SSO redirects through /_matrix/client/v3/login/sso/redirect(/{idpId})

Spawning from https://github.com/element-hq/sbg/pull/421#discussion_r1859497330 where we have a proxy that intercepts responses to /_matrix/client/v3/login/sso/redirect(/{idpId}) in order to upgrade them to use OAuth 2.0 Pushed Authorization Requests (PAR). Instead of needing to intercept multiple endpoints that redirect to the authorization endpoint, it seems better to just have Synapse consolidate to a single flow.

Testing strategy

  1. Create a new OAuth application. I'll be using GitHub for example but there are many options. Visit https://github.com/settings/developers -> New OAuth App
    • Application name: Synapse local testing
    • Homepage URL: http://localhost:8008
    • Authorization callback URL: http://localhost:8008/_synapse/client/oidc/callback
  2. Update your Synapse homeserver.yaml
    server_name: "my.synapse.server"
    public_baseurl: http://localhost:8008/
    listeners:
      - port: 8008
        bind_addresses: [
          #'::1',
          '127.0.0.1'
        ]
        tls: false
        type: http
        x_forwarded: true
        resources:
          - names: [client, federation, metrics]
            compress: false
    
    # SSO login testing
    oidc_providers:
      - idp_id: github
        idp_name: Github
        idp_brand: "github"  # optional: styling hint for clients
        discover: false
        issuer: "https://github.com/"
        client_id: "xxx" # TO BE FILLED
        client_secret: "xxx" # TO BE FILLED
        authorization_endpoint: "https://github.com/login/oauth/authorize"
        token_endpoint: "https://github.com/login/oauth/access_token"
        userinfo_endpoint: "https://api.github.com/user"
        scopes: ["read:user"]
        user_mapping_provider:
          config:
            subject_claim: "id"
            localpart_template: "{{ user.login }}"
            display_name_template: "{{ user.name }}"
  3. Start Synapse: poetry run synapse_homeserver --config-path homeserver.yaml
  4. Visit http://localhost:8008/_synapse/client/pick_idp?redirectUrl=http%3A%2F%2Fexample.com
  5. Choose GitHub
  6. Notice that you're redirected to GitHub to sign in (https://github.com/login/oauth/authorize?...)

Tested locally and works:

  1. http://localhost:8008/_synapse/client/pick_idp?idp=oidc-github&redirectUrl=http%3A//example.com ->
  2. http://localhost:8008/_matrix/client/v3/login/sso/redirect/oidc-github?redirectUrl=http://example.com ->
  3. https://github.com/login/oauth/authorize?response_type=code&client_id=xxx&redirect_uri=http%3A%2F%2Flocalhost%3A8008%2F_synapse%2Fclient%2Foidc%2Fcallback&scope=read%3Auser&state=xxx&nonce=xxx

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. 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.
  • Code style is correct
    (run the linters)

@MadLittleMods MadLittleMods marked this pull request as ready for review November 28, 2024 01:06
@MadLittleMods MadLittleMods requested a review from a team as a code owner November 28, 2024 01:06
Copy link
Member

@anoadragon453 anoadragon453 left a comment

Choose a reason for hiding this comment

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

Some tiny comments, but otherwise this LGTM!

synapse/api/urls.py Outdated Show resolved Hide resolved
synapse/rest/synapse/client/pick_idp.py Outdated Show resolved Hide resolved
tests/utils.py Outdated Show resolved Hide resolved
@MadLittleMods MadLittleMods merged commit 6a909aa into develop Nov 29, 2024
39 checks passed
@MadLittleMods MadLittleMods deleted the madlittlemods/unify-login-sso-redirect branch November 29, 2024 17:26
@MadLittleMods
Copy link
Contributor Author

Thanks for the review @anoadragon453 🐍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants