Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

defsetting should be aware of enterprise restrictions #23414

Closed
dpsutton opened this issue Jun 17, 2022 · 0 comments · Fixed by #23441
Closed

defsetting should be aware of enterprise restrictions #23414

dpsutton opened this issue Jun 17, 2022 · 0 comments · Fixed by #23441
Assignees
Labels
.Epic Feature Implementation or Project

Comments

@dpsutton
Copy link
Contributor

Settings for enterprise can be surprisingly difficult. There are largely two classes of enterprise settings

Whole feature

Settings that are required from a whole enterprise feature that is optionally on the classpath.
Things like sso and ldap. These settings are required and used from whole enterprise namespaces. Thus the calling code just requires the namespace as normal, and the code only executes (and the setting value accessed) in the case that the feature/token is checked.

A concrete example is

;; setting in metabase-enterprise.sso.integrations.sso-settings
(defsetting saml-identity-provider-uri
  (deferred-tru "This is the URL where your users go to log in to your identity provider. Depending on which IdP you''re
using, this usually looks like https://your-org-name.example.com or https://example.com/app/my_saml_app/abc123/sso/saml"))

;; and used from metabase-enterprise.sso.integrations.saml

(sso-settings/saml-identity-provider-uri)

These settings typically have no use outside of the encapsulated enterprise feature and token checks around the feature ensure that the setting is meaningless unless the code is on the classpath and the enterprise feature is active.

Some subtleties to this strategy: depending on how the enterprise code is required, these settings might or might not be registered. If the code is required top-level then the setting is registered and you can change its value. If the code is conditionally required then the setting is not registered and you cannot change its value. But due to the way these settings are used and thus changing values even when the feature is not enabled will not have any effect.

;; unconditionally required settings

;; from metabase.integrations.ldap.interface

;; Load the EE namespace up front so that the extra Settings it defines are available immediately.
;; Otherwise, this would only happen the first time one of the functions defined using `defenterprise` is called.
(u/ignore-exceptions (classloader/require ['metabase-enterprise.enhancements.integrations.ldap]))

;; conditionally required ee code/settings
;; from metabase.public-settings
(defn- ee-sso-configured? []
  (u/ignore-exceptions
    (classloader/require 'metabase-enterprise.sso.integrations.sso-settings))
  (when-let [varr (resolve 'metabase-enterprise.sso.integrations.sso-settings/other-sso-configured?)]
    (varr)))

Again the impact of a setting being registered or not impacts the setting being able to be set by the api. But these settings will not affect product functionality since the entire feature is correctly behind enterprise feature flags.

Enterprise allows modification of OSS behavior

This other class of enterprise setting lives inside of the OSS app. Concrete examples are application colors, soon-to-be reply-to header for emails, and other settings which modify the default OSS behavior of the app. Two concrete examples are

(defsetting application-colors
  (deferred-tru
   (str "These are the primary colors used in charts and throughout Metabase. "
        "You might need to refresh your browser to see your changes take effect."))
  :visibility :public
  :type       :json
  :default    {})

(defsetting application-font
  (deferred-tru
   (str "This is the primary font used in charts and throughout Metabase. "
        "You might need to refresh your browser to see your changes take effect."))
  :visibility :public
  :type       :string
  :default    "Lato"
  :setter (fn [new-value]
              (when new-value
                (when-not (u.fonts/available-font? new-value)
                  (throw (ex-info (tru "Invalid font {0}" (pr-str new-value)) {:status-code 400}))))
            (setting/set-value-of-type! :string :application-font new-value)))

These settings are registered in OSS (and thus can be set) and are directly used in OSS code. The way we attempt to lock down this functionality is by only exposing the UI to set them in enterprise frontend code. But this means that they can be set by the api and they will then affect application behavior without the enterprise features. For instance, (public-settings/application-colors) is used in metabase.pulse.render.body for static-viz.

Some code jumps through hoops to try to enforce that the setting's value is only used/set if the token is active.

;; definition in metabase-enterprise.embedding.utils
(defsetting notification-link-base-url
  (deferred-tru "By default \"Site Url\" is used in notification links, but can be overridden.")
  :visibility :internal
  :getter (fn []
            (when (premium-features/hide-embed-branding?)
              (or (setting/get-value-of-type :string :notification-link-base-url)
                  (public-settings/site-url)))))

;; awkward usage in regular code
;; we want to load this at the top level so the Setting the namespace defines gets loaded
(def ^:private site-url*
  (or (u/ignore-exceptions
        (classloader/require 'metabase-enterprise.embedding.utils)
        (resolve 'metabase-enterprise.embedding.utils/notification-link-base-url))
      (constantly nil)))

And here the individual implementation has to do two things:

  • custom getter that does the token check
  • an extra var to conditionally require the enterprise code and provide a fallback

All of this leads to lots of problems. Doing this correctly is a pain. It is so easy to forget to add these checks (like colors and font), and then if you do remember to add these checks, your reward is a painful experience. Further, forgetting these checks and putting the settings in metabase.public-settings obscures which settings are intended to be enterprise settings. Then the source of that lives in which ones are editable from enterprise admin panes in the frontend.

Solutions

Annotate with feature

Allow the setting to be defined in public-settings or any other OSS namespace. Extend the defsetting macro to understand an enterprise flag or set of flags.

(defsetting application-colors
  (deferred-tru
   (str "These are the primary colors used in charts and throughout Metabase. "
        "You might need to refresh your browser to see your changes take effect."))
  :visibility :public
  :type       :json
  :default    {}
  :enterprise :any)

This would add the necessary checks on get/set (see metabase.public-settings.premium-features for some related work). We can also send this information over to the frontend.

@dpsutton dpsutton added the .Epic Feature Implementation or Project label Jun 17, 2022
This was referenced Jun 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.Epic Feature Implementation or Project
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant