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 <mdnight@riseup.net>

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

… 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. 🙏

S7evinK

Still didn't get around to finish this (sorry!), but as a first.

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

clientapi/admin_test.go

+				if tc.wantOK {
+					b := make(map[string]bool, 1)
+					_ = json.NewDecoder(rec.Body).Decode(&b)
+					available, ok := b["available"]
+					if !ok {
+						t.Fatal("'available' not found in body")
+					}
+					if available != tc.isAvailable {
+						t.Fatalf("expected 'available' to be %t, got %t instead", tc.isAvailable, available)
+					}
+				}


I'd go with the below, easier to read IMO.

Suggested change

if tc.wantOK {

b := make(map[string]bool, 1)

_ = json.NewDecoder(rec.Body).Decode(&b)

available, ok := b["available"]

if !ok {

t.Fatal("'available' not found in body")

}

if available != tc.isAvailable {

t.Fatalf("expected 'available' to be %t, got %t instead", tc.isAvailable, available)

}

}

// Nothing more to check, test is done.

if !tc.wantOK {

return

}

b := make(map[string]bool, 1)

_ = json.NewDecoder(rec.Body).Decode(&b)

available, ok := b["available"]

if !ok {

t.Fatal("'available' not found in body")

}

if available != tc.isAvailable {

t.Fatalf("expected 'available' to be %t, got %t instead", tc.isAvailable, available)

}

S7evinK · 2025-01-31T20:25:12Z

clientapi/routing/routing.go

+				default:
+					return util.JSONResponse{Code: http.StatusMethodNotAllowed, JSON: nil}


Not that it matters much, but I think this is already handled at the router level. (i.e., we only accept PUT and GET, and have this for MethodNotAllowed)
(No change required, as this makes it clearer here)

Yes, I know it's dead code, but default is required by the compiler and I thought switch-case would be more readable than

if GET { return this } return that

but anyway, I'll change this

S7evinK · 2025-01-31T20:28:11Z

internal/httputil/httpapi.go

