From f228e2860dc775605fbeaa1ef044e13b86fa74b6 Mon Sep 17 00:00:00 2001 From: Michael Desa Date: Tue, 7 Nov 2017 17:05:47 -0500 Subject: [PATCH] Expose some organization routes to admins Cleanup tests appropriately Prevent Admins from patching organizations --- organizations/organizations.go | 130 +++++++++++ organizations/organizations_test.go | 345 ++++++++++++++++++++++++++++ server/auth.go | 1 - server/me.go | 5 +- server/mux.go | 4 +- server/organizations_test.go | 8 +- server/stores.go | 6 +- 7 files changed, 488 insertions(+), 11 deletions(-) create mode 100644 organizations/organizations_test.go diff --git a/organizations/organizations.go b/organizations/organizations.go index 3a35731f1b..6f9136b30e 100644 --- a/organizations/organizations.go +++ b/organizations/organizations.go @@ -3,6 +3,8 @@ package organizations import ( "context" "fmt" + + "github.com/influxdata/chronograf" ) type contextKey string @@ -26,3 +28,131 @@ func validOrganization(ctx context.Context) error { } return nil } + +// ensure that OrganizationsStore implements chronograf.OrganizationStore +var _ chronograf.OrganizationsStore = &OrganizationsStore{} + +// OrganizationsStore facade on a OrganizationStore that filters organizations +// by organization. +type OrganizationsStore struct { + store chronograf.OrganizationsStore + organization string +} + +// NewOrganizationsStore creates a new OrganizationsStore from an existing +// chronograf.OrganizationStore and an organization string +func NewOrganizationsStore(s chronograf.OrganizationsStore, org string) *OrganizationsStore { + return &OrganizationsStore{ + store: s, + organization: org, + } +} + +// All retrieves all organizations from the underlying OrganizationStore and filters them +// by organization. +func (s *OrganizationsStore) All(ctx context.Context) ([]chronograf.Organization, error) { + err := validOrganization(ctx) + if err != nil { + return nil, err + } + + ds, err := s.store.All(ctx) + if err != nil { + return nil, err + } + + defaultOrg, err := s.store.DefaultOrganization(ctx) + if err != nil { + return nil, err + } + + defaultOrgID := fmt.Sprintf("%d", defaultOrg.ID) + + // This filters organizations without allocating + // https://github.com/golang/go/wiki/SliceTricks#filtering-without-allocating + organizations := ds[:0] + for _, d := range ds { + id := fmt.Sprintf("%d", d.ID) + switch id { + case s.organization, defaultOrgID: + organizations = append(organizations, d) + default: + continue + } + } + + return organizations, nil +} + +// Add creates a new Organization in the OrganizationsStore with organization.Organization set to be the +// organization from the organization store. +func (s *OrganizationsStore) Add(ctx context.Context, o *chronograf.Organization) (*chronograf.Organization, error) { + return nil, fmt.Errorf("cannot create organization") +} + +// Delete the organization from OrganizationsStore +func (s *OrganizationsStore) Delete(ctx context.Context, o *chronograf.Organization) error { + err := validOrganization(ctx) + if err != nil { + return err + } + + o, err = s.store.Get(ctx, chronograf.OrganizationQuery{ID: &o.ID}) + if err != nil { + return err + } + + return s.store.Delete(ctx, o) +} + +// Get returns a Organization if the id exists and belongs to the organization that is set. +func (s *OrganizationsStore) Get(ctx context.Context, q chronograf.OrganizationQuery) (*chronograf.Organization, error) { + err := validOrganization(ctx) + if err != nil { + return nil, err + } + + d, err := s.store.Get(ctx, q) + if err != nil { + return nil, err + } + + if fmt.Sprintf("%d", d.ID) != s.organization { + return nil, chronograf.ErrOrganizationNotFound + } + + return d, nil +} + +// Update the organization in OrganizationsStore. +func (s *OrganizationsStore) Update(ctx context.Context, o *chronograf.Organization) error { + err := validOrganization(ctx) + if err != nil { + return err + } + + _, err = s.store.Get(ctx, chronograf.OrganizationQuery{ID: &o.ID}) + if err != nil { + return err + } + + return s.store.Update(ctx, o) +} + +func (s *OrganizationsStore) CreateDefault(ctx context.Context) error { + err := validOrganization(ctx) + if err != nil { + return err + } + + return s.store.CreateDefault(ctx) +} + +func (s *OrganizationsStore) DefaultOrganization(ctx context.Context) (*chronograf.Organization, error) { + err := validOrganization(ctx) + if err != nil { + return nil, err + } + + return s.store.DefaultOrganization(ctx) +} diff --git a/organizations/organizations_test.go b/organizations/organizations_test.go new file mode 100644 index 0000000000..e95e9ce27d --- /dev/null +++ b/organizations/organizations_test.go @@ -0,0 +1,345 @@ +package organizations_test + +import ( + "context" + "fmt" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + "github.com/influxdata/chronograf" + "github.com/influxdata/chronograf/mocks" + "github.com/influxdata/chronograf/organizations" +) + +// IgnoreFields is used because ID cannot be predicted reliably +// EquateEmpty is used because we want nil slices, arrays, and maps to be equal to the empty map +var organizationCmpOptions = cmp.Options{ + cmpopts.EquateEmpty(), + cmpopts.IgnoreFields(chronograf.Organization{}, "ID"), +} + +func TestOrganizations_All(t *testing.T) { + type fields struct { + OrganizationsStore chronograf.OrganizationsStore + } + type args struct { + organization string + ctx context.Context + } + tests := []struct { + name string + args args + fields fields + want []chronograf.Organization + wantRaw []chronograf.Organization + wantErr bool + }{ + { + name: "No Organizations", + fields: fields{ + OrganizationsStore: &mocks.OrganizationsStore{ + AllF: func(ctx context.Context) ([]chronograf.Organization, error) { + return nil, fmt.Errorf("No Organizations") + }, + DefaultOrganizationF: func(ctx context.Context) (*chronograf.Organization, error) { + return &chronograf.Organization{ + ID: 0, + Name: "Default", + }, nil + }, + }, + }, + wantErr: true, + }, + { + name: "All Organizations", + fields: fields{ + OrganizationsStore: &mocks.OrganizationsStore{ + DefaultOrganizationF: func(ctx context.Context) (*chronograf.Organization, error) { + return &chronograf.Organization{ + ID: 0, + Name: "Default", + }, nil + }, + AllF: func(ctx context.Context) ([]chronograf.Organization, error) { + return []chronograf.Organization{ + { + Name: "howdy", + ID: 1337, + }, + { + Name: "doody", + ID: 1447, + }, + }, nil + }, + }, + }, + args: args{ + organization: "1337", + ctx: context.Background(), + }, + want: []chronograf.Organization{ + { + Name: "howdy", + ID: 1337, + }, + { + Name: "Default", + ID: 0, + }, + }, + }, + } + for _, tt := range tests { + s := organizations.NewOrganizationsStore(tt.fields.OrganizationsStore, tt.args.organization) + tt.args.ctx = context.WithValue(tt.args.ctx, organizations.ContextKey, tt.args.organization) + gots, err := s.All(tt.args.ctx) + if (err != nil) != tt.wantErr { + t.Errorf("%q. OrganizationsStore.All() error = %v, wantErr %v", tt.name, err, tt.wantErr) + continue + } + for i, got := range gots { + if diff := cmp.Diff(got, tt.want[i], organizationCmpOptions...); diff != "" { + t.Errorf("%q. OrganizationsStore.All():\n-got/+want\ndiff %s", tt.name, diff) + } + } + } +} + +func TestOrganizations_Add(t *testing.T) { + type fields struct { + OrganizationsStore chronograf.OrganizationsStore + } + type args struct { + organizationID string + ctx context.Context + organization *chronograf.Organization + } + tests := []struct { + name string + args args + fields fields + want *chronograf.Organization + wantErr bool + }{ + { + name: "Add Organization", + fields: fields{ + OrganizationsStore: &mocks.OrganizationsStore{ + AddF: func(ctx context.Context, s *chronograf.Organization) (*chronograf.Organization, error) { + return s, nil + }, + GetF: func(ctx context.Context, q chronograf.OrganizationQuery) (*chronograf.Organization, error) { + return &chronograf.Organization{ + ID: 1229, + Name: "howdy", + }, nil + }, + }, + }, + args: args{ + organizationID: "1229", + ctx: context.Background(), + organization: &chronograf.Organization{ + Name: "howdy", + }, + }, + wantErr: true, + }, + } + for _, tt := range tests { + s := organizations.NewOrganizationsStore(tt.fields.OrganizationsStore, tt.args.organizationID) + tt.args.ctx = context.WithValue(tt.args.ctx, organizations.ContextKey, tt.args.organizationID) + d, err := s.Add(tt.args.ctx, tt.args.organization) + if (err != nil) != tt.wantErr { + t.Errorf("%q. OrganizationsStore.Add() error = %v, wantErr %v", tt.name, err, tt.wantErr) + continue + } + if tt.wantErr { + continue + } + got, err := s.Get(tt.args.ctx, chronograf.OrganizationQuery{ID: &d.ID}) + if diff := cmp.Diff(got, tt.want, organizationCmpOptions...); diff != "" { + t.Errorf("%q. OrganizationsStore.Add():\n-got/+want\ndiff %s", tt.name, diff) + } + } +} + +func TestOrganizations_Delete(t *testing.T) { + type fields struct { + OrganizationsStore chronograf.OrganizationsStore + } + type args struct { + organizationID string + ctx context.Context + organization *chronograf.Organization + } + tests := []struct { + name string + fields fields + args args + want []chronograf.Organization + addFirst bool + wantErr bool + }{ + { + name: "Delete organization", + fields: fields{ + OrganizationsStore: &mocks.OrganizationsStore{ + DeleteF: func(ctx context.Context, s *chronograf.Organization) error { + return nil + }, + GetF: func(ctx context.Context, q chronograf.OrganizationQuery) (*chronograf.Organization, error) { + return &chronograf.Organization{ + ID: 1229, + Name: "howdy", + }, nil + }, + }, + }, + args: args{ + organizationID: "1229", + ctx: context.Background(), + organization: &chronograf.Organization{ + ID: 1229, + Name: "howdy", + }, + }, + addFirst: true, + }, + } + for _, tt := range tests { + s := organizations.NewOrganizationsStore(tt.fields.OrganizationsStore, tt.args.organizationID) + tt.args.ctx = context.WithValue(tt.args.ctx, organizations.ContextKey, tt.args.organizationID) + err := s.Delete(tt.args.ctx, tt.args.organization) + if (err != nil) != tt.wantErr { + t.Errorf("%q. OrganizationsStore.All() error = %v, wantErr %v", tt.name, err, tt.wantErr) + continue + } + } +} + +func TestOrganizations_Get(t *testing.T) { + type fields struct { + OrganizationsStore chronograf.OrganizationsStore + } + type args struct { + organizationID string + ctx context.Context + organization *chronograf.Organization + } + tests := []struct { + name string + fields fields + args args + want *chronograf.Organization + addFirst bool + wantErr bool + }{ + { + name: "Get Organization", + fields: fields{ + OrganizationsStore: &mocks.OrganizationsStore{ + GetF: func(ctx context.Context, q chronograf.OrganizationQuery) (*chronograf.Organization, error) { + return &chronograf.Organization{ + ID: 1337, + Name: "howdy", + }, nil + }, + }, + }, + args: args{ + organizationID: "1337", + ctx: context.Background(), + organization: &chronograf.Organization{ + ID: 1337, + Name: "howdy", + }, + }, + want: &chronograf.Organization{ + ID: 1337, + Name: "howdy", + }, + }, + } + for _, tt := range tests { + s := organizations.NewOrganizationsStore(tt.fields.OrganizationsStore, tt.args.organizationID) + tt.args.ctx = context.WithValue(tt.args.ctx, organizations.ContextKey, tt.args.organizationID) + got, err := s.Get(tt.args.ctx, chronograf.OrganizationQuery{ID: &tt.args.organization.ID}) + if (err != nil) != tt.wantErr { + t.Errorf("%q. OrganizationsStore.Get() error = %v, wantErr %v", tt.name, err, tt.wantErr) + continue + } + if diff := cmp.Diff(got, tt.want, organizationCmpOptions...); diff != "" { + t.Errorf("%q. OrganizationsStore.Get():\n-got/+want\ndiff %s", tt.name, diff) + } + } +} + +func TestOrganizations_Update(t *testing.T) { + type fields struct { + OrganizationsStore chronograf.OrganizationsStore + } + type args struct { + organizationID string + ctx context.Context + organization *chronograf.Organization + name string + } + tests := []struct { + name string + fields fields + args args + want *chronograf.Organization + addFirst bool + wantErr bool + }{ + { + name: "Update Organization Name", + fields: fields{ + OrganizationsStore: &mocks.OrganizationsStore{ + UpdateF: func(ctx context.Context, s *chronograf.Organization) error { + return nil + }, + GetF: func(ctx context.Context, q chronograf.OrganizationQuery) (*chronograf.Organization, error) { + return &chronograf.Organization{ + ID: 1229, + Name: "doody", + }, nil + }, + }, + }, + args: args{ + organizationID: "1229", + ctx: context.Background(), + organization: &chronograf.Organization{ + ID: 1229, + Name: "howdy", + }, + name: "doody", + }, + want: &chronograf.Organization{ + Name: "doody", + }, + addFirst: true, + }, + } + for _, tt := range tests { + if tt.args.name != "" { + tt.args.organization.Name = tt.args.name + } + s := organizations.NewOrganizationsStore(tt.fields.OrganizationsStore, tt.args.organizationID) + tt.args.ctx = context.WithValue(tt.args.ctx, organizations.ContextKey, tt.args.organizationID) + err := s.Update(tt.args.ctx, tt.args.organization) + if (err != nil) != tt.wantErr { + t.Errorf("%q. OrganizationsStore.Update() error = %v, wantErr %v", tt.name, err, tt.wantErr) + continue + } + got, err := s.Get(tt.args.ctx, chronograf.OrganizationQuery{ID: &tt.args.organization.ID}) + if diff := cmp.Diff(got, tt.want, organizationCmpOptions...); diff != "" { + t.Errorf("%q. OrganizationsStore.Update():\n-got/+want\ndiff %s", tt.name, diff) + } + } +} diff --git a/server/auth.go b/server/auth.go index 6a5ee9a689..8cc70afd4c 100644 --- a/server/auth.go +++ b/server/auth.go @@ -115,7 +115,6 @@ func AuthorizedUser( return } ctx = context.WithValue(ctx, organizations.ContextKey, p.Organization) - // TODO: seems silly to look up a user twice u, err := store.Users(serverCtx).Get(serverCtx, chronograf.UserQuery{ Name: &p.Subject, diff --git a/server/me.go b/server/me.go index 8d343e7331..334c71f0f7 100644 --- a/server/me.go +++ b/server/me.go @@ -82,6 +82,7 @@ type meOrganizationRequest struct { func (s *Service) MeOrganization(auth oauth2.Authenticator) func(http.ResponseWriter, *http.Request) { return func(w http.ResponseWriter, r *http.Request) { ctx := r.Context() + serverCtx := serverContext(ctx) principal, err := auth.Validate(ctx, r) if err != nil { s.Logger.Error(fmt.Sprintf("Invalid principal: %v", err)) @@ -100,7 +101,7 @@ func (s *Service) MeOrganization(auth oauth2.Authenticator) func(http.ResponseWr Error(w, http.StatusInternalServerError, err.Error(), s.Logger) return } - _, err = s.Store.Organizations(ctx).Get(ctx, chronograf.OrganizationQuery{ID: &orgID}) + _, err = s.Store.Organizations(serverCtx).Get(serverCtx, chronograf.OrganizationQuery{ID: &orgID}) if err != nil { Error(w, http.StatusBadRequest, err.Error(), s.Logger) return @@ -115,7 +116,7 @@ func (s *Service) MeOrganization(auth oauth2.Authenticator) func(http.ResponseWr return } if p.Organization == "" { - defaultOrg, err := s.Store.Organizations(ctx).DefaultOrganization(ctx) + defaultOrg, err := s.Store.Organizations(serverCtx).DefaultOrganization(serverCtx) if err != nil { unknownErrorWithMessage(w, err, s.Logger) return diff --git a/server/mux.go b/server/mux.go index 94ec2e95ac..cd1a75baf7 100644 --- a/server/mux.go +++ b/server/mux.go @@ -111,10 +111,10 @@ func NewMux(opts MuxOpts, service Service) http.Handler { /* API */ // Organizations - router.GET("/chronograf/v1/organizations", EnsureSuperAdmin(service.Organizations)) + router.GET("/chronograf/v1/organizations", EnsureAdmin(service.Organizations)) router.POST("/chronograf/v1/organizations", EnsureSuperAdmin(service.NewOrganization)) - router.GET("/chronograf/v1/organizations/:id", EnsureSuperAdmin(service.OrganizationID)) + router.GET("/chronograf/v1/organizations/:id", EnsureAdmin(service.OrganizationID)) router.PATCH("/chronograf/v1/organizations/:id", EnsureSuperAdmin(service.UpdateOrganization)) router.DELETE("/chronograf/v1/organizations/:id", EnsureSuperAdmin(service.RemoveOrganization)) diff --git a/server/organizations_test.go b/server/organizations_test.go index 73abf98711..99724b89e5 100644 --- a/server/organizations_test.go +++ b/server/organizations_test.go @@ -70,7 +70,7 @@ func TestService_OrganizationID(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { s := &Service{ - Store: &Store{ + Store: &mocks.Store{ OrganizationsStore: tt.fields.OrganizationsStore, }, Logger: tt.fields.Logger, @@ -157,7 +157,7 @@ func TestService_Organizations(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { s := &Service{ - Store: &Store{ + Store: &mocks.Store{ OrganizationsStore: tt.fields.OrganizationsStore, }, Logger: tt.fields.Logger, @@ -238,7 +238,7 @@ func TestService_UpdateOrganization(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { s := &Service{ - Store: &Store{ + Store: &mocks.Store{ OrganizationsStore: tt.fields.OrganizationsStore, }, Logger: tt.fields.Logger, @@ -325,7 +325,7 @@ func TestService_RemoveOrganization(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { s := &Service{ - Store: &Store{ + Store: &mocks.Store{ OrganizationsStore: tt.fields.OrganizationsStore, }, Logger: tt.fields.Logger, diff --git a/server/stores.go b/server/stores.go index ddf2438d5e..d16388650c 100644 --- a/server/stores.go +++ b/server/stores.go @@ -178,9 +178,11 @@ func (s *Store) Dashboards(ctx context.Context) chronograf.DashboardsStore { // Organizations returns the underlying OrganizationsStore. func (s *Store) Organizations(ctx context.Context) chronograf.OrganizationsStore { - // TODO(desa): added for when https://github.com/influxdata/chronograf/pull/2294 lands if isServer := hasServerContext(ctx); isServer { return s.OrganizationsStore } - return s.OrganizationsStore + if org, ok := hasOrganizationContext(ctx); ok { + return organizations.NewOrganizationsStore(s.OrganizationsStore, org) + } + return &noop.OrganizationsStore{} }