From ae19db60e3c9c2907e2a6b293c2ce276a48466c6 Mon Sep 17 00:00:00 2001 From: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com> Date: Fri, 6 Jul 2018 03:28:49 -0700 Subject: [PATCH] Check userID instead of username from application services (#500) * Check UserID instead of username from AS's. Tests. * add tests to validateApplicationService * Use some literals, organize URLs & checks * Fix error messages and incorrect test --- .../dendrite/clientapi/routing/register.go | 29 ++++--- .../clientapi/routing/register_test.go | 85 +++++++++++++++++++ 2 files changed, 101 insertions(+), 13 deletions(-) diff --git a/src/github.com/matrix-org/dendrite/clientapi/routing/register.go b/src/github.com/matrix-org/dendrite/clientapi/routing/register.go index fc3a57cc..76a696f9 100644 --- a/src/github.com/matrix-org/dendrite/clientapi/routing/register.go +++ b/src/github.com/matrix-org/dendrite/clientapi/routing/register.go @@ -39,6 +39,7 @@ import ( "github.com/matrix-org/dendrite/clientapi/auth/storage/devices" "github.com/matrix-org/dendrite/clientapi/httputil" "github.com/matrix-org/dendrite/clientapi/jsonerror" + "github.com/matrix-org/dendrite/clientapi/userutil" "github.com/matrix-org/gomatrixserverlib" "github.com/matrix-org/util" "github.com/prometheus/client_golang/prometheus" @@ -186,12 +187,12 @@ func validateUsername(username string) *util.JSONResponse { } else if !validUsernameRegex.MatchString(username) { return &util.JSONResponse{ Code: http.StatusBadRequest, - JSON: jsonerror.InvalidUsername("User ID can only contain characters a-z, 0-9, or '_-./'"), + JSON: jsonerror.InvalidUsername("Username can only contain characters a-z, 0-9, or '_-./'"), } } else if username[0] == '_' { // Regex checks its not a zero length string return &util.JSONResponse{ Code: http.StatusBadRequest, - JSON: jsonerror.InvalidUsername("User ID cannot start with a '_'"), + JSON: jsonerror.InvalidUsername("Username cannot start with a '_'"), } } return nil @@ -207,7 +208,7 @@ func validateApplicationServiceUsername(username string) *util.JSONResponse { } else if !validUsernameRegex.MatchString(username) { return &util.JSONResponse{ Code: http.StatusBadRequest, - JSON: jsonerror.InvalidUsername("User ID can only contain characters a-z, 0-9, or '_-./'"), + JSON: jsonerror.InvalidUsername("Username can only contain characters a-z, 0-9, or '_-./'"), } } return nil @@ -296,20 +297,20 @@ func validateRecaptcha( return nil } -// UsernameIsWithinApplicationServiceNamespace checks to see if a username falls -// within any of the namespaces of a given application service. If no -// application service is given, it will check to see if it matches any -// application service's namespace. -func UsernameIsWithinApplicationServiceNamespace( +// UserIDIsWithinApplicationServiceNamespace checks to see if a given userID +// falls within any of the namespaces of a given Application Service. If no +// Application Service is given, it will check to see if it matches any +// Application Service's namespace. +func UserIDIsWithinApplicationServiceNamespace( cfg *config.Dendrite, - username string, + userID string, appservice *config.ApplicationService, ) bool { if appservice != nil { // Loop through given application service's namespaces and see if any match for _, namespace := range appservice.NamespaceMap["users"] { // AS namespaces are checked for validity in config - if namespace.RegexpObject.MatchString(username) { + if namespace.RegexpObject.MatchString(userID) { return true } } @@ -320,7 +321,7 @@ func UsernameIsWithinApplicationServiceNamespace( for _, knownAppservice := range cfg.Derived.ApplicationServices { for _, namespace := range knownAppservice.NamespaceMap["users"] { // AS namespaces are checked for validity in config - if namespace.RegexpObject.MatchString(username) { + if namespace.RegexpObject.MatchString(userID) { return true } } @@ -375,8 +376,10 @@ func validateApplicationService( } } + userID := userutil.MakeUserID(username, cfg.Matrix.ServerName) + // Ensure the desired username is within at least one of the application service's namespaces. - if !UsernameIsWithinApplicationServiceNamespace(cfg, username, matchedApplicationService) { + if !UserIDIsWithinApplicationServiceNamespace(cfg, userID, matchedApplicationService) { // If we didn't find any matches, return M_EXCLUSIVE return "", &util.JSONResponse{ Code: http.StatusBadRequest, @@ -386,7 +389,7 @@ func validateApplicationService( } // Check this user does not fit multiple application service namespaces - if UsernameMatchesMultipleExclusiveNamespaces(cfg, username) { + if UsernameMatchesMultipleExclusiveNamespaces(cfg, userID) { return "", &util.JSONResponse{ Code: http.StatusBadRequest, JSON: jsonerror.ASExclusive(fmt.Sprintf( diff --git a/src/github.com/matrix-org/dendrite/clientapi/routing/register_test.go b/src/github.com/matrix-org/dendrite/clientapi/routing/register_test.go index f141b849..fbf140c2 100644 --- a/src/github.com/matrix-org/dendrite/clientapi/routing/register_test.go +++ b/src/github.com/matrix-org/dendrite/clientapi/routing/register_test.go @@ -15,9 +15,13 @@ package routing import ( + "net/http" + "net/url" + "regexp" "testing" "github.com/matrix-org/dendrite/clientapi/auth/authtypes" + "github.com/matrix-org/dendrite/common/config" ) var ( @@ -145,3 +149,84 @@ func TestEmptyCompletedFlows(t *testing.T) { t.Error("Empty Completed Flow Stages should be a empty slice: returned ", ret, ". Should be []") } } + +// This method tests validation of the provided Application Service token and +// username that they're registering +func TestValidationOfApplicationServices(t *testing.T) { + // Set up application service namespaces + regex := "@_appservice_.*" + regexp, err := regexp.Compile(regex) + if err != nil { + t.Errorf("Error compiling regex: %s", regex) + } + + fakeNamespace := config.ApplicationServiceNamespace{ + Exclusive: true, + Regex: regex, + RegexpObject: regexp, + } + + // Create a fake application service + fakeID := "FakeAS" + fakeSenderLocalpart := "_appservice_bot" + fakeApplicationService := config.ApplicationService{ + ID: fakeID, + URL: "null", + ASToken: "1234", + HSToken: "4321", + SenderLocalpart: fakeSenderLocalpart, + NamespaceMap: map[string][]config.ApplicationServiceNamespace{ + "users": {fakeNamespace}, + }, + } + + // Set up a config + fakeConfig := config.Dendrite{} + fakeConfig.Matrix.ServerName = "localhost" + fakeConfig.Derived.ApplicationServices = []config.ApplicationService{fakeApplicationService} + + // Access token is correct, user_id omitted so we are acting as SenderLocalpart + URL, _ := url.Parse("http://localhost/register?access_token=1234") + fakeHTTPRequest := http.Request{ + Method: "POST", + URL: URL, + } + asID, resp := validateApplicationService(&fakeConfig, &fakeHTTPRequest, fakeSenderLocalpart) + if resp != nil || asID != fakeID { + t.Errorf("appservice should have validated and returned correct ID: %s", resp.JSON) + } + + // Access token is incorrect, user_id omitted so we are acting as SenderLocalpart + URL, _ = url.Parse("http://localhost/register?access_token=xxxx") + fakeHTTPRequest = http.Request{ + Method: "POST", + URL: URL, + } + asID, resp = validateApplicationService(&fakeConfig, &fakeHTTPRequest, fakeSenderLocalpart) + if resp == nil || asID == fakeID { + t.Errorf("access_token should have been marked as invalid") + } + + // Access token is correct, acting as valid user_id + URL, _ = url.Parse("http://localhost/register?access_token=1234&user_id=@_appservice_bob:localhost") + fakeHTTPRequest = http.Request{ + Method: "POST", + URL: URL, + } + asID, resp = validateApplicationService(&fakeConfig, &fakeHTTPRequest, "_appservice_bob") + if resp != nil || asID != fakeID { + t.Errorf("access_token and user_id should've been valid: %s", resp.JSON) + } + + // Access token is correct, acting as invalid user_id + URL, _ = url.Parse("http://localhost/register?access_token=1234&user_id=@_something_else:localhost") + fakeHTTPRequest = http.Request{ + Method: "POST", + URL: URL, + } + asID, resp = validateApplicationService(&fakeConfig, &fakeHTTPRequest, "_something_else") + if resp == nil || asID == fakeID { + t.Errorf("user_id should not have been valid: %s", + fakeHTTPRequest.URL.Query().Get("user_id")) + } +}