From c994e8c5ac2781e79dfc18379be6a598f3f788db Mon Sep 17 00:00:00 2001 From: Jared Scheib Date: Thu, 19 Oct 2017 13:17:40 -0500 Subject: [PATCH] Set Scheme to be OAuth2 explicitly for all users Add Provider to Users authenticated via /me Signed-off-by: Michael de Sa --- bolt/users.go | 6 +++--- bolt/users_test.go | 8 +++++++- chronograf.go | 1 + server/auth.go | 14 +++++++++++-- server/auth_test.go | 48 ++++++++++++++++++++++++++++++++++++++++++++- server/me.go | 25 +++++++++++++++++++++-- server/me_test.go | 12 +++++++++--- server/users.go | 5 +++++ 8 files changed, 107 insertions(+), 12 deletions(-) diff --git a/bolt/users.go b/bolt/users.go index 03ec4329f..feadb60c3 100644 --- a/bolt/users.go +++ b/bolt/users.go @@ -57,13 +57,13 @@ func (s *UsersStore) Get(ctx context.Context, q chronograf.UserQuery) (*chronogr return s.get(ctx, *q.ID) } - if q.Name != nil && q.Provider != nil { + if q.Name != nil && q.Provider != nil && q.Scheme != nil { var user *chronograf.User err := s.each(ctx, func(u *chronograf.User) { if user != nil { return } - if u.Name == *q.Name && u.Provider == *q.Provider { + if u.Name == *q.Name && u.Provider == *q.Provider && u.Scheme == *q.Scheme { user = u } }) @@ -79,7 +79,7 @@ func (s *UsersStore) Get(ctx context.Context, q chronograf.UserQuery) (*chronogr return user, nil } - return nil, fmt.Errorf("must specify ID, or Name and Provider in UserQuery") + return nil, fmt.Errorf("must specify either ID, or Name, Provider, and Scheme in UserQuery") } // Add a new Users in the UsersStore. diff --git a/bolt/users_test.go b/bolt/users_test.go index 0cda8dd0e..737de23c7 100644 --- a/bolt/users_test.go +++ b/bolt/users_test.go @@ -45,11 +45,13 @@ func TestUsersStore_GetWithID(t *testing.T) { usr: &chronograf.User{ Name: "billietta", Provider: "Google", + Scheme: "OAuth2", }, }, want: &chronograf.User{ Name: "billietta", Provider: "Google", + Scheme: "OAuth2", }, addFirst: true, }, @@ -82,7 +84,7 @@ func TestUsersStore_GetWithID(t *testing.T) { } } -func TestUsersStore_GetWithNameAndProvider(t *testing.T) { +func TestUsersStore_GetWithNameProviderScheme(t *testing.T) { type args struct { ctx context.Context name string @@ -103,6 +105,7 @@ func TestUsersStore_GetWithNameAndProvider(t *testing.T) { usr: &chronograf.User{ Name: "billietta", Provider: "Google", + Scheme: "OAuth2", }, }, wantErr: true, @@ -114,11 +117,13 @@ func TestUsersStore_GetWithNameAndProvider(t *testing.T) { usr: &chronograf.User{ Name: "billietta", Provider: "Google", + Scheme: "OAuth2", }, }, want: &chronograf.User{ Name: "billietta", Provider: "Google", + Scheme: "OAuth2", }, addFirst: true, }, @@ -144,6 +149,7 @@ func TestUsersStore_GetWithNameAndProvider(t *testing.T) { got, err := s.Get(tt.args.ctx, chronograf.UserQuery{ Name: &tt.args.usr.Name, Provider: &tt.args.usr.Provider, + Scheme: &tt.args.usr.Scheme, }) if (err != nil) != tt.wantErr { t.Errorf("%q. UsersStore.Get() error = %v, wantErr %v", tt.name, err, tt.wantErr) diff --git a/chronograf.go b/chronograf.go index d42d8edab..16187e159 100644 --- a/chronograf.go +++ b/chronograf.go @@ -612,6 +612,7 @@ type UserQuery struct { ID *uint64 Name *string Provider *string + Scheme *string } // UsersStore is the Storage and retrieval of authentication information diff --git a/server/auth.go b/server/auth.go index 70414784a..f7fc04d29 100644 --- a/server/auth.go +++ b/server/auth.go @@ -15,7 +15,7 @@ import ( func AuthorizedToken(auth oauth2.Authenticator, logger chronograf.Logger, next http.Handler) http.HandlerFunc { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { log := logger. - WithField("component", "auth"). + WithField("component", "token_auth"). WithField("remote_addr", r.RemoteAddr). WithField("method", r.Method). WithField("url", r.URL) @@ -83,8 +83,18 @@ func AuthorizedUser( Error(w, http.StatusUnauthorized, "User is not authorized", logger) return } + scheme, err := getScheme(ctx) + if err != nil { + log.Error("Failed to retrieve scheme from context") + Error(w, http.StatusUnauthorized, "User is not authorized", logger) + return + } - u, err := store.Get(ctx, chronograf.UserQuery{Name: &username, Provider: &provider}) + u, err := store.Get(ctx, chronograf.UserQuery{ + Name: &username, + Provider: &provider, + Scheme: &scheme, + }) if err != nil { log.Error("Failed to retrieve user") Error(w, http.StatusUnauthorized, "User is not authorized", logger) diff --git a/server/auth_test.go b/server/auth_test.go index 585f2aac7..b19dcb1e7 100644 --- a/server/auth_test.go +++ b/server/auth_test.go @@ -3,6 +3,7 @@ package server_test import ( "context" "errors" + "fmt" "net/http" "net/http/httptest" "testing" @@ -130,6 +131,9 @@ func TestAuthorizedUser(t *testing.T) { fields: fields{ UsersStore: &mocks.UsersStore{ GetF: func(ctx context.Context, q chronograf.UserQuery) (*chronograf.User, error) { + if q.Name == nil || q.Provider == nil || q.Scheme == nil { + return nil, fmt.Errorf("Invalid user query: missing Name, Provider, and/or Scheme") + } return &chronograf.User{ ID: 1337, Name: "billysteve", @@ -156,6 +160,9 @@ func TestAuthorizedUser(t *testing.T) { fields: fields{ UsersStore: &mocks.UsersStore{ GetF: func(ctx context.Context, q chronograf.UserQuery) (*chronograf.User, error) { + if q.Name == nil || q.Provider == nil || q.Scheme == nil { + return nil, fmt.Errorf("Invalid user query: missing Name, Provider, and/or Scheme") + } return &chronograf.User{ ID: 1337, Name: "billysteve", @@ -182,6 +189,9 @@ func TestAuthorizedUser(t *testing.T) { fields: fields{ UsersStore: &mocks.UsersStore{ GetF: func(ctx context.Context, q chronograf.UserQuery) (*chronograf.User, error) { + if q.Name == nil || q.Provider == nil || q.Scheme == nil { + return nil, fmt.Errorf("Invalid user query: missing Name, Provider, and/or Scheme") + } return &chronograf.User{ ID: 1337, Name: "billysteve", @@ -208,6 +218,9 @@ func TestAuthorizedUser(t *testing.T) { fields: fields{ UsersStore: &mocks.UsersStore{ GetF: func(ctx context.Context, q chronograf.UserQuery) (*chronograf.User, error) { + if q.Name == nil || q.Provider == nil || q.Scheme == nil { + return nil, fmt.Errorf("Invalid user query: missing Name, Provider, and/or Scheme") + } return &chronograf.User{ ID: 1337, Name: "billysteve", @@ -234,6 +247,9 @@ func TestAuthorizedUser(t *testing.T) { fields: fields{ UsersStore: &mocks.UsersStore{ GetF: func(ctx context.Context, q chronograf.UserQuery) (*chronograf.User, error) { + if q.Name == nil || q.Provider == nil || q.Scheme == nil { + return nil, fmt.Errorf("Invalid user query: missing Name, Provider, and/or Scheme") + } return &chronograf.User{ ID: 1337, Name: "billysteve", @@ -260,6 +276,9 @@ func TestAuthorizedUser(t *testing.T) { fields: fields{ UsersStore: &mocks.UsersStore{ GetF: func(ctx context.Context, q chronograf.UserQuery) (*chronograf.User, error) { + if q.Name == nil || q.Provider == nil || q.Scheme == nil { + return nil, fmt.Errorf("Invalid user query: missing Name, Provider, and/or Scheme") + } return &chronograf.User{ ID: 1337, Name: "billysteve", @@ -286,6 +305,9 @@ func TestAuthorizedUser(t *testing.T) { fields: fields{ UsersStore: &mocks.UsersStore{ GetF: func(ctx context.Context, q chronograf.UserQuery) (*chronograf.User, error) { + if q.Name == nil || q.Provider == nil || q.Scheme == nil { + return nil, fmt.Errorf("Invalid user query: missing Name, Provider, and/or Scheme") + } return &chronograf.User{ ID: 1337, Name: "billysteve", @@ -312,6 +334,9 @@ func TestAuthorizedUser(t *testing.T) { fields: fields{ UsersStore: &mocks.UsersStore{ GetF: func(ctx context.Context, q chronograf.UserQuery) (*chronograf.User, error) { + if q.Name == nil || q.Provider == nil || q.Scheme == nil { + return nil, fmt.Errorf("Invalid user query: missing Name, Provider, and/or Scheme") + } return &chronograf.User{ ID: 1337, Name: "billysteve", @@ -338,6 +363,9 @@ func TestAuthorizedUser(t *testing.T) { fields: fields{ UsersStore: &mocks.UsersStore{ GetF: func(ctx context.Context, q chronograf.UserQuery) (*chronograf.User, error) { + if q.Name == nil || q.Provider == nil || q.Scheme == nil { + return nil, fmt.Errorf("Invalid user query: missing Name, Provider, and/or Scheme") + } return &chronograf.User{ ID: 1337, Name: "billysteve", @@ -364,6 +392,9 @@ func TestAuthorizedUser(t *testing.T) { fields: fields{ UsersStore: &mocks.UsersStore{ GetF: func(ctx context.Context, q chronograf.UserQuery) (*chronograf.User, error) { + if q.Name == nil || q.Provider == nil || q.Scheme == nil { + return nil, fmt.Errorf("Invalid user query: missing Name, Provider, and/or Scheme") + } return &chronograf.User{ ID: 1337, Name: "billysteve", @@ -388,6 +419,9 @@ func TestAuthorizedUser(t *testing.T) { fields: fields{ UsersStore: &mocks.UsersStore{ GetF: func(ctx context.Context, q chronograf.UserQuery) (*chronograf.User, error) { + if q.Name == nil || q.Provider == nil || q.Scheme == nil { + return nil, fmt.Errorf("Invalid user query: missing Name, Provider, and/or Scheme") + } return &chronograf.User{ ID: 1337, Name: "billysteve", @@ -412,6 +446,9 @@ func TestAuthorizedUser(t *testing.T) { fields: fields{ UsersStore: &mocks.UsersStore{ GetF: func(ctx context.Context, q chronograf.UserQuery) (*chronograf.User, error) { + if q.Name == nil || q.Provider == nil || q.Scheme == nil { + return nil, fmt.Errorf("Invalid user query: missing Name, Provider, and/or Scheme") + } return &chronograf.User{ ID: 1337, Name: "billysteve", @@ -436,6 +473,9 @@ func TestAuthorizedUser(t *testing.T) { fields: fields{ UsersStore: &mocks.UsersStore{ GetF: func(ctx context.Context, q chronograf.UserQuery) (*chronograf.User, error) { + if q.Name == nil || q.Provider == nil || q.Scheme == nil { + return nil, fmt.Errorf("Invalid user query: missing Name, Provider, and/or Scheme") + } return &chronograf.User{ ID: 1337, Name: "billysteve", @@ -464,6 +504,9 @@ func TestAuthorizedUser(t *testing.T) { fields: fields{ UsersStore: &mocks.UsersStore{ GetF: func(ctx context.Context, q chronograf.UserQuery) (*chronograf.User, error) { + if q.Name == nil || q.Provider == nil || q.Scheme == nil { + return nil, fmt.Errorf("Invalid user query: missing Name, Provider, and/or Scheme") + } return &chronograf.User{ ID: 1337, Name: "billysteve", @@ -492,6 +535,9 @@ func TestAuthorizedUser(t *testing.T) { fields: fields{ UsersStore: &mocks.UsersStore{ GetF: func(ctx context.Context, q chronograf.UserQuery) (*chronograf.User, error) { + if q.Name == nil || q.Provider == nil || q.Scheme == nil { + return nil, fmt.Errorf("Invalid user query: missing Name, Provider, and/or Scheme") + } return &chronograf.User{ ID: 1337, Name: "billysteve", @@ -538,7 +584,7 @@ func TestAuthorizedUser(t *testing.T) { fn(w, r) if authorized != tt.authorized { - t.Errorf("%q. AuthorizedUser() = %v, expected %v", tt.name, tt.authorized, authorized) + t.Errorf("%q. AuthorizedUser() = %v, expected %v", tt.name, authorized, tt.authorized) } }) diff --git a/server/me.go b/server/me.go index b58ab0676..9f6ca0810 100644 --- a/server/me.go +++ b/server/me.go @@ -58,6 +58,13 @@ func getProvider(ctx context.Context) (string, error) { return principal.Issuer, nil } +// TODO: This Scheme value is hard-coded temporarily since we only currently +// support OAuth2. This hard-coding should be removed whenever we add +// support for other authentication schemes. +func getScheme(ctx context.Context) (string, error) { + return "OAuth2", nil +} + func getPrincipal(ctx context.Context) (oauth2.Principal, error) { principal, ok := ctx.Value(oauth2.PrincipalKey).(oauth2.Principal) if !ok { @@ -87,8 +94,17 @@ func (s *Service) Me(w http.ResponseWriter, r *http.Request) { invalidData(w, err, s.Logger) return } + scheme, err := getScheme(ctx) + if err != nil { + invalidData(w, err, s.Logger) + return + } - usr, err := s.UsersStore.Get(ctx, chronograf.UserQuery{Name: &username, Provider: &provider}) + usr, err := s.UsersStore.Get(ctx, chronograf.UserQuery{ + Name: &username, + Provider: &provider, + Scheme: &scheme, + }) if err != nil && err != chronograf.ErrUserNotFound { unknownErrorWithMessage(w, err, s.Logger) return @@ -102,7 +118,12 @@ func (s *Service) Me(w http.ResponseWriter, r *http.Request) { // Because we didnt find a user, making a new one user := &chronograf.User{ - Name: username, + Name: username, + Provider: provider, + // TODO: This Scheme value is hard-coded temporarily since we only currently + // support OAuth2. This hard-coding should be removed whenever we add + // support for other authentication schemes. + Scheme: "OAuth2", } newUser, err := s.UsersStore.Add(ctx, user) diff --git a/server/me_test.go b/server/me_test.go index b9692c85c..d5f9f7528 100644 --- a/server/me_test.go +++ b/server/me_test.go @@ -46,10 +46,13 @@ func TestService_Me(t *testing.T) { Logger: log.New(log.DebugLevel), UsersStore: &mocks.UsersStore{ GetF: func(ctx context.Context, q chronograf.UserQuery) (*chronograf.User, error) { + if q.Name == nil || q.Provider == nil || q.Scheme == nil { + return nil, fmt.Errorf("Invalid user query: missing Name, Provider, and/or Scheme") + } return &chronograf.User{ Name: "me", Provider: "GitHub", - Passwd: "hunter2", + Scheme: "OAuth2", }, nil }, }, @@ -60,7 +63,7 @@ func TestService_Me(t *testing.T) { }, wantStatus: http.StatusOK, wantContentType: "application/json", - wantBody: `{"name":"me","password":"hunter2","provider":"GitHub","links":{"self":"/chronograf/v1/users/me"}} + wantBody: `{"name":"me","provider":"GitHub","scheme":"OAuth2","links":{"self":"/chronograf/v1/users/me"}} `, }, { @@ -74,6 +77,9 @@ func TestService_Me(t *testing.T) { Logger: log.New(log.DebugLevel), UsersStore: &mocks.UsersStore{ GetF: func(ctx context.Context, q chronograf.UserQuery) (*chronograf.User, error) { + if q.Name == nil || q.Provider == nil || q.Scheme == nil { + return nil, fmt.Errorf("Invalid user query: missing Name, Provider, and/or Scheme") + } return nil, chronograf.ErrUserNotFound }, AddF: func(ctx context.Context, u *chronograf.User) (*chronograf.User, error) { @@ -87,7 +93,7 @@ func TestService_Me(t *testing.T) { }, wantStatus: http.StatusOK, wantContentType: "application/json", - wantBody: `{"name":"secret","links":{"self":"/chronograf/v1/users/secret"}} + wantBody: `{"name":"secret","provider":"Auth0","scheme":"OAuth2","links":{"self":"/chronograf/v1/users/secret"}} `, }, { diff --git a/server/users.go b/server/users.go index d12640a0c..071b537ba 100644 --- a/server/users.go +++ b/server/users.go @@ -29,6 +29,11 @@ func (r *userRequest) ValidCreate() error { if r.Scheme == "" { return fmt.Errorf("Scheme required on Chronograf User request body") } + + // TODO: This Scheme value is hard-coded temporarily since we only currently + // support OAuth2. This hard-coding should be removed whenever we add + // support for other authentication schemes. + r.Scheme = "OAuth2" return r.ValidRoles() }