MSC3861: MAS support #3493

mdnight · 2025-01-10T02:33:53Z

Pull Request Checklist

I have added Go unit tests or Complement integration tests for this PR or I have justified why this PR doesn't need tests
Pull request includes a sign off below using a legally identifiable name or I have already signed off privately

Signed-off-by: Roman Isaev <[email protected]>

i.e. /_synapse/admin/v1/users/{userID}/_allow_cross_signing_replacement_without_uia

If access token expires the client(i.e. element) expects a specific response with http code 401 and spec.UnknownToken

MAS requires this endpoint to fetch the data for the account management page

Extended logic of the endpoint in order to make it compatible with MAS

this change is based mostly on changes made in synapse https://github.com/element-hq/synapse/blob/develop/synapse/rest/client/keys.py#L392

Since MSC3861 is conflicting with standard reg/login flows, we require to disable them before running the server

codecov-commenter · 2025-01-15T21:25:47Z

Codecov Report

Attention: Patch coverage is 50.32258% with 770 lines in your changes missing coverage. Please review.

Project coverage is 49.44%. Comparing base (a41f9cc) to head (641f5b5).
Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
clientapi/routing/admin.go	52.26%	153 Missing and 37 partials ⚠️
setup/mscs/msc3861/msc3861_user_verifier.go	48.27%	106 Missing and 29 partials ⚠️
clientapi/routing/routing.go	54.35%	24 Missing and 65 partials ⚠️
clientapi/routing/key_crosssigning.go	25.24%	74 Missing and 3 partials ⚠️
...erapi/storage/postgres/cross_signing_keys_table.go	42.37%	30 Missing and 4 partials ⚠️
...serapi/storage/sqlite3/cross_signing_keys_table.go	42.37%	30 Missing and 4 partials ⚠️
...as/2024123101150000_drop_primary_key_constraint.go	44.44%	29 Missing and 1 partial ⚠️
clientapi/routing/profile.go	12.50%	18 Missing and 3 partials ⚠️
userapi/storage/shared/storage.go	54.34%	15 Missing and 6 partials ⚠️
setup/config/config_clientapi.go	54.28%	14 Missing and 2 partials ⚠️
... and 19 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3493      +/-   ##
==========================================
- Coverage   49.44%   49.44%   -0.01%     
==========================================
  Files         524      533       +9     
  Lines       59799    60996    +1197     
==========================================
+ Hits        29569    30159     +590     
- Misses      26750    27251     +501     
- Partials     3480     3586     +106

