Skip to content

Commit

Permalink
Merge branch 'master' into alerting_reminder
Browse files Browse the repository at this point in the history
* master:
  changelog: add notes about closing grafana#11076
  snapshot: copy correct props when creating a snapshot
  set current org when adding/removing user to org
  changelog: add notes about closing grafana#10707
  Include the vendor directory when copying source in to Docker (grafana#12305)
  changelog: adds note about closing grafana#12199
  adds inTransactionCtx that calls inTransactionWithRetryCtx
  merge create user handlers
  transactions: start sessions and transactions at the same place
  cloudwatch: handle invalid time range
  make sure to use real ip when validating white listed ip's
  changelog: adds note about closing grafana#12286
  • Loading branch information
bergquist committed Jun 18, 2018
2 parents 83a12af + 9271e48 commit ffda5ef
Show file tree
Hide file tree
Showing 17 changed files with 273 additions and 130 deletions.
3 changes: 0 additions & 3 deletions .dockerignore
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,5 @@ dump.rdb
node_modules
/local
/tmp
/vendor
*.yml
*.md
/vendor
/tmp
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@
* **Units**: W/m2 (energy), l/h (flow) and kPa (pressure) [#11233](https://github.com/grafana/grafana/pull/11233), thx [@flopp999](https://github.com/flopp999)
* **Units**: Litre/min (flow) and milliLitre/min (flow) [#12282](https://github.com/grafana/grafana/pull/12282), thx [@flopp999](https://github.com/flopp999)
* **Alerting**: Fix mobile notifications for Microsoft Teams alert notifier [#11484](https://github.com/grafana/grafana/pull/11484), thx [@manacker](https://github.com/manacker)
* **Influxdb**: Add support for mode function [#12286](https://github.com/grafana/grafana/issues/12286)
* **Cloudwatch**: Fixes panic caused by bad timerange settings [#12199](https://github.com/grafana/grafana/issues/12199)
* **Auth Proxy**: Whitelist proxy IP address instead of client IP address [#10707](https://github.com/grafana/grafana/issues/10707)
* **User Management**: Make sure that a user always has a current org assigned [#11076](https://github.com/grafana/grafana/issues/11076)

# 5.2.0-beta1 (2018-06-05)

Expand Down
20 changes: 12 additions & 8 deletions pkg/middleware/auth_proxy.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package middleware

import (
"fmt"
"net"
"net/mail"
"reflect"
"strings"
Expand All @@ -29,7 +28,7 @@ func initContextWithAuthProxy(ctx *m.ReqContext, orgID int64) bool {
}

// if auth proxy ip(s) defined, check if request comes from one of those
if err := checkAuthenticationProxy(ctx.Req.RemoteAddr, proxyHeaderValue); err != nil {
if err := checkAuthenticationProxy(ctx.RemoteAddr(), proxyHeaderValue); err != nil {
ctx.Handle(407, "Proxy authentication required", err)
return true
}
Expand Down Expand Up @@ -197,18 +196,23 @@ func checkAuthenticationProxy(remoteAddr string, proxyHeaderValue string) error
return nil
}

proxies := strings.Split(setting.AuthProxyWhitelist, ",")
sourceIP, _, err := net.SplitHostPort(remoteAddr)
if err != nil {
return err
// Multiple ip addresses? Right-most IP address is the IP address of the most recent proxy
if strings.Contains(remoteAddr, ",") {
sourceIPs := strings.Split(remoteAddr, ",")
remoteAddr = strings.TrimSpace(sourceIPs[len(sourceIPs)-1])
}

remoteAddr = strings.TrimPrefix(remoteAddr, "[")
remoteAddr = strings.TrimSuffix(remoteAddr, "]")

proxies := strings.Split(setting.AuthProxyWhitelist, ",")

// Compare allowed IP addresses to actual address
for _, proxyIP := range proxies {
if sourceIP == strings.TrimSpace(proxyIP) {
if remoteAddr == strings.TrimSpace(proxyIP) {
return nil
}
}

return fmt.Errorf("Request for user (%s) from %s is not from the authentication proxy", proxyHeaderValue, sourceIP)
return fmt.Errorf("Request for user (%s) from %s is not from the authentication proxy", proxyHeaderValue, remoteAddr)
}
55 changes: 55 additions & 0 deletions pkg/middleware/middleware_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,61 @@ func TestMiddlewareContext(t *testing.T) {
})
})

middlewareScenario("When auth_proxy is enabled and request has X-Forwarded-For that is not trusted", func(sc *scenarioContext) {
setting.AuthProxyEnabled = true
setting.AuthProxyHeaderName = "X-WEBAUTH-USER"
setting.AuthProxyHeaderProperty = "username"
setting.AuthProxyWhitelist = "192.168.1.1, 2001::23"

bus.AddHandler("test", func(query *m.GetSignedInUserQuery) error {
query.Result = &m.SignedInUser{OrgId: 4, UserId: 33}
return nil
})

bus.AddHandler("test", func(cmd *m.UpsertUserCommand) error {
cmd.Result = &m.User{Id: 33}
return nil
})

sc.fakeReq("GET", "/")
sc.req.Header.Add("X-WEBAUTH-USER", "torkelo")
sc.req.Header.Add("X-Forwarded-For", "client-ip, 192.168.1.1, 192.168.1.2")
sc.exec()

Convey("should return 407 status code", func() {
So(sc.resp.Code, ShouldEqual, 407)
So(sc.resp.Body.String(), ShouldContainSubstring, "Request for user (torkelo) from 192.168.1.2 is not from the authentication proxy")
})
})

middlewareScenario("When auth_proxy is enabled and request has X-Forwarded-For that is trusted", func(sc *scenarioContext) {
setting.AuthProxyEnabled = true
setting.AuthProxyHeaderName = "X-WEBAUTH-USER"
setting.AuthProxyHeaderProperty = "username"
setting.AuthProxyWhitelist = "192.168.1.1, 2001::23"

bus.AddHandler("test", func(query *m.GetSignedInUserQuery) error {
query.Result = &m.SignedInUser{OrgId: 4, UserId: 33}
return nil
})

bus.AddHandler("test", func(cmd *m.UpsertUserCommand) error {
cmd.Result = &m.User{Id: 33}
return nil
})

sc.fakeReq("GET", "/")
sc.req.Header.Add("X-WEBAUTH-USER", "torkelo")
sc.req.Header.Add("X-Forwarded-For", "client-ip, 192.168.1.2, 192.168.1.1")
sc.exec()

Convey("Should init context with user info", func() {
So(sc.context.IsSignedIn, ShouldBeTrue)
So(sc.context.UserId, ShouldEqual, 33)
So(sc.context.OrgId, ShouldEqual, 4)
})
})

middlewareScenario("When session exists for previous user, create a new session", func(sc *scenarioContext) {
setting.AuthProxyEnabled = true
setting.AuthProxyHeaderName = "X-WEBAUTH-USER"
Expand Down
3 changes: 2 additions & 1 deletion pkg/services/sqlstore/dashboard_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package sqlstore

import (
"context"
"fmt"
"testing"
"time"
Expand Down Expand Up @@ -389,7 +390,7 @@ func createUser(name string, role string, isAdmin bool) m.User {
setting.AutoAssignOrgRole = role

currentUserCmd := m.CreateUserCommand{Login: name, Email: name + "@test.com", Name: "a " + name, IsAdmin: isAdmin}
err := CreateUser(&currentUserCmd)
err := CreateUser(context.Background(), &currentUserCmd)
So(err, ShouldBeNil)

q1 := m.GetUserOrgListQuery{UserId: currentUserCmd.Result.Id}
Expand Down
27 changes: 20 additions & 7 deletions pkg/services/sqlstore/org_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package sqlstore

import (
"context"
"testing"
"time"

Expand All @@ -22,9 +23,9 @@ func TestAccountDataAccess(t *testing.T) {
ac1cmd := m.CreateUserCommand{Login: "ac1", Email: "ac1@test.com", Name: "ac1 name"}
ac2cmd := m.CreateUserCommand{Login: "ac2", Email: "ac2@test.com", Name: "ac2 name"}

err := CreateUser(&ac1cmd)
err := CreateUser(context.Background(), &ac1cmd)
So(err, ShouldBeNil)
err = CreateUser(&ac2cmd)
err = CreateUser(context.Background(), &ac2cmd)
So(err, ShouldBeNil)

q1 := m.GetUserOrgListQuery{UserId: ac1cmd.Result.Id}
Expand All @@ -43,8 +44,8 @@ func TestAccountDataAccess(t *testing.T) {
ac1cmd := m.CreateUserCommand{Login: "ac1", Email: "ac1@test.com", Name: "ac1 name"}
ac2cmd := m.CreateUserCommand{Login: "ac2", Email: "ac2@test.com", Name: "ac2 name", IsAdmin: true}

err := CreateUser(&ac1cmd)
err = CreateUser(&ac2cmd)
err := CreateUser(context.Background(), &ac1cmd)
err = CreateUser(context.Background(), &ac2cmd)
So(err, ShouldBeNil)

ac1 := ac1cmd.Result
Expand Down Expand Up @@ -150,7 +151,7 @@ func TestAccountDataAccess(t *testing.T) {
})

Convey("Can set using org", func() {
cmd := m.SetUsingOrgCommand{UserId: ac2.Id, OrgId: ac1.Id}
cmd := m.SetUsingOrgCommand{UserId: ac2.Id, OrgId: ac1.OrgId}
err := SetUsingOrg(&cmd)
So(err, ShouldBeNil)

Expand All @@ -159,13 +160,25 @@ func TestAccountDataAccess(t *testing.T) {
err := GetSignedInUser(&query)

So(err, ShouldBeNil)
So(query.Result.OrgId, ShouldEqual, ac1.Id)
So(query.Result.OrgId, ShouldEqual, ac1.OrgId)
So(query.Result.Email, ShouldEqual, "ac2@test.com")
So(query.Result.Name, ShouldEqual, "ac2 name")
So(query.Result.Login, ShouldEqual, "ac2")
So(query.Result.OrgName, ShouldEqual, "ac1@test.com")
So(query.Result.OrgRole, ShouldEqual, "Viewer")
})

Convey("Should set last org as current when removing user from current", func() {
remCmd := m.RemoveOrgUserCommand{OrgId: ac1.OrgId, UserId: ac2.Id}
err := RemoveOrgUser(&remCmd)
So(err, ShouldBeNil)

query := m.GetSignedInUserQuery{UserId: ac2.Id}
err = GetSignedInUser(&query)

So(err, ShouldBeNil)
So(query.Result.OrgId, ShouldEqual, ac2.OrgId)
})
})

Convey("Cannot delete last admin org user", func() {
Expand All @@ -182,7 +195,7 @@ func TestAccountDataAccess(t *testing.T) {

Convey("Given an org user with dashboard permissions", func() {
ac3cmd := m.CreateUserCommand{Login: "ac3", Email: "ac3@test.com", Name: "ac3 name", IsAdmin: false}
err := CreateUser(&ac3cmd)
err := CreateUser(context.Background(), &ac3cmd)
So(err, ShouldBeNil)
ac3 := ac3cmd.Result

Expand Down
64 changes: 62 additions & 2 deletions pkg/services/sqlstore/org_users.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,14 @@ func init() {
func AddOrgUser(cmd *m.AddOrgUserCommand) error {
return inTransaction(func(sess *DBSession) error {
// check if user exists
if res, err := sess.Query("SELECT 1 from org_user WHERE org_id=? and user_id=?", cmd.OrgId, cmd.UserId); err != nil {
var user m.User
if exists, err := sess.Id(cmd.UserId).Get(&user); err != nil {
return err
} else if !exists {
return m.ErrUserNotFound
}

if res, err := sess.Query("SELECT 1 from org_user WHERE org_id=? and user_id=?", cmd.OrgId, user.Id); err != nil {
return err
} else if len(res) == 1 {
return m.ErrOrgUserAlreadyAdded
Expand All @@ -41,7 +48,26 @@ func AddOrgUser(cmd *m.AddOrgUserCommand) error {
}

_, err := sess.Insert(&entity)
return err
if err != nil {
return err
}

var userOrgs []*m.UserOrgDTO
sess.Table("org_user")
sess.Join("INNER", "org", "org_user.org_id=org.id")
sess.Where("org_user.user_id=? AND org_user.org_id=?", user.Id, user.OrgId)
sess.Cols("org.name", "org_user.role", "org_user.org_id")
err = sess.Find(&userOrgs)

if err != nil {
return err
}

if len(userOrgs) == 0 {
return setUsingOrgInTransaction(sess, user.Id, cmd.OrgId)
}

return nil
})
}

Expand Down Expand Up @@ -110,6 +136,14 @@ func GetOrgUsers(query *m.GetOrgUsersQuery) error {

func RemoveOrgUser(cmd *m.RemoveOrgUserCommand) error {
return inTransaction(func(sess *DBSession) error {
// check if user exists
var user m.User
if exists, err := sess.Id(cmd.UserId).Get(&user); err != nil {
return err
} else if !exists {
return m.ErrUserNotFound
}

deletes := []string{
"DELETE FROM org_user WHERE org_id=? and user_id=?",
"DELETE FROM dashboard_acl WHERE org_id=? and user_id = ?",
Expand All @@ -123,6 +157,32 @@ func RemoveOrgUser(cmd *m.RemoveOrgUserCommand) error {
}
}

var userOrgs []*m.UserOrgDTO
sess.Table("org_user")
sess.Join("INNER", "org", "org_user.org_id=org.id")
sess.Where("org_user.user_id=?", user.Id)
sess.Cols("org.name", "org_user.role", "org_user.org_id")
err := sess.Find(&userOrgs)

if err != nil {
return err
}

hasCurrentOrgSet := false
for _, userOrg := range userOrgs {
if user.OrgId == userOrg.OrgId {
hasCurrentOrgSet = true
break
}
}

if !hasCurrentOrgSet && len(userOrgs) > 0 {
err = setUsingOrgInTransaction(sess, user.Id, userOrgs[0].OrgId)
if err != nil {
return err
}
}

return validateOneAdminLeftInOrg(cmd.OrgId, sess)
})
}
Expand Down
19 changes: 14 additions & 5 deletions pkg/services/sqlstore/session.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,21 +22,30 @@ func newSession() *DBSession {
return &DBSession{Session: x.NewSession()}
}

func startSession(ctx context.Context) *DBSession {
func startSession(ctx context.Context, engine *xorm.Engine, beginTran bool) (*DBSession, error) {
value := ctx.Value(ContextSessionName)
var sess *DBSession
sess, ok := value.(*DBSession)

if !ok {
newSess := newSession()
return newSess
newSess := &DBSession{Session: engine.NewSession()}
if beginTran {
err := newSess.Begin()
if err != nil {
return nil, err
}
}
return newSess, nil
}

return sess
return sess, nil
}

func withDbSession(ctx context.Context, callback dbTransactionFunc) error {
sess := startSession(ctx)
sess, err := startSession(ctx, x, false)
if err != nil {
return err
}

return callback(sess)
}
Expand Down
3 changes: 2 additions & 1 deletion pkg/services/sqlstore/team_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package sqlstore

import (
"context"
"fmt"
"testing"

Expand All @@ -22,7 +23,7 @@ func TestTeamCommandsAndQueries(t *testing.T) {
Name: fmt.Sprint("user", i),
Login: fmt.Sprint("loginuser", i),
}
err := CreateUser(userCmd)
err := CreateUser(context.Background(), userCmd)
So(err, ShouldBeNil)
userIds = append(userIds, userCmd.Result.Id)
}
Expand Down
Loading

0 comments on commit ffda5ef

Please sign in to comment.