From ef21bed096d5a6d077608e89d80d4fbaa75ffca2 Mon Sep 17 00:00:00 2001 From: Anand Vasudevan Date: Mon, 21 Sep 2020 17:41:37 +0530 Subject: [PATCH] Updated sso implementation Changed with comments from Kegsay and Half-Shot - changed some return error codes - moved sso url creation &validation to startup time - added test to sytest whitelist --- clientapi/routing/login.go | 21 +++++++++----- clientapi/routing/sso.go | 45 ++++++++++------------------- internal/config/config_clientapi.go | 11 +++++++ sytest-whitelist | 4 ++- 4 files changed, 43 insertions(+), 38 deletions(-) diff --git a/clientapi/routing/login.go b/clientapi/routing/login.go index fe61235d6..8fad235f3 100644 --- a/clientapi/routing/login.go +++ b/clientapi/routing/login.go @@ -84,10 +84,9 @@ func Login( // TODO: is the the right way to read the body and re-add it? body, err := ioutil.ReadAll(req.Body) if err != nil { - // TODO: is this appropriate? return util.JSONResponse{ - Code: http.StatusMethodNotAllowed, - JSON: jsonerror.NotFound("Bad method"), + Code: http.StatusBadRequest, + JSON: jsonerror.BadJSON("Bad JSON"), } } // add the body back to the request because ioutil.ReadAll consumes the body @@ -97,12 +96,20 @@ func Login( var jsonBody map[string]interface{} if err := json.Unmarshal([]byte(body), &jsonBody); err != nil { return util.JSONResponse{ - Code: http.StatusMethodNotAllowed, - JSON: jsonerror.NotFound("Bad method"), + Code: http.StatusBadRequest, + JSON: jsonerror.BadJSON("Bad JSON"), } } - loginType := jsonBody["type"].(string) + var loginType string + if val, ok := jsonBody["type"]; ok { + loginType = val.(string) + } else { + return util.JSONResponse{ + Code: http.StatusBadRequest, + JSON: jsonerror.BadJSON("No 'type' parameter"), + } + } if loginType == "m.login.password" { return doPasswordLogin(req, accountDB, userAPI, cfg) } else if loginType == "m.login.token" { @@ -164,7 +171,7 @@ func doTokenLogin(req *http.Request, accountDB accounts.Database, userAPI userap // the login is successful, delete the login token before returning the access token to the client if authResult.Code == http.StatusOK { if err := auth.DeleteLoginToken(r.(*auth.LoginTokenRequest).Token); err != nil { - // TODO: what to do here? + util.GetLogger(req.Context()).WithError(err).Error("Could not delete login ticket from DB") } } return authResult diff --git a/clientapi/routing/sso.go b/clientapi/routing/sso.go index 31a94317a..94ce82423 100644 --- a/clientapi/routing/sso.go +++ b/clientapi/routing/sso.go @@ -1,4 +1,4 @@ -// Copyright 2017 Vector Creations Ltd +// Copyright 2020 The Matrix.org Foundation C.I.C. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -49,17 +49,8 @@ func SSORedirect( // If dendrite is not configured to use SSO by the admin return bad method if !cfg.CAS.Enabled || cfg.CAS.Server == "" { return util.JSONResponse{ - Code: http.StatusMethodNotAllowed, - JSON: jsonerror.NotFound("Bad method"), - } - } - - // Try to parse the SSO URL configured to a url.URL type - ssoURL, err := url.Parse(cfg.CAS.Server) - if err != nil { - return util.JSONResponse{ - Code: http.StatusInternalServerError, - JSON: jsonerror.Unknown("Failed to parse SSO URL configured: " + err.Error()), + Code: http.StatusNotImplemented, + JSON: jsonerror.NotFound("Method disabled"), } } @@ -75,8 +66,8 @@ func SSORedirect( redirectURL, err := url.Parse(redirectURLStr) if err != nil { return util.JSONResponse{ - Code: http.StatusInternalServerError, - JSON: jsonerror.Unknown("Invalid redirectURL: " + err.Error()), + Code: http.StatusBadRequest, + JSON: jsonerror.InvalidArgumentValue("Invalid redirectURL: " + err.Error()), } } @@ -86,10 +77,10 @@ func SSORedirect( } // Adding the params to the sso url + ssoURL := cfg.CAS.URL ssoQueries := make(url.Values) // the service url that we send to CAS is homeserver.com/_matrix/client/r0/login/sso/redirect?redirectUrl=xyz ssoQueries.Set("service", req.RequestURI) - ssoURL.RawQuery = ssoQueries.Encode() return util.RedirectResponse(ssoURL.String()) @@ -106,23 +97,16 @@ func ssoTicket( cfg *config.ClientAPI, ) util.JSONResponse { // form the ticket validation URL from the config - ssoURL, err := url.Parse(cfg.CAS.Server + cfg.CAS.ValidateEndpoint) - if err != nil { - return util.JSONResponse{ - Code: http.StatusInternalServerError, - JSON: jsonerror.Unknown("Failed to parse SSO URL configured: " + err.Error()), - } - } - + validateURL := cfg.CAS.ValidateURL ticket := req.FormValue("ticket") // append required params to the CAS validate endpoint - ssoQueries := make(url.Values) - ssoQueries.Set("ticket", ticket) - ssoURL.RawQuery = ssoQueries.Encode() + validateQueries := make(url.Values) + validateQueries.Set("ticket", ticket) + validateURL.RawQuery = validateQueries.Encode() // validate the ticket - casUsername, err := validateTicket(ssoURL.String()) + casUsername, err := validateTicket(validateURL.String()) if err != nil { // TODO: should I be logging these? What else should I log? util.GetLogger(req.Context()).WithError(err).Error("CAS SSO ticket validation failed") @@ -182,10 +166,11 @@ func completeSSOAuth( // if the user exists, then we pick that user, else we create a new user account, err := accountDB.CreateAccount(req.Context(), username, "", "") if err != nil { - // some error if err != sqlutil.ErrUserExists { + // some error + util.GetLogger(req.Context()).WithError(err).Error("Could not create new user") return util.JSONResponse{ - Code: http.StatusUnauthorized, + Code: http.StatusInternalServerError, JSON: jsonerror.Unknown("Could not create new user"), } } else { @@ -193,7 +178,7 @@ func completeSSOAuth( account, err = accountDB.GetAccountByLocalpart(req.Context(), username) if err != nil { return util.JSONResponse{ - Code: http.StatusUnauthorized, + Code: http.StatusInternalServerError, JSON: jsonerror.Unknown("Could not query user"), } } diff --git a/internal/config/config_clientapi.go b/internal/config/config_clientapi.go index 96a966186..43db6173d 100644 --- a/internal/config/config_clientapi.go +++ b/internal/config/config_clientapi.go @@ -2,6 +2,8 @@ package config import ( "fmt" + "net/url" + "path" "time" ) @@ -77,12 +79,21 @@ type CAS struct { Enabled bool `yaml:"cas_enabled"` Server string `yaml:"cas_server"` ValidateEndpoint string `yaml:"cas_validate_endpoint"` + URL *url.URL + ValidateURL *url.URL } func (cas *CAS) Verify(ConfigErrors *ConfigErrors) { if cas.Enabled { checkURL(ConfigErrors, "client_api.cas.cas_server", cas.Server) checkNotEmpty(ConfigErrors, "client_api.cas.cas_validate_endpoint", cas.ValidateEndpoint) + var err error + cas.URL, err = url.Parse(cas.Server) + if err != nil { + ConfigErrors.Add(fmt.Sprintf("Couldn't parse %q (%q)to a URL", "client_api.cas.cas_server", cas.Server)) + } + cas.ValidateURL.Path = path.Join(cas.URL.Path, cas.ValidateEndpoint) + checkURL(ConfigErrors, "client_api.cas.cas_validate_endpoint", cas.ValidateURL.String()) } } diff --git a/sytest-whitelist b/sytest-whitelist index 0adeaee6f..91516428d 100644 --- a/sytest-whitelist +++ b/sytest-whitelist @@ -470,4 +470,6 @@ We can't peek into rooms with shared history_visibility We can't peek into rooms with invited history_visibility We can't peek into rooms with joined history_visibility Local users can peek by room alias -Peeked rooms only turn up in the sync for the device who peeked them \ No newline at end of file +Peeked rooms only turn up in the sync for the device who peeked them +Room state at a rejected message event is the same as its predecessor +Room state at a rejected state event is the same as its predecessor \ No newline at end of file