From 4e22b0b1af487e4308456fc45bf2f09ac686e202 Mon Sep 17 00:00:00 2001 From: Lorenzo Affetti Date: Wed, 11 Mar 2020 15:57:21 +0100 Subject: [PATCH] fix(authorizer): fix endpoint handler auth --- authorizer/notification_endpoint.go | 52 +++++++++----- authorizer/notification_endpoint_test.go | 86 ++++++++++++------------ http/notification_endpoint.go | 15 ++++- 3 files changed, 91 insertions(+), 62 deletions(-) diff --git a/authorizer/notification_endpoint.go b/authorizer/notification_endpoint.go index e0cb6b596d..c0891a7f2e 100644 --- a/authorizer/notification_endpoint.go +++ b/authorizer/notification_endpoint.go @@ -29,6 +29,36 @@ func NewNotificationEndpointService( } } +func newNotificationEndpointPermission(a influxdb.Action, orgID, id influxdb.ID) (*influxdb.Permission, error) { + return influxdb.NewPermissionAtID(id, a, influxdb.NotificationEndpointResourceType, orgID) +} + +func authorizeReadNotificationEndpoint(ctx context.Context, orgID, id influxdb.ID) error { + p, err := newNotificationEndpointPermission(influxdb.ReadAction, orgID, id) + if err != nil { + return err + } + + if err := IsAllowed(ctx, *p); err != nil { + return err + } + + return nil +} + +func authorizeWriteNotificationEndpoint(ctx context.Context, orgID, id influxdb.ID) error { + p, err := newNotificationEndpointPermission(influxdb.WriteAction, orgID, id) + if err != nil { + return err + } + + if err := IsAllowed(ctx, *p); err != nil { + return err + } + + return nil +} + // FindNotificationEndpointByID checks to see if the authorizer on context has read access to the id provided. func (s *NotificationEndpointService) FindNotificationEndpointByID(ctx context.Context, id influxdb.ID) (influxdb.NotificationEndpoint, error) { edp, err := s.s.FindNotificationEndpointByID(ctx, id) @@ -36,7 +66,7 @@ func (s *NotificationEndpointService) FindNotificationEndpointByID(ctx context.C return nil, err } - if err := authorizeReadOrg(ctx, edp.GetOrgID()); err != nil { + if err := authorizeReadNotificationEndpoint(ctx, edp.GetOrgID(), edp.GetID()); err != nil { return nil, err } @@ -64,7 +94,7 @@ func (s *NotificationEndpointService) FindNotificationEndpoints(ctx context.Cont // https://github.com/golang/go/wiki/SliceTricks#filtering-without-allocating endpoints := edps[:0] for _, edp := range edps { - err := authorizeReadOrg(ctx, edp.GetOrgID()) + err := authorizeReadNotificationEndpoint(ctx, edp.GetOrgID(), edp.GetID()) if err != nil && influxdb.ErrorCode(err) != influxdb.EUnauthorized { return nil, 0, err } @@ -85,19 +115,9 @@ func (s *NotificationEndpointService) CreateNotificationEndpoint(ctx context.Con if err != nil { return err } - - pOrg, err := newOrgPermission(influxdb.WriteAction, edp.GetOrgID()) - if err != nil { + if err := IsAllowed(ctx, *p); err != nil { return err } - - err0 := IsAllowed(ctx, *p) - err1 := IsAllowed(ctx, *pOrg) - - if err0 != nil && err1 != nil { - return err0 - } - return s.s.CreateNotificationEndpoint(ctx, edp, userID) } @@ -108,7 +128,7 @@ func (s *NotificationEndpointService) UpdateNotificationEndpoint(ctx context.Con return nil, err } - if err := authorizeWriteOrg(ctx, edp.GetOrgID()); err != nil { + if err := authorizeWriteNotificationEndpoint(ctx, edp.GetOrgID(), edp.GetID()); err != nil { return nil, err } @@ -122,7 +142,7 @@ func (s *NotificationEndpointService) PatchNotificationEndpoint(ctx context.Cont return nil, err } - if err := authorizeWriteOrg(ctx, edp.GetOrgID()); err != nil { + if err := authorizeWriteNotificationEndpoint(ctx, edp.GetOrgID(), edp.GetID()); err != nil { return nil, err } @@ -136,7 +156,7 @@ func (s *NotificationEndpointService) DeleteNotificationEndpoint(ctx context.Con return nil, 0, err } - if err := authorizeWriteOrg(ctx, edp.GetOrgID()); err != nil { + if err := authorizeWriteNotificationEndpoint(ctx, edp.GetOrgID(), edp.GetID()); err != nil { return nil, 0, err } diff --git a/authorizer/notification_endpoint_test.go b/authorizer/notification_endpoint_test.go index 65c30a1fa4..01df4e9ec4 100644 --- a/authorizer/notification_endpoint_test.go +++ b/authorizer/notification_endpoint_test.go @@ -63,10 +63,10 @@ func TestNotificationEndpointService_FindNotificationEndpointByID(t *testing.T) }, args: args{ permission: influxdb.Permission{ - Action: "read", + Action: influxdb.ReadAction, Resource: influxdb.Resource{ - Type: influxdb.OrgsResourceType, - ID: influxdbtesting.IDPtr(10), + Type: influxdb.NotificationEndpointResourceType, + OrgID: influxdbtesting.IDPtr(10), }, }, id: 1, @@ -92,7 +92,7 @@ func TestNotificationEndpointService_FindNotificationEndpointByID(t *testing.T) }, args: args{ permission: influxdb.Permission{ - Action: "read", + Action: influxdb.ReadAction, Resource: influxdb.Resource{ Type: influxdb.NotificationEndpointResourceType, ID: influxdbtesting.IDPtr(2), @@ -102,7 +102,7 @@ func TestNotificationEndpointService_FindNotificationEndpointByID(t *testing.T) }, wants: wants{ err: &influxdb.Error{ - Msg: "read:orgs/000000000000000a is unauthorized", + Msg: "read:orgs/000000000000000a/notificationEndpoints/0000000000000001 is unauthorized", Code: influxdb.EUnauthorized, }, }, @@ -164,10 +164,10 @@ func TestNotificationEndpointService_FindNotificationEndpoints(t *testing.T) { }, args: args{ permission: influxdb.Permission{ - Action: "read", + Action: influxdb.ReadAction, Resource: influxdb.Resource{ - Type: influxdb.OrgsResourceType, - ID: influxdbtesting.IDPtr(10), + Type: influxdb.NotificationEndpointResourceType, + OrgID: influxdbtesting.IDPtr(10), }, }, }, @@ -254,17 +254,17 @@ func TestNotificationEndpointService_UpdateNotificationEndpoint(t *testing.T) { id: 1, permissions: []influxdb.Permission{ { - Action: "write", + Action: influxdb.WriteAction, Resource: influxdb.Resource{ - Type: influxdb.OrgsResourceType, - ID: influxdbtesting.IDPtr(10), + Type: influxdb.NotificationEndpointResourceType, + OrgID: influxdbtesting.IDPtr(10), }, }, { - Action: "read", + Action: influxdb.ReadAction, Resource: influxdb.Resource{ - Type: influxdb.OrgsResourceType, - ID: influxdbtesting.IDPtr(10), + Type: influxdb.NotificationEndpointResourceType, + OrgID: influxdbtesting.IDPtr(10), }, }, }, @@ -299,17 +299,17 @@ func TestNotificationEndpointService_UpdateNotificationEndpoint(t *testing.T) { id: 1, permissions: []influxdb.Permission{ { - Action: "read", + Action: influxdb.ReadAction, Resource: influxdb.Resource{ - Type: influxdb.OrgsResourceType, - ID: influxdbtesting.IDPtr(10), + Type: influxdb.NotificationEndpointResourceType, + OrgID: influxdbtesting.IDPtr(10), }, }, }, }, wants: wants{ err: &influxdb.Error{ - Msg: "write:orgs/000000000000000a is unauthorized", + Msg: "write:orgs/000000000000000a/notificationEndpoints/0000000000000001 is unauthorized", Code: influxdb.EUnauthorized, }, }, @@ -375,17 +375,17 @@ func TestNotificationEndpointService_PatchNotificationEndpoint(t *testing.T) { id: 1, permissions: []influxdb.Permission{ { - Action: "write", + Action: influxdb.WriteAction, Resource: influxdb.Resource{ - Type: influxdb.OrgsResourceType, - ID: influxdbtesting.IDPtr(10), + Type: influxdb.NotificationEndpointResourceType, + OrgID: influxdbtesting.IDPtr(10), }, }, { - Action: "read", + Action: influxdb.ReadAction, Resource: influxdb.Resource{ - Type: influxdb.OrgsResourceType, - ID: influxdbtesting.IDPtr(10), + Type: influxdb.NotificationEndpointResourceType, + OrgID: influxdbtesting.IDPtr(10), }, }, }, @@ -420,17 +420,17 @@ func TestNotificationEndpointService_PatchNotificationEndpoint(t *testing.T) { id: 1, permissions: []influxdb.Permission{ { - Action: "read", + Action: influxdb.ReadAction, Resource: influxdb.Resource{ - Type: influxdb.OrgsResourceType, - ID: influxdbtesting.IDPtr(10), + Type: influxdb.NotificationEndpointResourceType, + OrgID: influxdbtesting.IDPtr(10), }, }, }, }, wants: wants{ err: &influxdb.Error{ - Msg: "write:orgs/000000000000000a is unauthorized", + Msg: "write:orgs/000000000000000a/notificationEndpoints/0000000000000001 is unauthorized", Code: influxdb.EUnauthorized, }, }, @@ -490,17 +490,17 @@ func TestNotificationEndpointService_DeleteNotificationEndpoint(t *testing.T) { id: 1, permissions: []influxdb.Permission{ { - Action: "write", + Action: influxdb.WriteAction, Resource: influxdb.Resource{ - Type: influxdb.OrgsResourceType, - ID: influxdbtesting.IDPtr(10), + Type: influxdb.NotificationEndpointResourceType, + OrgID: influxdbtesting.IDPtr(10), }, }, { - Action: "read", + Action: influxdb.ReadAction, Resource: influxdb.Resource{ - Type: influxdb.OrgsResourceType, - ID: influxdbtesting.IDPtr(10), + Type: influxdb.NotificationEndpointResourceType, + OrgID: influxdbtesting.IDPtr(10), }, }, }, @@ -530,17 +530,17 @@ func TestNotificationEndpointService_DeleteNotificationEndpoint(t *testing.T) { id: 1, permissions: []influxdb.Permission{ { - Action: "read", + Action: influxdb.ReadAction, Resource: influxdb.Resource{ - Type: influxdb.OrgsResourceType, - ID: influxdbtesting.IDPtr(10), + Type: influxdb.NotificationEndpointResourceType, + OrgID: influxdbtesting.IDPtr(10), }, }, }, }, wants: wants{ err: &influxdb.Error{ - Msg: "write:orgs/000000000000000a is unauthorized", + Msg: "write:orgs/000000000000000a/notificationEndpoints/0000000000000001 is unauthorized", Code: influxdb.EUnauthorized, }, }, @@ -592,7 +592,7 @@ func TestNotificationEndpointService_CreateNotificationEndpoint(t *testing.T) { args: args{ orgID: 10, permission: influxdb.Permission{ - Action: "write", + Action: influxdb.WriteAction, Resource: influxdb.Resource{ Type: influxdb.NotificationEndpointResourceType, OrgID: influxdbtesting.IDPtr(10), @@ -615,10 +615,10 @@ func TestNotificationEndpointService_CreateNotificationEndpoint(t *testing.T) { args: args{ orgID: 10, permission: influxdb.Permission{ - Action: "write", + Action: influxdb.WriteAction, Resource: influxdb.Resource{ - Type: influxdb.OrgsResourceType, - ID: influxdbtesting.IDPtr(10), + Type: influxdb.NotificationEndpointResourceType, + OrgID: influxdbtesting.IDPtr(10), }, }, }, @@ -638,7 +638,7 @@ func TestNotificationEndpointService_CreateNotificationEndpoint(t *testing.T) { args: args{ orgID: 10, permission: influxdb.Permission{ - Action: "write", + Action: influxdb.WriteAction, Resource: influxdb.Resource{ Type: influxdb.NotificationEndpointResourceType, ID: influxdbtesting.IDPtr(1), diff --git a/http/notification_endpoint.go b/http/notification_endpoint.go index 7885b6415f..dfd729c343 100644 --- a/http/notification_endpoint.go +++ b/http/notification_endpoint.go @@ -573,6 +573,9 @@ var _ influxdb.NotificationEndpointService = (*NotificationEndpointService)(nil) // FindNotificationEndpointByID returns a single notification endpoint by ID. func (s *NotificationEndpointService) FindNotificationEndpointByID(ctx context.Context, id influxdb.ID) (influxdb.NotificationEndpoint, error) { + if !id.Valid() { + return nil, fmt.Errorf("invalid ID: please provide a valid ID") + } var resp notificationEndpointDecoder err := s.Client. Get(prefixNotificationEndpoints, id.String()). @@ -621,8 +624,6 @@ func (s *NotificationEndpointService) FindNotificationEndpoints(ctx context.Cont // TODO(@jsteenb2): this is unsatisfactory, we have no way of grabbing the new notification endpoint without // serious hacky hackertoning. Put it on the list... func (s *NotificationEndpointService) CreateNotificationEndpoint(ctx context.Context, ne influxdb.NotificationEndpoint, userID influxdb.ID) error { - // userID is ignored here since server reads it off - // the token/auth. its a nothing burger here var resp notificationEndpointDecoder err := s.Client. PostJSON(¬ificationEndpointEncoder{ne: ne}, prefixNotificationEndpoints). @@ -640,7 +641,9 @@ func (s *NotificationEndpointService) CreateNotificationEndpoint(ctx context.Con // UpdateNotificationEndpoint updates a single notification endpoint. // Returns the new notification endpoint after update. func (s *NotificationEndpointService) UpdateNotificationEndpoint(ctx context.Context, id influxdb.ID, ne influxdb.NotificationEndpoint, userID influxdb.ID) (influxdb.NotificationEndpoint, error) { - // userID is ignored since userID is grabbed off the http auth set on the client + if !id.Valid() { + return nil, fmt.Errorf("invalid ID: please provide a valid ID") + } var resp notificationEndpointDecoder err := s.Client. PutJSON(¬ificationEndpointEncoder{ne: ne}, prefixNotificationEndpoints, id.String()). @@ -655,6 +658,9 @@ func (s *NotificationEndpointService) UpdateNotificationEndpoint(ctx context.Con // PatchNotificationEndpoint updates a single notification endpoint with changeset. // Returns the new notification endpoint state after update. func (s *NotificationEndpointService) PatchNotificationEndpoint(ctx context.Context, id influxdb.ID, upd influxdb.NotificationEndpointUpdate) (influxdb.NotificationEndpoint, error) { + if !id.Valid() { + return nil, fmt.Errorf("invalid ID: please provide a valid ID") + } if err := upd.Valid(); err != nil { return nil, err } @@ -676,6 +682,9 @@ func (s *NotificationEndpointService) PatchNotificationEndpoint(ctx context.Cont // then see what falls out :flushed... for now returning nothing for secrets, orgID, and only returning an error. This makes // the code/design smell super obvious imo func (s *NotificationEndpointService) DeleteNotificationEndpoint(ctx context.Context, id influxdb.ID) ([]influxdb.SecretField, influxdb.ID, error) { + if !id.Valid() { + return nil, 0, fmt.Errorf("invalid ID: please provide a valid ID") + } err := s.Client. Delete(prefixNotificationEndpoints, id.String()). Do(ctx)