From fefbbc65a8e645413870260bf1b7ed8763deb98a Mon Sep 17 00:00:00 2001 From: Sofia Papagiannaki Date: Tue, 16 Jun 2020 16:33:44 +0300 Subject: [PATCH] Auth: Add support for forcing authentication in anonymous mode and modify SignIn to use it instead of redirect (#25567) * Forbid additional redirect urls * Optionally force login in anonymous mode * Update LoginCtrl page to ignore redirect parameter * Modify SignIn to set forceLogin query instead of redirect * Pass appUrl to frontend and use URL API for updating url query * Apply suggestions from code review Co-authored-by: Arve Knudsen * Fix SignIn test Co-authored-by: Arve Knudsen --- packages/grafana-runtime/src/config.ts | 2 + pkg/api/frontendsettings.go | 1 + pkg/api/login.go | 14 +++ pkg/api/login_test.go | 86 ++++++++++++++++++- pkg/login/auth.go | 1 + pkg/middleware/auth.go | 14 ++- .../app/core/components/Login/LoginCtrl.tsx | 9 +- .../core/components/sidemenu/SignIn.test.tsx | 4 + .../app/core/components/sidemenu/SignIn.tsx | 14 ++- .../__snapshots__/SignIn.test.tsx.snap | 4 +- 10 files changed, 133 insertions(+), 16 deletions(-) diff --git a/packages/grafana-runtime/src/config.ts b/packages/grafana-runtime/src/config.ts index b4d970edf58..fa44fad7978 100644 --- a/packages/grafana-runtime/src/config.ts +++ b/packages/grafana-runtime/src/config.ts @@ -15,6 +15,7 @@ export class GrafanaBootConfig implements GrafanaConfig { datasources: { [str: string]: DataSourceInstanceSettings } = {}; panels: { [key: string]: PanelPluginMeta } = {}; minRefreshInterval = ''; + appUrl = ''; appSubUrl = ''; windowTitlePrefix = ''; buildInfo: BuildInfo = {} as BuildInfo; @@ -66,6 +67,7 @@ export class GrafanaBootConfig implements GrafanaConfig { newPanelTitle: 'Panel Title', playlist_timespan: '1m', unsaved_changes_warning: true, + appUrl: '', appSubUrl: '', buildInfo: { version: 'v1.0', diff --git a/pkg/api/frontendsettings.go b/pkg/api/frontendsettings.go index 0c9681f7b95..1e6530f0132 100644 --- a/pkg/api/frontendsettings.go +++ b/pkg/api/frontendsettings.go @@ -172,6 +172,7 @@ func (hs *HTTPServer) getFrontendSettingsMap(c *models.ReqContext) (map[string]i "datasources": datasources, "minRefreshInterval": setting.MinRefreshInterval, "panels": panels, + "appUrl": setting.AppUrl, "appSubUrl": setting.AppSubUrl, "allowOrgCreate": (setting.AllowUserOrgCreate && c.IsSignedIn) || c.IsGrafanaAdmin, "authProxyEnabled": setting.AuthProxyEnabled, diff --git a/pkg/api/login.go b/pkg/api/login.go index 995fc9e1468..fdcdf527cfa 100644 --- a/pkg/api/login.go +++ b/pkg/api/login.go @@ -37,11 +37,25 @@ func (hs *HTTPServer) ValidateRedirectTo(redirectTo string) error { if to.IsAbs() { return login.ErrAbsoluteRedirectTo } + + if to.Host != "" { + return login.ErrForbiddenRedirectTo + } + + // path should have exactly one leading slash + if !strings.HasPrefix(to.Path, "/") { + return login.ErrForbiddenRedirectTo + } + if strings.HasPrefix(to.Path, "//") { + return login.ErrForbiddenRedirectTo + } + // when using a subUrl, the redirect_to should start with the subUrl (which contains the leading slash), otherwise the redirect // will send the user to the wrong location if hs.Cfg.AppSubUrl != "" && !strings.HasPrefix(to.Path, hs.Cfg.AppSubUrl+"/") { return login.ErrInvalidRedirectTo } + return nil } diff --git a/pkg/api/login_test.go b/pkg/api/login_test.go index a0c713ac46f..bf76921eeb3 100644 --- a/pkg/api/login_test.go +++ b/pkg/api/login_test.go @@ -207,6 +207,48 @@ func TestLoginViewRedirect(t *testing.T) { appURL: "http://localhost:3000/", status: 302, }, + { + desc: "non-Grafana URL without scheme", + url: "example.com", + redirectURL: "/", + appURL: "http://localhost:3000/", + status: 302, + }, + { + desc: "non-Grafana URL without scheme", + url: "www.example.com", + redirectURL: "/", + appURL: "http://localhost:3000/", + status: 302, + }, + { + desc: "URL path is a host with two leading slashes", + url: "//example.com", + redirectURL: "/", + appURL: "http://localhost:3000/", + status: 302, + }, + { + desc: "URL path is a host with three leading slashes", + url: "///example.com", + redirectURL: "/", + appURL: "http://localhost:3000/", + status: 302, + }, + { + desc: "URL path is an IP address with two leading slashes", + url: "//0.0.0.0", + redirectURL: "/", + appURL: "http://localhost:3000/", + status: 302, + }, + { + desc: "URL path is an IP address with three leading slashes", + url: "///0.0.0.0", + redirectURL: "/", + appURL: "http://localhost:3000/", + status: 302, + }, } for _, c := range redirectCases { @@ -232,7 +274,7 @@ func TestLoginViewRedirect(t *testing.T) { if c.status == 302 { location, ok := sc.resp.Header()["Location"] assert.True(t, ok) - assert.Equal(t, location[0], c.redirectURL) + assert.Equal(t, c.redirectURL, location[0]) setCookie, ok := sc.resp.Header()["Set-Cookie"] assert.True(t, ok, "Set-Cookie exists") @@ -333,6 +375,48 @@ func TestLoginPostRedirect(t *testing.T) { appURL: "https://localhost:3000/", err: login.ErrAbsoluteRedirectTo, }, + { + desc: "invalid URL", + url: ":foo", + appURL: "http://localhost:3000/", + err: login.ErrInvalidRedirectTo, + }, + { + desc: "non-Grafana URL without scheme", + url: "example.com", + appURL: "http://localhost:3000/", + err: login.ErrForbiddenRedirectTo, + }, + { + desc: "non-Grafana URL without scheme", + url: "www.example.com", + appURL: "http://localhost:3000/", + err: login.ErrForbiddenRedirectTo, + }, + { + desc: "URL path is a host with two leading slashes", + url: "//example.com", + appURL: "http://localhost:3000/", + err: login.ErrForbiddenRedirectTo, + }, + { + desc: "URL path is a host with three leading slashes", + url: "///example.com", + appURL: "http://localhost:3000/", + err: login.ErrForbiddenRedirectTo, + }, + { + desc: "URL path is an IP address with two leading slashes", + url: "//0.0.0.0", + appURL: "http://localhost:3000/", + err: login.ErrForbiddenRedirectTo, + }, + { + desc: "URL path is an IP address with three leading slashes", + url: "///0.0.0.0", + appURL: "http://localhost:3000/", + err: login.ErrForbiddenRedirectTo, + }, } for _, c := range redirectCases { diff --git a/pkg/login/auth.go b/pkg/login/auth.go index 98ad360a1f2..2c7778ca009 100644 --- a/pkg/login/auth.go +++ b/pkg/login/auth.go @@ -20,6 +20,7 @@ var ( ErrUserDisabled = errors.New("User is disabled") ErrAbsoluteRedirectTo = errors.New("Absolute urls are not allowed for redirect_to cookie value") ErrInvalidRedirectTo = errors.New("Invalid redirect_to cookie value") + ErrForbiddenRedirectTo = errors.New("Forbidden redirect_to cookie value") ) var loginLogger = log.New("login") diff --git a/pkg/middleware/auth.go b/pkg/middleware/auth.go index d61914a2eb7..cf6aa4f2314 100644 --- a/pkg/middleware/auth.go +++ b/pkg/middleware/auth.go @@ -51,8 +51,16 @@ func notAuthorized(c *models.ReqContext) { if setting.AppSubUrl != "" && !strings.HasPrefix(redirectTo, setting.AppSubUrl) { redirectTo = setting.AppSubUrl + c.Req.RequestURI } - WriteCookie(c.Resp, "redirect_to", url.QueryEscape(redirectTo), 0, newCookieOptions) + // remove forceLogin query param if it exists + if parsed, err := url.ParseRequestURI(redirectTo); err == nil { + params := parsed.Query() + params.Del("forceLogin") + parsed.RawQuery = params.Encode() + WriteCookie(c.Resp, "redirect_to", url.QueryEscape(parsed.String()), 0, newCookieOptions) + } else { + c.Logger.Debug("Failed parsing request URI; redirect cookie will not be set", "redirectTo", redirectTo, "error", err) + } c.Redirect(setting.AppSubUrl + "/login") } @@ -79,7 +87,9 @@ func RoleAuth(roles ...models.RoleType) macaron.Handler { func Auth(options *AuthOptions) macaron.Handler { return func(c *models.ReqContext) { - if !c.IsSignedIn && options.ReqSignedIn && !c.AllowAnonymous { + forceLogin := c.AllowAnonymous && c.QueryBool("forceLogin") + requireLogin := !c.AllowAnonymous || forceLogin + if !c.IsSignedIn && options.ReqSignedIn && requireLogin { notAuthorized(c) return } diff --git a/public/app/core/components/Login/LoginCtrl.tsx b/public/app/core/components/Login/LoginCtrl.tsx index 5442e0ed05a..da854447d8d 100644 --- a/public/app/core/components/Login/LoginCtrl.tsx +++ b/public/app/core/components/Login/LoginCtrl.tsx @@ -101,15 +101,8 @@ export class LoginCtrl extends PureComponent { }; toGrafana = () => { - const params = this.props.routeParams; // Use window.location.href to force page reload - if (params.redirect && params.redirect[0] === '/') { - if (config.appSubUrl !== '' && !params.redirect.startsWith(config.appSubUrl)) { - window.location.href = config.appSubUrl + params.redirect; - } else { - window.location.href = params.redirect; - } - } else if (this.result.redirectUrl) { + if (this.result.redirectUrl) { if (config.appSubUrl !== '' && !this.result.redirectUrl.startsWith(config.appSubUrl)) { window.location.href = config.appSubUrl + this.result.redirectUrl; } else { diff --git a/public/app/core/components/sidemenu/SignIn.test.tsx b/public/app/core/components/sidemenu/SignIn.test.tsx index 69144e1f8fc..4aa50de5fc4 100644 --- a/public/app/core/components/sidemenu/SignIn.test.tsx +++ b/public/app/core/components/sidemenu/SignIn.test.tsx @@ -2,6 +2,10 @@ import React from 'react'; import { shallow } from 'enzyme'; import { SignIn } from './SignIn'; +jest.mock('../../config', () => ({ + appUrl: 'http://localhost:3000/', +})); + describe('Render', () => { it('should render component', () => { const wrapper = shallow(); diff --git a/public/app/core/components/sidemenu/SignIn.tsx b/public/app/core/components/sidemenu/SignIn.tsx index 47a4a8eca9f..817f0433554 100644 --- a/public/app/core/components/sidemenu/SignIn.tsx +++ b/public/app/core/components/sidemenu/SignIn.tsx @@ -1,18 +1,26 @@ import React, { FC } from 'react'; +import config from 'app/core/config'; import { connectWithStore } from 'app/core/utils/connectWithReduxStore'; import { StoreState } from 'app/types'; import { Icon } from '@grafana/ui'; +const getForcedLoginUrl = (url: string) => { + const urlObj = new URL(url, config.appUrl); + let params = urlObj.searchParams; + params.set('forceLogin', 'true'); + return urlObj.toString(); +}; + export const SignIn: FC = ({ url }) => { - const loginUrl = `login?redirect=${encodeURIComponent(url)}`; + const forcedLoginUrl = getForcedLoginUrl(url); return (