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

OIDC/OAuth client and Nextcloud backend #8511

Closed
EdGeraghty opened this issue Oct 9, 2020 · 22 comments · Fixed by #14753
Closed

OIDC/OAuth client and Nextcloud backend #8511

EdGeraghty opened this issue Oct 9, 2020 · 22 comments · Fixed by #14753
Assignees
Labels
A-SSO Single Sign-On (maybe OIDC) T-Enhancement New features, changes in functionality, improvements in performance, or user-facing enhancements. Z-Help-Wanted We know exactly how to fix this issue, and would be grateful for any contribution z-p3 (Deprecated Label)

Comments

@EdGeraghty
Copy link

Description:
When using Nextcloud as an external identity provider, its JSON response from the userinfo endpoint is as follows:

{
	"ocs":{
		"meta":{
			"status":"ok",
			"statuscode":200,
			"message":"OK"
		},
		"data":{
			"enabled":true,
			"storageLocation":"\\/var\\/www\\/nextcloud\\/data\\/[email protected]",
			"id":"[email protected]",
			"lastLogin":1602147919000,
			"backend":"Database",
			"subadmin":[],
			"quota":{"free":78983929856,"used":17659078,"total":79001588934,"relative":0.02,"quota":-3},
			"email":null,
			"phone":"",
			"address":"",
			"website":"",
			"twitter":"",
			"groups":["admin"],
			"language":"en",
			"locale":"",
			"backendCapabilities":{"setDisplayName":true,"setPassword":true},
			"display-name":"[email protected]"
		}
	}
}

Which obviously isn't mapped properly.

I've manually hacked the OIDC mapping provider so it brings in the id, but it'd be great if someone who knew what they were doing on the Synapse side could make this connection work properly!

@clokep
Copy link
Member

clokep commented Oct 9, 2020

I've manually hacked the OIDC mapping provider so it brings in the id

What were the changes you had to make? This is one of the reasons that the mapping provider is pluggable, but if this is a standard setup we should potentially support it.

@clokep clokep added Z-Help-Wanted We know exactly how to fix this issue, and would be grateful for any contribution z-p3 (Deprecated Label) A-SSO Single Sign-On (maybe OIDC) labels Oct 9, 2020
@EdGeraghty
Copy link
Author

EdGeraghty commented Oct 9, 2020

return userinfo[self._config.subject_claim]

<         return userinfo[self._config.subject_claim]
---
>         return userinfo['ocs']['data']['id']

localpart = self._config.localpart_template.render(user=userinfo).strip()

<         localpart = self._config.localpart_template.render(user=userinfo).strip()
---
>         localpart = userinfo['ocs']['data']['id']

I've no doubt I'm doing it wrong, but I tried all sorts of combinations for subject_claim in homeserver.yaml first:

homeserver.yaml:

oidc_config:
    enabled: true
    discover: false
    issuer: "https://<>/"
    client_id: "<>"
    client_secret: "<>"
    authorization_endpoint: "https://<>/apps/oauth2/authorize"
    token_endpoint: "https://<>/apps/oauth2/api/v1/token"
    userinfo_endpoint: "https://<>/ocs/v2.php/cloud/user?format=json"
    skip_verification: false
    scopes: ["nc doesn't implement"]
    user_mapping_provider:
      config:
        subject_claim: "ocs, data, display-name"
        localpart_template: "{{ user.login }}"
        display_name_template: "{{ user.name }}"

@clokep
Copy link
Member

clokep commented Oct 9, 2020

It seems like their response includes stuff that is normally only sent in the HTTP header and the "real" data is under the data key. This seems to match their API docs, but only for that endpoint weirdly.

I checked around briefly but don't see anything about compatibility issues between Nextcloud and authlib (which is the underlying Python library we use for a lot of the OpenID code), so it might just be specific to how we're using the userinfo.

It could be reasonable to let the subject_claim take a dotted path?

I think you can set localpart_template: "{{ ocs.data.id }}", but I'm not 100% sure about that.

Alternately you can provide a completely separate mapping provider (something like) which knows how to deal with this nesting. See the oidc_config.user_mapping_provider.module setting in your config, this allows you to point it to any python object.

@EdGeraghty
Copy link
Author

It could be reasonable to let the subject_claim take a dotted path?

That would be ace, and the "logical" approach for me - I'd tried that approach initially (having read that it was just using it as an array key), but it was just throwing me errors, so I hardcoded to prove to myself I wasn't going mad!

I think you can set localpart_template: "{{ ocs.data.id }}", but I'm not 100% sure about that.

Sadly not, I tried that too

Oops! Something went wrong during authentication.

