Refactor Deployment Register/Login functions (#673)

* Refactor Deployment Register/Login functions

Define just 2 functions which can be used universally. By having
this as an interface, this also means we keep the door open for
matrix-authentication-service to swap out the impl for one which
registers with MAS.

* Fix up localpart calcs

helpers/clientopts.go

@@ -0,0 +1,12 @@
+package helpers
+
+type RegistrationOpts struct {
+	Localpart string // default '' (don't care)
+	DeviceID  string // default '' (generate new)
+	Password  string // default 'complement_meets_min_password_requirement'
+	IsAdmin   bool   // default false
+}
+
+type LoginOpts struct {
+	Password string // default 'complement_meets_min_password_requirement'
+}

internal/docker/deployment.go

@@ -1,13 +1,17 @@
 package docker
 
 import (
+	"fmt"
 	"net/http"
 	"sync"
+	"sync/atomic"
 	"testing"
 	"time"
 
 	"github.com/matrix-org/complement/client"
+	"github.com/matrix-org/complement/helpers"
 	"github.com/matrix-org/complement/internal/config"
+	"github.com/matrix-org/gomatrixserverlib"
 )
 
 // Deployment is the complete instantiation of a Blueprint, with running containers
@@ -18,8 +22,9 @@ type Deployment struct {
 	// The name of the deployed blueprint
 	BlueprintName string
 	// A map of HS name to a HomeserverDeployment
-	HS     map[string]*HomeserverDeployment
-	Config *config.Complement
+	HS               map[string]*HomeserverDeployment
+	Config           *config.Complement
+	localpartCounter atomic.Int64
 }
 
 // HomeserverDeployment represents a running homeserver in a container.
@@ -60,31 +65,13 @@ func (d *Deployment) RoundTripper() http.RoundTripper {
 	return &RoundTripper{Deployment: d}
 }
 
-// Client returns a CSAPI client targeting the given hsName, using the access token for the given userID.
-// Fails the test if the hsName is not found. Returns an unauthenticated client if userID is "", fails the test
-// if the userID is otherwise not found.
-func (d *Deployment) Client(t *testing.T, hsName, userID string) *client.CSAPI {
-	t.Helper()
+func (d *Deployment) Register(t *testing.T, hsName string, opts helpers.RegistrationOpts) *client.CSAPI {
 	dep, ok := d.HS[hsName]
 	if !ok {
-		t.Fatalf("Deployment.Client - HS name '%s' not found", hsName)
+		t.Fatalf("Deployment.Register - HS name '%s' not found", hsName)
 		return nil
 	}
-	dep.accessTokensMutex.RLock()
-	token := dep.AccessTokens[userID]
-	dep.accessTokensMutex.RUnlock()
-	if token == "" && userID != "" {
-		t.Fatalf("Deployment.Client - HS name '%s' - user ID '%s' not found", hsName, userID)
-		return nil
-	}
-	deviceID := dep.DeviceIDs[userID]
-	if deviceID == "" && userID != "" {
-		t.Logf("WARNING: Deployment.Client - HS name '%s' - user ID '%s' - deviceID not found", hsName, userID)
-	}
 	client := &client.CSAPI{
-		UserID:           userID,
-		AccessToken:      token,
-		DeviceID:         deviceID,
 		BaseURL:          dep.BaseURL,
 		Client:           client.NewLoggedClient(t, hsName, nil),
 		SyncUntilTimeout: 5 * time.Second,
@@ -94,25 +81,43 @@ func (d *Deployment) Client(t *testing.T, hsName, userID string) *client.CSAPI {
 	dep.CSAPIClientsMutex.Lock()
 	dep.CSAPIClients = append(dep.CSAPIClients, client)
 	dep.CSAPIClientsMutex.Unlock()
+	password := opts.Password
+	if password == "" {
+		password = "complement_meets_min_password_req"
+	}
+	localpart := opts.Localpart
+	if localpart == "" {
+		localpart = fmt.Sprintf("user-%v", d.localpartCounter.Add(1))
+	}
+	var userID, accessToken, deviceID string
+	if opts.IsAdmin {
+		userID, accessToken, deviceID = client.RegisterSharedSecret(t, localpart, password, opts.IsAdmin)
+	} else {
+		userID, accessToken, deviceID = client.RegisterUser(t, localpart, password)
+	}
 
-	return client
-}
+	// remember the token so subsequent calls to deployment.Client return the user
+	dep.accessTokensMutex.Lock()
+	dep.AccessTokens[userID] = accessToken
+	dep.accessTokensMutex.Unlock()
 
-// NewUser creates a new user as a convenience method to RegisterUser.
-//
-// It registers the user with a deterministic password, and without admin privileges.
-func (d *Deployment) NewUser(t *testing.T, localpart, hs string) *client.CSAPI {
-	return d.RegisterUser(t, hs, localpart, "complement_meets_min_pasword_req_"+localpart, false)
+	client.UserID = userID
+	client.AccessToken = accessToken
+	client.DeviceID = deviceID
+	return client
 }
 
-// RegisterUser within a homeserver and return an authenticatedClient, Fails the test if the hsName is not found.
-func (d *Deployment) RegisterUser(t *testing.T, hsName, localpart, password string, isAdmin bool) *client.CSAPI {
+func (d *Deployment) Login(t *testing.T, hsName string, existing *client.CSAPI, opts helpers.LoginOpts) *client.CSAPI {
 	t.Helper()
 	dep, ok := d.HS[hsName]
 	if !ok {
-		t.Fatalf("Deployment.Client - HS name '%s' not found", hsName)
+		t.Fatalf("Deployment.Login: HS name '%s' not found", hsName)
 		return nil
 	}
+	localpart, _, err := gomatrixserverlib.SplitID('@', existing.UserID)
+	if err != nil {
+		t.Fatalf("Deployment.Login: existing CSAPI client has invalid user ID '%s', cannot login as this user: %s", existing.UserID, err)
+	}
 	client := &client.CSAPI{
 		BaseURL:          dep.BaseURL,
 		Client:           client.NewLoggedClient(t, hsName, nil),
@@ -123,34 +128,39 @@ func (d *Deployment) RegisterUser(t *testing.T, hsName, localpart, password stri
 	dep.CSAPIClientsMutex.Lock()
 	dep.CSAPIClients = append(dep.CSAPIClients, client)
 	dep.CSAPIClientsMutex.Unlock()
-	var userID, accessToken, deviceID string
-	if isAdmin {
-		userID, accessToken, deviceID = client.RegisterSharedSecret(t, localpart, password, isAdmin)
-	} else {
-		userID, accessToken, deviceID = client.RegisterUser(t, localpart, password)
-	}
-
-	// remember the token so subsequent calls to deployment.Client return the user
-	dep.accessTokensMutex.Lock()
-	dep.AccessTokens[userID] = accessToken
-	dep.accessTokensMutex.Unlock()
+	userID, accessToken, deviceID := client.LoginUser(t, localpart, opts.Password)
 
 	client.UserID = userID
 	client.AccessToken = accessToken
 	client.DeviceID = deviceID
 	return client
 }
 
-// LoginUser within a homeserver and return an authenticatedClient. Fails the test if the hsName is not found.
-// Note that this will not change the access token of the client that is returned by `deployment.Client`.
-func (d *Deployment) LoginUser(t *testing.T, hsName, localpart, password string) *client.CSAPI {
+// Client returns a CSAPI client targeting the given hsName, using the access token for the given userID.
+// Fails the test if the hsName is not found. Returns an unauthenticated client if userID is "", fails the test
+// if the userID is otherwise not found.
+func (d *Deployment) Client(t *testing.T, hsName, userID string) *client.CSAPI {
 	t.Helper()
 	dep, ok := d.HS[hsName]
 	if !ok {
 		t.Fatalf("Deployment.Client - HS name '%s' not found", hsName)
 		return nil
 	}
+	dep.accessTokensMutex.RLock()
+	token := dep.AccessTokens[userID]
+	dep.accessTokensMutex.RUnlock()
+	if token == "" && userID != "" {
+		t.Fatalf("Deployment.Client - HS name '%s' - user ID '%s' not found", hsName, userID)
+		return nil
+	}
+	deviceID := dep.DeviceIDs[userID]
+	if deviceID == "" && userID != "" {
+		t.Logf("WARNING: Deployment.Client - HS name '%s' - user ID '%s' - deviceID not found", hsName, userID)
+	}
 	client := &client.CSAPI{
+		UserID:           userID,
+		AccessToken:      token,
+		DeviceID:         deviceID,
 		BaseURL:          dep.BaseURL,
 		Client:           client.NewLoggedClient(t, hsName, nil),
 		SyncUntilTimeout: 5 * time.Second,
@@ -160,11 +170,7 @@ func (d *Deployment) LoginUser(t *testing.T, hsName, localpart, password string)
 	dep.CSAPIClientsMutex.Lock()
 	dep.CSAPIClients = append(dep.CSAPIClients, client)
 	dep.CSAPIClientsMutex.Unlock()
-	userID, accessToken, deviceID := client.LoginUser(t, localpart, password)
 
-	client.UserID = userID
-	client.AccessToken = accessToken
-	client.DeviceID = deviceID
 	return client
 }
 

test_package.go

@@ -11,6 +11,7 @@ import (
 
 	"github.com/matrix-org/complement/b"
 	"github.com/matrix-org/complement/client"
+	"github.com/matrix-org/complement/helpers"
 	"github.com/matrix-org/complement/internal/config"
 	"github.com/matrix-org/complement/internal/docker"
 	"github.com/sirupsen/logrus"
@@ -22,16 +23,11 @@ type Deployment interface {
 	// Fails the test if the hsName is not found. Returns an unauthenticated client if userID is "", fails the test
 	// if the userID is otherwise not found.
 	Client(t *testing.T, serverName, userID string) *client.CSAPI
-	// RegisterUser within a homeserver and return an authenticatedClient, Fails the test if the hsName is not found.
-	RegisterUser(t *testing.T, hsName, localpart, password string, isAdmin bool) *client.CSAPI
-	// NewUser creates a new user as a convenience method to RegisterUser. TODO REMOVE
-	//
-	// It registers the user with a deterministic password, and without admin privileges.
-	NewUser(t *testing.T, localpart, hs string) *client.CSAPI
-	// TODO remove this, only used in 1 test in msc3890
-	// LoginUser within a homeserver and return an authenticatedClient. Fails the test if the hsName is not found.
-	// Note that this will not change the access token of the client that is returned by `deployment.Client`.
-	LoginUser(t *testing.T, hsName, localpart, password string) *client.CSAPI
+	// Register a new user on the given server.
+	Register(t *testing.T, hsName string, opts helpers.RegistrationOpts) *client.CSAPI
+	// Login to an existing user account on the given server. In order to make tests not hardcode full user IDs,
+	// an existing logged in client must be supplied.
+	Login(t *testing.T, hsName string, existing *client.CSAPI, opts helpers.LoginOpts) *client.CSAPI
 	// Restart a deployment.
 	Restart(t *testing.T) error
 	// Destroy the entire deployment. Destroys all running containers. If `printServerLogs` is true,

tests/csapi/account_change_password_pushers_test.go

@@ -9,6 +9,7 @@ import (
 	"github.com/matrix-org/complement"
 	"github.com/matrix-org/complement/b"
 	"github.com/matrix-org/complement/client"
+	"github.com/matrix-org/complement/helpers"
 	"github.com/matrix-org/complement/match"
 	"github.com/matrix-org/complement/must"
 
@@ -20,7 +21,10 @@ func TestChangePasswordPushers(t *testing.T) {
 	defer deployment.Destroy(t)
 	password1 := "superuser"
 	password2 := "my_new_password"
-	passwordClient := deployment.RegisterUser(t, "hs1", "test_change_password_pusher_user", password1, false)
+	passwordClient := deployment.Register(t, "hs1", helpers.RegistrationOpts{
+		Localpart: "test_change_password_pusher_user",
+		Password:  password1,
+	})
 
 	// sytest: Pushers created with a different access token are deleted on password change
 	t.Run("Pushers created with a different access token are deleted on password change", func(t *testing.T) {

tests/csapi/account_change_password_test.go

@@ -7,6 +7,7 @@ import (
 	"github.com/matrix-org/complement"
 	"github.com/matrix-org/complement/b"
 	"github.com/matrix-org/complement/client"
+	"github.com/matrix-org/complement/helpers"
 	"github.com/matrix-org/complement/match"
 	"github.com/matrix-org/complement/must"
 
@@ -18,7 +19,10 @@ func TestChangePassword(t *testing.T) {
 	defer deployment.Destroy(t)
 	password1 := "superuser"
 	password2 := "my_new_password"
-	passwordClient := deployment.RegisterUser(t, "hs1", "test_change_password_user", password1, false)
+	passwordClient := deployment.Register(t, "hs1", helpers.RegistrationOpts{
+		Localpart: "test_change_password_user",
+		Password:  password1,
+	})
 	unauthedClient := deployment.Client(t, "hs1", "")
 	_, sessionTest := createSession(t, deployment, "test_change_password_user", "superuser")
 	// sytest: After changing password, can't log in with old password

tests/csapi/account_deactivate_test.go

@@ -9,6 +9,7 @@ import (
 	"github.com/matrix-org/complement"
 	"github.com/matrix-org/complement/b"
 	"github.com/matrix-org/complement/client"
+	"github.com/matrix-org/complement/helpers"
 	"github.com/matrix-org/complement/match"
 	"github.com/matrix-org/complement/must"
 )
@@ -17,7 +18,10 @@ func TestDeactivateAccount(t *testing.T) {
 	deployment := complement.Deploy(t, b.BlueprintAlice)
 	defer deployment.Destroy(t)
 	password := "superuser"
-	authedClient := deployment.RegisterUser(t, "hs1", "test_deactivate_user", password, false)
+	authedClient := deployment.Register(t, "hs1", helpers.RegistrationOpts{
+		Localpart: "test_deactivate_user",
+		Password:  password,
+	})
 	unauthedClient := deployment.Client(t, "hs1", "")
 
 	// Ensure that the first step, in which the client queries the server's user-interactive auth flows, returns

tests/csapi/admin_test.go

@@ -10,6 +10,7 @@ import (
 	"github.com/matrix-org/complement"
 	"github.com/matrix-org/complement/b"
 	"github.com/matrix-org/complement/client"
+	"github.com/matrix-org/complement/helpers"
 	"github.com/matrix-org/complement/match"
 	"github.com/matrix-org/complement/must"
 )
@@ -19,14 +20,22 @@ import (
 func TestCanRegisterAdmin(t *testing.T) {
 	deployment := complement.Deploy(t, b.BlueprintAlice)
 	defer deployment.Destroy(t)
-	deployment.RegisterUser(t, "hs1", "admin", "adminpassword", true)
+	deployment.Register(t, "hs1", helpers.RegistrationOpts{
+		Localpart: "admin",
+		Password:  "adminpassword",
+		IsAdmin:   true,
+	})
 }
 
 // Test if the implemented /_synapse/admin/v1/send_server_notice behaves as expected
 func TestServerNotices(t *testing.T) {
 	deployment := complement.Deploy(t, b.BlueprintAlice)
 	defer deployment.Destroy(t)
-	admin := deployment.RegisterUser(t, "hs1", "admin", "adminpassword", true)
+	admin := deployment.Register(t, "hs1", helpers.RegistrationOpts{
+		Localpart: "admin",
+		Password:  "adminpassword",
+		IsAdmin:   true,
+	})
 	alice := deployment.Client(t, "hs1", "@alice:hs1")
 
 	reqBody := client.WithJSONBody(t, map[string]interface{}{

tests/csapi/apidoc_device_management_test.go

@@ -8,6 +8,7 @@ import (
 	"github.com/matrix-org/complement"
 	"github.com/matrix-org/complement/b"
 	"github.com/matrix-org/complement/client"
+	"github.com/matrix-org/complement/helpers"
 	"github.com/matrix-org/complement/match"
 	"github.com/matrix-org/complement/must"
 )
@@ -16,7 +17,10 @@ func TestDeviceManagement(t *testing.T) {
 	deployment := complement.Deploy(t, b.BlueprintAlice)
 	defer deployment.Destroy(t)
 	unauthedClient := deployment.Client(t, "hs1", "")
-	authedClient := deployment.RegisterUser(t, "hs1", "test_device_management_user", "superuser", false)
+	authedClient := deployment.Register(t, "hs1", helpers.RegistrationOpts{
+		Localpart: "test_device_management_user",
+		Password:  "superuser",
+	})
 
 	// sytest: GET /device/{deviceId}
 	t.Run("GET /device/{deviceId}", func(t *testing.T) {
@@ -194,7 +198,10 @@ func TestDeviceManagement(t *testing.T) {
 	})
 	// sytest: DELETE /device/{deviceId} requires UI auth user to match device owner
 	t.Run("DELETE /device/{deviceId} requires UI auth user to match device owner", func(t *testing.T) {
-		bob := deployment.RegisterUser(t, "hs1", "bob", "bobspassword", false)
+		bob := deployment.Register(t, "hs1", helpers.RegistrationOpts{
+			Localpart: "bob",
+			Password:  "bobspassword",
+		})
 
 		newDeviceID, session2 := createSession(t, deployment, authedClient.UserID, "superuser")
 		session2.MustSync(t, client.SyncReq{})

tests/csapi/apidoc_login_test.go

@@ -10,6 +10,7 @@ import (
 	"github.com/matrix-org/complement"
 	"github.com/matrix-org/complement/b"
 	"github.com/matrix-org/complement/client"
+	"github.com/matrix-org/complement/helpers"
 	"github.com/matrix-org/complement/match"
 	"github.com/matrix-org/complement/must"
 )
@@ -18,7 +19,10 @@ func TestLogin(t *testing.T) {
 	deployment := complement.Deploy(t, b.BlueprintAlice)
 	defer deployment.Destroy(t)
 	unauthedClient := deployment.Client(t, "hs1", "")
-	_ = deployment.RegisterUser(t, "hs1", "test_login_user", "superuser", false)
+	deployment.Register(t, "hs1", helpers.RegistrationOpts{
+		Localpart: "test_login_user",
+		Password:  "superuser",
+	})
 	t.Run("parallel", func(t *testing.T) {
 		// sytest: GET /login yields a set of flows
 		t.Run("GET /login yields a set of flows", func(t *testing.T) {

tests/csapi/apidoc_logout_test.go

@@ -9,6 +9,7 @@ import (
 
 	"github.com/matrix-org/complement"
 	"github.com/matrix-org/complement/b"
+	"github.com/matrix-org/complement/helpers"
 	"github.com/matrix-org/complement/match"
 	"github.com/matrix-org/complement/must"
 )
@@ -18,7 +19,10 @@ func TestLogout(t *testing.T) {
 	defer deployment.Destroy(t)
 
 	password := "superuser"
-	verifyClientUser := deployment.RegisterUser(t, "hs1", "testuser", password, false)
+	verifyClientUser := deployment.Register(t, "hs1", helpers.RegistrationOpts{
+		Localpart: "testuser",
+		Password:  password,
+	})
 
 	// sytest: Can logout current device
 	t.Run("Can logout current device", func(t *testing.T) {