Implement MSC3356 to support additional fields in the OpenID user info #10384

jkanefendt · 2021-07-13T14:39:27Z

Add option to include additional fields in the OpenID user info (currently the user's display name an effective power levels).
This should simplify room-based access control in third-party widgets. Currently, joined rooms and power levels can only be determined via Admin-API.

See MSC3356.

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

…enty the display name an the user's effective power levels)

clokep · 2021-07-30T11:23:22Z

I believe that this would need an MSC to accompany it since it is a specified endpoint? (In which case we wouldn't need to have any config options and could just include them, most likely.)

richvdh

I believe that this would need an MSC to accompany it since it is a specified endpoint? (In which case we wouldn't need to have any config options and could just include them, most likely.)

Yeah, there's not much point in supporting extra fields for Synapses deployed on the open federation unless the spec is extended to document the additional fields.

That said, there's probably a usecase for custom fields in private federations - but in that case, I'd expect the extension to happen via a Synapse module (see https://matrix-org.github.io/synapse/latest/modules.html) rather than an eclectic mix of configuration options.

@jkanefendt: could describe your usecase here, so we can suggest how to proceed?

jkanefendt · 2021-08-24T07:17:25Z

I believe that this would need an MSC to accompany it since it is a specified endpoint? (In which case we wouldn't need to have any config options and could just include them, most likely.)

Yeah, there's not much point in supporting extra fields for Synapses deployed on the open federation unless the spec is extended to document the additional fields.

That said, there's probably a usecase for custom fields in private federations - but in that case, I'd expect the extension to happen via a Synapse module (see https://matrix-org.github.io/synapse/latest/modules.html) rather than an eclectic mix of configuration options.

@jkanefendt: could describe your usecase here, so we can suggest how to proceed?

I finally found the time to create a MSC with a brief description and usecase - see https://github.com/matrix-org/matrix-doc/blob/4c415fb7bc2d991a4515820d8c4fda75e98ce94e/proposals/3356-add-openid-userinfo-fields.md

…add-openid-userinfo-fields

… a fixed, statically configured set. Extend the set of available fields by the user's avatar url.

anoadragon453 · 2021-09-14T15:41:12Z

@jkanefendt That MSC will need some unstable prefixes defined (or the MSC otherwise needs to be merged) before we can implement it in Synapse. See here for an example of defining unstable prefixes. You would need to add an unstable prefix to each of the new key names in the OpenID response.

It also appears that there are some unaddressed comments on the MSC, which addressing will help move the MSC forwards 🙂

jkanefendt · 2021-09-16T15:02:21Z

@jkanefendt That MSC will need some unstable prefixes defined (or the MSC otherwise needs to be merged) before we can implement it in Synapse. See here for an example of defining unstable prefixes. You would need to add an unstable prefix to each of the new key names in the OpenID response.

It also appears that there are some unaddressed comments on the MSC, which addressing will help move the MSC forwards

Thanks for your feedback @anoadragon453 . I'm going to add the unstable prefixes ASAP.
In the meantime, i found some time for some security considerations regarding MSC3356. My proposal for releasing fields on a per-token basis is implemented in jkanefendt@92e663c.

anoadragon453 · 2021-09-16T15:50:19Z

In the meantime, i found some time for some security considerations regarding MSC3356.

I think that's an excellent addition. And since we're on the topic of implementing additional features as they come into the MSC, don't be afraid to break up the feature into multiple PRs, especially for self-contained changes like specifying which fields to return. It can help make review easier and get your code merged quicker.

Looking forward to the unstable prefixes. Do poke on the MSC PR if you need any further clarifications on how to add them.

jkanefendt · 2021-12-14T07:46:42Z

Yeah, there's not much point in supporting extra fields for Synapses deployed on the open federation unless the spec is extended to document the additional fields.

That said, there's probably a usecase for custom fields in private federations - but in that case, I'd expect the extension to happen via a Synapse module (see https://matrix-org.github.io/synapse/latest/modules.html) rather than an eclectic mix of configuration options.

@jkanefendt: could describe your usecase here, so we can suggest how to proceed?

All these points have been covered in MSC3356 by now.

DMRobertson · 2021-12-14T12:52:23Z

@jkanefendt I think that number is already in use: https://github.com/matrix-org/matrix-doc/issues/3556. If you make a pull request to that repo from the branch you linked, you should be assigned a unique number. cc @matrix-org/spec-core-team .

Sorry, I got my numbers mixed up. 3356 versus 3556.

…ental_features.msc3356_enabled`

richvdh

Generally this looks great to me, though I have a few nits it would be nice to clean up.

changelog.d/10384.misc

synapse/api/constants.py

richvdh · 2022-04-14T10:42:50Z

synapse/federation/federation_server.py

+    async def on_openid_userinfo(
+        self, token: str
+    ) -> Optional[Tuple[str, Optional[List[str]]]]:


please could you add a docstring documenting what this returns?

richvdh · 2022-04-14T10:53:27Z

synapse/storage/databases/main/openid.py

+    async def get_user_id_and_userinfo_fields_for_open_id_token(
        self, token: str, ts_now_ms: int
-    ) -> Optional[str]:
-        def get_user_id_for_token_txn(txn: LoggingTransaction) -> Optional[str]:
+    ) -> Optional[Tuple[str, Optional[List[str]]]]:


again, please can you give this a docstring.

richvdh · 2022-04-14T10:54:40Z

synapse/storage/schema/main/delta/70/01openid_token_userinfo_fields.sql

@@ -0,0 +1 @@
+ALTER TABLE open_id_tokens ADD COLUMN userinfo_fields TEXT;


please could you add a sql comment describing what this column contains.

synapse/storage/databases/main/openid.py

richvdh · 2022-04-14T11:01:53Z

synapse/federation/transport/server/__init__.py

+            room_state = await self.hs.get_state_handler().get_current_state(
+                room.room_id
+            )


please can you give this event_type and state_key arguments. Pulling out the entire room state for a large room is quite expensive.

Solved in 29664b1

richvdh · 2022-04-14T11:03:31Z

synapse/federation/transport/server/__init__.py

+                stripped_content = copy.deepcopy(room_power_levels.content)
+                stripped_content["users"] = {
+                    k: v for k, v in stripped_content["users"].items() if k == user_id
+                }
+                power_levels[room.room_id] = stripped_content


I don't think I have strong feelings on this. I think the current behaviour is probably fine. It would be nice if the MSC made the behaviour here explicit though.

synapse/federation/transport/server/__init__.py

richvdh · 2022-04-14T11:08:23Z

One other thing: it would be really nice to see some tests for this stuff, especially the power-levels stuff which looks fiddly. It might be best to wait for more progress on the MSC before putting in too much work in that direction though.

…add-openid-userinfo-fields

anoadragon453 · 2022-05-31T16:08:43Z

Hi again @jkanefendt. Just checking in to see if you're able to continue with the suggested changes? It'd be good to see this PR finished up :)

jkanefendt · 2022-06-01T06:28:14Z

Hi again @jkanefendt. Just checking in to see if you're able to continue with the suggested changes? It'd be good to see this PR finished up :)

Hi @anoadragon453. I'm quite busy right now, i hope i'll find the time to add the requested changes and documentation in June!

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

…_fields_for_open_id_token (storage.databases.main.openid.OpenIdStore) is not optional any more

…erver)

…d_token (synapse.storage.databases.main.openid)

anoadragon453 · 2022-06-08T11:07:02Z

Hi @jkanefendt, thank you for updating this PR.

Looking at MSC3356, there have been concerns raised about whether the design is the right approach. I'm a little hesitant to merge this PR before that opinion in engaged, even behind an experimental flag. Could you respond with your thoughts on the MSC?

DMRobertson · 2022-06-14T11:14:26Z

Taking this out of the review queue until the discussions on the MSC are resolved.

jkanefendt added 2 commits July 13, 2021 16:25

Add option to include additional fields in the OpenID user info (curr…

f3a6ac5

…enty the display name an the user's effective power levels)

Added changelog entry

d982fd1

erikjohnston requested a review from a team July 23, 2021 16:17

Fix unhandled KeyError

3f88ab2

richvdh suggested changes Aug 3, 2021

View reviewed changes

richvdh added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Aug 3, 2021

jkanefendt added 3 commits August 24, 2021 10:52

Merge branch 'develop' of https://github.com/jkanefendt/synapse into …

9f71e78

…add-openid-userinfo-fields

Merge branch 'develop' into add-openid-userinfo-fields

486af13

Accept a user defined set of extra OpenId user info fields instead of…

92e663c

… a fixed, statically configured set. Extend the set of available fields by the user's avatar url.

anoadragon453 added the z-blocked (Deprecated Label) label Sep 14, 2021

jkanefendt added 3 commits September 16, 2021 15:39

Fix coding style

f15047e

Fix coding style

7a62795

Fix typing warning

587bebd

jkanefendt added 5 commits December 3, 2021 14:35

Add unstable prefixes for API field names and constants

5c34abc

Merge branch 'master' into add-openid-userinfo-fields

a9b1940

Merge branch 'develop' into add-openid-userinfo-fields

08034e5

Fixed trailing whitespace

1d2edab

Fix typing

2582fb0

jkanefendt changed the title ~~Add option to include additional fields in the OpenID user info~~ Implement MSC3356 to support additional fields in the OpenID user info Dec 3, 2021

DMRobertson requested a review from a team December 9, 2021 16:16

jkanefendt closed this Dec 14, 2021

jkanefendt reopened this Dec 14, 2021

DMRobertson changed the title ~~Implement MSC3356 to support additional fields in the OpenID user info~~ Implement [MSC3356](https://github.com/matrix-org/matrix-doc/issues/3356) to support additional fields in the OpenID user info Dec 14, 2021

Experimental feature MSC3356 must be explicetely enabled per `experim…

b429c47

…ental_features.msc3356_enabled`

richvdh self-assigned this Apr 14, 2022

richvdh reviewed Apr 14, 2022

View reviewed changes

richvdh mentioned this pull request Apr 14, 2022

MSC3356: Add additional OpenID user info fields matrix-org/matrix-spec-proposals#3356

Open

richvdh removed their assignment Apr 14, 2022

Merge branch 'develop' of https://github.com/matrix-org/synapse into …

46bfefe

…add-openid-userinfo-fields

jkanefendt and others added 11 commits June 1, 2022 08:44

Update changelog.d/10384.misc

0cef603

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

Update synapse/storage/databases/main/openid.py

22e673a

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

Merge branch 'develop' into add-openid-userinfo-fields

54c7f16

MSC3356: Only pull the power levels from room state

29664b1

Add description for OpenIdUserInfoFields constants

f96343d

Simplify some things as the list returned by get_user_id_and_userinfo…

372b9dc

…_fields_for_open_id_token (storage.databases.main.openid.OpenIdStore) is not optional any more

Add docstring for on_openid_userinfo (synapse.federation.federation_s…

48a9bf1

…erver)

Fix linting

d7b3ea1

Add docstring for function get_user_id_and_userinfo_fields_for_open_i…

ed7f4b2

…d_token (synapse.storage.databases.main.openid)

Add description for schema delta 72/01openid_token_userinfo_fields.sql

e145f9b

Add code comment

96e7a9c

jkanefendt requested a review from richvdh June 2, 2022 09:11

richvdh requested a review from a team June 6, 2022 12:12

richvdh removed the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Jun 6, 2022

richvdh removed their request for review June 6, 2022 12:12

DMRobertson removed the request for review from a team June 14, 2022 11:14

reivilibre added the X-Needs-Info This issue is blocked awaiting information from the reporter label Aug 5, 2022

clokep added the Z-Spec-Blocked This change is blocked on specification (e.g. an MSC). label Sep 25, 2023

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

Implement MSC3356 to support additional fields in the OpenID user info #10384

Implement MSC3356 to support additional fields in the OpenID user info #10384

jkanefendt commented Jul 13, 2021 •

edited by clokep

Loading

clokep commented Jul 30, 2021

richvdh left a comment

jkanefendt commented Aug 24, 2021

anoadragon453 commented Sep 14, 2021

jkanefendt commented Sep 16, 2021 •

edited

Loading

anoadragon453 commented Sep 16, 2021

jkanefendt commented Dec 14, 2021

DMRobertson commented Dec 14, 2021 •

edited

Loading

richvdh left a comment

richvdh Apr 14, 2022

jkanefendt Jun 1, 2022

richvdh Apr 14, 2022

jkanefendt Jun 1, 2022

richvdh Apr 14, 2022

jkanefendt Jun 1, 2022

richvdh Apr 14, 2022

jkanefendt Jun 1, 2022

richvdh Apr 14, 2022

richvdh commented Apr 14, 2022

anoadragon453 commented May 31, 2022

jkanefendt commented Jun 1, 2022

anoadragon453 commented Jun 8, 2022 •

edited

Loading

DMRobertson commented Jun 14, 2022

		@@ -0,0 +1 @@
		ALTER TABLE open_id_tokens ADD COLUMN userinfo_fields TEXT;

Implement MSC3356 to support additional fields in the OpenID user info #10384

Are you sure you want to change the base?

Implement MSC3356 to support additional fields in the OpenID user info #10384

Conversation

jkanefendt commented Jul 13, 2021 • edited by clokep Loading

clokep commented Jul 30, 2021

richvdh left a comment

Choose a reason for hiding this comment

jkanefendt commented Aug 24, 2021

anoadragon453 commented Sep 14, 2021

jkanefendt commented Sep 16, 2021 • edited Loading

anoadragon453 commented Sep 16, 2021

jkanefendt commented Dec 14, 2021

DMRobertson commented Dec 14, 2021 • edited Loading

richvdh 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

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

Choose a reason for hiding this comment

richvdh commented Apr 14, 2022

anoadragon453 commented May 31, 2022

jkanefendt commented Jun 1, 2022

anoadragon453 commented Jun 8, 2022 • edited Loading

DMRobertson commented Jun 14, 2022

jkanefendt commented Jul 13, 2021 •

edited by clokep

Loading

jkanefendt commented Sep 16, 2021 •

edited

Loading

DMRobertson commented Dec 14, 2021 •

edited

Loading

anoadragon453 commented Jun 8, 2022 •

edited

Loading