Try logging in again from your Matrix client and if the problem persists please contact the server's administrator.

Error: mapping_error

Could not extract user attributes from OIDC response: 'ocs' is undefined

Alternately you can provide a completely separate mapping provider (something like) which knows how to deal with this nesting. See the oidc_config.user_mapping_provider.module setting in your config, this allows you to point it to any python object.

I've not written any python in a decade - can you point me to something to get me started, if I end up having to do this way?

@clokep
Copy link
Member

clokep commented Oct 9, 2020

Alternately you can provide a completely separate mapping provider (something like) which knows how to deal with this nesting. See the oidc_config.user_mapping_provider.module setting in your config, this allows you to point it to any python object.

I've not written any python in a decade - can you point me to something to get me started, if I end up having to do this way?

There's some (light) documentation, and then taking a look at the current implementation is probably useful:

https://github.com/matrix-org/synapse/blob/v1.20.1/synapse/handlers/oidc_handler.py#L1002-L1061

If you don't plain to make it flexible a lot of that code could go and you can essentially take your patches from above and just plop them in.

@EdGeraghty
Copy link
Author

Thanks for your help, @clokep

Given the response from that endpoint isn't going to be changing any time soon, I'll write a mapper. Is it likely a pull request would be merged upstream? I may as well share the result if I'm going to the effort of dusting off my python to build it 😊

@clokep
Copy link
Member

clokep commented Oct 9, 2020

Thanks for your help, @clokep

No problem!

Given the response from that endpoint isn't going to be changing any time soon, I'll write a mapper. Is it likely a pull request would be merged upstream? I may as well share the result if I'm going to the effort of dusting off my python to build it 😊

I suspect that in core it might make more sense to add a configuration option which (given a response) lets you pull out the user info from it. I'm envisioning something like: path_to_userinfo: "ocs.data", while the default would be "empty" (meaning the userinfo is the root object returned)? This could be done as part of the mapping provider or outside of it, I haven't fully thought through pros/cons of that one. The change would probably be somewhere around

resp = await self._http_client.get_json(
metadata["userinfo_endpoint"],
headers={"Authorization": ["Bearer {}".format(token["access_token"])]},
)
return UserInfo(resp)
?

Feel free to join #synapse-dev:matrix.org if you have any questions!

@EdGeraghty
Copy link
Author

Thanks.

Obviously I've no real knowledge of the internals of the project (other than the FAQs, the sledgehammer hack-job I did of reverse-engineering the oidc mapper, & the bits of the API docs I've skimmed today) but I can't see a con in enabling the ability to change the root of the returned JSON to the bit which actually follows standards, other than it stops requiring e.g. Nextcloud from properly implementing the spec (and it doesn't look like they'll change their minds any time soon).

The pro is the ability for SSO from a Nextcloud/Owncloud/any other non-compliant backend 😊

Our use case is Element Web in Nextcloud doing SSO to anyone who can log into the instance.

@EdGeraghty
Copy link
Author

Looks like authlib already supports this - could you expose it somehow?

https://docs.authlib.org/en/latest/client/oauth2.html#compliance-fix-oauth2

@clokep
Copy link
Member

clokep commented Oct 13, 2020

Looks like authlib already supports this - could you expose it somehow?

docs.authlib.org/en/latest/client/oauth2.html#compliance-fix-oauth2

