From 8ff136e595af05c8e844d2f9bf23831a9a9eddc0 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Tue, 6 Nov 2018 14:40:37 +0000 Subject: [PATCH] Fix interactive registration failing (#583) * Fix interactive registration failing because of being confused with AS registration * Fix AS registration tests * Move AS registration handling to dedicated function and split the switch/case to avoid unnecessary condition * Ignore handleRegistrationFlow() for gocyclo and add some doc/comments on the code --- .../dendrite/clientapi/routing/register.go | 88 ++++++++++++++----- .../clientapi/routing/register_test.go | 33 ++----- 2 files changed, 72 insertions(+), 49 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 d3797634..b1522e82 100644 --- a/src/github.com/matrix-org/dendrite/clientapi/routing/register.go +++ b/src/github.com/matrix-org/dendrite/clientapi/routing/register.go @@ -369,18 +369,11 @@ func UsernameMatchesExclusiveNamespaces( // two requirements are met, no error will be returned. func validateApplicationService( cfg *config.Dendrite, - req *http.Request, username string, + accessToken string, ) (string, *util.JSONResponse) { // Check if the token if the application service is valid with one we have // registered in the config. - accessToken, err := auth.ExtractAccessToken(req) - if err != nil { - return "", &util.JSONResponse{ - Code: http.StatusUnauthorized, - JSON: jsonerror.MissingToken(err.Error()), - } - } var matchedApplicationService *config.ApplicationService for _, appservice := range cfg.Derived.ApplicationServices { if appservice.ASToken == accessToken { @@ -497,6 +490,7 @@ func Register( // handleRegistrationFlow will direct and complete registration flow stages // that the client has requested. +// nolint: gocyclo func handleRegistrationFlow( req *http.Request, r registerRequest, @@ -542,21 +536,29 @@ func handleRegistrationFlow( // Add SharedSecret to the list of completed registration stages sessions.AddCompletedStage(sessionID, authtypes.LoginTypeSharedSecret) - case "", authtypes.LoginTypeApplicationService: - // not passing a Auth.Type is allowed for ApplicationServices. So assume that as well - // Check application service register user request is valid. - // The application service's ID is returned if so. - appserviceID, err := validateApplicationService(cfg, req, r.Username) - if err != nil { - return *err + case "": + // Extract the access token from the request, if there's one to extract + // (which we can know by checking whether the error is nil or not). + accessToken, err := auth.ExtractAccessToken(req) + + // A missing auth type can mean either the registration is performed by + // an AS or the request is made as the first step of a registration + // using the User-Interactive Authentication API. This can be determined + // by whether the request contains an access token. + if err == nil { + return handleApplicationServiceRegistration( + accessToken, err, req, r, cfg, accountDB, deviceDB, + ) } - // If no error, application service was successfully validated. - // Don't need to worry about appending to registration stages as - // application service registration is entirely separate. - return completeRegistration( - req.Context(), accountDB, deviceDB, r.Username, "", appserviceID, - r.InhibitLogin, r.InitialDisplayName, + case authtypes.LoginTypeApplicationService: + // Extract the access token from the request. + accessToken, err := auth.ExtractAccessToken(req) + // Let the AS registration handler handle the process from here. We + // don't need a condition on that call since the registration is clearly + // stated as being AS-related. + return handleApplicationServiceRegistration( + accessToken, err, req, r, cfg, accountDB, deviceDB, ) case authtypes.LoginTypeDummy: @@ -578,6 +580,50 @@ func handleRegistrationFlow( req, r, sessionID, cfg, accountDB, deviceDB) } +// handleApplicationServiceRegistration handles the registration of an +// application service's user by validating the AS from its access token and +// registering the user. Its two first parameters must be the two return values +// of the auth.ExtractAccessToken function. +// Returns an error if the access token couldn't be extracted from the request +// at an earlier step of the registration workflow, or if the provided access +// token doesn't belong to a valid AS, or if there was an issue completing the +// registration process. +func handleApplicationServiceRegistration( + accessToken string, + tokenErr error, + req *http.Request, + r registerRequest, + cfg *config.Dendrite, + accountDB *accounts.Database, + deviceDB *devices.Database, +) util.JSONResponse { + // Check if we previously had issues extracting the access token from the + // request. + if tokenErr != nil { + return util.JSONResponse{ + Code: http.StatusUnauthorized, + JSON: jsonerror.MissingToken(tokenErr.Error()), + } + } + + // Check application service register user request is valid. + // The application service's ID is returned if so. + appserviceID, err := validateApplicationService( + cfg, r.Username, accessToken, + ) + if err != nil { + return *err + } + + // If no error, application service was successfully validated. + // Don't need to worry about appending to registration stages as + // application service registration is entirely separate. + return completeRegistration( + req.Context(), accountDB, deviceDB, r.Username, "", appserviceID, + r.InhibitLogin, r.InitialDisplayName, + ) +} + // checkAndCompleteFlow checks if a given registration flow is completed given // a set of allowed flows. If so, registration is completed, otherwise a // response with 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 fbf140c2..6fcf0bc3 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,8 +15,6 @@ package routing import ( - "net/http" - "net/url" "regexp" "testing" @@ -186,47 +184,26 @@ func TestValidationOfApplicationServices(t *testing.T) { 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) + asID, resp := validateApplicationService(&fakeConfig, fakeSenderLocalpart, "1234") 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) + asID, resp = validateApplicationService(&fakeConfig, fakeSenderLocalpart, "xxxx") 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") + asID, resp = validateApplicationService(&fakeConfig, "_appservice_bob", "1234") 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") + asID, resp = validateApplicationService(&fakeConfig, "_something_else", "1234") if resp == nil || asID == fakeID { - t.Errorf("user_id should not have been valid: %s", - fakeHTTPRequest.URL.Query().Get("user_id")) + t.Errorf("user_id should not have been valid: @_something_else:localhost") } }