msc3861: validate access_token uniqueness manually

If MAS is in use, server does not generate access tokens for its client devices. As a result, the access_token value in device table will always be empty. The unique constraint is enabled for the access_token it does not allow storing empty values in the database so we had to drop it. Since we still have old login logic, the constraint is still required, so we have to check the uniqueness manually.
2025-03-14 14:15:35 +00:00 · 2025-03-02 20:16:20 +00:00 · 2025-03-02 20:16:20 +00:00 · 20b3917084
commit 20b3917084
parent 950555a5a5
9 changed files with 99 additions and 73 deletions
--- a/.golangci.yml
+++ b/.golangci.yml
@ -28,7 +28,6 @@ run:
  # the dependency descriptions in go.mod.
  #modules-download-mode: (release|readonly|vendor)

-
 # output configuration options
 output:
  # colored-line-number|line-number|json|tab|checkstyle|code-climate, default is "colored-line-number"
@ -41,7 +40,6 @@ output:
  # print linter name in the end of issue text, default is true
  print-linter-name: true

-
 # all available settings of specific linters
 linters-settings:
  errcheck:
@ -72,22 +70,12 @@ linters-settings:
          - (github.com/golangci/golangci-lint/pkg/logutils.Log).Warnf
          - (github.com/golangci/golangci-lint/pkg/logutils.Log).Errorf
          - (github.com/golangci/golangci-lint/pkg/logutils.Log).Fatalf
-  golint:
-    # minimal confidence for issues, default is 0.8
-    min-confidence: 0.8
  gofmt:
    # simplify code: gofmt with `-s` option, true by default
    simplify: true
-  goimports:
-    # put imports beginning with prefix after 3rd-party packages;
-    # it's a comma-separated list of prefixes
-    #local-prefixes: github.com/org/project
  gocyclo:
    # minimal code complexity to report, 30 by default (but we recommend 10-20)
    min-complexity: 25
-  maligned:
-    # print struct with more effective memory layout or not, false by default
-    suggest-new: true
  dupl:
    # tokens count to trigger issue, 150 by default
    threshold: 100
@ -96,30 +84,17 @@ linters-settings:
    min-len: 3
    # minimal occurrences count to trigger, 3 by default
    min-occurrences: 3
-  depguard:
-    list-type: blacklist
-    include-go-root: false
-    packages:
-    #  - github.com/davecgh/go-spew/spew
  misspell:
    # Correct spellings using locale preferences for US or UK.
    # Default is to use a neutral variety of English.
    # Setting locale to US will correct the British spelling of 'colour' to 'color'.
    locale: UK
-    ignore-words:
-    #  - someword
  lll:
    # max line length, lines longer will be reported. Default is 120.
    # '\t' is counted as 1 character by default, and can be changed with the tab-width option
    line-length: 96
    # tab width in spaces. Default to 1.
    tab-width: 1
-  unused:
-    # treat code as a program (not a library) and report unused exported identifiers; default is false.
-    # XXX: if you enable this setting, unused will report a lot of false-positives in text editors:
-    # if it's called for subdir of a project it can't find funcs usages. All text editor integrations
-    # with golangci-lint call it on a directory with the changed file.
-    check-exported: false
  unparam:
    # Inspect exported functions, default is false. Set to true if no external program/library imports your code.
    # XXX: if you enable this setting, unparam will report a lot of false-positives in text editors:
@ -189,7 +164,6 @@ linters:
    - unconvert # Should turn back on soon
    - goconst # Slightly annoying, as it reports "issues" in SQL statements
  disable-all: false
-  presets:
  fast: false


@ -212,13 +186,6 @@ issues:
    - bin
    - docs

-  # List of regexps of issue texts to exclude, empty list by default.
-  # But independently from this option we use default exclude patterns,
-  # it can be disabled by `exclude-use-default: false`. To list all
-  # excluded by default patterns execute `golangci-lint run --help`
-  exclude:
-  # - abcdef
-
  # Excluding configuration per-path, per-linter, per-text and per-source
  exclude-rules:
    # Exclude some linters from running on tests files.
