From 7e4df6d46108b9be89c5d243bd2333908fe0d3b0 Mon Sep 17 00:00:00 2001 From: Matthew Russell Date: Wed, 23 Jun 2021 20:16:47 -0700 Subject: [PATCH 1/8] Get SSO login with new openid provider working --- app/oauth.go | 4 +- cmd/mattermost/main.go | 1 + config/client.go | 10 ++- model/openid/openid.go | 135 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 142 insertions(+), 8 deletions(-) create mode 100644 model/openid/openid.go diff --git a/app/oauth.go b/app/oauth.go index f36d2ee230b..1e423bbd58a 100644 --- a/app/oauth.go +++ b/app/oauth.go @@ -921,7 +921,7 @@ func (a *App) AuthorizeOAuthUser(w http.ResponseWriter, r *http.Request, service } func (a *App) SwitchEmailToOAuth(w http.ResponseWriter, r *http.Request, email, password, code, service string) (string, *model.AppError) { - if a.Srv().License() != nil && !*a.Config().ServiceSettings.ExperimentalEnableAuthenticationTransfer { + if !*a.Config().ServiceSettings.ExperimentalEnableAuthenticationTransfer { return "", model.NewAppError("emailToOAuth", "api.user.email_to_oauth.not_available.app_error", nil, "", http.StatusForbidden) } @@ -951,7 +951,7 @@ func (a *App) SwitchEmailToOAuth(w http.ResponseWriter, r *http.Request, email, } func (a *App) SwitchOAuthToEmail(email, password, requesterId string) (string, *model.AppError) { - if a.Srv().License() != nil && !*a.Config().ServiceSettings.ExperimentalEnableAuthenticationTransfer { + if !*a.Config().ServiceSettings.ExperimentalEnableAuthenticationTransfer { return "", model.NewAppError("oauthToEmail", "api.user.oauth_to_email.not_available.app_error", nil, "", http.StatusForbidden) } diff --git a/cmd/mattermost/main.go b/cmd/mattermost/main.go index 2d7a307771a..47bf78957e3 100644 --- a/cmd/mattermost/main.go +++ b/cmd/mattermost/main.go @@ -23,6 +23,7 @@ import ( _ "github.com/mattermost/mattermost-server/v5/app/slashcommands" // Plugins _ "github.com/mattermost/mattermost-server/v5/model/gitlab" + _ "github.com/mattermost/mattermost-server/v5/model/openid" // Enterprise Imports _ "github.com/mattermost/mattermost-server/v5/imports" ) diff --git a/config/client.go b/config/client.go index ca6a29dcd75..67b48dd3033 100644 --- a/config/client.go +++ b/config/client.go @@ -312,6 +312,10 @@ func GenerateLimitedClientConfig(c *model.Config, telemetryID string, license *m props["EnableGuestAccounts"] = strconv.FormatBool(*c.GuestAccountsSettings.Enable) props["GuestAccountsEnforceMultifactorAuthentication"] = strconv.FormatBool(*c.GuestAccountsSettings.EnforceMultifactorAuthentication) + props["EnableSignUpWithOpenId"] = strconv.FormatBool(*c.OpenIdSettings.Enable) + props["OpenIdButtonColor"] = *c.OpenIdSettings.ButtonColor + props["OpenIdButtonText"] = *c.OpenIdSettings.ButtonText + if license != nil { if *license.Features.LDAP { props["EnableLdap"] = strconv.FormatBool(*c.LdapSettings.Enable) @@ -337,12 +341,6 @@ func GenerateLimitedClientConfig(c *model.Config, telemetryID string, license *m props["EnableSignUpWithOffice365"] = strconv.FormatBool(*c.Office365Settings.Enable) } - if *license.Features.OpenId { - props["EnableSignUpWithOpenId"] = strconv.FormatBool(*c.OpenIdSettings.Enable) - props["OpenIdButtonColor"] = *c.OpenIdSettings.ButtonColor - props["OpenIdButtonText"] = *c.OpenIdSettings.ButtonText - } - if *license.Features.CustomTermsOfService { props["EnableCustomTermsOfService"] = strconv.FormatBool(*c.SupportSettings.CustomTermsOfServiceEnabled) props["CustomTermsOfServiceReAcceptancePeriod"] = strconv.FormatInt(int64(*c.SupportSettings.CustomTermsOfServiceReAcceptancePeriod), 10) diff --git a/model/openid/openid.go b/model/openid/openid.go new file mode 100644 index 00000000000..65f604b7395 --- /dev/null +++ b/model/openid/openid.go @@ -0,0 +1,135 @@ +package oauthopenid + +import ( + "encoding/json" + "net/http" + "io" + "io/ioutil" + "strings" + + "github.com/mattermost/mattermost-server/v5/einterfaces" + "github.com/mattermost/mattermost-server/v5/model" + "github.com/mattermost/mattermost-server/v5/services/httpservice" + "github.com/mattermost/mattermost-server/v5/shared/mlog" + "github.com/mattermost/mattermost-server/v5/utils/testutils" +) + +type OpenIdProvider struct { +} + +type OpenIdProviderUrls struct { + AuthEndpoint string `json:"authorization_endpoint"` + TokenEndpoint string `json:"token_endpoint"` + UserApiEndpoint string `json:"userinfo_endpoint"` +} + +// Keycloak sepcific +type OpenIdUser struct { + Id string `json:"sub"` + Username string `json:"preferred_username"` + Email string `json:"email"` + FirstName string `json:"given_name"` + LastName string `json:"family_name"` +} + +func init() { + provider := &OpenIdProvider{} + einterfaces.RegisterOauthProvider(model.SERVICE_OPENID, provider) +} + +func userFromOpenIdUser(oiu *OpenIdUser) *model.User { + user := &model.User{} + username := oiu.Username + user.Username = model.CleanUsername(username) + user.FirstName = oiu.FirstName + user.LastName = oiu.LastName + user.Email = oiu.Email + user.Email = strings.ToLower(user.Email) + user.AuthData = model.NewString(oiu.getAuthData()) + user.AuthService = model.SERVICE_OPENID + + return user +} + +func openIdUserFromJson(data io.Reader) (*OpenIdUser, error) { + var oiu OpenIdUser + body, err1 := ioutil.ReadAll(data) + if err1 != nil { + return nil, err1 + } + mlog.Debug("Received OpenID User data: " + string(body)) + err2 := json.Unmarshal(body, &oiu) + if err2 != nil { + return nil, err2 + } + return &oiu, nil +} + +func (oiu *OpenIdUser) ToJson() string { + b, err := json.Marshal(oiu) + if err != nil { + return "" + } + return string(b) +} + + +func (oiu *OpenIdUser) getAuthData() string { + return oiu.Id +} + +func (m *OpenIdProvider) GetUserFromJson(data io.Reader, tokenUser *model.User) (*model.User, error) { + oiu, err := openIdUserFromJson(data) + if err != nil { + return nil, err + } + return userFromOpenIdUser(oiu), nil +} + +func (m *OpenIdProvider) GetSSOSettings(config *model.Config, service string) (*model.SSOSettings, error) { + // This is suuuper janky. But needed a good way to make the http client with just a config + h := httpservice.MakeHTTPService(&testutils.StaticConfigService{Cfg: config}) + req, err := http.NewRequest("GET", *config.OpenIdSettings.DiscoveryEndpoint, nil) + if err != nil { + mlog.Warn("Error while making discovery request", mlog.Err(err)) + } + + req.Header.Set("Accept", "application/json") + resp, err2 := h.MakeClient(true).Do(req) + if err2 != nil { + mlog.Warn("Error while fetching discovery request", mlog.Err(err2)) + } + + defer resp.Body.Close() + + var oidcUrls OpenIdProviderUrls + json.NewDecoder(resp.Body).Decode(&oidcUrls) + + _, err3 := json.Marshal(oidcUrls) + if err3 != nil { + mlog.Warn("Error while deserializing discovery response", mlog.Err(err3)) + } + + // Merge the 'discovered' endpoints with the original settings + newSettings := &model.SSOSettings{ + Enable: config.OpenIdSettings.Enable, + Id: config.OpenIdSettings.Id, + Secret: config.OpenIdSettings.Secret, + Scope: config.OpenIdSettings.Scope, + AuthEndpoint: &oidcUrls.AuthEndpoint, + TokenEndpoint: &oidcUrls.TokenEndpoint, + UserApiEndpoint: &oidcUrls.UserApiEndpoint, + DiscoveryEndpoint: config.OpenIdSettings.DiscoveryEndpoint, + ButtonText: config.OpenIdSettings.ButtonText, + ButtonColor: config.OpenIdSettings.ButtonColor, + } + return newSettings, nil +} + +func (m *OpenIdProvider) GetUserFromIdToken(idToken string) (*model.User, error) { + return nil, nil +} + +func (m *OpenIdProvider) IsSameUser(dbUser, oauthUser *model.User) bool { + return dbUser.AuthData == oauthUser.AuthData +} From 45b897a2827a822ebb7659fbd057589b4d67ecf5 Mon Sep 17 00:00:00 2001 From: Matthew Russell Date: Wed, 23 Jun 2021 20:24:49 -0700 Subject: [PATCH 2/8] Cleanup code --- model/openid/openid.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/model/openid/openid.go b/model/openid/openid.go index 65f604b7395..93299c786e5 100644 --- a/model/openid/openid.go +++ b/model/openid/openid.go @@ -92,22 +92,23 @@ func (m *OpenIdProvider) GetSSOSettings(config *model.Config, service string) (* req, err := http.NewRequest("GET", *config.OpenIdSettings.DiscoveryEndpoint, nil) if err != nil { mlog.Warn("Error while making discovery request", mlog.Err(err)) + return nil, err } req.Header.Set("Accept", "application/json") resp, err2 := h.MakeClient(true).Do(req) if err2 != nil { mlog.Warn("Error while fetching discovery request", mlog.Err(err2)) + return nil, err2 } defer resp.Body.Close() var oidcUrls OpenIdProviderUrls - json.NewDecoder(resp.Body).Decode(&oidcUrls) - - _, err3 := json.Marshal(oidcUrls) + err3 := json.NewDecoder(resp.Body).Decode(&oidcUrls) if err3 != nil { mlog.Warn("Error while deserializing discovery response", mlog.Err(err3)) + return nil, err3 } // Merge the 'discovered' endpoints with the original settings From 0cf5f1475595e8f38e216260a06e421fc724ea23 Mon Sep 17 00:00:00 2001 From: Matthew Russell Date: Sun, 27 Jun 2021 21:02:00 -0700 Subject: [PATCH 3/8] Make SSO mattermost admin ability work --- app/user.go | 5 +++++ model/openid/openid.go | 20 +++++++++++++++----- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/app/user.go b/app/user.go index b0217969e6c..56c35cc9da5 100644 --- a/app/user.go +++ b/app/user.go @@ -2017,6 +2017,11 @@ func (a *App) UpdateOAuthUserAttrs(userData io.Reader, user *model.User, provide } } + if oauthUser.Roles != user.Roles { + user.Roles = oauthUser.Roles + userAttrsChanged = true + } + if user.DeleteAt > 0 { // Make sure they are not disabled user.DeleteAt = 0 diff --git a/model/openid/openid.go b/model/openid/openid.go index 93299c786e5..f0ff1b72b27 100644 --- a/model/openid/openid.go +++ b/model/openid/openid.go @@ -25,11 +25,12 @@ type OpenIdProviderUrls struct { // Keycloak sepcific type OpenIdUser struct { - Id string `json:"sub"` - Username string `json:"preferred_username"` - Email string `json:"email"` - FirstName string `json:"given_name"` - LastName string `json:"family_name"` + Id string `json:"sub"` + Username string `json:"preferred_username"` + Email string `json:"email"` + FirstName string `json:"given_name"` + LastName string `json:"family_name"` + Roles []string `json:"roles"` } func init() { @@ -47,6 +48,15 @@ func userFromOpenIdUser(oiu *OpenIdUser) *model.User { user.Email = strings.ToLower(user.Email) user.AuthData = model.NewString(oiu.getAuthData()) user.AuthService = model.SERVICE_OPENID + var roles string + for _, r := range oiu.Roles { + if r == "mattermost_admins" { + roles = model.SYSTEM_ADMIN_ROLE_ID + } else { + mlog.Debug("Skipping unknown role when processing user: " + username + " role: " + r) + } + } + user.Roles = roles return user } From 17bc69fe6aed8c6d12f9f9af337d11c13907df38 Mon Sep 17 00:00:00 2001 From: Matthew Russell Date: Mon, 28 Jun 2021 22:47:34 -0700 Subject: [PATCH 4/8] Use email for auth data make for an easier migration --- model/openid/openid.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/model/openid/openid.go b/model/openid/openid.go index f0ff1b72b27..91dcbd692ca 100644 --- a/model/openid/openid.go +++ b/model/openid/openid.go @@ -85,7 +85,7 @@ func (oiu *OpenIdUser) ToJson() string { func (oiu *OpenIdUser) getAuthData() string { - return oiu.Id + return oiu.Email } func (m *OpenIdProvider) GetUserFromJson(data io.Reader, tokenUser *model.User) (*model.User, error) { From 6b34860e117f4264d72ec666fc366cb1476522bc Mon Sep 17 00:00:00 2001 From: Matthew Russell Date: Fri, 9 Jul 2021 12:58:59 -0700 Subject: [PATCH 5/8] Always include the system user role and add the system admin if an admin --- model/openid/openid.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/model/openid/openid.go b/model/openid/openid.go index 91dcbd692ca..884c53c1501 100644 --- a/model/openid/openid.go +++ b/model/openid/openid.go @@ -48,10 +48,10 @@ func userFromOpenIdUser(oiu *OpenIdUser) *model.User { user.Email = strings.ToLower(user.Email) user.AuthData = model.NewString(oiu.getAuthData()) user.AuthService = model.SERVICE_OPENID - var roles string + roles := model.SYSTEM_USER_ROLE_ID for _, r := range oiu.Roles { if r == "mattermost_admins" { - roles = model.SYSTEM_ADMIN_ROLE_ID + roles += " " + model.SYSTEM_ADMIN_ROLE_ID } else { mlog.Debug("Skipping unknown role when processing user: " + username + " role: " + r) } From 17f4ad5df320dd1c9c69a996e6174e897694fd9e Mon Sep 17 00:00:00 2001 From: Matthew Russell Date: Fri, 9 Jul 2021 13:32:51 -0700 Subject: [PATCH 6/8] Try to figure out go programming --- model/openid/openid.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/model/openid/openid.go b/model/openid/openid.go index 884c53c1501..98a0f66efa6 100644 --- a/model/openid/openid.go +++ b/model/openid/openid.go @@ -48,10 +48,11 @@ func userFromOpenIdUser(oiu *OpenIdUser) *model.User { user.Email = strings.ToLower(user.Email) user.AuthData = model.NewString(oiu.getAuthData()) user.AuthService = model.SERVICE_OPENID - roles := model.SYSTEM_USER_ROLE_ID + var roles string + roles = model.SYSTEM_USER_ROLE_ID for _, r := range oiu.Roles { if r == "mattermost_admins" { - roles += " " + model.SYSTEM_ADMIN_ROLE_ID + roles = model.SYSTEM_USER_ROLE_ID+" "+model.SYSTEM_ADMIN_ROLE_ID } else { mlog.Debug("Skipping unknown role when processing user: " + username + " role: " + r) } From 82c5aa8798c00f645339ceb253a365f481564464 Mon Sep 17 00:00:00 2001 From: Matthew Russell Date: Fri, 28 Jan 2022 15:56:21 -0800 Subject: [PATCH 7/8] Switch to new uniqueId as uniqueness in PCTE --- model/openid/openid.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/model/openid/openid.go b/model/openid/openid.go index 98a0f66efa6..f8f9a72f3b6 100644 --- a/model/openid/openid.go +++ b/model/openid/openid.go @@ -25,7 +25,7 @@ type OpenIdProviderUrls struct { // Keycloak sepcific type OpenIdUser struct { - Id string `json:"sub"` + Id string `json:"uniqueId"` Username string `json:"preferred_username"` Email string `json:"email"` FirstName string `json:"given_name"` @@ -86,7 +86,7 @@ func (oiu *OpenIdUser) ToJson() string { func (oiu *OpenIdUser) getAuthData() string { - return oiu.Email + return oiu.Id } func (m *OpenIdProvider) GetUserFromJson(data io.Reader, tokenUser *model.User) (*model.User, error) { @@ -143,5 +143,8 @@ func (m *OpenIdProvider) GetUserFromIdToken(idToken string) (*model.User, error) } func (m *OpenIdProvider) IsSameUser(dbUser, oauthUser *model.User) bool { - return dbUser.AuthData == oauthUser.AuthData + // PCTE converted from emails as unique to SSO ID as unique + // so check IDs first (which will be in authData), then check email + return dbUser.AuthData == oauthUser.AuthData || + dbUser.Email == oauthUser.Email } From 4ff0ab1965aa4544eb42a38390f77711e1d83fee Mon Sep 17 00:00:00 2001 From: Matthew Russell Date: Wed, 16 Mar 2022 16:34:04 -0700 Subject: [PATCH 8/8] Fix pointer comparison and add a little more debugging --- model/openid/openid.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/model/openid/openid.go b/model/openid/openid.go index f8f9a72f3b6..ce440c55dc6 100644 --- a/model/openid/openid.go +++ b/model/openid/openid.go @@ -58,6 +58,7 @@ func userFromOpenIdUser(oiu *OpenIdUser) *model.User { } } user.Roles = roles + mlog.Debug("Parsed user from openId as model user: " + user.ToJson()) return user } @@ -145,6 +146,6 @@ func (m *OpenIdProvider) GetUserFromIdToken(idToken string) (*model.User, error) func (m *OpenIdProvider) IsSameUser(dbUser, oauthUser *model.User) bool { // PCTE converted from emails as unique to SSO ID as unique // so check IDs first (which will be in authData), then check email - return dbUser.AuthData == oauthUser.AuthData || + return *dbUser.AuthData == *oauthUser.AuthData || dbUser.Email == oauthUser.Email }