MSC3356: Add additional OpenID user info fields #3356

jkanefendt · 2021-08-24T07:02:02Z

Signed-off-by: Johannes Kanefendt johannes.kanefendt@krzn.de

Synapse implementation: matrix-org/synapse#10384

anoadragon453

Indeed, currently applications intending to gather information from the room after querying the OpenID token need to use homeserver-specific query methods.

One could just use the C-S API with a user token to get information about the user's profile, but I suppose it's convenient to include it here as well.

proposals/3356-add-openid-userinfo-fields.md

kevincox · 2021-09-22T22:42:08Z

proposals/3356-add-openid-userinfo-fields.md

+Content-Type: application/json
+
+{ "userinfo_fields": [ "display_name", "room_powerlevels" ] }
+```


IIUC this information is normative and needs to be in the design not the security considerations section. For example the security considerations can just express that users will likely share more than they strictly need to and that this API doesn't provide fine-grained requests so the user has to pick between over-sharing and under-sharing.

Security considerations are part of the proposal design fwiw.

+1, please can this be moved to the main proposal section.

Security considerations are part of the proposal design fwiw.

This is true, but I'd still like to see these changes called out as part of the core proposal. It's rather misleading to discover that only half the proposed changes are in that section.

proposals/3356-add-openid-userinfo-fields.md

turt2live

Overall the proposal seems to work well, particularly with the added security considerations section (this wasn't there when I last read it).

We're also working on OAuth integration into Matrix concurrent to this, which might ultimately replace this sort of system. If a user can stamp an access token with just enough scope to see power levels + membership, then the downstream consumers won't have to do an awkward federation dance to get user information. Though, this remains purely scifi until we can test/prove otherwise :)

On the MSC process side of things, there's a few small things that should be addressed:

Line wrap to about 100 characters for legibility
The PR should be signed off for when we get to merging it.
An unstable prefix section is required. For this particular MSC, the line "While this proposal is not considered stable, implementations should use org.matrix.msc3356. as a prefix for all introduced fields." should suffice, though a table mapping name to org.matrix.msc3356.name and such would be appreciated. You're also welcome to use your own namespace.
Links to the specification should be versioned, not using /latest, as these will degrade over time.

…he matrix profile field

jkanefendt · 2021-12-03T15:15:47Z

@turt2live Please see my latest commits - i hope i have covered all the points of your last comment.

turt2live

Thanks for taking care of the administrative stuff :)

Resolution on the power levels thread would be nice to have this move forward to the next steps.

reivilibre · 2022-01-24T14:32:47Z

proposals/3356-add-openid-userinfo-fields.md

+
+## Security Considerations
+
+As the additional fields may contain sensitive information, they shall not be exposed by default. Instead, the 


In my opinion it would be nice to see the ability to restrict which rooms are exposed in the OpenID user info fields.

I might want to join a private Jitsi call, but that doesn't necessarily mean I want to open myself up to the service knowing all of the rooms that I'm in, when it only needs know about one room...

It would be good to at least see a mention of this issue here, even if we don't do anything about it right now; what could be done about it?
(I don't think it seems like it would be too hard to explicitly indicate which room we're interested in to /request_token, but maybe I'm wrong and don't understand enough.)

reivilibre · 2022-01-24T14:44:31Z

proposals/3356-add-openid-userinfo-fields.md

+
+2. `avatar_url` The user's avatar url _(optional)_.
+
+3. `room_powerlevels` A map between the id of every local room the user is joined to and the content of the room's power 


I think it's worth noting that someone can set up a fake/patched homeserver and forge the response to be whatever they like here.
In other words, it's not safe to rely on this for room-based access control unless you trust the user's homeserver — I think it would be worth noting since this effectively means doing this with decentralisation is unsafe.

I would have thought the safest way to get this information would be using a bot on a homeserver that the service controls — the bot doesn't need to use Synapse-specific endpoints; it can use standard client-server endpoints. It must however be in the rooms it wants to know about.

it does sort of depend on whether the server is in a controlled setup or not. In the proposed case, it's not likely to be abused however if we do indeed include the information then there's a good chance someone will expect it to be safe when it's not...

I think this ties back into blocking such a feature behind auth improvements more generally, such as the OAuth 2.0 experiments.

richvdh · 2022-04-14T10:49:06Z

proposals/3356-add-openid-userinfo-fields.md

+To enable third party applications for simple room-based access control, the OpenID user info returned by 
+`/_matrix/federation/v1/openid/userinfo` shall be extended by the following fields:


it's super helpul to readers if you can provide links to the current spec:

Suggested change

To enable third party applications for simple room-based access control, the OpenID user info returned by

`/_matrix/federation/v1/openid/userinfo` shall be extended by the following fields:

To enable third party applications for simple room-based access control, the OpenID user info returned by

[`/_matrix/federation/v1/openid/userinfo`](https://spec.matrix.org/v1.2/server-server-api/#get_matrixfederationv1openiduserinfo)

shall be extended by the following fields:

richvdh · 2022-04-14T10:51:16Z

proposals/3356-add-openid-userinfo-fields.md

+[OpenID token request endpoint](https://matrix.org/docs/spec/client_server/latest#id603) 
+`/_matrix/client/r0/user/.../openid/request_token` shall be extended to accept a user-/client-defined set of 


Suggested change

[OpenID token request endpoint](https://matrix.org/docs/spec/client_server/latest#id603)

`/_matrix/client/r0/user/.../openid/request_token` shall be extended to accept a user-/client-defined set of

OpenID token request endpoint

[`/_matrix/client/v3/user/.../openid/request_token`](https://spec.matrix.org/v1.2/client-server-api/#post_matrixclientv3useruseridopenidrequest_token)

shall be extended to accept a user-/client-defined set of

richvdh · 2022-04-14T11:09:51Z

@turt2live (as someone on the SCT who seems to have had opinions on this previously): matrix-org/synapse#10384 looks like a complete implementation of this on the Synapse side. Do you think that is sufficient to remove the Needs-Implementation label, and if so, do you think this MSC is ready to progress to FCP?

turt2live · 2022-04-14T20:14:39Z

proposals/3356-add-openid-userinfo-fields.md

+
+2. `avatar_url` The user's avatar url _(optional)_.
+
+3. `room_powerlevels` A map between the id of every local room the user is joined to and the content of the room's power 


This feels like a lot of data to return on a large account. Ideally we'd have a way to limit it to certain rooms.

I can see a use case where one might want to issue a token proving membership of a certain room. This could perhaps be done by supplying a (list of) room ID(s) when POSTing to the request_token endpoint: { "userinfo_fields": [ "display_name", {"room_powerlevels": ["!abcdef0123456789"]} ] }

turt2live

I've linked the server implementation in the PR description, though I think this needs a client implementation to fully validate this is the best path. The client-side implementation will further validate the security aspects by nature of the usage. Even more ideally, though not required, would be several client implementations to test the security implications.

Overall though, I'm very concerned about this PR's direction. I don't think it's in a state where we can put it through FCP because it ideologically conflicts with how OAuth scopes would work (which loop the user into the auth process, so they can choose to not grant access whereas modifying the response of /openid/userinfo does not give such luxury, even if limiting on the request_token side because the user still doesn't get to choose).

Ultimately I think OAuth scopes will better solve the problem case at hand, so would encourage more investigation into that area - it feels like trying to make OpenID do what we want will result in re-inventing OAuth scopes.

jkanefendt · 2022-06-09T12:07:59Z

@turt2live I agree with you that there should be some kind of user consent before releasing sensitive information to third party applications - but this is up to the Matrix client which is requesting the OIDC token containing additional claims from the Matrix server, so i think it's beyond the scope of this MSC.

I'll take a deeper dive into OAuth/OIDC regarding scopes/claims and change my proposal accordingly.

HarHarLinks · 2022-11-08T14:38:08Z

proposals/3356-add-openid-userinfo-fields.md

+
+## Security Considerations
+
+As the additional fields may contain sensitive information, they shall not be exposed by default. Instead, the 


Since one can obtain potentially sensitive info using this token, shouldn't the SS API /_matrix/federation/v1/openid/userinfo endpoint be a POST instead of GET to protect the token?

we might be limited by the OpenID spec a bit on this, ftr. Not actually sure if it requires anything of us.

Fair point.
It makes sense to use GET here as that may imply calls to the API being idempotent.
However GET arguments tend to leak to logs etc as they are part of the URL. The attack vector is probably usually limited, since the admin controlling matrix server and DB likely is likely the same controlling SSL termination.

Import proposal for adding openid user info fields

1941d21

jkanefendt changed the title ~~MSCNNNN: Add additional OpenID user info fields~~ MSC3356: Add additional OpenID user info fields Aug 24, 2021

jkanefendt added 2 commits August 24, 2021 09:04

Renamed proposal corresponding to the actual PR number

09eee49

Change headline of proposal according to the actual PR number

4c415fb

turt2live added client-server Client-Server API kind:maintenance MSC which clarifies/updates existing spec proposal A matrix spec change proposal proposal-in-review labels Aug 24, 2021

anoadragon453 reviewed Aug 27, 2021

View reviewed changes

proposals/3356-add-openid-userinfo-fields.md Outdated Show resolved Hide resolved

kevincox suggested changes Aug 28, 2021

View reviewed changes

proposals/3356-add-openid-userinfo-fields.md Show resolved Hide resolved

proposals/3356-add-openid-userinfo-fields.md Show resolved Hide resolved

afranke force-pushed the old_master branch from dca99ee to 70192e8 Compare August 30, 2021 13:00

turt2live force-pushed the old_master branch from e895827 to dca99ee Compare August 30, 2021 22:34

anoadragon453 mentioned this pull request Sep 14, 2021

Implement MSC3356 to support additional fields in the OpenID user info matrix-org/synapse#10384

Open

jkanefendt added 2 commits September 16, 2021 14:11

Add field avatar_url

79fa185

Add security considerations for MSC3356

0cf339e

kevincox reviewed Sep 22, 2021

View reviewed changes

turt2live added the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Sep 28, 2021

turt2live reviewed Nov 23, 2021

View reviewed changes

proposals/3356-add-openid-userinfo-fields.md Outdated Show resolved Hide resolved

turt2live reviewed Nov 23, 2021

View reviewed changes

jkanefendt added 3 commits December 3, 2021 11:55

Line wrapping

7f43ddf

Added unstable prefix section

dd21e6b

Rename userinfo field name to display_name so it corresponds to t…

0aa92f6

…he matrix profile field

turt2live self-requested a review December 3, 2021 16:05

turt2live reviewed Dec 14, 2021

View reviewed changes

Include the full content of the power level state event

75f7125

turt2live self-requested a review January 12, 2022 15:08

reivilibre reviewed Jan 24, 2022

View reviewed changes

turt2live removed their request for review April 8, 2022 21:35

richvdh reviewed Apr 14, 2022

View reviewed changes

turt2live self-requested a review April 14, 2022 12:02

turt2live reviewed Apr 14, 2022

View reviewed changes

turt2live requested changes Apr 14, 2022

View reviewed changes

turt2live removed the proposal-in-review label May 5, 2022

HarHarLinks suggested changes Nov 8, 2022

View reviewed changes

matrixbot mentioned this pull request Dec 21, 2023

Implement MSC3356 to support additional fields in the OpenID user info element-hq/synapse#10384

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MSC3356: Add additional OpenID user info fields #3356

MSC3356: Add additional OpenID user info fields #3356

jkanefendt commented Aug 24, 2021 •

edited by turt2live

Loading

anoadragon453 left a comment

kevincox Sep 22, 2021

turt2live Nov 23, 2021

richvdh Apr 14, 2022

turt2live left a comment

jkanefendt commented Dec 3, 2021

turt2live left a comment

reivilibre Jan 24, 2022 •

edited

Loading

reivilibre Jan 24, 2022

turt2live Apr 8, 2022

richvdh Apr 14, 2022

richvdh Apr 14, 2022

richvdh commented Apr 14, 2022

turt2live Apr 14, 2022

HarHarLinks Nov 8, 2022

turt2live left a comment

jkanefendt commented Jun 9, 2022

HarHarLinks Nov 8, 2022

turt2live Nov 8, 2022

HarHarLinks Nov 17, 2022


		## Security Considerations

		As the additional fields may contain sensitive information, they shall not be exposed by default. Instead, the


		2. `avatar_url` The user's avatar url _(optional)_.

		3. `room_powerlevels` A map between the id of every local room the user is joined to and the content of the room's power

		To enable third party applications for simple room-based access control, the OpenID user info returned by
		`/_matrix/federation/v1/openid/userinfo` shall be extended by the following fields:

		[OpenID token request endpoint](https://matrix.org/docs/spec/client_server/latest#id603)
		`/_matrix/client/r0/user/.../openid/request_token` shall be extended to accept a user-/client-defined set of

MSC3356: Add additional OpenID user info fields #3356

Are you sure you want to change the base?

MSC3356: Add additional OpenID user info fields #3356

Conversation

jkanefendt commented Aug 24, 2021 • edited by turt2live Loading

anoadragon453 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turt2live left a comment

Choose a reason for hiding this comment

jkanefendt commented Dec 3, 2021

turt2live left a comment

Choose a reason for hiding this comment

reivilibre Jan 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh commented Apr 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turt2live left a comment

Choose a reason for hiding this comment

jkanefendt commented Jun 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkanefendt commented Aug 24, 2021 •

edited by turt2live

Loading

reivilibre Jan 24, 2022 •

edited

Loading