Add config option for always using userinfo endpoint

[BBBSnowball]

This fixes issue #7632.

Signed-off-by: Benjamin Koch [email protected]

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file.
• Pull request includes a sign off
• Code style is correct (run the linters)

[BBBSnowball]

Why does this fail for "POST /createRoom makes a private room"? I haven't changed anything related to rooms and the new option should be inactive by default. How do I debug this?

Contributor guide says not to rebase. What about rebase to current head of develop branch?

[clokep]

@BBBSnowball I kicked that build again, it might be a flakey test.

[richvdh]

I note that if I set the new option to true, but forget to set userinfo_endpoint, then I'll get the following error:

provider has no "userinfo_endpoint", even though it is required because the "openid" scope is not requested

that would be quite confiusing if the openid scope was in fact requested.

As an alternative approach: rather than adding a new option, how about if we made it so that, whenever userinfo_endpoint was set, we used it?

[richvdh]

@sandhose you might be interested in this.

[richvdh]

Contributor guide says not to rebase. What about rebase to current head of develop branch?

please don't rebase at all once someone has done a review.

[BBBSnowball]

that would be quite confiusing if the openid scope was in fact requested.

Indeed. This would be an unusual config but we shouldn't have misleading error messages, of course.

As an alternative approach: rather than adding a new option, how about if we made it so that, whenever userinfo_endpoint was set, we used it?

I'm not completely sure what you are suggesting:

1. The user should set userinfo_endpoint: https://accounts.example.com/gitlab/userinfo although Synapse would be able to discover the endpoint.
2. The user should use some magic value, e.g. userinfo_endpoint: default.
3. We always use the endpoint if it is set; even if it is auto-discovered.

Option 3 will make it work without any additional settings for Gitlab. However, any of those options might change the behavior for existing installations. In particular, this will break for users who have a broken userinfo endpoint in their config. Is this ok? How would we test this?

[richvdh]

oh, I'd forgotten about autodiscovery. hrm. let me think about it a bit more.

[sandhose]

This change makes sense. I thought of implementing that kind of flag when I did the OIDC PR.

IMHO what would make sense is:

• stop relying on the openid scope to "auto-detect" if the userinfo endpoint is needed or not. This seems to be kinda dependant on the IdP, and some IdP requires the openid scope anyway to access the userinfo endpoint
• have a config flag called something like "user profile method", with
  • userinfo_endpoint, to retrieve from the userinfo_endpoint (which is either auto-discovered or in the config)
  • id_token, to parse the id_token from the token response
  • auto, the default, which would use the id_token if present in the token response, and fallback to the userinfo_endpoint.

Also this PR lacks of unit tests for that feature

[richvdh]

My concern with the existing implementation is that it's quite confusing to understand the behaviour: "userinfo will be extracted from the userinfo_endpoint either when the openid scope is not requested, or when uses_userinfo is true". So how am I supposed to configure it?

I like @sandhose 's suggestion as it seems to automate the functionality as far as possible. (Although I'm wondering if user profile method really needs to be configurable, or if we can can just hardcode the auto behaviour?)

[BBBSnowball]

auto, the default, which would use the id_token if present in the token response, and fallback to the userinfo_endpoint.

With Gitlab, some properties are available without requesting userinfo (e.g. sub and sub_legacy), while others are only available via userinfo (e.g. nickname). I think this means that "auto" wouldn't work for Gitlab, right?

Do you think "auto" will work for other IdP that use the userinfo endpoint, e.g. GitHub?

I'm a bit hesitant with regard to incompatible changes to oidc_config. If Synapse stops using the userinfo endpoint in existing setups, the user mapping will be wrong. This will go unnoticed until a new user logs in and hits one of the error cases (e.g. localpart is empty or clashes with an existing user).

[BBBSnowball]

The current code should implement sandhose's suggestion. It is missing documentation and example config.

The tests for oidc pass. I haven't done any tests with an actual identity provider, yet.

For user_provider_method=="auto", we need the token in _uses_userinfo. I have added it as a parameter but we also have to pass it through load_jwks. Furthermore, _validate_metadata also needs to know whether we will use the userinfo_endpoint. We don't always have a token when we call it. Therefore, the check has become a no-op for the "auto" case and I have copied the check to _fetch_userinfo so we still check it (albeit at a later stage of the auth process). This is quite a mess. I need to think about it some more. Do you have any suggestions?

[BBBSnowball]

With Gitlab:

• auto does not work: Error: mapping_error, localpart is empty
• If I add user_profile_method: userinfo_endpoint to the config, it does work.

[BBBSnowball]

@BBBSnowball Are you available to continue this PR?

This took more time than anticipated and I have to prioritize other projects, at the moment. However, I'm going to continue this as soon as I find some time to do so. If someone else picks it up before then, that's fine with me.

[clokep]

I've pushed some changes which I hope are what were described above but please let me know if I've gone horribly wrong.

• Merged in develop since this has been stale for a while.
• Revert most of the changes and make "auto" behave as the previous behavior.
• Adds documentation and tests.

[clokep]

I managed to test this finally and was able to log into a test Synapse using the given config. I think this is ready for review.

requested changes have been made

[anoadragon453]

Some minor nits, but functionally it looks good. Glad to see test updates and hear that it works in manual testing :)

stale

[clokep]

@anoadragon453 This should be ready for another look. 👍