--- a/appservice/appservice.go
+++ b/appservice/appservice.go
@ -53,6 +53,7 @@ func NewInternalAPI(
 		if err := generateAppServiceAccount(userAPI, appservice, cfg.Global.ServerName); err != nil {
 			logrus.WithFields(logrus.Fields{
 				"appservice": appservice.ID,
+				"as_token":   appservice.ASToken,
 			}).WithError(err).Panicf("failed to generate bot account for appservice")
 		}
 	}
@ -92,12 +93,13 @@ func generateAppServiceAccount(
 	}
 	var devRes userapi.PerformDeviceCreationResponse
 	err = userAPI.PerformDeviceCreation(context.Background(), &userapi.PerformDeviceCreationRequest{
-		Localpart:          as.SenderLocalpart,
-		ServerName:         serverName,
-		AccessToken:        as.ASToken,
-		DeviceID:           &as.SenderLocalpart,
-		DeviceDisplayName:  &as.SenderLocalpart,
-		NoDeviceListUpdate: true,
+		Localpart:                           as.SenderLocalpart,
+		ServerName:                          serverName,
+		AccessToken:                         as.ASToken,
+		DeviceID:                            &as.SenderLocalpart,
+		DeviceDisplayName:                   &as.SenderLocalpart,
+		NoDeviceListUpdate:                  true,
+		AccessTokenUniqueConstraintDisabled: false,
 	}, &devRes)
 	return err
 }