Good find! 👍 That's probably a more flexible way of handling this, although it doesn't seem that we actually use OAuth2Session (this is because Synapse uses Twisted to do the I/O here). I think adding similar hooks would be a reasonable way to add this support. To do that I would:

  • Add config to support optional functions (via it's Python module path) for each of these parameters
  • Read the config (in synapse.config.oidc_config) and ensure the function is importable (I think we have helpers to do this already, it would be similar to how oidc_config.user_mapping_provider.module is handled).
  • Optionally, add those hooks from synapse.handlers.oidc_handler if they're configured.

The response function would be similar, but not identical since it would be receiving a Twisted response. Overall it should be pretty similar though.

@Ramblurr
Copy link

Ramblurr commented Nov 2, 2020

Here is a simple Nextcloud OIDC mapping provider that is working on my nextcloud + synapse setup:

from synapse.handlers.oidc import OidcMappingProvider
from synapse.types import map_username_to_mxid_localpart

class NextcloudOidcMappingProvider(OidcMappingProvider):
    def __init__(self, config):
        self._config = config

    @staticmethod
    def parse_config(config):
        return {}

    def get_remote_user_id(self, userinfo):
        return userinfo["ocs"]["data"]["id"]

    async def map_user_attributes(self, userinfo, token):
        localpart = map_username_to_mxid_localpart(userinfo["ocs"]["data"]["id"])
        display_name = userinfo["ocs"]["data"]["display-name"]
        return {"localpart": localpart, "display_name": display_name}

    async def get_extra_attributes(self, userinfo, token):
        extras = {}
        return extras

I just dropped it into the site-packages dir on my python path, and use it with a config like so:

   password_config:
     enabled: false 
   sso:
     client_whitelist:
         - https://element.example.com/
         - https://app.element.io/
   oidc_config:
     enabled: true
     client_id: "changeme"
     client_secret: "changeme"
     scopes: ["profile", "email"]
     issuer: "https://example.com"
     discover: false
     authorization_endpoint: "https://example.com/index.php/apps/oauth2/authorize"
     userinfo_endpoint: "https://example.com/ocs/v2.php/cloud/user?format=json"
     token_endpoint: "https://example.com/index.php/apps/oauth2/api/v1/token"
     user_mapping_provider:
       module: nextcloud_oicd_mapping_provider.NextcloudOidcMappingProvider
       config: {}

@skylarmt
Copy link

I've implemented @Ramblurr's solution and it almost works perfectly, except after logging in with Nextcloud and granting Synapse permission, I get redirected to a Synapse-generated error page saying the session cookie wasn't found:
Authentication failed

I haven't found a single other thing about this error online except for the source code that generates it, which isn't too helpful really. Firefox's developer tools show the request that returns the error message is sending of cookies: nc_username, nc_token, nc_session_id, and oc_sessionPassphrase.

I even have Nextcloud and Synapse using the same subdomain, with Matrix behind an Apache proxy, so it's not some browser security problem.

@ACiDGRiM
Copy link

ACiDGRiM commented Apr 9, 2022

I was able to sucessfully log some next cloud users in using the provider submitted by Ramblurr, however I'm not able to re-login to the same account from another device. I receive the below error:

Mapping provider does not support de-duplicating Matrix IDs

I'm able to sign in using a password after I created my user on a moblie device and set a password, but this defeats the value of using a central identity provider.

@EdGeraghty
Copy link
Author

Add allow_existing_users: true to your oidc_config block

@ACiDGRiM
Copy link

ACiDGRiM commented Apr 11, 2022

Does it matter that I'm using a oidc_providers config list instead of the legacy oidc_config block? This still results in the de-duplicaiton error.

According to this doc, the de-duplication only applies to the situation where two providers return the same username assuming two different people are registering (i.e. John Doe: jdoe and Jerry Doe: jdoe registers Jerry Doe as jdoe1) https://matrix-org.github.io/synapse/develop/sso_mapping_providers.html

So my issue is that even though my users are all registered with idp-nextcloud in the user_external_ids table, but is being detected as a new signup.

2022-04-11 06:56:23,972 - synapse.handlers.oidc - 231 - INFO - GET-6 - Received OIDC callback for IdP oidc-nextcloud
2022-04-11 06:56:24,330 - synapse.http.client - 446 - INFO - GET-6 - Received response to POST https://cloud.xxx.net/apps/oauth2/api/v1/token: 200
2022-04-11 06:56:24,530 - synapse.http.client - 446 - INFO - GET-6 - Received response to GET https://cloud.xxx.net/ocs/v2.php/cloud/user?format=json: 200
2022-04-11 06:56:24,533 - synapse.handlers.sso - 349 - INFO - GET-6 - Found existing mapping for IdP 'oidc-nextcloud' and remote_user_id 'name': @name:xxx.net
2022-04-11 06:56:24,533 - synapse.handlers.oidc - 901 - ERROR - GET-6 - Could not map user

I also added this to the Nextcloud OIDC mapping provider, but doesn't improve it either

async def map_user_attributes(self, userinfo, token):
        localpart = map_username_to_mxid_localpart(userinfo["ocs"]["data"]["id"])
        display_name = userinfo["ocs"]["data"]["display-name"]
++        emails = userinfo["ocs"]["data"]["email"]
        return {"localpart": localpart, "display_name": display_name, ++"emails": emails}

remove ++, obviously

oidc_providers:
  # Generic example
  #
   - idp_id: nextcloud
     idp_name: "Nextcloud"
   #  idp_icon: "mxc://example.com/mediaid"
     discover: false
     issuer: "https://cloud.xxx.net"
     client_id:  "asdfasdf"
     client_secret: "asdfasdf"
  # client_auth_method: client_secret_post
     scopes: ["na"]
     authorization_endpoint: "https://cloud.xxx.net/apps/oauth2/authorize"
     token_endpoint: "https://cloud.xxx.net/apps/oauth2/api/v1/token"
     userinfo_endpoint: "https://cloud.xxx.net/ocs/v2.php/cloud/user?format=json"
     user_profile_method: "userinfo_endpoint"
  #  jwks_uri: "https://accounts.example.com/.well-known/jwks.json"
     allow_existing_users: true
     user_mapping_provider:
        module: nextcloud_oidc_mapping_provider.NextcloudOidcMappingProvider
        config: {}

@squahtx
Copy link
Contributor

squahtx commented Apr 11, 2022

Looks like a bug introduced in #10108. Filed as #12432.

https://github.com/matrix-org/synapse/pull/10108/files#diff-05be763854c3b4a72fd8c85ffb5a6b3a10da786f46c53ca4b64d1f150d8fc992R469-R470
sso_to_matrix_id_mapper(failures) is oidc_response_to_user_attributes(failures), which calls map_user_attributes.
_call_attribute_mapper will retry calls to sso_to_matrix_id_mapper until it generates an unused mxid.

Because map_user_attributes() doesn't have a failures parameter Synapse gives up and raises a MappingError instead.

As a workaround, you can try disabling sso.update_profile_information or adding a failures parameter to map_user_attributes.

@ACiDGRiM
Copy link

Thanks, turning this off does address the issue.

@clokep
Copy link
Member

clokep commented Dec 29, 2022

#14753 should add support for this.

@clokep
Copy link
Member

clokep commented Jan 4, 2023

Starting in Synapse v1.75.0 you should be able to do something like:

oidc_config:
    enabled: true
    discover: false
    issuer: "https://<>/"
    client_id: "<>"
    client_secret: "<>"
    authorization_endpoint: "https://<>/apps/oauth2/authorize"
    token_endpoint: "https://<>/apps/oauth2/api/v1/token"
    userinfo_endpoint: "https://<>/ocs/v2.php/cloud/user?format=json"
    skip_verification: false
    scopes: ["nc doesn't implement"]
    user_mapping_provider:
      config:
        subject_claim: "{{ user.ocs.data.id }}"
        localpart_template: "{{ user.login }}"
        display_name_template: "{{ user.name }}"

(Note that I used id not display-name since you really want to use something that's immutable here.)

@EdGeraghty
Copy link
Author

I'm so pleased to see this get merged upstream 🎉

@gymnae
Copy link

gymnae commented Apr 28, 2023

Starting in Synapse v1.75.0 you should be able to do something like:

oidc_config:
    enabled: true
    discover: false
    issuer: "https://<>/"
    client_id: "<>"
    client_secret: "<>"
    authorization_endpoint: "https://<>/apps/oauth2/authorize"
    token_endpoint: "https://<>/apps/oauth2/api/v1/token"
    userinfo_endpoint: "https://<>/ocs/v2.php/cloud/user?format=json"
    skip_verification: false
    scopes: ["nc doesn't implement"]
    user_mapping_provider:
      config:
        subject_claim: "{{ user.ocs.data.id }}"
        localpart_template: "{{ user.login }}"
        display_name_template: "{{ user.name }}"

(Note that I used id not display-name since you really want to use something that's immutable here.)

Nice, just needed to adjust it slightly for me to work:


  - idp_id: nextcloud
    idp_name: "Nextcloud OpenID"
    enabled: true
    discover: false
    enable_registration: true
    issuer: "https://<>/"
    client_id: "<>"
    client_secret: "<>"
    allow_existing_users: true
    authorization_endpoint: "https://<>/apps/oauth2/authorize"
    token_endpoint: "https://<>/apps/oauth2/api/v1/token"
    userinfo_endpoint: "https://<>/ocs/v2.php/cloud/user?format=json"
    skip_verification: false
    scopes: ["nc doesn't implement"]
    user_mapping_provider:
      config:
        subject_template: "{{ user.ocs.data.id }}"
        localpart_template: "{{ user.ocs.data.id }}"
        display_name_template: "{{ user.name }}"

@tpokorra
Copy link

For me, user.ocs.data.id does not work.

I use user.preferred_username. See your options at https://mycloud.example.org/index.php/.well-known/openid-configuration

            user_mapping_provider:
              config:
                subject_template: "{{ user.preferred_username }}"
                localpart_template: "{{ user.preferred_username }}"
                display_name_template: "{{ user.name }}"
                email_template: "{{ user.email }}"

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-SSO Single Sign-On (maybe OIDC) T-Enhancement New features, changes in functionality, improvements in performance, or user-facing enhancements. Z-Help-Wanted We know exactly how to fix this issue, and would be grateful for any contribution z-p3 (Deprecated Label)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants