Merge pull request #108 from owncloud/update-ocis-pkg-roles-manager

Switch over from roleCache+middleware to roleManager

.drone.star

@@ -1,7 +1,7 @@
 def main(ctx):
   before = [
     testing(ctx),
-    UITests(ctx, 'account-management-permission-checks', '9c24a3388e80f373f094c2b122594039c167c318', 'master', 'e0746d8d3a5879d2c0cd4aaf30c07ee98ab2b945')
+    UITests(ctx, 'account-management-permission-checks', 'cbc3ac599589db685ed2f007df6e979c00bdcf29', 'master', 'e0746d8d3a5879d2c0cd4aaf30c07ee98ab2b945')
   ]
 
   stages = [

changelog/unreleased/use-roles-manager.md

@@ -0,0 +1,7 @@
+Change: We make use of the roles manager to enforce permission checks
+
+The roles cache and its cache update middleware have been replaced with a roles manager in ocis-pkg/v2. We've switched
+over to the new roles manager implementation, to prepare for permission checks on grpc requests as well.
+
+https://github.com/owncloud/ocis-accounts/pull/108
+https://github.com/owncloud/ocis-pkg/pull/60

go.mod

@@ -28,7 +28,7 @@ require (
 	github.com/olekukonko/tablewriter v0.0.4
 	github.com/onsi/ginkgo v1.10.1 // indirect
 	github.com/onsi/gomega v1.7.0 // indirect
-	github.com/owncloud/ocis-pkg/v2 v2.4.1-0.20200828095914-d3b859484b2b
+	github.com/owncloud/ocis-pkg/v2 v2.4.1-0.20200902134813-1e87c6173ada
 	github.com/owncloud/ocis-settings v0.3.2-0.20200828130413-0cc0f5bf26fe
 	github.com/remyoudompheng/bigfft v0.0.0-20200410134404-eec4a21b6bb0 // indirect
 	github.com/restic/calens v0.2.0

go.sum

@@ -601,6 +601,7 @@ github.com/hashicorp/logutils v1.0.0/go.mod h1:QIAnNjmIWmVIIkWDTG1z5v++HQmx9WQRO
 github.com/hashicorp/mdns v1.0.0/go.mod h1:tL+uN++7HEJ6SQLQ2/p+z2pH24WQKWjBPkE0mNTz8vQ=
 github.com/hashicorp/memberlist v0.1.3/go.mod h1:ajVTdAv/9Im8oMAAj5G31PhhMCZJV2pPBoIllUwCN7I=
 github.com/hashicorp/serf v0.8.2/go.mod h1:6hOLApaqBFA1NXqRQAsxw9QxuDEvNxSQRwA/JwenrHc=
+github.com/haya14busa/goverage v0.0.0-20180129164344-eec3514a20b5 h1:FdBGmSkD2QpQzRWup//SGObvWf2nq89zj9+ta9OvI3A=
 github.com/haya14busa/goverage v0.0.0-20180129164344-eec3514a20b5/go.mod h1:0YZ2wQSuwviXXXGUiK6zXzskyBLAbLXhamxzcFHSLoM=
 github.com/hpcloud/tail v1.0.0 h1:nfCOvKYfkgYP8hkirhJocXT2+zOD8yUNjXaWfTlyFKI=
 github.com/hpcloud/tail v1.0.0/go.mod h1:ab1qPbhIpdTxEkNHXyeSf5vhxWSCs/tWer42PpOxQnU=
@@ -840,6 +841,10 @@ github.com/ovh/go-ovh v0.0.0-20181109152953-ba5adb4cf014/go.mod h1:joRatxRJaZBsY
 github.com/owncloud/ocis-pkg/v2 v2.4.0/go.mod h1:FSzIvhx9HcZcq4jgNaDowNvM7PTX/XCyoMvyfzidUpE=
 github.com/owncloud/ocis-pkg/v2 v2.4.1-0.20200828095914-d3b859484b2b h1:PRw0b4abdrDKloh417qPsS5lkB/bjJ3Rc4txzHx/hBg=
 github.com/owncloud/ocis-pkg/v2 v2.4.1-0.20200828095914-d3b859484b2b/go.mod h1:WdcVM54z0X7aQzS8eyGl7S5sjEMVBtLpfpzsPX3Z+Pw=
+github.com/owncloud/ocis-pkg/v2 v2.4.1-0.20200902134315-45210145492a h1:6iW1JWmAl5TFDhxGnPhL7VwolrcQsHWvXdZt+GremZI=
+github.com/owncloud/ocis-pkg/v2 v2.4.1-0.20200902134315-45210145492a/go.mod h1:WdcVM54z0X7aQzS8eyGl7S5sjEMVBtLpfpzsPX3Z+Pw=
+github.com/owncloud/ocis-pkg/v2 v2.4.1-0.20200902134813-1e87c6173ada h1:iVknnuT/z8QCAeBpHEcbI/AiQ9FOBvF5aOAFT3TIM+I=
+github.com/owncloud/ocis-pkg/v2 v2.4.1-0.20200902134813-1e87c6173ada/go.mod h1:WdcVM54z0X7aQzS8eyGl7S5sjEMVBtLpfpzsPX3Z+Pw=
 github.com/owncloud/ocis-settings v0.3.2-0.20200828091056-47af10a0e872/go.mod h1:vRge9QDkOsc6j76gPBmZs1Z5uOPrV4DIkZCgZCEFwBA=
 github.com/owncloud/ocis-settings v0.3.2-0.20200828130413-0cc0f5bf26fe h1:kiU5lz12R0LNJE1/zI2vxesZPWm6BvSO7hvZC8yOoAc=
 github.com/owncloud/ocis-settings v0.3.2-0.20200828130413-0cc0f5bf26fe/go.mod h1:vRge9QDkOsc6j76gPBmZs1Z5uOPrV4DIkZCgZCEFwBA=

pkg/proto/v0/accounts.pb.micro_test.go

@@ -170,8 +170,6 @@ func init() {
 		log.Fatalf("Could not create new service")
 	}
 
-	hdlr.Client = mockClient{}
-
 	err = proto.RegisterAccountsServiceHandler(service.Server(), hdlr)
 	if err != nil {
 		log.Fatal("could not register the Accounts handler")
@@ -1189,37 +1187,3 @@ func TestAccountUpdateReadOnlyField(t *testing.T) {
 
 	cleanUp(t)
 }
-
-type mockClient struct{}
-
-func (c mockClient) Init(option ...client.Option) error {
-	return nil
-}
-
-func (c mockClient) Options() client.Options {
-	return client.Options{}
-}
-
-func (c mockClient) NewMessage(topic string, msg interface{}, opts ...client.MessageOption) client.Message {
-	return nil
-}
-
-func (c mockClient) NewRequest(service, endpoint string, req interface{}, reqOpts ...client.RequestOption) client.Request {
-	return nil
-}
-
-func (c mockClient) Call(ctx context.Context, req client.Request, rsp interface{}, opts ...client.CallOption) error {
-	return nil
-}
-
-func (c mockClient) Stream(ctx context.Context, req client.Request, opts ...client.CallOption) (client.Stream, error) {
-	return nil, nil
-}
-
-func (c mockClient) Publish(ctx context.Context, msg client.Message, opts ...client.PublishOption) error {
-	return nil
-}
-
-func (c mockClient) String() string {
-	return "ClientMock"
-}

pkg/server/http/server.go

@@ -30,9 +30,21 @@ func Server(opts ...Option) http.Service {
 		http.Flags(options.Flags...),
 	)
 
-	roleCache := roles.NewCache(roles.Size(1024), roles.TTL(time.Hour*24*7))
-
-	handler, err := svc.New(svc.Logger(options.Logger), svc.Config(options.Config), svc.RoleCache(&roleCache))
+	// TODO this won't work with a registry other than mdns. Look into Micro's client initialization.
+	// https://github.com/owncloud/ocis-proxy/issues/38
+	rs := settings.NewRoleService("com.owncloud.api.settings", mclient.DefaultClient)
+	roleManager := roles.NewManager(
+		roles.CacheSize(1024),
+		roles.CacheTTL(time.Hour*24*7),
+		roles.Logger(options.Logger),
+		roles.RoleService(rs),
+	)
+	handler, err := svc.New(
+		svc.Logger(options.Logger),
+		svc.Config(options.Config),
+		svc.RoleManager(&roleManager),
+		svc.RoleService(rs),
+	)
 	if err != nil {
 		options.Logger.Fatal().Err(err).Msg("could not initialize service handler")
 	}
@@ -48,14 +60,6 @@ func Server(opts ...Option) http.Service {
 		account.Logger(options.Logger),
 		account.JWTSecret(options.Config.TokenManager.JWTSecret)),
 	)
-	// TODO this won't work with a registry other than mdns. Look into Micro's client initialization.
-	// https://github.com/owncloud/ocis-proxy/issues/38
-	rs := settings.NewRoleService("com.owncloud.api.settings", mclient.DefaultClient)
-	mux.Use(middleware.Roles(
-		options.Logger,
-		rs,
-		&roleCache,
-	))
 
 	mux.Use(middleware.Version(
 		options.Name,

pkg/service/v0/accounts.go

@@ -19,7 +19,7 @@ import (
 	merrors "github.com/micro/go-micro/v2/errors"
 	"github.com/owncloud/ocis-accounts/pkg/proto/v0"
 	"github.com/owncloud/ocis-accounts/pkg/provider"
-	"github.com/owncloud/ocis-pkg/v2/middleware"
+	"github.com/owncloud/ocis-pkg/v2/roles"
 	settings "github.com/owncloud/ocis-settings/pkg/proto/v0"
 	settings_svc "github.com/owncloud/ocis-settings/pkg/service/v0"
 	"github.com/rs/zerolog"
@@ -159,14 +159,18 @@ func (s Service) passwordIsValid(hash string, pwd string) (ok bool) {
 
 func (s Service) hasAccountManagementPermissions(ctx context.Context) bool {
 	// get roles from context
-	roleIDs, ok := middleware.ReadRoleIDsFromContext(ctx)
+	roleIDs, ok := roles.ReadRoleIDsFromContext(ctx)
 	if !ok {
-		// if there were no roleIDs on the request we have to assume that the request didn't come from the proxy
+		/**
+		 * FIXME: with this we are skipping permission checks on all requests that are coming in without roleIDs in the
+		 * metadata context. This is a huge security impairment, as that's the case not only for grpc requests but also
+		 * for unauthenticated http requests and http requests coming in without hitting the ocis-proxy first.
+		 */
 		return true
 	}
 
 	// check if permission is present in roles of the authenticated account
-	return s.RoleCache.FindPermissionByID(roleIDs, AccountManagementPermissionID) != nil
+	return s.RoleManager.FindPermissionByID(ctx, roleIDs, AccountManagementPermissionID) != nil
 }
 
 // ListAccounts implements the AccountsServiceHandler interface
@@ -346,9 +350,8 @@ func (s Service) CreateAccount(ctx context.Context, in *proto.CreateAccountReque
 		return
 	}
 
-	// TODO: All users for now as create Account request does not have any role field
-	rs := settings.NewRoleService("com.owncloud.api.settings", s.Client)
-	_, err = rs.AssignRoleToUser(ctx, &settings.AssignRoleToUserRequest{
+	// TODO: assign user role to all new users for now, as create Account request does not have any role field
+	_, err = s.RoleService.AssignRoleToUser(ctx, &settings.AssignRoleToUserRequest{
 		AccountUuid: acc.Id,
 		RoleId:      settings_svc.BundleUUIDRoleUser,
 	})

pkg/service/v0/option.go

@@ -15,7 +15,7 @@ type Options struct {
 	Logger      log.Logger
 	Config      *config.Config
 	RoleService settings.RoleService
-	RoleCache   *roles.Cache
+	RoleManager *roles.Manager
 }
 
 func newOptions(opts ...Option) Options {
@@ -49,9 +49,9 @@ func RoleService(val settings.RoleService) Option {
 	}
 }
 
-// RoleCache provides a function to set the roles cache option.
-func RoleCache(val *roles.Cache) Option {
+// RoleManager provides a function to set the roles manager option.
+func RoleManager(val *roles.Manager) Option {
 	return func(o *Options) {
-		o.RoleCache = val
+		o.RoleManager = val
 	}
 }

pkg/service/v0/service.go

@@ -17,8 +17,6 @@ import (
 	"github.com/blevesearch/bleve/analysis/analyzer/standard"
 	"github.com/blevesearch/bleve/analysis/token/lowercase"
 	"github.com/blevesearch/bleve/analysis/tokenizer/unicode"
-	mclient "github.com/micro/go-micro/v2/client"
-	mgrpc "github.com/micro/go-micro/v2/client/grpc"
 	"github.com/owncloud/ocis-accounts/pkg/config"
 	"github.com/owncloud/ocis-accounts/pkg/proto/v0"
 	"github.com/owncloud/ocis-pkg/v2/log"
@@ -33,10 +31,7 @@ func New(opts ...Option) (s *Service, err error) {
 	logger := options.Logger
 	cfg := options.Config
 	roleService := options.RoleService
-	if roleService == nil {
-		roleService = settings.NewRoleService("com.owncloud.api.settings", mgrpc.NewClient())
-	}
-	roleCache := options.RoleCache
+	roleManager := options.RoleManager
 	// read all user and group records
 
 	accountsDir := filepath.Join(cfg.Server.AccountsDataPath, "accounts")
@@ -327,11 +322,11 @@ func New(opts ...Option) (s *Service, err error) {
 	indexMapping.TypeField = "BleveType"
 
 	s = &Service{
-		id:        cfg.GRPC.Namespace + "." + cfg.Server.Name,
-		log:       logger,
-		Config:    cfg,
-		Client:    mgrpc.NewClient(),
-		RoleCache: roleCache,
+		id:          cfg.GRPC.Namespace + "." + cfg.Server.Name,
+		log:         logger,
+		Config:      cfg,
+		RoleService: roleService,
+		RoleManager: roleManager,
 	}
 
 	indexDir := filepath.Join(cfg.Server.AccountsDataPath, "index.bleve")
@@ -368,12 +363,12 @@ func assignRoleToUser(accountID, roleID string, rs settings.RoleService, logger
 
 // Service implements the AccountsServiceHandler interface
 type Service struct {
-	id        string
-	log       log.Logger
-	Config    *config.Config
-	index     bleve.Index
-	Client    mclient.Client
-	RoleCache *roles.Cache
+	id          string
+	log         log.Logger
+	Config      *config.Config
+	index       bleve.Index
+	RoleService settings.RoleService
+	RoleManager *roles.Manager
 }
 
 func cleanupID(id string) (string, error) {