From 31ac054feddb1269457c56ac51dda684bf2045a8 Mon Sep 17 00:00:00 2001 From: Alexander Morozov Date: Thu, 12 Dec 2019 11:00:56 -0800 Subject: [PATCH] OAuth: Removes send_client_credentials_via_post setting (#20044) Removes send_client_credentials_via_post oauth setting and use auto-detect mechanism instead. By these changes also fixes statichcheck errors Ref #8968 --- conf/defaults.ini | 1 - conf/sample.ini | 4 -- docs/sources/auth/generic-oauth.md | 14 ----- docs/sources/installation/upgrading.md | 6 ++- pkg/login/social/social.go | 53 +++++++++---------- .../provisioning/dashboards/file_reader.go | 5 +- pkg/setting/setting_oauth.go | 33 ++++++------ 7 files changed, 47 insertions(+), 69 deletions(-) diff --git a/conf/defaults.ini b/conf/defaults.ini index 5b834e8e89c0c..0be9f182b79e7 100644 --- a/conf/defaults.ini +++ b/conf/defaults.ini @@ -387,7 +387,6 @@ tls_skip_verify_insecure = false tls_client_cert = tls_client_key = tls_client_ca = -send_client_credentials_via_post = false #################################### SAML Auth ########################### [auth.saml] # Enterprise only diff --git a/conf/sample.ini b/conf/sample.ini index a641b2b6f6948..a266c316202c1 100644 --- a/conf/sample.ini +++ b/conf/sample.ini @@ -378,10 +378,6 @@ ;tls_client_key = ;tls_client_ca = -; Set to true to enable sending client_id and client_secret via POST body instead of Basic authentication HTTP header -; This might be required if the OAuth provider is not RFC6749 compliant, only supporting credentials passed via POST payload -;send_client_credentials_via_post = false - #################################### SAML Auth ########################### [auth.saml] # Enterprise only # Defaults to false. If true, the feature is enabled. diff --git a/docs/sources/auth/generic-oauth.md b/docs/sources/auth/generic-oauth.md index 3f5a3f8758f4e..2866e0aaa1222 100755 --- a/docs/sources/auth/generic-oauth.md +++ b/docs/sources/auth/generic-oauth.md @@ -219,20 +219,6 @@ allowed_organizations = auth_url = https://.my.centrify.com/OAuth2/Authorize/ token_url = https://.my.centrify.com/OAuth2/Token/ api_url = https://.my.centrify.com/OAuth2/UserInfo/ - ``` - -## Set up OAuth2 with non-compliant providers - -> Only available in Grafana v6.0 and above. - -Some OAuth2 providers might not support `client_id` and `client_secret` passed via Basic Authentication HTTP header, which -results in `invalid_client` error. To allow Grafana to authenticate via these type of providers, the client identifiers must be -send via POST body, which can be enabled via the following settings: - -```bash -[auth.generic_oauth] -send_client_credentials_via_post = true -``` ## JMESPath examples diff --git a/docs/sources/installation/upgrading.md b/docs/sources/installation/upgrading.md index d33ae23fb4e4c..720d71406cdd9 100755 --- a/docs/sources/installation/upgrading.md +++ b/docs/sources/installation/upgrading.md @@ -85,7 +85,7 @@ sudo apt-get update #### Upgrade from binary .tar file -If you downloaded the binary `.tar.gz` package, then you can just download and extract the new package and overwrite all your existing files. However, this might overwrite your config changes. +If you downloaded the binary `.tar.gz` package, then you can just download and extract the new package and overwrite all your existing files. However, this might overwrite your config changes. We recommend that you save your custom config changes in a file named `/conf/custom.ini`. This allows you to upgrade Grafana without risking losing your configuration changes. @@ -222,3 +222,7 @@ Pre Grafana 6.5.0, the CloudWatch datasource used the GetMetricStatistics API fo Each request to the GetMetricData API can include 100 queries. This means that each panel in Grafana will only issue one GetMetricData request, regardless of the number of query rows that are present in the panel. Consequently as it is no longer possible to set `HighRes` on a per query level anymore, this switch is now removed from the query editor. High resolution can still be achieved by choosing a smaller minimum period in the query editor. The handling of multi-valued template variables in dimension values has been changed in Grafana 6.5. When a multi template variable is being used, Grafana will generate a search expression. In the GetMetricData API, expressions are limited to 1024 characters, so it might be the case that this limit is reached when a multi-valued template variable that has a lot of values is being used. If this is the case, we suggest you start using `*` wildcard as dimension value instead of a multi-valued template variable. + +## Upgrading to v6.6 + +The Generic OAuth setting `send_client_credentials_via_post`, used for supporting non-compliant providers, has been removed. From now on, Grafana will automatically detect if credentials should be sent as part of the URL or request body for a specific provider. The result will be remembered and used for additional OAuth requests for that provider. diff --git a/pkg/login/social/social.go b/pkg/login/social/social.go index f677d0fdc79bb..cdc243ded3bb3 100644 --- a/pkg/login/social/social.go +++ b/pkg/login/social/social.go @@ -65,37 +65,30 @@ func NewOAuthService() { for _, name := range allOauthes { sec := setting.Raw.Section("auth." + name) info := &setting.OAuthInfo{ - ClientId: sec.Key("client_id").String(), - ClientSecret: sec.Key("client_secret").String(), - Scopes: util.SplitString(sec.Key("scopes").String()), - AuthUrl: sec.Key("auth_url").String(), - TokenUrl: sec.Key("token_url").String(), - ApiUrl: sec.Key("api_url").String(), - Enabled: sec.Key("enabled").MustBool(), - EmailAttributeName: sec.Key("email_attribute_name").String(), - EmailAttributePath: sec.Key("email_attribute_path").String(), - RoleAttributePath: sec.Key("role_attribute_path").String(), - AllowedDomains: util.SplitString(sec.Key("allowed_domains").String()), - HostedDomain: sec.Key("hosted_domain").String(), - AllowSignup: sec.Key("allow_sign_up").MustBool(), - Name: sec.Key("name").MustString(name), - TlsClientCert: sec.Key("tls_client_cert").String(), - TlsClientKey: sec.Key("tls_client_key").String(), - TlsClientCa: sec.Key("tls_client_ca").String(), - TlsSkipVerify: sec.Key("tls_skip_verify_insecure").MustBool(), - SendClientCredentialsViaPost: sec.Key("send_client_credentials_via_post").MustBool(), + ClientId: sec.Key("client_id").String(), + ClientSecret: sec.Key("client_secret").String(), + Scopes: util.SplitString(sec.Key("scopes").String()), + AuthUrl: sec.Key("auth_url").String(), + TokenUrl: sec.Key("token_url").String(), + ApiUrl: sec.Key("api_url").String(), + Enabled: sec.Key("enabled").MustBool(), + EmailAttributeName: sec.Key("email_attribute_name").String(), + EmailAttributePath: sec.Key("email_attribute_path").String(), + RoleAttributePath: sec.Key("role_attribute_path").String(), + AllowedDomains: util.SplitString(sec.Key("allowed_domains").String()), + HostedDomain: sec.Key("hosted_domain").String(), + AllowSignup: sec.Key("allow_sign_up").MustBool(), + Name: sec.Key("name").MustString(name), + TlsClientCert: sec.Key("tls_client_cert").String(), + TlsClientKey: sec.Key("tls_client_key").String(), + TlsClientCa: sec.Key("tls_client_ca").String(), + TlsSkipVerify: sec.Key("tls_skip_verify_insecure").MustBool(), } if !info.Enabled { continue } - // handle the clients that do not properly support Basic auth headers and require passing client_id/client_secret via POST payload - if info.SendClientCredentialsViaPost { - // TODO: Fix the staticcheck error - oauth2.RegisterBrokenAuthHeaderProvider(info.TokenUrl) //nolint:staticcheck - } - if name == "grafananet" { name = grafanaCom } @@ -106,8 +99,9 @@ func NewOAuthService() { ClientID: info.ClientId, ClientSecret: info.ClientSecret, Endpoint: oauth2.Endpoint{ - AuthURL: info.AuthUrl, - TokenURL: info.TokenUrl, + AuthURL: info.AuthUrl, + TokenURL: info.TokenUrl, + AuthStyle: oauth2.AuthStyleAutoDetect, }, RedirectURL: strings.TrimSuffix(setting.AppUrl, "/") + SocialBaseUrl + name, Scopes: info.Scopes, @@ -181,8 +175,9 @@ func NewOAuthService() { ClientID: info.ClientId, ClientSecret: info.ClientSecret, Endpoint: oauth2.Endpoint{ - AuthURL: setting.GrafanaComUrl + "/oauth2/authorize", - TokenURL: setting.GrafanaComUrl + "/api/oauth2/token", + AuthURL: setting.GrafanaComUrl + "/oauth2/authorize", + TokenURL: setting.GrafanaComUrl + "/api/oauth2/token", + AuthStyle: oauth2.AuthStyleInHeader, }, RedirectURL: strings.TrimSuffix(setting.AppUrl, "/") + SocialBaseUrl + name, Scopes: info.Scopes, diff --git a/pkg/services/provisioning/dashboards/file_reader.go b/pkg/services/provisioning/dashboards/file_reader.go index ce1ebf37504a6..698a3587b4bf3 100644 --- a/pkg/services/provisioning/dashboards/file_reader.go +++ b/pkg/services/provisioning/dashboards/file_reader.go @@ -54,11 +54,10 @@ func NewDashboardFileReader(cfg *DashboardsAsConfig, log log.Logger) (*fileReade // pollChanges periodically runs startWalkingDisk based on interval specified in the config. func (fr *fileReader) pollChanges(ctx context.Context) { - // TODO: Fix the staticcheck error - ticker := time.Tick(time.Duration(int64(time.Second) * fr.Cfg.UpdateIntervalSeconds)) //nolint:staticcheck + ticker := time.NewTicker(time.Duration(int64(time.Second) * fr.Cfg.UpdateIntervalSeconds)) for { select { - case <-ticker: + case <-ticker.C: if err := fr.startWalkingDisk(); err != nil { fr.log.Error("failed to search for dashboards", "error", err) } diff --git a/pkg/setting/setting_oauth.go b/pkg/setting/setting_oauth.go index 2d46ed7977d90..dd3cfd4ca6afe 100644 --- a/pkg/setting/setting_oauth.go +++ b/pkg/setting/setting_oauth.go @@ -1,23 +1,22 @@ package setting type OAuthInfo struct { - ClientId, ClientSecret string - Scopes []string - AuthUrl, TokenUrl string - Enabled bool - EmailAttributeName string - EmailAttributePath string - RoleAttributePath string - AllowedDomains []string - HostedDomain string - ApiUrl string - AllowSignup bool - Name string - TlsClientCert string - TlsClientKey string - TlsClientCa string - TlsSkipVerify bool - SendClientCredentialsViaPost bool + ClientId, ClientSecret string + Scopes []string + AuthUrl, TokenUrl string + Enabled bool + EmailAttributeName string + EmailAttributePath string + RoleAttributePath string + AllowedDomains []string + HostedDomain string + ApiUrl string + AllowSignup bool + Name string + TlsClientCert string + TlsClientKey string + TlsClientCa string + TlsSkipVerify bool } type OAuther struct {