matrix-org / synapse

Synapse: Matrix homeserver written in Python/Twisted.
https://matrix-org.github.io/synapse
Apache License 2.0
11.78k stars 2.13k forks source link

social login accepts a numeric user ID, then bombs out later with an ugly error #9545

Open richvdh opened 3 years ago

richvdh commented 3 years ago

image

richvdh commented 3 years ago

hopefully this should be an easy fix

clokep commented 3 years ago

7795 is vaguely related to this. I think I had suggested to the EMS team that they do something like "u{{ user.id }}" in their OIDC config?

pmontepagano commented 3 years ago

I'd like to tackle this issue, but before I make a PR, I'd like to discuss options. Here are a few that come to mind:

  1. Exposing an option in homeserver.yml that allows you to set a minimum integer for guest account IDs. That way, if SSO has numeric IDs bellow that minimum, there wouldn't be any collisions between guest accounts and real accounts. That minimum number would be used to initialize the sequence user_id_seq in PostgreSQL, although we would also need to change find_max_generated_user_id_localpart for SQLite.

  2. Another alternative is to simply expose in homeserver.yml an option that allows non-guest users to have a numeric user ID and hope that collistions don't occur.

  3. Another alternative would be to prepend some arbitrary string to guest accounts, so that they are generated like guest-1234

What do you think? I'm guessing that option 1 is the most appropiate... If I implement this in the next few days, do you have time to review the PR?

Thanks in advance

richvdh commented 3 years ago

sorry about the slow reply.

I think all we need to do to fix this is to update the pick_username resources (served at /_synapse/client/pick_username) to reject numeric user-ids?

pmontepagano commented 3 years ago

hmmm, but I do need numeric user IDs, because those are the usernames in my IdP.

Is there a reason in the spec for using numeric IDs for guest accounts? O is it an implementation decision of synapse?

richvdh commented 3 years ago

hmmm, but I do need numeric user IDs, because those are the usernames in my IdP.

well, this bug is about the flow where you let the users choose their own user ID.

(If you're assigning user IDs rather than letting the users choose their own, why not use localpart_template in the oidc config to specify a mapping to something like "u{{ user.id }}" ?)

InoryS commented 1 year ago

hmmm, but I do need numeric user IDs, because those are the usernames in my IdP.

well, this bug is about the flow where you let the users choose their own user ID.

(If you're assigning user IDs rather than letting the users choose their own, why not use localpart_template in the oidc config to specify a mapping to something like "u{{ user.id }}" ?)

Hi, I'm using Mastodon as a provider with the following configuration:

userinfo_endpoint: "https://example.com/api/v1/accounts/verify_credentials"
     user_mapping_provider:
       config:
         subject_claim: "id"
         localpart_template: "{{ user. username }}"
         display_name_template: "{{ user.display_name }}"

There are users with purely numeric IDs, and I don't want to ask them to change, so I want to prepend the ID with a "u" or something.

I tried:

subject_claim: "u {{ id }}"
subject_claim: "u{{ id }}"
subject_claim: "{{ id }}u"
subject_claim: u+"id"
subject_claim: "u+id"
subject_claim: "str(id)"

None of them work, can you tell me how to do that? Thanks.

verify_credentials doc here: https://docs.joinmastodon.org/methods/accounts/#verify_credentials

clokep commented 1 year ago

You need to update the localpart_template with it: localpart_template: "u{{ user.username }}"

InoryS commented 1 year ago

You need to update the with it: localpart_template``localpart_template: "u{{ user.username }}"

Oh, thanks I thouth it is id's problem.