--- a/appservice/appservice_test.go
+++ b/appservice/appservice_test.go
@ -139,7 +139,7 @@ func TestAppserviceInternalAPI(t *testing.T) {
 		as := &config.ApplicationService{
 			ID:              "someID",
 			URL:             srv.URL,
-			ASToken:         "",
+			ASToken:         util.RandomString(12),
 			HSToken:         "",
 			SenderLocalpart: "senderLocalPart",
 			NamespaceMap: map[string][]config.ApplicationServiceNamespace{
@ -233,7 +233,7 @@ func TestAppserviceInternalAPI_UnixSocket_Simple(t *testing.T) {
 	as := &config.ApplicationService{
 		ID:              "someID",
 		URL:             fmt.Sprintf("unix://%s", socket),
-		ASToken:         "",
+		ASToken:         util.RandomString(8),
 		HSToken:         "",
 		SenderLocalpart: "senderLocalPart",
 		NamespaceMap: map[string][]config.ApplicationServiceNamespace{
@ -377,7 +377,7 @@ func TestRoomserverConsumerOneInvite(t *testing.T) {
 		as := &config.ApplicationService{
 			ID:              "someID",
 			URL:             srv.URL,
-			ASToken:         "",
+			ASToken:         util.RandomString(8),
 			HSToken:         "",
 			SenderLocalpart: "senderLocalPart",
 			NamespaceMap: map[string][]config.ApplicationServiceNamespace{
@ -510,7 +510,7 @@ func TestOutputAppserviceEvent(t *testing.T) {
 		as := &config.ApplicationService{
 			ID:              "someID",
 			URL:             srv.URL,
-			ASToken:         "",
+			ASToken:         util.RandomString(8),
 			HSToken:         "",
 			SenderLocalpart: "senderLocalPart",
 			NamespaceMap: map[string][]config.ApplicationServiceNamespace{
--- a/clientapi/admin_test.go
+++ b/clientapi/admin_test.go
@ -1637,8 +1637,9 @@ func TestAdminUserDeviceRetrieveCreate(t *testing.T) {
 		t.Run("Retrieve device", func(t *testing.T) {
 			var deviceRes uapi.PerformDeviceCreationResponse
 			if err := userAPI.PerformDeviceCreation(ctx, &uapi.PerformDeviceCreationRequest{
-				Localpart:  alice.Localpart,
-				ServerName: cfg.Global.ServerName,
+				Localpart:                           alice.Localpart,
+				ServerName:                          cfg.Global.ServerName,
+				AccessTokenUniqueConstraintDisabled: true,
 			}, &deviceRes); err != nil {
 				t.Errorf("failed to create account: %s", err)
 			}
@ -1747,8 +1748,9 @@ func TestAdminUserDeviceDelete(t *testing.T) {
 		t.Run("Delete existing device", func(t *testing.T) {
 			var deviceRes uapi.PerformDeviceCreationResponse
 			if err := userAPI.PerformDeviceCreation(ctx, &uapi.PerformDeviceCreationRequest{
-				Localpart:  alice.Localpart,
-				ServerName: cfg.Global.ServerName,
+				Localpart:                           alice.Localpart,
+				ServerName:                          cfg.Global.ServerName,
+				AccessTokenUniqueConstraintDisabled: true,
 			}, &deviceRes); err != nil {
 				t.Errorf("failed to create account: %s", err)
 			}
@ -1844,8 +1846,9 @@ func TestAdminUserDevicesDelete(t *testing.T) {
 		t.Run("Delete existing user's devices", func(t *testing.T) {
 			var deviceRes uapi.PerformDeviceCreationResponse
 			if err := userAPI.PerformDeviceCreation(ctx, &uapi.PerformDeviceCreationRequest{
-				Localpart:  alice.Localpart,
-				ServerName: cfg.Global.ServerName,
+				Localpart:                           alice.Localpart,
+				ServerName:                          cfg.Global.ServerName,
+				AccessTokenUniqueConstraintDisabled: true,
 			}, &deviceRes); err != nil {
 				t.Errorf("failed to create account: %s", err)
 			}
--- a/clientapi/routing/admin.go
+++ b/clientapi/routing/admin.go
@ -584,14 +584,15 @@ func AdminUserDeviceRetrieveCreate(
 		if !userDeviceExists {
 			var rs userapi.PerformDeviceCreationResponse
 			if err = userAPI.PerformDeviceCreation(req.Context(), &userapi.PerformDeviceCreationRequest{
-				Localpart:          local,
-				ServerName:         domain,
-				DeviceID:           &payload.DeviceID,
-				DeviceDisplayName:  &deviceDisplayName,
-				IPAddr:             "",
-				UserAgent:          req.UserAgent(),
-				NoDeviceListUpdate: false,
-				FromRegistration:   false,
+				Localpart:                           local,
+				ServerName:                          domain,
+				DeviceID:                            &payload.DeviceID,
+				DeviceDisplayName:                   &deviceDisplayName,
+				IPAddr:                              "",
+				UserAgent:                           req.UserAgent(),
+				NoDeviceListUpdate:                  false,
+				FromRegistration:                    false,
+				AccessTokenUniqueConstraintDisabled: true,
 			}, &rs); err != nil {
 				logger.WithError(err).Error("PerformDeviceCreation")
 				return util.JSONResponse{
--- a/setup/mscs/msc3861/msc3861_user_verifier.go
+++ b/setup/mscs/msc3861/msc3861_user_verifier.go
@ -96,7 +96,8 @@ func (r *mscError) Error() string {

 // VerifyUserFromRequest authenticates the HTTP request, on success returns Device of the requester.
 func (m *MSC3861UserVerifier) VerifyUserFromRequest(req *http.Request) (*api.Device, *util.JSONResponse) {
-	util.GetLogger(req.Context()).Debug("MSC3861.VerifyUserFromRequest")
+	ctx := req.Context()
+	util.GetLogger(ctx).Debug("MSC3861.VerifyUserFromRequest")
 	// Try to find the Application Service user
 	token, err := auth.ExtractAccessToken(req)
 	if err != nil {
@ -105,8 +106,22 @@ func (m *MSC3861UserVerifier) VerifyUserFromRequest(req *http.Request) (*api.Dev
 			JSON: spec.MissingToken(err.Error()),
 		}
 	}
-	// TODO: try to get appservice user first. See https://github.com/element-hq/synapse/blob/develop/synapse/api/auth/msc3861_delegated.py#L273
-	userData, err := m.getUserByAccessToken(req.Context(), token)
+	if appServiceUserID := req.URL.Query().Get("user_id"); appServiceUserID != "" {
+		var res api.QueryAccessTokenResponse
+		err = m.userAPI.QueryAccessToken(ctx, &api.QueryAccessTokenRequest{
+			AccessToken:      token,
+			AppServiceUserID: appServiceUserID,
+		}, &res)
+		if err != nil {
+			util.GetLogger(ctx).WithError(err).Error("userAPI.QueryAccessToken failed")
+			return nil, &util.JSONResponse{
+				Code: http.StatusInternalServerError,
+				JSON: spec.InternalServerError{},
+			}
+		}
+	}
+
+	userData, err := m.getUserByAccessToken(ctx, token)
 	if err != nil {
 		switch e := err.(type) {
 		case (*mscError):
@ -306,11 +321,12 @@ func (m *MSC3861UserVerifier) getUserByAccessToken(ctx context.Context, token st
 		var rs api.PerformDeviceCreationResponse
 		deviceDisplayName := "OIDC-native client"
 		if err := m.userAPI.PerformDeviceCreation(ctx, &api.PerformDeviceCreationRequest{
-			Localpart:         localpart,
-			ServerName:        m.serverName,
-			AccessToken:       "",
-			DeviceID:          &deviceID,
-			DeviceDisplayName: &deviceDisplayName,
+			Localpart:                           localpart,
+			ServerName:                          m.serverName,
+			AccessToken:                         "",
+			DeviceID:                            &deviceID,
+			DeviceDisplayName:                   &deviceDisplayName,
+			AccessTokenUniqueConstraintDisabled: true,
 			// TODO: Cannot add IPAddr and Useragent values here. Should we care about it here?
 		}, &rs); err != nil {
 			logger.WithError(err).Error("PerformDeviceCreation")
--- a/userapi/api/api.go
+++ b/userapi/api/api.go
@ -381,6 +381,11 @@ type PerformDeviceCreationRequest struct {
 	// FromRegistration determines if this request comes from registering a new account
 	// and is in most cases false.
 	FromRegistration bool
+
+	// AccessTokenUniqueConstraintDisabled determines if unique constraint is applicable for the AccessToken.
+	// It is false if an external auth service is in use (e.g. MAS) and server does not generate its own
+	// auth tokens. Otherwise, if traditional login is in use, the value is true. Default is false.
+	AccessTokenUniqueConstraintDisabled bool
 }

 // PerformDeviceCreationResponse is the response for PerformDeviceCreation
--- a/userapi/internal/user_api.go
+++ b/userapi/internal/user_api.go
@ -306,8 +306,15 @@ func (a *UserInternalAPI) PerformDeviceCreation(ctx context.Context, req *api.Pe
 		"device_id":    req.DeviceID,
 		"display_name": req.DeviceDisplayName,
 	}).Info("PerformDeviceCreation")
-	// TODO: Since we have deleted access_token's unique constraint from the db,
-	// we probably should check its uniqueness if msc3861 is disabled
+	if !req.AccessTokenUniqueConstraintDisabled {
+		dev, err := a.DB.GetDeviceByAccessToken(ctx, req.AccessToken)
+		if err != nil && !errors.Is(err, sql.ErrNoRows) {
+			return err
+		}
+		if dev.UserID != "" {
+			return errors.New("unique constraint violation. Access token is not unique" + dev.AccessToken)
+		}
+	}
 	dev, err := a.DB.CreateDevice(ctx, req.Localpart, serverName, req.DeviceID, req.AccessToken, req.DeviceDisplayName, req.IPAddr, req.UserAgent)
 	if err != nil {
 		return err
--- a/userapi/userapi_test.go
+++ b/userapi/userapi_test.go
@ -445,6 +445,8 @@ func TestAccountData(t *testing.T) {
 func TestDevices(t *testing.T) {
 	ctx := context.Background()

+	dupeAccessToken := util.RandomString(8)
+
 	displayName := "testing"

 	creationTests := []struct {
@ -455,25 +457,42 @@ func TestDevices(t *testing.T) {
 	}{
 		{
 			name:      "not a local user",
-			inputData: &api.PerformDeviceCreationRequest{Localpart: "test1", ServerName: "notlocal"},
+			inputData: &api.PerformDeviceCreationRequest{Localpart: "test1", ServerName: "notlocal", AccessTokenUniqueConstraintDisabled: true},
 			wantErr:   true,
 		},
 		{
 			name:      "implicit local user",
-			inputData: &api.PerformDeviceCreationRequest{Localpart: "test1", AccessToken: util.RandomString(8), NoDeviceListUpdate: true, DeviceDisplayName: &displayName},
+			inputData: &api.PerformDeviceCreationRequest{Localpart: "test1", AccessToken: util.RandomString(8), NoDeviceListUpdate: true, DeviceDisplayName: &displayName, AccessTokenUniqueConstraintDisabled: true},
 		},
 		{
 			name:      "explicit local user",
-			inputData: &api.PerformDeviceCreationRequest{Localpart: "test2", ServerName: "test", AccessToken: util.RandomString(8), NoDeviceListUpdate: true},
+			inputData: &api.PerformDeviceCreationRequest{Localpart: "test2", ServerName: "test", AccessToken: util.RandomString(8), NoDeviceListUpdate: true, AccessTokenUniqueConstraintDisabled: true},
 		},
 		{
 			name:      "test3 second device", // used to test deletion later
-			inputData: &api.PerformDeviceCreationRequest{Localpart: "test3", ServerName: "test", AccessToken: util.RandomString(8), NoDeviceListUpdate: true},
+			inputData: &api.PerformDeviceCreationRequest{Localpart: "test3", ServerName: "test", AccessToken: util.RandomString(8), NoDeviceListUpdate: true, AccessTokenUniqueConstraintDisabled: true},
 		},
 		{
 			name:         "test3 third device", // used to test deletion later
 			wantNewDevID: true,
-			inputData:    &api.PerformDeviceCreationRequest{Localpart: "test3", ServerName: "test", AccessToken: util.RandomString(8), NoDeviceListUpdate: true},
+			inputData:    &api.PerformDeviceCreationRequest{Localpart: "test3", ServerName: "test", AccessToken: util.RandomString(8), NoDeviceListUpdate: true, AccessTokenUniqueConstraintDisabled: true},
+		},
+		{
+			name:      "dupe token - ok (unique constraint enabled)",
+			inputData: &api.PerformDeviceCreationRequest{Localpart: "test3", ServerName: "test", AccessToken: dupeAccessToken, NoDeviceListUpdate: true, AccessTokenUniqueConstraintDisabled: false},
+		},
+		{
+			name:      "dupe token - not ok (unique constraint enabled)",
+			inputData: &api.PerformDeviceCreationRequest{Localpart: "test3", ServerName: "test", AccessToken: dupeAccessToken, NoDeviceListUpdate: true, AccessTokenUniqueConstraintDisabled: false},
+			wantErr:   true,
+		},
+		{
+			name:      "dupe token - ok (unique constraint disabled)",
+			inputData: &api.PerformDeviceCreationRequest{Localpart: "test3", ServerName: "test", AccessToken: dupeAccessToken, NoDeviceListUpdate: true, AccessTokenUniqueConstraintDisabled: true},
+		},
+		{
+			name:      "dupe token - not ok (unique constraint disabled)",
+			inputData: &api.PerformDeviceCreationRequest{Localpart: "test3", ServerName: "test", AccessToken: dupeAccessToken, NoDeviceListUpdate: true, AccessTokenUniqueConstraintDisabled: true},
 		},
 	}

@ -618,7 +637,13 @@ func TestDeviceIDReuse(t *testing.T) {
 		res := api.PerformDeviceCreationResponse{}
 		// create a first device
 		deviceID := util.RandomString(8)
-		req := api.PerformDeviceCreationRequest{Localpart: "alice", ServerName: "test", DeviceID: &deviceID, NoDeviceListUpdate: true}
+		req := api.PerformDeviceCreationRequest{
+			Localpart:                           "alice",
+			ServerName:                          "test",
+			DeviceID:                            &deviceID,
+			NoDeviceListUpdate:                  true,
+			AccessTokenUniqueConstraintDisabled: true,
+		}
 		err := intAPI.PerformDeviceCreation(ctx, &req, &res)
 		if err != nil {
 			t.Fatal(err)