+			}
+		}
+		if token != serviceToken {
+			logger.Debugf("Invalid service token '%s'", token)


Even if it is invalid (maybe just a typo in the token at the end or such), better don't log it.

Suggested change

logger.Debugf("Invalid service token '%s'", token)

logger.Debugf("Invalid service token")

You’re right. I should have removed the variable from the message. I forgot to change it after debugging.

S7evinK · 2025-01-31T21:01:40Z

clientapi/admin_test.go

+		}
+
+		for _, tc := range testCase {
+			t.Run("Retrieve existing account", func(t *testing.T) {


Suggested change

t.Run("Retrieve existing account", func(t *testing.T) {

t.Run(tc.Name, func(t *testing.T) {

S7evinK · 2025-01-31T21:19:45Z

clientapi/routing/admin.go

+		{
+			var rs api.QueryDevicesResponse
+			if err = userAPI.QueryDevices(req.Context(), &api.QueryDevicesRequest{UserID: userID}, &rs); err != nil {
+				logger.WithError(err).Error("QueryDevices")
+				return util.JSONResponse{
+					Code: http.StatusInternalServerError,
+					JSON: spec.InternalServerError{},
+				}
+			}
+			if !rs.UserExists {
+				return util.JSONResponse{
+					Code: http.StatusNotFound,
+					JSON: spec.NotFound("Given user ID does not exist"),
+				}
+			}
+			for i := range rs.Devices {
+				if d := rs.Devices[i]; d.ID == payload.DeviceID && d.UserID == userID {
+					userDeviceExists = true
+					break
+				}
+			}
+		}


No need to put this in a block, I guess? (Or I missed something)

S7evinK · 2025-02-02T19:03:02Z

userapi/internal/key_api.go

@@ -234,6 +234,19 @@ func (a *UserInternalAPI) PerformMarkAsStaleIfNeeded(ctx context.Context, req *a
 	return a.Updater.ManualUpdate(ctx, req.Domain, req.UserID)
 }

+func (a *UserInternalAPI) QueryMasterKeys(ctx context.Context, req *api.QueryMasterKeysRequest, res *api.QueryMasterKeysResponse) {


May not be needed as noted elsewhere.

S7evinK · 2025-02-02T19:05:21Z

setup/monolith.go

@@ -46,6 +50,8 @@ type Monolith struct {
 	// Optional
 	ExtPublicRoomsProvider   api.ExtraPublicRoomsProvider
 	ExtUserDirectoryProvider userapi.QuerySearchProfilesAPI
+
+	UserVerifierProvider *UserVerifierProvider


Suggested change

UserVerifierProvider *UserVerifierProvider

UserVerifierProvider httputil.UserVerifier

S7evinK · 2025-02-02T19:09:24Z

setup/monolith.go

+type UserVerifierProvider struct {
+	UserVerifier httputil.UserVerifier
+}
+
+func (u *UserVerifierProvider) VerifyUserFromRequest(req *http.Request) (*userapi.Device, *util.JSONResponse) {
+	return u.UserVerifier.VerifyUserFromRequest(req)
+}


Suggested change

type UserVerifierProvider struct {

UserVerifier httputil.UserVerifier

}

func (u *UserVerifierProvider) VerifyUserFromRequest(req *http.Request) (*userapi.Device, *util.JSONResponse) {

return u.UserVerifier.VerifyUserFromRequest(req)

}

type UserVerifierProvider struct {

httputil.UserVerifier

}

func (u *UserVerifierProvider) VerifyUserFromRequest(req *http.Request) (*userapi.Device, *util.JSONResponse) {

return u.VerifyUserFromRequest(req)

}

IMHO this is easier to read and doesn't stutter.

Wow, this is better, indeed. Didn't know about this

have to keep return u.UserVerifier.VerifyUserFromRequest(req) as it is because return u.VerifyUserFromRequest(req) causes recursive calls

userapi/storage/sqlite3/cross_signing_keys_table.go

S7evinK · 2025-02-02T19:22:03Z

userapi/storage/postgres/localpart_external_ids_table.go

+-- Stores data about connections between accounts and third-party auth providers
+CREATE TABLE IF NOT EXISTS userapi_localpart_external_ids (
+    -- The Matrix user ID for this account
+	localpart TEXT NOT NULL,


Given an initial implementation for vhosting in Dendrite, localpart should either be user_id (@test:example.com) or use an additional server_name column. (same for SQLite)

We use this table to map local users with their MAS identifiers. Using localpart to identify local users is more than enough, whereas using the server name here does not make any sense because it will always be the same. It will be the same server name in each row.

…t and no longer needed

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 added 9 commits January 23, 2025 01:25

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 6 commits January 29, 2025 11:41

Merge branch 'main' into msc3861

ba919d7

Merge branch 'main' into msc3861

6a741b2

Merge branch 'main' into msc3861

9ab9a8d

Merge branch 'main' into msc3861

8d39801

Merge branch 'main' into msc3861

0924e99

msd3861: added license headers for the new files

8df6442

S7evinK requested changes Feb 12, 2025

View reviewed changes

mdnight added 3 commits February 12, 2025 16:13

msc3861: cr fixes

3eb4c7e

msc3861: cr fixes

f91cc64

msc3861: cr

fd52c7e

mdnight force-pushed the msc3861 branch from d3696ec to adee985 Compare February 12, 2025 18:08

msc3861: ++

ff2ba03

mdnight force-pushed the msc3861 branch from adee985 to ff2ba03 Compare February 12, 2025 18:43

mdnight added 4 commits February 12, 2025 20:40

msc3861: delete QueryMasterKeys function and related as it's redundan…

c490bad

…t and no longer needed

msc3861: tests

1b8a659

remove deprecated linters from golangci-lint

b74b52d

goimports

950555a

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

mdnight commented Jan 25, 2025

S7evinK left a comment

S7evinK Jan 31, 2025

S7evinK Jan 31, 2025

mdnight Feb 12, 2025

S7evinK Jan 31, 2025

mdnight Feb 12, 2025

S7evinK Jan 31, 2025

S7evinK Jan 31, 2025 •

edited

Loading

S7evinK Feb 2, 2025

S7evinK Feb 2, 2025

S7evinK Feb 2, 2025

mdnight Feb 12, 2025

mdnight Feb 12, 2025 •

edited

Loading

S7evinK Feb 2, 2025 •

edited

Loading

mdnight Feb 12, 2025 •

edited

Loading

		default:
		return util.JSONResponse{Code: http.StatusMethodNotAllowed, JSON: nil}

	logger.Debugf("Invalid service token '%s'", token)
	logger.Debugf("Invalid service token")

	t.Run("Retrieve existing account", func(t *testing.T) {
	t.Run(tc.Name, func(t *testing.T) {

	UserVerifierProvider *UserVerifierProvider
	UserVerifierProvider httputil.UserVerifier

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

mdnight commented Jan 25, 2025

S7evinK 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

S7evinK Jan 31, 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

mdnight Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

S7evinK Feb 2, 2025 • edited Loading

Choose a reason for hiding this comment

mdnight Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

mdnight commented Jan 10, 2025 •

edited

Loading

S7evinK Jan 31, 2025 •

edited

Loading

mdnight Feb 12, 2025 •

edited

Loading

S7evinK Feb 2, 2025 •

edited

Loading

mdnight Feb 12, 2025 •

edited

Loading