Flag	Coverage Δ
unittests	`49.44% <50.32%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

S7evinK

From what I've seen so far (and tested), this is awesome! Thanks a lot for this!

The requested changes are only minor, still need to go through 34 changed files 🙃

internal/httputil/httpapi.go

S7evinK · 2025-01-19T09:56:17Z

clientapi/routing/register.go

@@ -817,7 +819,7 @@ func checkAndCompleteFlow(
 	return util.JSONResponse{
 		Code: http.StatusUnauthorized,
 		JSON: newUserInteractiveResponse(sessionID,
-			cfg.Derived.Registration.Flows, cfg.Derived.Registration.Params),
+			cfg.Derived.Registration.Flows, cfg.Derived.Registration.Params, ""),


We should add a default message instead of returning nothing, I think.

We need the message field only once, when we want to show this: To reset your end-to-end encryption cross-signing identity, you first need to approve it at ${URL} and then try again.
For other cases, we simply ignore the empty field and do not return it in the response.

type userInteractiveResponse struct { Flows []authtypes.Flow `json:"flows"` ... Msg string `json:"msg,omitempty"` <--- this }

Also, I can't find msg in the spec 🤷

S7evinK · 2025-01-19T10:00:13Z

userapi/api/api.go

@@ -129,6 +132,7 @@ type QuerySearchProfilesAPI interface {
 	QuerySearchProfiles(ctx context.Context, req *QuerySearchProfilesRequest, res *QuerySearchProfilesResponse) error
 }

+// FIXME: typo in Acccess


🙃 Ooops.

S7evinK · 2025-01-19T10:01:35Z

userapi/api/api.go

+	Localpart    string
+	ExternalID   string
+	AuthProvider string
+	CreatedTs    int64


Suggested change

CreatedTs int64

CreatedTS int64

S7evinK · 2025-01-21T20:23:54Z

clientapi/routing/routing.go

+			})).Methods(http.MethodPost)
+	} else {
+		// If msc3861 is enabled, these endpoints are either redundant or replaced by Matrix Auth Service (MAS)
+		// Once we migrate to MAS completely, these edndpoints should be removed


Suggested change

// Once we migrate to MAS completely, these edndpoints should be removed

// Once we migrate to MAS completely, these endpoints should be removed

S7evinK · 2025-01-21T20:52:20Z

syncapi/syncapi_test.go

@@ -119,6 +120,20 @@ func (s *syncUserAPI) PerformLastSeenUpdate(ctx context.Context, req *userapi.Pe
 	return nil
 }

+type userVerifier struct {
+	m map[string]struct {


What is m? (more descriptive field name please, something like accessTokenToDeviceAndResponse, maybe?)

S7evinK · 2025-01-21T21:07:22Z

...api/storage/postgres/deltas/2025011001110000_add_xsigning_updatable_without_uia_before_ms.go

@@ -0,0 +1,23 @@
+package deltas


As mentioned OOB, we probably don't need this migration and can instead use

dendrite/clientapi/routing/register.go

Lines 57 to 69 in f4506a0

// sessionsDict keeps track of completed auth stages for each session.

// It shouldn't be passed by value because it contains a mutex.

type sessionsDict struct {

sync.RWMutex

sessions map[string][]authtypes.LoginType

sessionCompletedResult map[string]registerResponse

params map[string]registerRequest

timer map[string]*time.Timer

// deleteSessionToDeviceID protects requests to DELETE /devices/{deviceID} from being abused.

// If a UIA session is started by trying to delete device1, and then UIA is completed by deleting device2,

// the delete request will fail for device2 since the UIA was initiated by trying to delete device1.

deleteSessionToDeviceID map[string]string

}

instead? (same goes for the SQLite migration etc.)

Not sure I have a clear understanding of how this structure might apply to our case. I need to reflect on it a bit. 🤔

Done. Added a new map to the structure for managing this kind of session and covered new methods with tests. Also deleted xsigning_updatable_without_uia_before_ms column from the database and reverted all related logic

S7evinK · 2025-01-21T21:12:53Z

userapi/storage/postgres/accounts_table.go

@@ -116,6 +116,7 @@ func (s *accountsStatements) InsertAccount(
 	localpart string, serverName spec.ServerName,
 	hash, appserviceID string, accountType api.AccountType,
 ) (*api.Account, error) {
+	// TODO: can we replace "UnixNano() / 1M" with "UnixMilli()"?


Yes we can! :)

As a followup PR, use spec.AsTimestamp(time.Now()) which does the same, and update AsTimestamp in gomatrixserverlib to use UnixMilli().

created a pull requests for gomatrixserverlib 🙂 matrix-org/gomatrixserverlib#447

S7evinK · 2025-01-21T21:39:02Z

setup/config/config_mscs.go

 	// 'msc2444': Peeking over federation - https://github.com/matrix-org/matrix-doc/pull/2444
 	// 'msc2753': Peeking via /sync - https://github.com/matrix-org/matrix-doc/pull/2753
 	// 'msc2836': Threading - https://github.com/matrix-org/matrix-doc/pull/2836
 	MSCs []string `yaml:"mscs"`

+	// MSC3861 contains config related to the experimental feature MSC3861. It takes effect only if 'msc3861' is included in 'MSCs' array


Suggested change

// MSC3861 contains config related to the experimental feature MSC3861. It takes effect only if 'msc3861' is included in 'MSCs' array

// MSC3861 contains config related to the experimental feature MSC3861.

// It takes effect only if 'msc3861' is included in 'MSCs' array.

S7evinK · 2025-01-21T21:46:31Z

setup/config/config_clientapi.go

-			c.RecaptchaApiJsUrl = "https://www.google.com/recaptcha/api.js"
+
+	if c.MSCs.MSC3861Enabled() {
+		if c.RecaptchaEnabled || !c.RegistrationDisabled {


Maybe just me, but I prefer reading the "not" path first.

Suggested change

if c.RecaptchaEnabled || !c.RegistrationDisabled {

if !c.RegistrationDisabled || c.RecaptchaEnabled {

For a different PR: We should only have either Enabled or Disabled in our config. Otherwise stuff like this might become confusing.

I don't have any preferences regarding the "not" path, but I agree that registration should go first. 😄

S7evinK

Didn't want to request changes again, just wanted to add a few more comments..

S7evinK · 2025-01-22T07:48:33Z

userapi/storage/postgres/localpart_external_ids_table.go

+const deleteUserExternalIDSQL = "" +
+	"SELECT localpart, external_id, auth_provider, created_ts FROM userapi_localpart_external_ids WHERE external_id = $1 AND auth_provider = $2"


This doesn't look like a DELETE. (same for SQLite)

S7evinK · 2025-01-22T07:48:56Z

setup/mscs/msc3861/msc3861_user_verifier.go

+	serverName spec.ServerName,
+	cfg *config.MSC3861,
+	allowGuest bool,
+	httpClient *http.Client,


httpClient should probably be a fclient.Client, see https://github.com/matrix-org/gomatrixserverlib/blob/0a1b2bafb5cf72727787359d39653493b3318398/fclient/client.go#L44-L50

And it should be an error if it is not set, we shouldn't use http.DefaultClient.

… instead of db

…and related logic

mdnight · 2025-01-25T02:24:07Z

@S7evinK the PR is ready for review again. I believe I have addressed all the comments. Please have another look when you have a moment. 🙏

mdnight added 30 commits December 21, 2024 01:00

mas: added /auth_issuer endpoint

ba542fe

mas: added username_available endpoint

2c47959

mas: rename msc2965 to msc3861

e1dfe62

mas: added localpart_external_ids table

150be58

mas: first successful attempt of login with via mas

63a199c

mas: added "admin's replacement without uia" endpoint

9d9841d

i.e. /_synapse/admin/v1/users/{userID}/_allow_cross_signing_replacement_without_uia

minor goimports fix

4f406e2

mas: return correct http code

b950703

If access token expires the client(i.e. element) expects a specific response with http code 401 and spec.UnknownToken

another goimports fix

9ebcebe

mas: implemented PUT /admin/v2/users/{userID} endpoint

be8d490

MAS requires this endpoint to fetch the data for the account management page

mas: add AccountTypeOIDCService

524f65c

mas: modify PUT /profile/{userID}/displayname endpoint

ff63e7f

Extended logic of the endpoint in order to make it compatible with MAS

drop primary key constraint from userapi_devices.access_token

bf310d5

mas: refactor admin user device handler

f4ff426

mas: added admin's delete devices endpoint

803cce8

mas: minor fixes in cross_signing_keys_table files

7ffb2c1

refactor logger calls

c06e0aa

mas: added /admin/v1/deactivate/{userID} endpoint

48f3cd3

minor refactoring

9b064b1

mas: added support of msc3861 to /keys/device_signing/upload endpoint

cc7deb2

this change is based mostly on changes made in synapse https://github.com/element-hq/synapse/blob/develop/synapse/rest/client/keys.py#L392

mas: fix displayname handling

5cffc2c

mas: handle 3pids from mas

811a504

mas: acced msc3861 config example to the dendrite-sample.yaml

17576cc

mas: fail if conflicts in config occur

e943ba5

Since MSC3861 is conflicting with standard reg/login flows, we require to disable them before running the server

mas: reorganise endpoints

7eec60e

unit tests fix

fb15db7

Bump golang version

b44a79c

more fixes

7311d3e

linter fixes

0990676

fix cross_signing_keys_table

1afe2b9

mdnight requested a review from a team as a code owner January 15, 2025 19:02

mdnight changed the title ~~WIP: MSC3861: MAS support~~ MSC3861: MAS support Jan 15, 2025

mas: refactoring

3619a6d

mdnight force-pushed the msc3861 branch from 5280b4f to 3619a6d Compare January 15, 2025 19:35

mdnight added 6 commits January 17, 2025 00:09

mas: add missing server_name field to sqlite migration

64f308b

Merge branch 'main' of github.com:element-hq/dendrite into msc3861

6833e99

mas: cross signing fixes after merge

b44f899

mas: fix key_crosssigning_test.go

021431c

mas: todo comment

641f5b5

Merge branch 'main' into msc3861

7d076a9

S7evinK requested changes Jan 21, 2025

View reviewed changes

S7evinK requested changes Jan 22, 2025

View reviewed changes

sandhose mentioned this pull request Jan 22, 2025

MSC2966: Usage of OAuth 2.0 Dynamic Client Registration in Matrix matrix-org/matrix-spec-proposals#2966

Open

mdnight added 10 commits January 22, 2025 22:50

fix typo in api.QueryAccessTokenAPI

17b7677

code review fixes

8a05a66

Merge branch 'msc3861' of github.com:mdnight/dendrite into msc3861

ea875b3

more fixes

bf31c44

cr fixes

a185027

fix test

b5f34df

mas: store crossSigngingKeysReplacement period in sessionsDict struct…

4534456

… instead of db

mas: revert cross_signing_keys.updatable_without_uia_before_ms field …

0b4cf3b

…and related logic

mas: fix tests

27f7a5e

more test fixes

c1ad175

mdnight force-pushed the msc3861 branch from 779a7ed to c1ad175 Compare January 25, 2025 01:09

tests for sessionsDict.crossSigningKeysReplacement

b8ea41b

mdnight requested a review from S7evinK January 25, 2025 02:20

mdnight added 2 commits January 29, 2025 11:41

Merge branch 'main' into msc3861

ba919d7

Merge branch 'main' into msc3861

6a741b2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MSC3861: MAS support #3493

MSC3861: MAS support #3493

mdnight commented Jan 10, 2025 •

edited

Loading

codecov-commenter commented Jan 15, 2025 •

edited

Loading

S7evinK left a comment

S7evinK Jan 19, 2025

mdnight Jan 23, 2025 •

edited

Loading

S7evinK Jan 19, 2025

S7evinK Jan 19, 2025

S7evinK Jan 21, 2025

S7evinK Jan 21, 2025

S7evinK Jan 21, 2025

mdnight Jan 23, 2025 •

edited

Loading

mdnight Jan 25, 2025 •

edited

Loading

S7evinK Jan 21, 2025

mdnight Jan 22, 2025 •

edited

Loading

S7evinK Jan 21, 2025

S7evinK Jan 21, 2025

mdnight Jan 22, 2025

S7evinK left a comment •

edited

Loading

S7evinK Jan 22, 2025

mdnight Jan 22, 2025

S7evinK Jan 22, 2025

mdnight commented Jan 25, 2025

	// Once we migrate to MAS completely, these edndpoints should be removed
	// Once we migrate to MAS completely, these endpoints should be removed

	// sessionsDict keeps track of completed auth stages for each session.
	// It shouldn't be passed by value because it contains a mutex.
	type sessionsDict struct {
	sync.RWMutex
	sessions map[string][]authtypes.LoginType
	sessionCompletedResult map[string]registerResponse
	params map[string]registerRequest
	timer map[string]*time.Timer
	// deleteSessionToDeviceID protects requests to DELETE /devices/{deviceID} from being abused.
	// If a UIA session is started by trying to delete device1, and then UIA is completed by deleting device2,
	// the delete request will fail for device2 since the UIA was initiated by trying to delete device1.
	deleteSessionToDeviceID map[string]string
	}

	// MSC3861 contains config related to the experimental feature MSC3861. It takes effect only if 'msc3861' is included in 'MSCs' array
	// MSC3861 contains config related to the experimental feature MSC3861.
	// It takes effect only if 'msc3861' is included in 'MSCs' array.

	if c.RecaptchaEnabled \|\| !c.RegistrationDisabled {
	if !c.RegistrationDisabled \|\| c.RecaptchaEnabled {

		const deleteUserExternalIDSQL = "" +
		"SELECT localpart, external_id, auth_provider, created_ts FROM userapi_localpart_external_ids WHERE external_id = $1 AND auth_provider = $2"

MSC3861: MAS support #3493

Are you sure you want to change the base?

MSC3861: MAS support #3493

Conversation

mdnight commented Jan 10, 2025 • edited Loading

Pull Request Checklist

codecov-commenter commented Jan 15, 2025 • edited Loading

Codecov Report

S7evinK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdnight Jan 23, 2025 • 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

Choose a reason for hiding this comment

mdnight Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

mdnight Jan 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdnight Jan 22, 2025 • 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

S7evinK left a comment • 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

mdnight commented Jan 25, 2025

mdnight commented Jan 10, 2025 •

edited

Loading

codecov-commenter commented Jan 15, 2025 •

edited

Loading

mdnight Jan 23, 2025 •

edited

Loading

mdnight Jan 23, 2025 •

edited

Loading

mdnight Jan 25, 2025 •

edited

Loading

mdnight Jan 22, 2025 •

edited

Loading

S7evinK left a comment •

edited

Loading