From bbba66f97f32e69fef3bfd37915658453676bc55 Mon Sep 17 00:00:00 2001 From: Chris Smith Date: Wed, 1 May 2024 17:38:41 -0700 Subject: [PATCH] Use newer config settings if available (allow local SAMS/SSC dev) (#62344) * Totally unrelated refactoring * Use newer config settings if available --- .../graphqlbackend/external_account.go | 3 +- cmd/frontend/internal/app/ui/router.go | 19 +++--- cmd/frontend/internal/cody/subscription.go | 8 +-- .../internal/cody/subscription_test.go | 3 +- .../codygateway_dotcom_user_test.go | 9 ++- cmd/frontend/internal/httpapi/ssc.go | 2 +- cmd/frontend/internal/ssc/ssc.go | 58 ++++++++++++++----- 7 files changed, 64 insertions(+), 38 deletions(-) diff --git a/cmd/frontend/graphqlbackend/external_account.go b/cmd/frontend/graphqlbackend/external_account.go index 364926d324d..b6a3a835d59 100644 --- a/cmd/frontend/graphqlbackend/external_account.go +++ b/cmd/frontend/graphqlbackend/external_account.go @@ -2,7 +2,6 @@ package graphqlbackend import ( "context" - "fmt" "github.com/graph-gophers/graphql-go" "github.com/graph-gophers/graphql-go/relay" @@ -63,7 +62,7 @@ func (r *externalAccountResolver) CodySubscription(ctx context.Context) (*CodySu return nil, errors.New("this feature is only available on sourcegraph.com") } - if r.account.ServiceType != "openidconnect" || r.account.ServiceID != fmt.Sprintf("https://%s", ssc.GetSAMSHostName()) { + if r.account.ServiceType != "openidconnect" || r.account.ServiceID != ssc.GetSAMSServiceID() { return nil, nil } diff --git a/cmd/frontend/internal/app/ui/router.go b/cmd/frontend/internal/app/ui/router.go index e66afe0c65b..920f47fa620 100644 --- a/cmd/frontend/internal/app/ui/router.go +++ b/cmd/frontend/internal/app/ui/router.go @@ -80,6 +80,13 @@ var aboutRedirects = map[string]string{ "help/terms": "terms", } +type staticPageInfo struct { + // Specify either path OR pathPrefix. + path, pathPrefix string + name, title string + index bool +} + // Router returns the router that serves pages for our web app. func Router() *mux.Router { return uirouter.Router @@ -111,13 +118,8 @@ func InitRouter(db database.DB) { ghAppRouter := r.PathPrefix("/githubapp/").Subrouter() githubapp.SetupGitHubAppRoutes(ghAppRouter, db) - // Basic pages with static titles - for _, p := range []struct { - // Specify either path OR pathPrefix. - path, pathPrefix string - name, title string - index bool - }{ + // Basic pages with static titles. + staticPages := []staticPageInfo{ // with index: {pathPrefix: "/insights", name: "insights", title: "Insights", index: true}, {pathPrefix: "/search-jobs", name: "search-jobs", title: "Search Jobs", index: true}, @@ -160,7 +162,8 @@ func InitRouter(db database.DB) { {path: "/survey", name: "survey", title: "Survey", index: false}, {path: "/survey/{score}", name: "survey-score", title: "Survey", index: false}, {path: "/welcome", name: "welcome", title: "Welcome", index: false}, - } { + } + for _, p := range staticPages { var handler http.Handler if p.index { handler = brandedIndex(p.title) diff --git a/cmd/frontend/internal/cody/subscription.go b/cmd/frontend/internal/cody/subscription.go index 229f05289e0..003bb46b28e 100644 --- a/cmd/frontend/internal/cody/subscription.go +++ b/cmd/frontend/internal/cody/subscription.go @@ -2,7 +2,6 @@ package cody import ( "context" - "fmt" "sync" "time" @@ -88,14 +87,10 @@ func consolidateSubscriptionDetails(ctx context.Context, user types.User, subscr // will return ("", nil). After we migrate all dotcom user accounts to SAMS, that // should no longer be possible. func getSAMSAccountIDsForUser(ctx context.Context, db database.DB, dotcomUserID int32) ([]string, error) { - // NOTE: We hard-code this to look for the SAMS-prod environment, meaning there isn't a way - // to test dotcom pulling subscription data from a local SAMS/SSC instance. To support that - // we'd need to make the SAMSHostname configurable. (Or somehow identify which OIDC provider - // is SAMS.) oidcAccounts, err := db.UserExternalAccounts().List(ctx, database.ExternalAccountsListOptions{ UserID: dotcomUserID, ServiceType: "openidconnect", - ServiceID: fmt.Sprintf("https://%s", ssc.GetSAMSHostName()), + ServiceID: ssc.GetSAMSServiceID(), }) if err != nil { return []string{}, errors.Wrap(err, "listing external accounts") @@ -137,7 +132,6 @@ func SubscriptionForUser(ctx context.Context, db database.DB, user types.User) ( // While developing the SSC backend, we only fetch subscription data for users based on a flag. var subscription *ssc.Subscription if samsAccountID != "" { - subscription, err = sscClient.FetchSubscriptionBySAMSAccountID(ctx, samsAccountID) if err != nil { return nil, errors.Wrap(err, "fetching subscription from SSC") diff --git a/cmd/frontend/internal/cody/subscription_test.go b/cmd/frontend/internal/cody/subscription_test.go index e9581986637..ecb77d810c4 100644 --- a/cmd/frontend/internal/cody/subscription_test.go +++ b/cmd/frontend/internal/cody/subscription_test.go @@ -2,7 +2,6 @@ package cody import ( "context" - "fmt" "testing" "time" @@ -130,7 +129,7 @@ func TestGetSubscriptionForUser(t *testing.T) { accounts = append(accounts, &extsvc.Account{AccountSpec: extsvc.AccountSpec{ AccountID: MockSSCValue.SAMSAccountID, ServiceType: "openidconnect", - ServiceID: fmt.Sprintf("https://%s/", ssc.GetSAMSHostName()), + ServiceID: ssc.GetSAMSServiceID(), }}) } diff --git a/cmd/frontend/internal/dotcom/productsubscription/codygateway_dotcom_user_test.go b/cmd/frontend/internal/dotcom/productsubscription/codygateway_dotcom_user_test.go index 1385d21a6bb..b14a00dae44 100644 --- a/cmd/frontend/internal/dotcom/productsubscription/codygateway_dotcom_user_test.go +++ b/cmd/frontend/internal/dotcom/productsubscription/codygateway_dotcom_user_test.go @@ -2,7 +2,6 @@ package productsubscription_test import ( "context" - "fmt" "math" "testing" "time" @@ -313,7 +312,7 @@ func TestCodyGatewayCompletionsRateLimit(t *testing.T) { AccountSpec: extsvc.AccountSpec{ AccountID: "123", ServiceType: "openidconnect", - ServiceID: fmt.Sprintf("https://%s", ssc.GetSAMSHostName()), + ServiceID: ssc.GetSAMSServiceID(), }, }) require.NoError(t, err) @@ -328,7 +327,7 @@ func TestCodyGatewayCompletionsRateLimit(t *testing.T) { AccountSpec: extsvc.AccountSpec{ AccountID: "456", ServiceType: "openidconnect", - ServiceID: fmt.Sprintf("https://%s", ssc.GetSAMSHostName()), + ServiceID: ssc.GetSAMSServiceID(), }, }) require.NoError(t, err) @@ -343,7 +342,7 @@ func TestCodyGatewayCompletionsRateLimit(t *testing.T) { AccountSpec: extsvc.AccountSpec{ AccountID: "789", ServiceType: "openidconnect", - ServiceID: fmt.Sprintf("https://%s", ssc.GetSAMSHostName()), + ServiceID: ssc.GetSAMSServiceID(), }, }) require.NoError(t, err) @@ -358,7 +357,7 @@ func TestCodyGatewayCompletionsRateLimit(t *testing.T) { AccountSpec: extsvc.AccountSpec{ AccountID: "abc", ServiceType: "openidconnect", - ServiceID: fmt.Sprintf("https://%s", ssc.GetSAMSHostName()), + ServiceID: ssc.GetSAMSServiceID(), }, }) require.NoError(t, err) diff --git a/cmd/frontend/internal/httpapi/ssc.go b/cmd/frontend/internal/httpapi/ssc.go index 9ca4ee6de7a..b1c55a3930e 100644 --- a/cmd/frontend/internal/httpapi/ssc.go +++ b/cmd/frontend/internal/httpapi/ssc.go @@ -27,7 +27,7 @@ func newSSCRefreshCodyRateLimitHandler(logger log.Logger, db database.DB) http.H oidcAccounts, err := db.UserExternalAccounts().List(ctx, database.ExternalAccountsListOptions{ AccountID: samsAccountID, ServiceType: "openidconnect", - ServiceID: fmt.Sprintf("https://%s", ssc.GetSAMSHostName()), + ServiceID: ssc.GetSAMSServiceID(), LimitOffset: &database.LimitOffset{ Limit: 1, }, diff --git a/cmd/frontend/internal/ssc/ssc.go b/cmd/frontend/internal/ssc/ssc.go index abee5db55c4..8ca81775284 100644 --- a/cmd/frontend/internal/ssc/ssc.go +++ b/cmd/frontend/internal/ssc/ssc.go @@ -113,13 +113,52 @@ func (c *client) FetchSubscriptionBySAMSAccountID(ctx context.Context, samsAccou } } -func GetSAMSHostName() string { - sgconf := conf.Get().SiteConfig() +// getSSCBaseURL returns the base URL for the SSC backend's REST API for +// service-to-service requests. +func getSSCBaseURL() string { + config := conf.Get() + + // Prefer the newer "dotcom.codyProConfig.sscBackendOrigin" config setting if available. + // This allows for local development (not hard-coding the https scheme). + if dotcomConfig := config.Dotcom; dotcomConfig != nil { + if codyProConfig := dotcomConfig.CodyProConfig; codyProConfig != nil { + return fmt.Sprintf("%s/cody/api", codyProConfig.SscBackendOrigin) + } + } + + // Fall back to original logic, using the "ssc.apiBaseUrl" setting. + // (To be removed when the codyProConfig changes are in production.) + siteConfig := config.SiteConfig() + baseURL := siteConfig.SscApiBaseUrl + if baseURL == "" { + baseURL = "https://accounts.sourcegraph.com/cody/api" + } + + return baseURL +} + +// GetSAMSServiceID returns the ServiceID of the currently registered SAMS identity provider. +// This is found in the site configuration, and must match the auth.providers configuration +// exactly. +func GetSAMSServiceID() string { + config := conf.Get() + + // Prefer the newer "dotcom.codyProConfig.samsBackendOrigin" config setting if available. + // This allows for local development (not hard-coding the https scheme). + if dotcomConfig := config.Dotcom; dotcomConfig != nil { + if codyProConfig := dotcomConfig.CodyProConfig; codyProConfig != nil { + return codyProConfig.SamsBackendOrigin + } + } + + // Fallback to the original logic, using the "ssc.samsHostName" setting. + // (To be removed when the codyProConfig changes are in production.) + sgconf := config.SiteConfig() if sgconf.SscSamsHostName == "" { // If unset, default to the production hostname. - return "accounts.sourcegraph.com" + return "https://accounts.sourcegraph.com" } - return sgconf.SscSamsHostName + return fmt.Sprintf("https://%s", sgconf.SscSamsHostName) } // NewClient returns a new SSC API client. It is important to avoid creating new @@ -130,8 +169,6 @@ func GetSAMSHostName() string { // If no SAMS authorization provider is configured, this function will not panic, // but instead will return an error on every call. func NewClient() (Client, error) { - sgconf := conf.Get().SiteConfig() - // Fetch the SAMS configuration data. var samsConfig *clientcredentials.Config for _, provider := range conf.Get().AuthProviders { @@ -140,7 +177,7 @@ func NewClient() (Client, error) { continue } - if strings.Contains(oidcInfo.Issuer, GetSAMSHostName()) { + if oidcInfo.Issuer == GetSAMSServiceID() { samsConfig = &clientcredentials.Config{ ClientID: oidcInfo.ClientID, ClientSecret: oidcInfo.ClientSecret, @@ -155,17 +192,12 @@ func NewClient() (Client, error) { return &client{}, errors.New("no SAMS authorization provider configured") } - baseURL := sgconf.SscApiBaseUrl - if baseURL == "" { - baseURL = "https://accounts.sourcegraph.com/cody/api" - } - // We want this tokenSource to be long lived, so we benefit from reusing existing // SAMS tokens if repeated requests are made within the token's lifetime. (Under // the hood it returns an oauth2.ReuseTokenSource.) tokenSource := samsConfig.TokenSource(context.Background()) return &client{ - baseURL: baseURL, + baseURL: getSSCBaseURL(), samsTokenSource: tokenSource, }, nil }