Refactor SSO configuration.

It makes more sense to base provider defaults on brand. Type is not
1:1 to brand.

Splits apart OIDC and OAuth2 to match actual specs.

clientapi/auth/sso/oauth2.go

@@ -48,7 +48,7 @@ type oauth2IdentityProvider struct {
 
 func (p *oauth2IdentityProvider) AuthorizationURL(ctx context.Context, callbackURL, nonce string) (string, error) {
 	u, err := resolveURL(p.authorizationURL, url.Values{
-		"client_id":     []string{p.cfg.OIDC.ClientID},
+		"client_id":     []string{p.cfg.OAuth2.ClientID},
 		"response_type": []string{"code"},
 		"redirect_uri":  []string{callbackURL},
 		"scope":         []string{strings.Join(p.scopes, " ")},
@@ -121,8 +121,8 @@ func (p *oauth2IdentityProvider) getAccessToken(ctx context.Context, callbackURL
 		"grant_type":    []string{"authorization_code"},
 		"code":          []string{code},
 		"redirect_uri":  []string{callbackURL},
-		"client_id":     []string{p.cfg.OIDC.ClientID},
+		"client_id":     []string{p.cfg.OAuth2.ClientID},
-		"client_secret": []string{p.cfg.OIDC.ClientSecret},
+		"client_secret": []string{p.cfg.OAuth2.ClientSecret},
 	}
 	hreq, err := http.NewRequestWithContext(ctx, http.MethodPost, p.accessTokenURL, strings.NewReader(body.Encode()))
 	if err != nil {

clientapi/auth/sso/oidc.go

@@ -36,7 +36,7 @@ type oidcIdentityProvider struct {
 }
 
 func newOIDCIdentityProvider(ctx context.Context, cfg *config.IdentityProvider, hc *http.Client) (*oidcIdentityProvider, error) {
-	p := &oidcIdentityProvider{
+	return &oidcIdentityProvider{
 		oauth2IdentityProvider: &oauth2IdentityProvider{
 			cfg: cfg,
 			hc:  hc,
@@ -48,18 +48,7 @@ func newOIDCIdentityProvider(ctx context.Context, cfg *config.IdentityProvider,
 			displayNamePath:     "name",
 			suggestedUserIDPath: "preferred_username",
 		},
-	}
+	}, nil
-
-	// TODO: Complement starts and waits for the "base image" without
-	// first starting httpmockserver, which means we cannot always do
-	// this sanity check.
-	if false {
-		if _, _, err := p.get(ctx); err != nil {
-			return nil, err
-		}
-	}
-
-	return p, nil
 }
 
 func (p *oidcIdentityProvider) AuthorizationURL(ctx context.Context, callbackURL, nonce string) (string, error) {

clientapi/auth/sso/sso.go

@@ -42,12 +42,9 @@ func NewAuthenticator(ctx context.Context, cfg *config.SSO) (*Authenticator, err
 		providers: make(map[string]identityProvider, len(cfg.Providers)),
 	}
 	for _, pcfg := range cfg.Providers {
-		typ := pcfg.Type
+		pcfg = pcfg.WithDefaults()
-		if typ == "" {
-			typ = config.IdentityProviderType(pcfg.ID)
-		}
 
-		switch typ {
+		switch pcfg.Type {
 		case config.SSOTypeOIDC:
 			p, err := newOIDCIdentityProvider(ctx, &pcfg, hc)
 			if err != nil {
@@ -57,7 +54,7 @@ func NewAuthenticator(ctx context.Context, cfg *config.SSO) (*Authenticator, err
 		case config.SSOTypeGitHub:
 			a.providers[pcfg.ID] = newGitHubIdentityProvider(&pcfg, hc)
 		default:
-			return nil, fmt.Errorf("unknown SSO provider type: %s", typ)
+			return nil, fmt.Errorf("unknown SSO provider type: %s", pcfg.Type)
 		}
 	}
 

cmd/generate-config/main.go

@@ -85,20 +85,19 @@ func main() {
 		cfg.ClientAPI.Login.SSO.Enabled = true
 		cfg.ClientAPI.Login.SSO.Providers = []config.IdentityProvider{
 			{
-				ID:   "github",
+				Brand: "github",
-				Name: "Fake GitHub",
+				OAuth2: config.OAuth2{
-				OIDC: config.OIDC{
 					ClientID:     "aclientid",
 					ClientSecret: "aclientsecret",
 				},
 			},
 			{
-				ID:   "google",
+				Brand: "google",
-				Name: "Fake Google",
+				OAuth2: config.OAuth2{
-				Type: "oidc",
-				OIDC: config.OIDC{
 					ClientID:     "aclientid",
 					ClientSecret: "aclientsecret",
+				},
+				OIDC: config.OIDC{
 					DiscoveryURL: "https://accounts.google.com/.well-known/openid-configuration",
 				},
 			},

setup/config/config_clientapi.go

@@ -138,7 +138,8 @@ func (sso *SSO) Verify(configErrs *ConfigErrors) {
 	var foundDefaultProvider bool
 	seenPIDs := make(map[string]bool, len(sso.Providers))
 	for _, p := range sso.Providers {
-		p.Verify(configErrs)
+		p = p.WithDefaults()
+		p.verifyNormalized(configErrs)
 		if p.ID == sso.DefaultProviderID {
 			foundDefaultProvider = true
 		}
@@ -158,42 +159,71 @@ func (sso *SSO) Verify(configErrs *ConfigErrors) {
 	}
 }
 
-// See https://github.com/matrix-org/matrix-doc/blob/old_master/informal/idp-brands.md.
 type IdentityProvider struct {
-	// ID is the unique identifier of this IdP. We use the brand identifiers as provider
+	// ID is the unique identifier of this IdP. If empty, the brand will be used.
-	// identifiers for simplicity.
 	ID string `yaml:"id"`
 
-	// Name is a human-friendly name of the provider.
+	// Name is a human-friendly name of the provider. If empty, a default based on
+	// the brand will be used.
 	Name string `yaml:"name"`
 
-	// Brand is a hint on how to display the IdP to the user. If this is empty, a default
+	// Brand is a hint on how to display the IdP to the user.
-	// based on the type is used.
+	//
+	// See https://github.com/matrix-org/matrix-doc/blob/old_master/informal/idp-brands.md.
 	Brand SSOBrand `yaml:"brand"`
 
 	// Icon is an MXC URI describing how to display the IdP to the user. Prefer using `brand`.
 	Icon string `yaml:"icon"`
 
-	// Type describes how this provider is implemented. It must match "github". If this is
+	// Type describes how this IdP is implemented. If this is empty, a default is chosen
-	// empty, the ID is used, which means there is a weak expectation that ID is also a
+	// based on brand.
-	// valid type, unless you have a complicated setup.
 	Type IdentityProviderType `yaml:"type"`
 
-	// OIDC contains settings for providers based on OpenID Connect (OAuth 2).
+	// OAuth2 contains settings for IdPs based on OpenID Connect and OAuth2.
+	OAuth2 OAuth2 `yaml:"oauth2"`
+
+	// OIDC contains settings for IdPs based on OpenID Connect.
 	OIDC OIDC `yaml:"oidc"`
 }
 
-type OIDC struct {
+func (idp *IdentityProvider) WithDefaults() IdentityProvider {
+	p := *idp
+	if p.ID == "" {
+		p.ID = string(p.Brand)
+	}
+	if p.OIDC.DiscoveryURL == "" {
+		p.OIDC.DiscoveryURL = oidcDefaultDiscoveryURLs[idp.Brand]
+	}
+	if p.Type == "" {
+		if p.OIDC.DiscoveryURL != "" {
+			p.Type = SSOTypeOIDC
+		} else if p.Brand == SSOBrandGitHub {
+			p.Type = SSOTypeGitHub
+		}
+	}
+	if p.Name == "" {
+		p.Name = oidcDefaultNames[p.Brand]
+	}
+
+	return p
+}
+
+type OAuth2 struct {
 	ClientID     string `yaml:"client_id"`
 	ClientSecret string `yaml:"client_secret"`
+}
+
+type OIDC struct {
 	DiscoveryURL string `yaml:"discovery_url"`
 }
 
 func (idp *IdentityProvider) Verify(configErrs *ConfigErrors) {
+	p := idp.WithDefaults()
+	p.verifyNormalized(configErrs)
+}
+
+func (idp *IdentityProvider) verifyNormalized(configErrs *ConfigErrors) {
 	checkNotEmpty(configErrs, "client_api.sso.providers.id", idp.ID)
-	if !checkIdentityProviderBrand(SSOBrand(idp.ID)) {
-		configErrs.Add(fmt.Sprintf("unrecognised ID config key %q: %s", "client_api.sso.providers", idp.ID))
-	}
 	checkNotEmpty(configErrs, "client_api.sso.providers.name", idp.Name)
 	if idp.Brand != "" && !checkIdentityProviderBrand(idp.Brand) {
 		configErrs.Add(fmt.Sprintf("unrecognised brand in identity provider %q for config key %q: %s", idp.ID, "client_api.sso.providers", idp.Brand))
@@ -201,23 +231,19 @@ func (idp *IdentityProvider) Verify(configErrs *ConfigErrors) {
 	if idp.Icon != "" {
 		checkURL(configErrs, "client_api.sso.providers.icon", idp.Icon)
 	}
-	typ := idp.Type
-	if idp.Type == "" {
-		typ = IdentityProviderType(idp.ID)
-	}
 
-	switch typ {
+	switch idp.Type {
 	case SSOTypeOIDC:
-		checkNotEmpty(configErrs, "client_api.sso.providers.oidc.client_id", idp.OIDC.ClientID)
+		checkNotEmpty(configErrs, "client_api.sso.providers.oidc.client_id", idp.OAuth2.ClientID)
-		checkNotEmpty(configErrs, "client_api.sso.providers.oidc.client_secret", idp.OIDC.ClientSecret)
+		checkNotEmpty(configErrs, "client_api.sso.providers.oidc.client_secret", idp.OAuth2.ClientSecret)
 		checkNotEmpty(configErrs, "client_api.sso.providers.oidc.discovery_url", idp.OIDC.DiscoveryURL)
 
 	case SSOTypeGitHub:
-		checkNotEmpty(configErrs, "client_api.sso.providers.oidc.client_id", idp.OIDC.ClientID)
+		checkNotEmpty(configErrs, "client_api.sso.providers.oidc.client_id", idp.OAuth2.ClientID)
-		checkNotEmpty(configErrs, "client_api.sso.providers.oidc.client_secret", idp.OIDC.ClientSecret)
+		checkNotEmpty(configErrs, "client_api.sso.providers.oidc.client_secret", idp.OAuth2.ClientSecret)
 
 	default:
-		configErrs.Add(fmt.Sprintf("unrecognised type in identity provider %q for config key %q: %s", idp.ID, "client_api.sso.providers", typ))
+		configErrs.Add(fmt.Sprintf("unrecognised type in identity provider %q for config key %q: %s", idp.ID, "client_api.sso.providers", idp.Type))
 	}
 }
 
@@ -231,6 +257,7 @@ func checkIdentityProviderBrand(s SSOBrand) bool {
 	}
 }
 
+// SSOBrand corresponds to https://github.com/matrix-org/matrix-spec-proposals/blob/old_master/informal/idp-brands.md
 type SSOBrand string
 
 const (
@@ -242,6 +269,25 @@ const (
 	SSOBrandTwitter  SSOBrand = "twitter"
 )
 
+var (
+	oidcDefaultDiscoveryURLs = map[SSOBrand]string{
+		// https://developers.facebook.com/docs/facebook-login/limited-login/token/
+		SSOBrandFacebook: "https://www.facebook.com/.well-known/openid-configuration/",
+		// https://docs.gitlab.com/ee/integration/openid_connect_provider.html
+		SSOBrandGitLab: "https://gitlab.com/.well-known/openid-configuration",
+		// https://developers.google.com/identity/protocols/oauth2/openid-connect
+		SSOBrandGoogle: "https://accounts.google.com/.well-known/openid-configuration",
+	}
+	oidcDefaultNames = map[SSOBrand]string{
+		SSOBrandApple:    "Apple",
+		SSOBrandFacebook: "Facebook",
+		SSOBrandGitHub:   "GitHub",
+		SSOBrandGitLab:   "GitLab",
+		SSOBrandGoogle:   "Google",
+		SSOBrandTwitter:  "Twitter",
+	}
+)
+
 type IdentityProviderType string
 
 const (