From 54b7bee1fdccd3ef275c9493640ed25eb914a28f Mon Sep 17 00:00:00 2001 From: Erwan Guyader Date: Tue, 26 Nov 2024 17:12:05 +0100 Subject: [PATCH 1/2] test: Setup cloudery config in WithManager helper The `WithManager` test helper is meant to help setup the test context for requests to a cloudery but it also depends on modifications made to the context config. We move these modifications to the helper itself to make it self-contained and avoid unexpected test results when using it without deep knowledge of its dependencies. We alos make sure to modify the config as little as possible and the changes once the test is completed. --- model/oauth/client_test.go | 4 +--- pkg/config/config/config.go | 1 + tests/testutils/test_utils.go | 42 ++++++++++++++++++++++++++--------- web/apps/apps_test.go | 3 +-- web/settings/settings_test.go | 5 +++-- 5 files changed, 38 insertions(+), 17 deletions(-) diff --git a/model/oauth/client_test.go b/model/oauth/client_test.go index e3d7cbb6f7a..952d9d0c4a3 100644 --- a/model/oauth/client_test.go +++ b/model/oauth/client_test.go @@ -32,8 +32,6 @@ func TestClient(t *testing.T) { } config.UseTestFile(t) - conf := config.GetConfig() - conf.Contexts[config.DefaultInstanceContext] = map[string]interface{}{"manager_url": "http://manager.example.org"} setup := testutils.NewSetup(t, t.Name()) testInstance := setup.GetTestInstance() @@ -197,7 +195,7 @@ func TestClient(t *testing.T) { premiumLink := assertClientsLimitAlertMailWasSent(t, testInstance, "notificationWithoutPremium", 1) assert.Empty(t, premiumLink) - testutils.WithManager(t, testInstance) + testutils.WithManager(t, testInstance, testutils.ManagerConfig{URL: "http://manager.example.org", WithPremiumLinks: true}) notificationWithPremium = &oauth.Client{ ClientName: "notificationWithPremium", diff --git a/pkg/config/config/config.go b/pkg/config/config/config.go index 8199fb1c122..0b9e31f412b 100644 --- a/pkg/config/config/config.go +++ b/pkg/config/config/config.go @@ -1150,6 +1150,7 @@ func createTestViper() *viper.Viper { v.SetDefault("log.level", "info") v.SetDefault("assets_polling_disabled", false) v.SetDefault("assets_polling_interval", 2*time.Minute) + v.SetDefault("contexts", map[string]interface{}{DefaultInstanceContext: map[string]interface{}{}}) applyDefaults(v) return v } diff --git a/tests/testutils/test_utils.go b/tests/testutils/test_utils.go index 66ff3f5b274..34ca8db5f98 100644 --- a/tests/testutils/test_utils.go +++ b/tests/testutils/test_utils.go @@ -3,6 +3,7 @@ package testutils import ( "bytes" "context" + "encoding/json" "errors" "flag" "fmt" @@ -469,7 +470,14 @@ func compress(content string) []byte { return buf.Bytes() } -func WithManager(t *testing.T, inst *instance.Instance) (shouldRemoveUUID bool) { +type ManagerConfig struct { + URL string + WithPremiumLinks bool +} + +func WithManager(t *testing.T, inst *instance.Instance, managerConfig ManagerConfig) (shouldRemoveUUID bool) { + require.NotEmpty(t, managerConfig.URL, "Could not enable test instance manager: cloudery API URL is required") + if inst.UUID == "" { uuid, err := uuid.NewV7() require.NoError(t, err, "Could not enable test instance manager") @@ -477,18 +485,19 @@ func WithManager(t *testing.T, inst *instance.Instance) (shouldRemoveUUID bool) shouldRemoveUUID = true } - config, ok := inst.SettingsContext() - require.True(t, ok, "Could not enable test instance manager: could not fetch test instance settings context") - - managerURL, ok := config["manager_url"].(string) - require.True(t, ok, "Could not enable test instance manager: manager_url config is required") - require.NotEmpty(t, managerURL, "Could not enable test instance manager: manager_url config is required") + cfg := config.GetConfig() + originalCfg, _ := json.Marshal(cfg) - was := config["enable_premium_links"] - config["enable_premium_links"] = true + if cfg.Contexts == nil { + cfg.Contexts = map[string]interface{}{} + } + context, contextName := getContext(inst, cfg) + context["manager_url"] = managerConfig.URL + context["enable_premium_links"] = managerConfig.WithPremiumLinks + cfg.Contexts[contextName] = context t.Cleanup(func() { - config["enable_premium_links"] = was + json.Unmarshal(originalCfg, cfg) if shouldRemoveUUID { inst.UUID = "" @@ -502,6 +511,19 @@ func WithManager(t *testing.T, inst *instance.Instance) (shouldRemoveUUID bool) return shouldRemoveUUID } +// getContext returns the most relevant context config depending on the +// instance context name or creates a new one if none exist. +// It also returns the name of the context associated with the config. +func getContext(inst *instance.Instance, cfg *config.Config) (map[string]interface{}, string) { + if context, ok := cfg.Contexts[inst.ContextName].(map[string]interface{}); ok { + return context, inst.ContextName + } + if context, ok := cfg.Contexts[config.DefaultInstanceContext].(map[string]interface{}); ok { + return context, config.DefaultInstanceContext + } + return map[string]interface{}{}, config.DefaultInstanceContext +} + func DisableManager(inst *instance.Instance, shouldRemoveUUID bool) error { config, ok := inst.SettingsContext() if !ok { diff --git a/web/apps/apps_test.go b/web/apps/apps_test.go index c8b02d83dd8..e85523cea5b 100644 --- a/web/apps/apps_test.go +++ b/web/apps/apps_test.go @@ -66,7 +66,6 @@ func TestApps(t *testing.T) { Host: "localhost", Path: tempdir, } - cfg.Contexts[config.DefaultInstanceContext] = map[string]interface{}{"manager_url": "http://manager.example.org"} was := cfg.Subdomains cfg.Subdomains = config.NestedSubdomains defer func() { cfg.Subdomains = was }() @@ -205,7 +204,7 @@ func TestApps(t *testing.T) { // TOS not signed warning - testutils.WithManager(t, testInstance) + testutils.WithManager(t, testInstance, testutils.ManagerConfig{URL: "http://manager.example.org", WithPremiumLinks: true}) tosSigned := testInstance.TOSSigned tosLatest := testInstance.TOSLatest diff --git a/web/settings/settings_test.go b/web/settings/settings_test.go index b476a39dff7..50b2dd5d374 100644 --- a/web/settings/settings_test.go +++ b/web/settings/settings_test.go @@ -74,7 +74,6 @@ func TestSettings(t *testing.T) { conf := config.GetConfig() conf.Assets = "../../assets" conf.Contexts[config.DefaultInstanceContext] = map[string]interface{}{ - "manager_url": "http://manager.example.org", "logos": map[string]interface{}{ "home": map[string]interface{}{ "light": []interface{}{ @@ -112,6 +111,8 @@ func TestSettings(t *testing.T) { t.Run("GetContext", func(t *testing.T) { e := testutils.CreateTestClient(t, tsURL) + testutils.WithManager(t, testInstance, testutils.ManagerConfig{URL: "http://manager.example.org"}) + obj := e.GET("/settings/context"). WithCookie(sessCookie, "connected"). WithHeader("Accept", "application/vnd.api+json"). @@ -1021,7 +1022,7 @@ func TestSettings(t *testing.T) { Contains("/#/connectedDevices"). NotContains("http://manager.example.org") - testutils.WithManager(t, testInstance) + testutils.WithManager(t, testInstance, testutils.ManagerConfig{URL: "http://manager.example.org", WithPremiumLinks: true}) e.GET("/settings/clients/limit-exceeded"). WithCookie(sessCookie, "connected"). From d364aca6465f1f2e1f0d34fdee6b44bf6199e656 Mon Sep 17 00:00:00 2001 From: Erwan Guyader Date: Wed, 27 Nov 2024 10:53:03 +0100 Subject: [PATCH 2/2] fix: Remove duplicated manager url config We used to store the manager url in the context config in the `manager_url` attribute. We're now storing it as part of the `clouderies` config so we can remove the old attribute and its use. However, some clients still make use of the old context attribute so we're adding it back in the context API response for backwards compatibility. --- model/instance/instance_test.go | 4 +-- model/instance/manager.go | 35 ++++++++++++++------- pkg/config/config/config_test.go | 7 ++++- pkg/config/config/testdata/full_config.yaml | 5 ++- tests/testutils/test_utils.go | 20 ++++++++++++ web/auth/confirm.go | 8 ++--- web/settings/context.go | 22 +++++++++++-- 7 files changed, 77 insertions(+), 24 deletions(-) diff --git a/model/instance/instance_test.go b/model/instance/instance_test.go index af18fde7b5d..d2c8837c56c 100644 --- a/model/instance/instance_test.go +++ b/model/instance/instance_test.go @@ -61,7 +61,6 @@ func TestInstance(t *testing.T) { cfg.Contexts = map[string]interface{}{ "context": map[string]interface{}{ - "manager_url": "http://manager.example.org", "logos": map[string]interface{}{ "coachco2": map[string]interface{}{ "light": []interface{}{ @@ -173,8 +172,7 @@ func TestInstance(t *testing.T) { } ] } - }, - "manager_url": "http://manager.example.org" + } }` assert.Equal(t, expected, string(bytes)) }) diff --git a/model/instance/manager.go b/model/instance/manager.go index ebfe579baac..61bdc61054f 100644 --- a/model/instance/manager.go +++ b/model/instance/manager.go @@ -19,22 +19,24 @@ const ( ManagerPremiumURL // ManagerBlockedURL is the kind for a redirection of a blocked instance. ManagerBlockedURL + // ManagerBaseURL is the kind for building other manager URLs + ManagerBaseURL ) // ManagerURL returns an external string for the given ManagerURL kind. It is // used for redirecting the user to a manager URL. func (i *Instance) ManagerURL(k ManagerURLKind) (string, error) { - if i.UUID == "" { + c := clouderyConfig(i) + if c == nil { return "", nil } - config, ok := i.SettingsContext() - if !ok { + if i.UUID == "" { return "", nil } - base, ok := config["manager_url"].(string) - if !ok { + base := c.API.URL + if base == "" { return "", nil } @@ -51,6 +53,8 @@ func (i *Instance) ManagerURL(k ManagerURLKind) (string, error) { path = fmt.Sprintf("/cozy/instances/%s/tos", url.PathEscape(i.UUID)) case ManagerBlockedURL: path = fmt.Sprintf("/cozy/instances/%s/blocked", url.PathEscape(i.UUID)) + case ManagerBaseURL: + path = "" default: panic("unknown ManagerURLKind") } @@ -61,6 +65,20 @@ func (i *Instance) ManagerURL(k ManagerURLKind) (string, error) { // APIManagerClient returns a client to talk to the manager via its API. func APIManagerClient(inst *Instance) *manager.APIClient { + c := clouderyConfig(inst) + if c == nil { + return nil + } + + api := c.API + if api.URL == "" || api.Token == "" { + return nil + } + + return manager.NewAPIClient(api.URL, api.Token) +} + +func clouderyConfig(inst *Instance) *config.ClouderyConfig { clouderies := config.GetConfig().Clouderies if clouderies == nil { return nil @@ -75,10 +93,5 @@ func APIManagerClient(inst *Instance) *manager.APIClient { return nil } - api := cloudery.API - if api.URL == "" || api.Token == "" { - return nil - } - - return manager.NewAPIClient(api.URL, api.Token) + return &cloudery } diff --git a/pkg/config/config/config_test.go b/pkg/config/config/config_test.go index de8c219a796..c3af112285b 100644 --- a/pkg/config/config/config_test.go +++ b/pkg/config/config/config_test.go @@ -116,7 +116,6 @@ func TestConfigUnmarshal(t *testing.T) { // Contexts assert.EqualValues(t, map[string]interface{}{ "my-context": map[string]interface{}{ - "manager_url": "https://manager-url", "onboarded_redirection": "home/intro", "default_redirection": "home/", "help_link": "https://cozy.io/fr/support", @@ -216,6 +215,12 @@ func TestConfigUnmarshal(t *testing.T) { Token: "some-token", }, }, + "my-context": { + API: ClouderyAPI{ + URL: "https://manager-url", + Token: "manager-token", + }, + }, }, cfg.Clouderies) // CSPs diff --git a/pkg/config/config/testdata/full_config.yaml b/pkg/config/config/testdata/full_config.yaml index d827a587f0c..ab753424ab6 100644 --- a/pkg/config/config/testdata/full_config.yaml +++ b/pkg/config/config/testdata/full_config.yaml @@ -109,6 +109,10 @@ clouderies: api: url: https://some-url token: some-token + my-context: + api: + url: https://manager-url + token: manager-token password_reset_interval: 1h @@ -163,7 +167,6 @@ log: contexts: my-context: - manager_url: https://manager-url onboarded_redirection: home/intro default_redirection: home/ help_link: https://cozy.io/fr/support diff --git a/tests/testutils/test_utils.go b/tests/testutils/test_utils.go index 34ca8db5f98..e792f445a7c 100644 --- a/tests/testutils/test_utils.go +++ b/tests/testutils/test_utils.go @@ -488,6 +488,13 @@ func WithManager(t *testing.T, inst *instance.Instance, managerConfig ManagerCon cfg := config.GetConfig() originalCfg, _ := json.Marshal(cfg) + if cfg.Clouderies == nil { + cfg.Clouderies = map[string]config.ClouderyConfig{} + } + cloudery, contextName := getCloudery(inst, cfg) + cloudery.API = config.ClouderyAPI{URL: managerConfig.URL, Token: ""} + cfg.Clouderies[contextName] = cloudery + if cfg.Contexts == nil { cfg.Contexts = map[string]interface{}{} } @@ -511,6 +518,19 @@ func WithManager(t *testing.T, inst *instance.Instance, managerConfig ManagerCon return shouldRemoveUUID } +// getCloudery returns the most relevant cloudery config depending on the +// instance context name or creates a new one if none exist. +// It also returns the name of the context associated with the config. +func getCloudery(inst *instance.Instance, cfg *config.Config) (config.ClouderyConfig, string) { + if cloudery, ok := cfg.Clouderies[inst.ContextName]; ok { + return cloudery, inst.ContextName + } + if cloudery, ok := cfg.Clouderies[config.DefaultInstanceContext]; ok { + return cloudery, config.DefaultInstanceContext + } + return config.ClouderyConfig{}, config.DefaultInstanceContext +} + // getContext returns the most relevant context config depending on the // instance context name or creates a new one if none exist. // It also returns the name of the context associated with the config. diff --git a/web/auth/confirm.go b/web/auth/confirm.go index 0a2c64ffe4a..895db8479da 100644 --- a/web/auth/confirm.go +++ b/web/auth/confirm.go @@ -149,12 +149,8 @@ func checkRedirectToAuthorized(c echo.Context) (*url.URL, error) { } func checkRedirectToManager(inst *instance.Instance, redirect *url.URL) bool { - config, ok := inst.SettingsContext() - if !ok { - return false - } - managerURL, ok := config["manager_url"].(string) - if !ok { + managerURL, err := inst.ManagerURL(instance.ManagerBaseURL) + if err != nil { return false } manager, err := url.Parse(managerURL) diff --git a/web/settings/context.go b/web/settings/context.go index fc036e06d3b..2af737d9f6c 100644 --- a/web/settings/context.go +++ b/web/settings/context.go @@ -4,6 +4,7 @@ import ( "encoding/json" "net/http" + "github.com/cozy/cozy-stack/model/instance" "github.com/cozy/cozy-stack/model/instance/lifecycle" "github.com/cozy/cozy-stack/pkg/consts" "github.com/cozy/cozy-stack/pkg/couchdb" @@ -78,6 +79,23 @@ func (h *HTTPHandler) context(c echo.Context) error { } i := middlewares.GetInstance(c) - doc := &apiContext{i.GetContextWithSponsorships()} - return jsonapi.Data(c, http.StatusOK, doc, nil) + context := &apiContext{i.GetContextWithSponsorships()} + + managerURL, err := i.ManagerURL(instance.ManagerBaseURL) + if err != nil { + return err + } + if managerURL != "" { + // XXX: The manager URL used to be stored in the config in + // `context..manager_url`. It's now stored in + // `clouderies..api.url` and can be retrieved via a call + // to `instance.ManagerURL()`. + // + // However, some external apps and clients (e.g. `cozy-client`) still + // expect to find the `manager_url` attribute in the context document + // so we add it back for backwards compatibility. + context.doc["manager_url"] = managerURL + } + + return jsonapi.Data(c, http.StatusOK, context, nil) }