From 09985503409e596f958b9b9976f453937e37a2bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Tue, 6 Dec 2022 15:17:20 +0000 Subject: [PATCH 01/10] standalone graph service with LDAP MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- services/graph/pkg/config/http.go | 1 + services/graph/pkg/server/http/server.go | 50 +++++++++++++++++------- services/graph/pkg/service/v0/option.go | 26 +++++++----- services/graph/pkg/service/v0/service.go | 15 ++++--- services/graph/pkg/service/v0/users.go | 37 +++++++++--------- 5 files changed, 79 insertions(+), 50 deletions(-) diff --git a/services/graph/pkg/config/http.go b/services/graph/pkg/config/http.go index ec521094c89..7fcb40572f1 100644 --- a/services/graph/pkg/config/http.go +++ b/services/graph/pkg/config/http.go @@ -8,4 +8,5 @@ type HTTP struct { Namespace string `yaml:"-"` Root string `yaml:"root" env:"GRAPH_HTTP_ROOT" desc:"Subdirectory that serves as the root for this HTTP service."` TLS shared.HTTPServiceTLS `yaml:"tls"` + APIToken string `yaml:"addr" env:"GRAPH_HTTP_API_TOKEN" desc:"An optional API bearer token"` } diff --git a/services/graph/pkg/server/http/server.go b/services/graph/pkg/server/http/server.go index fdb0c2fa08e..9f08878aab0 100644 --- a/services/graph/pkg/server/http/server.go +++ b/services/graph/pkg/server/http/server.go @@ -4,6 +4,7 @@ import ( "crypto/tls" "crypto/x509" "fmt" + stdhttp "net/http" "os" "github.com/cs3org/reva/v2/pkg/events/server" @@ -12,8 +13,10 @@ import ( "github.com/owncloud/ocis/v2/ocis-pkg/account" ociscrypto "github.com/owncloud/ocis/v2/ocis-pkg/crypto" "github.com/owncloud/ocis/v2/ocis-pkg/middleware" + "github.com/owncloud/ocis/v2/ocis-pkg/service/grpc" "github.com/owncloud/ocis/v2/ocis-pkg/service/http" "github.com/owncloud/ocis/v2/ocis-pkg/version" + settingssvc "github.com/owncloud/ocis/v2/protogen/gen/ocis/services/settings/v0" graphMiddleware "github.com/owncloud/ocis/v2/services/graph/pkg/middleware" svc "github.com/owncloud/ocis/v2/services/graph/pkg/service/v0" "github.com/pkg/errors" @@ -82,25 +85,42 @@ func Server(opts ...Option) (http.Service, error) { } } - handle := svc.NewService( - svc.Logger(options.Logger), - svc.Config(options.Config), - svc.Middleware( - middleware.TraceContext, - chimiddleware.RequestID, - middleware.Version( - "graph", - version.GetString(), - ), - middleware.Logger( - options.Logger, - ), + middlewares := []func(stdhttp.Handler) stdhttp.Handler{ + middleware.TraceContext, + chimiddleware.RequestID, + middleware.Version( + "graph", + version.GetString(), + ), + middleware.Logger( + options.Logger, + ), + } + // how do we secure the api? + var requireAdminMiddleware func(stdhttp.Handler) stdhttp.Handler + var roleService svc.RoleService + if options.Config.HTTP.APIToken == "" { + middlewares = append(middlewares, graphMiddleware.Auth( account.Logger(options.Logger), account.JWTSecret(options.Config.TokenManager.JWTSecret), - ), - ), + )) + roleService = settingssvc.NewRoleService("com.owncloud.api.settings", grpc.DefaultClient()) + } else { + middlewares = append(middlewares, middleware.Token(options.Config.HTTP.APIToken)) + // use a dummy admin middleware for the chi router + requireAdminMiddleware = func(next stdhttp.Handler) stdhttp.Handler { + return next + } + } + + handle := svc.NewService( + svc.Logger(options.Logger), + svc.Config(options.Config), + svc.Middleware(middlewares...), svc.EventsPublisher(publisher), + svc.WithRoleService(roleService), + svc.RequireAdminMiddleware(requireAdminMiddleware), ) if handle == nil { diff --git a/services/graph/pkg/service/v0/option.go b/services/graph/pkg/service/v0/option.go index a9a70b5bbd2..44cc9a9f5ec 100644 --- a/services/graph/pkg/service/v0/option.go +++ b/services/graph/pkg/service/v0/option.go @@ -16,15 +16,16 @@ type Option func(o *Options) // Options defines the available options for this package. type Options struct { - Logger log.Logger - Config *config.Config - Middleware []func(http.Handler) http.Handler - GatewayClient GatewayClient - IdentityBackend identity.Backend - RoleService RoleService - PermissionService Permissions - RoleManager *roles.Manager - EventsPublisher events.Publisher + Logger log.Logger + Config *config.Config + Middleware []func(http.Handler) http.Handler + RequireAdminMiddleware func(http.Handler) http.Handler + GatewayClient GatewayClient + IdentityBackend identity.Backend + RoleService RoleService + PermissionService Permissions + RoleManager *roles.Manager + EventsPublisher events.Publisher } // newOptions initializes the available default options. @@ -59,6 +60,13 @@ func Middleware(val ...func(http.Handler) http.Handler) Option { } } +// RequireAdminMiddleware provides a function to set the RequireAdminMiddleware option. +func RequireAdminMiddleware(val func(http.Handler) http.Handler) Option { + return func(o *Options) { + o.RequireAdminMiddleware = val + } +} + // WithGatewayClient provides a function to set the gateway client option. func WithGatewayClient(val GatewayClient) Option { return func(o *Options) { diff --git a/services/graph/pkg/service/v0/service.go b/services/graph/pkg/service/v0/service.go index 6e3f67a18c4..fdbdf1f6b41 100644 --- a/services/graph/pkg/service/v0/service.go +++ b/services/graph/pkg/service/v0/service.go @@ -145,12 +145,6 @@ func NewService(opts ...Option) Service { svc.identityBackend = options.IdentityBackend } - if options.RoleService == nil { - svc.roleService = settingssvc.NewRoleService("com.owncloud.api.settings", grpc.DefaultClient()) - } else { - svc.roleService = options.RoleService - } - if options.PermissionService == nil { svc.permissionsService = settingssvc.NewPermissionService("com.owncloud.api.settings", grpc.DefaultClient()) } else { @@ -167,12 +161,17 @@ func NewService(opts ...Option) Service { m := roles.NewManager( roles.StoreOptions(storeOptions), roles.Logger(options.Logger), - roles.RoleService(svc.roleService), + roles.RoleService(options.RoleService), ) roleManager = &m } - requireAdmin := graphm.RequireAdmin(roleManager, options.Logger) + var requireAdmin func(http.Handler) http.Handler + if options.RequireAdminMiddleware == nil { + requireAdmin = graphm.RequireAdmin(roleManager, options.Logger) + } else { + requireAdmin = options.RequireAdminMiddleware + } m.Route(options.Config.HTTP.Root, func(r chi.Router) { r.Use(middleware.StripSlashes) diff --git a/services/graph/pkg/service/v0/users.go b/services/graph/pkg/service/v0/users.go index 9089a00e2dc..709dc930264 100644 --- a/services/graph/pkg/service/v0/users.go +++ b/services/graph/pkg/service/v0/users.go @@ -158,26 +158,27 @@ func (g Graph) PostUser(w http.ResponseWriter, r *http.Request) { return } - // All users get the user role by default currently. - // to all new users for now, as create Account request does not have any role field - if g.roleService == nil { - // log as error, admin needs to do something about it - logger.Error().Str("id", *u.Id).Msg("could not create user: role service not configured") - errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, "could not assign role to account: roleService not configured") - return - } - if _, err = g.roleService.AssignRoleToUser(r.Context(), &settings.AssignRoleToUserRequest{ - AccountUuid: *u.Id, - RoleId: settingssvc.BundleUUIDRoleUser, - }); err != nil { - // log as error, admin eventually needs to do something - logger.Error().Err(err).Str("id", *u.Id).Str("role", settingssvc.BundleUUIDRoleUser).Msg("could not create user: role assignment failed") - errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, "role assignment failed") - return + // assign roles if possible + if g.roleService != nil { + // All users get the user role by default currently. + // to all new users for now, as create Account request does not have any role field + if _, err = g.roleService.AssignRoleToUser(r.Context(), &settings.AssignRoleToUserRequest{ + AccountUuid: *u.Id, + RoleId: settingssvc.BundleUUIDRoleUser, + }); err != nil { + // log as error, admin eventually needs to do something + logger.Error().Err(err).Str("id", *u.Id).Str("role", settingssvc.BundleUUIDRoleUser).Msg("could not create user: role assignment failed") + errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, "role assignment failed") + return + } } - currentUser := revactx.ContextMustGetUser(r.Context()) - g.publishEvent(events.UserCreated{Executant: currentUser.Id, UserID: *u.Id}) + e := events.UserCreated{UserID: *u.Id} + currentUser, ok := revactx.ContextGetUser(r.Context()) + if ok { + e.Executant = currentUser.GetId() + } + g.publishEvent(e) render.Status(r, http.StatusOK) render.JSON(w, r, u) From f77c6c7aff7c27c3ab7226ddfd182a55e52fee8e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Tue, 6 Dec 2022 15:45:35 +0000 Subject: [PATCH 02/10] no panic on PATCH and DELETE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- services/graph/pkg/service/v0/users.go | 35 +++++++++++++------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/services/graph/pkg/service/v0/users.go b/services/graph/pkg/service/v0/users.go index 709dc930264..c2547458774 100644 --- a/services/graph/pkg/service/v0/users.go +++ b/services/graph/pkg/service/v0/users.go @@ -174,8 +174,7 @@ func (g Graph) PostUser(w http.ResponseWriter, r *http.Request) { } e := events.UserCreated{UserID: *u.Id} - currentUser, ok := revactx.ContextGetUser(r.Context()) - if ok { + if currentUser, ok := revactx.ContextGetUser(r.Context()); ok { e.Executant = currentUser.GetId() } g.publishEvent(e) @@ -308,12 +307,14 @@ func (g Graph) DeleteUser(w http.ResponseWriter, r *http.Request) { return } - currentUser := revactx.ContextMustGetUser(r.Context()) - - if currentUser.GetId().GetOpaqueId() == user.GetId() { - logger.Debug().Msg("could not delete user: self deletion forbidden") - errorcode.NotAllowed.Render(w, r, http.StatusForbidden, "self deletion forbidden") - return + e := events.UserDeleted{UserID: user.GetId()} + if currentUser, ok := revactx.ContextGetUser(r.Context()); ok { + if currentUser.GetId().GetOpaqueId() == user.GetId() { + logger.Debug().Msg("could not delete user: self deletion forbidden") + errorcode.NotAllowed.Render(w, r, http.StatusForbidden, "self deletion forbidden") + return + } + e.Executant = currentUser.GetId() } logger.Debug(). @@ -383,7 +384,7 @@ func (g Graph) DeleteUser(w http.ResponseWriter, r *http.Request) { } } - g.publishEvent(events.UserDeleted{Executant: currentUser.Id, UserID: user.GetId()}) + g.publishEvent(e) render.Status(r, http.StatusNoContent) render.NoContent(w, r) @@ -444,14 +445,14 @@ func (g Graph) PatchUser(w http.ResponseWriter, r *http.Request) { return } - currentUser := revactx.ContextMustGetUser(r.Context()) - g.publishEvent( - events.UserFeatureChanged{ - Executant: currentUser.Id, - UserID: nameOrID, - Features: features, - }, - ) + e := events.UserFeatureChanged{ + UserID: nameOrID, + Features: features, + } + if currentUser, ok := revactx.ContextGetUser(r.Context()); ok { + e.Executant = currentUser.GetId() + } + g.publishEvent(e) render.Status(r, http.StatusOK) render.JSON(w, r, u) From 90698906fa631424e62b9f12bb874a27a1553cb3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Wed, 7 Dec 2022 11:37:45 +0000 Subject: [PATCH 03/10] fix apitoken yaml key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- services/graph/pkg/config/http.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/graph/pkg/config/http.go b/services/graph/pkg/config/http.go index 7fcb40572f1..00d4144d9a3 100644 --- a/services/graph/pkg/config/http.go +++ b/services/graph/pkg/config/http.go @@ -8,5 +8,5 @@ type HTTP struct { Namespace string `yaml:"-"` Root string `yaml:"root" env:"GRAPH_HTTP_ROOT" desc:"Subdirectory that serves as the root for this HTTP service."` TLS shared.HTTPServiceTLS `yaml:"tls"` - APIToken string `yaml:"addr" env:"GRAPH_HTTP_API_TOKEN" desc:"An optional API bearer token"` + APIToken string `yaml:"apitoken" env:"GRAPH_HTTP_API_TOKEN" desc:"An optional API bearer token"` } From e3ae0fe5d9eeb5b6bf8f4fb885774b9eeeb3fd3e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Wed, 7 Dec 2022 12:26:22 +0000 Subject: [PATCH 04/10] update user, fix response codes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- services/graph/pkg/identity/ldap.go | 1 + services/graph/pkg/server/http/server.go | 10 ++- services/graph/pkg/service/v0/option.go | 4 +- services/graph/pkg/service/v0/service.go | 13 +-- services/graph/pkg/service/v0/users.go | 96 ++++++++++++--------- services/graph/pkg/service/v0/users_test.go | 4 +- 6 files changed, 69 insertions(+), 59 deletions(-) diff --git a/services/graph/pkg/identity/ldap.go b/services/graph/pkg/identity/ldap.go index f9a3d1afab1..ea879e6be6b 100644 --- a/services/graph/pkg/identity/ldap.go +++ b/services/graph/pkg/identity/ldap.go @@ -280,6 +280,7 @@ func (i *LDAP) UpdateUser(ctx context.Context, nameOrID string, user libregraph. updateNeeded = true } } + // TODO implement account disabled/enabled if updateNeeded { if err := i.conn.Modify(&mr); err != nil { diff --git a/services/graph/pkg/server/http/server.go b/services/graph/pkg/server/http/server.go index 9f08878aab0..5e7bb62f5e8 100644 --- a/services/graph/pkg/server/http/server.go +++ b/services/graph/pkg/server/http/server.go @@ -8,6 +8,7 @@ import ( "os" "github.com/cs3org/reva/v2/pkg/events/server" + "github.com/cs3org/reva/v2/pkg/rgrpc/todo/pool" chimiddleware "github.com/go-chi/chi/v5/middleware" "github.com/go-micro/plugins/v4/events/natsjs" "github.com/owncloud/ocis/v2/ocis-pkg/account" @@ -99,6 +100,7 @@ func Server(opts ...Option) (http.Service, error) { // how do we secure the api? var requireAdminMiddleware func(stdhttp.Handler) stdhttp.Handler var roleService svc.RoleService + var gatewayClient svc.GatewayClient if options.Config.HTTP.APIToken == "" { middlewares = append(middlewares, graphMiddleware.Auth( @@ -106,12 +108,17 @@ func Server(opts ...Option) (http.Service, error) { account.JWTSecret(options.Config.TokenManager.JWTSecret), )) roleService = settingssvc.NewRoleService("com.owncloud.api.settings", grpc.DefaultClient()) + gatewayClient, err = pool.GetGatewayServiceClient(options.Config.Reva.Address, options.Config.Reva.GetRevaOptions()...) + if err != nil { + return http.Service{}, errors.Wrap(err, "could not initialize gateway client") + } } else { middlewares = append(middlewares, middleware.Token(options.Config.HTTP.APIToken)) // use a dummy admin middleware for the chi router requireAdminMiddleware = func(next stdhttp.Handler) stdhttp.Handler { return next } + // no gatewayclient needed } handle := svc.NewService( @@ -120,7 +127,8 @@ func Server(opts ...Option) (http.Service, error) { svc.Middleware(middlewares...), svc.EventsPublisher(publisher), svc.WithRoleService(roleService), - svc.RequireAdminMiddleware(requireAdminMiddleware), + svc.WithRequireAdminMiddleware(requireAdminMiddleware), + svc.WithGatewayClient(gatewayClient), ) if handle == nil { diff --git a/services/graph/pkg/service/v0/option.go b/services/graph/pkg/service/v0/option.go index 44cc9a9f5ec..8588fe4dd80 100644 --- a/services/graph/pkg/service/v0/option.go +++ b/services/graph/pkg/service/v0/option.go @@ -60,8 +60,8 @@ func Middleware(val ...func(http.Handler) http.Handler) Option { } } -// RequireAdminMiddleware provides a function to set the RequireAdminMiddleware option. -func RequireAdminMiddleware(val func(http.Handler) http.Handler) Option { +// WithRequireAdminMiddleware provides a function to set the RequireAdminMiddleware option. +func WithRequireAdminMiddleware(val func(http.Handler) http.Handler) Option { return func(o *Options) { o.RequireAdminMiddleware = val } diff --git a/services/graph/pkg/service/v0/service.go b/services/graph/pkg/service/v0/service.go index fdbdf1f6b41..c08c5ebdd64 100644 --- a/services/graph/pkg/service/v0/service.go +++ b/services/graph/pkg/service/v0/service.go @@ -7,7 +7,6 @@ import ( "os" "strconv" - "github.com/cs3org/reva/v2/pkg/rgrpc/todo/pool" "github.com/go-chi/chi/v5" "github.com/go-chi/chi/v5/middleware" "github.com/jellydator/ttlcache/v2" @@ -69,17 +68,9 @@ func NewService(opts ...Option) Service { logger: &options.Logger, spacePropertiesCache: ttlcache.NewCache(), eventsPublisher: options.EventsPublisher, + gatewayClient: options.GatewayClient, } - if options.GatewayClient == nil { - var err error - svc.gatewayClient, err = pool.GetGatewayServiceClient(options.Config.Reva.Address, options.Config.Reva.GetRevaOptions()...) - if err != nil { - options.Logger.Error().Err(err).Msg("Could not get gateway client") - return nil - } - } else { - svc.gatewayClient = options.GatewayClient - } + if options.IdentityBackend == nil { switch options.Config.Identity.Backend { case "cs3": diff --git a/services/graph/pkg/service/v0/users.go b/services/graph/pkg/service/v0/users.go index c2547458774..cb5f108fd4b 100644 --- a/services/graph/pkg/service/v0/users.go +++ b/services/graph/pkg/service/v0/users.go @@ -179,7 +179,7 @@ func (g Graph) PostUser(w http.ResponseWriter, r *http.Request) { } g.publishEvent(e) - render.Status(r, http.StatusOK) + render.Status(r, http.StatusCreated) render.JSON(w, r, u) } @@ -317,57 +317,59 @@ func (g Graph) DeleteUser(w http.ResponseWriter, r *http.Request) { e.Executant = currentUser.GetId() } - logger.Debug(). - Str("user", user.GetId()). - Msg("calling list spaces with user filter to fetch the personal space for deletion") - opaque := utils.AppendPlainToOpaque(nil, "unrestricted", "T") - f := listStorageSpacesUserFilter(user.GetId()) - lspr, err := g.gatewayClient.ListStorageSpaces(r.Context(), &storageprovider.ListStorageSpacesRequest{ - Opaque: opaque, - Filters: []*storageprovider.ListStorageSpacesRequest_Filter{f}, - }) - if err != nil { - // transport error, log as error - logger.Error().Err(err).Msg("could not fetch spaces: transport error") - errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, "could not fetch spaces for deletion, aborting") - return - } - for _, sp := range lspr.GetStorageSpaces() { - if !(sp.SpaceType == "personal" && sp.Owner.Id.OpaqueId == user.GetId()) { - continue + if g.gatewayClient != nil { + logger.Debug(). + Str("user", user.GetId()). + Msg("calling list spaces with user filter to fetch the personal space for deletion") + opaque := utils.AppendPlainToOpaque(nil, "unrestricted", "T") + f := listStorageSpacesUserFilter(user.GetId()) + lspr, err := g.gatewayClient.ListStorageSpaces(r.Context(), &storageprovider.ListStorageSpacesRequest{ + Opaque: opaque, + Filters: []*storageprovider.ListStorageSpacesRequest_Filter{f}, + }) + if err != nil { + // transport error, log as error + logger.Error().Err(err).Msg("could not fetch spaces: transport error") + errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, "could not fetch spaces for deletion, aborting") + return } - // TODO: check if request contains a homespace and if, check if requesting user has the privilege to - // delete it and make sure it is not deleting its own homespace - // needs modification of the cs3api - - // Deleting a space a two step process (1. disabling/trashing, 2. purging) - // Do the "disable/trash" step only if the space is not marked as trashed yet: - if _, ok := sp.Opaque.Map["trashed"]; !ok { + for _, sp := range lspr.GetStorageSpaces() { + if !(sp.SpaceType == "personal" && sp.Owner.Id.OpaqueId == user.GetId()) { + continue + } + // TODO: check if request contains a homespace and if, check if requesting user has the privilege to + // delete it and make sure it is not deleting its own homespace + // needs modification of the cs3api + + // Deleting a space a two step process (1. disabling/trashing, 2. purging) + // Do the "disable/trash" step only if the space is not marked as trashed yet: + if _, ok := sp.Opaque.Map["trashed"]; !ok { + _, err := g.gatewayClient.DeleteStorageSpace(r.Context(), &storageprovider.DeleteStorageSpaceRequest{ + Id: &storageprovider.StorageSpaceId{ + OpaqueId: sp.Id.OpaqueId, + }, + }) + if err != nil { + logger.Error().Err(err).Msg("could not disable homespace: transport error") + errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, "could not disable homespace, aborting") + return + } + } + purgeFlag := utils.AppendPlainToOpaque(nil, "purge", "") _, err := g.gatewayClient.DeleteStorageSpace(r.Context(), &storageprovider.DeleteStorageSpaceRequest{ + Opaque: purgeFlag, Id: &storageprovider.StorageSpaceId{ OpaqueId: sp.Id.OpaqueId, }, }) if err != nil { - logger.Error().Err(err).Msg("could not disable homespace: transport error") - errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, "could not disable homespace, aborting") + // transport error, log as error + logger.Error().Err(err).Msg("could not delete homespace: transport error") + errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, "could not delete homespace, aborting") return } + break } - purgeFlag := utils.AppendPlainToOpaque(nil, "purge", "") - _, err := g.gatewayClient.DeleteStorageSpace(r.Context(), &storageprovider.DeleteStorageSpaceRequest{ - Opaque: purgeFlag, - Id: &storageprovider.StorageSpaceId{ - OpaqueId: sp.Id.OpaqueId, - }, - }) - if err != nil { - // transport error, log as error - logger.Error().Err(err).Msg("could not delete homespace: transport error") - errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, "could not delete homespace, aborting") - return - } - break } logger.Debug().Str("id", user.GetId()).Msg("calling delete user on backend") @@ -432,6 +434,14 @@ func (g Graph) PatchUser(w http.ResponseWriter, r *http.Request) { features = append(features, events.UserFeature{Name: "displayname", Value: *name}) } + if enabled, ok := changes.GetAccountEnabledOk(); ok { + e := events.UserFeature{Name: "enabled", Value: "false"} + if *enabled { + e.Value = "true" + } + features = append(features, e) + } + logger.Debug().Str("nameid", nameOrID).Interface("changes", *changes).Msg("calling update user on backend") u, err := g.identityBackend.UpdateUser(r.Context(), nameOrID, *changes) if err != nil { @@ -453,7 +463,7 @@ func (g Graph) PatchUser(w http.ResponseWriter, r *http.Request) { e.Executant = currentUser.GetId() } g.publishEvent(e) - render.Status(r, http.StatusOK) + render.Status(r, http.StatusNoContent) render.JSON(w, r, u) } diff --git a/services/graph/pkg/service/v0/users_test.go b/services/graph/pkg/service/v0/users_test.go index ddc98a3ab52..5a3d07bef0a 100644 --- a/services/graph/pkg/service/v0/users_test.go +++ b/services/graph/pkg/service/v0/users_test.go @@ -393,7 +393,7 @@ var _ = Describe("Users", func() { r = r.WithContext(revactx.ContextSetUser(ctx, currentUser)) svc.PostUser(rr, r) - Expect(rr.Code).To(Equal(http.StatusOK)) + Expect(rr.Code).To(Equal(http.StatusCreated)) }) }) @@ -516,7 +516,7 @@ var _ = Describe("Users", func() { r = r.WithContext(context.WithValue(revactx.ContextSetUser(ctx, currentUser), chi.RouteCtxKey, rctx)) svc.PatchUser(rr, r) - Expect(rr.Code).To(Equal(http.StatusOK)) + Expect(rr.Code).To(Equal(http.StatusNoContent)) data, err = io.ReadAll(rr.Body) Expect(err).ToNot(HaveOccurred()) From 9fdbed50bd942d4e2c94686dda331169b75f79e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Thu, 8 Dec 2022 09:53:40 +0000 Subject: [PATCH 05/10] fix group creation return code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- services/graph/pkg/service/v0/groups.go | 2 +- services/graph/pkg/service/v0/groups_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/services/graph/pkg/service/v0/groups.go b/services/graph/pkg/service/v0/groups.go index 50c0977c7db..1538f60d4f2 100644 --- a/services/graph/pkg/service/v0/groups.go +++ b/services/graph/pkg/service/v0/groups.go @@ -92,7 +92,7 @@ func (g Graph) PostGroup(w http.ResponseWriter, r *http.Request) { currentUser := revactx.ContextMustGetUser(r.Context()) g.publishEvent(events.GroupCreated{Executant: currentUser.Id, GroupID: *grp.Id}) } - render.Status(r, http.StatusOK) + render.Status(r, http.StatusCreated) render.JSON(w, r, grp) } diff --git a/services/graph/pkg/service/v0/groups_test.go b/services/graph/pkg/service/v0/groups_test.go index c09be377f93..770258ea68c 100644 --- a/services/graph/pkg/service/v0/groups_test.go +++ b/services/graph/pkg/service/v0/groups_test.go @@ -222,7 +222,7 @@ var _ = Describe("Groups", func() { Expect(rr.Code).To(Equal(http.StatusBadRequest)) }) - It("disallows user create ids", func() { + It("disallows group create ids", func() { newGroup = libregraph.NewGroup() newGroup.SetId("disallowed") newGroup.SetDisplayName("New Group") @@ -248,7 +248,7 @@ var _ = Describe("Groups", func() { svc.PostGroup(rr, r) - Expect(rr.Code).To(Equal(http.StatusOK)) + Expect(rr.Code).To(Equal(http.StatusCreated)) }) }) Describe("PatchGroup", func() { From 51cf5b3284134bdd5ca31aa3bc7b79f7908781d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Thu, 8 Dec 2022 09:54:09 +0000 Subject: [PATCH 06/10] remove unknown user property MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- services/graph/pkg/service/v0/users.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/services/graph/pkg/service/v0/users.go b/services/graph/pkg/service/v0/users.go index cb5f108fd4b..e9dd7b70853 100644 --- a/services/graph/pkg/service/v0/users.go +++ b/services/graph/pkg/service/v0/users.go @@ -434,14 +434,6 @@ func (g Graph) PatchUser(w http.ResponseWriter, r *http.Request) { features = append(features, events.UserFeature{Name: "displayname", Value: *name}) } - if enabled, ok := changes.GetAccountEnabledOk(); ok { - e := events.UserFeature{Name: "enabled", Value: "false"} - if *enabled { - e.Value = "true" - } - features = append(features, e) - } - logger.Debug().Str("nameid", nameOrID).Interface("changes", *changes).Msg("calling update user on backend") u, err := g.identityBackend.UpdateUser(r.Context(), nameOrID, *changes) if err != nil { From 4126c7a2473bad9ee09beee177ce6f21dd512dff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Thu, 8 Dec 2022 10:40:57 +0000 Subject: [PATCH 07/10] fix create return code checks in graph feature context MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- tests/acceptance/features/apiGraph/createGroup.feature | 2 +- tests/acceptance/features/apiGraph/createUser.feature | 6 +++--- tests/acceptance/features/bootstrap/GraphContext.php | 10 +++++----- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/acceptance/features/apiGraph/createGroup.feature b/tests/acceptance/features/apiGraph/createGroup.feature index 78f0e6968fc..7571dde2f77 100644 --- a/tests/acceptance/features/apiGraph/createGroup.feature +++ b/tests/acceptance/features/apiGraph/createGroup.feature @@ -9,7 +9,7 @@ Feature: create group Scenario Outline: admin user creates a group When user "Alice" creates a group "" using the Graph API - Then the HTTP status code should be "200" + Then the HTTP status code should be "201" And group "" should exist Examples: | groupname | diff --git a/tests/acceptance/features/apiGraph/createUser.feature b/tests/acceptance/features/apiGraph/createUser.feature index 1378a4be7de..ef6877bd971 100644 --- a/tests/acceptance/features/apiGraph/createUser.feature +++ b/tests/acceptance/features/apiGraph/createUser.feature @@ -20,9 +20,9 @@ Feature: create user And user "" exist Examples: | userName | displayName | email | password | code | shouldOrNot | - | SameDisplayName | Alice Hansen | new@example.org | containsCharacters(*:!;_+-&) | 200 | should | - | withoutPassSameEmail | without pass | alice@example.org | | 200 | should | - | name | pass with space | example@example.org | my pass | 200 | should | + | SameDisplayName | Alice Hansen | new@example.org | containsCharacters(*:!;_+-&) | 201 | should | + | withoutPassSameEmail | without pass | alice@example.org | | 201 | should | + | name | pass with space | example@example.org | my pass | 201 | should | | nameWithCharacters(*:!;_+-&) | user | new@example.org | 123 | 400 | should not | | withoutEmail | without email | | 123 | 400 | should not | | Alice | same userName | new@example.org | 123 | 400 | should | diff --git a/tests/acceptance/features/bootstrap/GraphContext.php b/tests/acceptance/features/bootstrap/GraphContext.php index dc5bca82763..bcdc84a05cc 100644 --- a/tests/acceptance/features/bootstrap/GraphContext.php +++ b/tests/acceptance/features/bootstrap/GraphContext.php @@ -84,7 +84,7 @@ public function userHasBeenEditedUsingTheGraphApi( $displayName ); $this->featureContext->setResponse($response); - $this->featureContext->theHttpStatusCodeShouldBe(200); + $this->featureContext->theHttpStatusCodeShouldBe(204); } /** @@ -540,7 +540,7 @@ public function theAdminHasCreatedUser( $email, $displayName ); - if ($response->getStatusCode() !== 200) { + if ($response->getStatusCode() !== 201) { $this->throwHttpException($response, "Could not create user $user"); } else { $this->featureContext->setResponse($response); @@ -600,7 +600,7 @@ public function theUserHasCreatedANewUserUsingGraphapiWithTheFollowingSettings(s $rows = $table->getRowsHash(); $response = $this->featureContext->getResponse(); - if ($response->getStatusCode() !== 200) { + if ($response->getStatusCode() !== 201) { $this->throwHttpException($response, "Could not create user '$rows[userName]'"); } } @@ -751,7 +751,7 @@ public function userCreatesGroupUsingTheGraphApi(string $group, ?string $user = $this->featureContext->setResponse($response); $this->featureContext->pushToLastHttpStatusCodesArray((string) $response->getStatusCode()); - if ($response->getStatusCode() === 200) { + if ($response->getStatusCode() === 201) { $groupId = $this->featureContext->getJsonDecodedResponse($response)["id"]; $this->featureContext->addGroupToCreatedGroupsList($group, true, true, $groupId); } @@ -768,7 +768,7 @@ public function userCreatesGroupUsingTheGraphApi(string $group, ?string $user = */ public function adminHasCreatedGroupUsingTheGraphApi(string $group): array { $result = $this->createGroup($group); - if ($result->getStatusCode() === 200) { + if ($result->getStatusCode() === 201) { return $this->featureContext->getJsonDecodedResponse($result); } else { $this->throwHttpException($result, "Could not create group '$group'."); From 4a516420519f4155868838ca6af9d27cc8064e7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Thu, 8 Dec 2022 15:04:14 +0000 Subject: [PATCH 08/10] updating uses 200 OK when returning a body MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- services/graph/pkg/service/v0/users.go | 3 ++- services/graph/pkg/service/v0/users_test.go | 2 +- tests/acceptance/features/bootstrap/GraphContext.php | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/services/graph/pkg/service/v0/users.go b/services/graph/pkg/service/v0/users.go index e9dd7b70853..d2494997337 100644 --- a/services/graph/pkg/service/v0/users.go +++ b/services/graph/pkg/service/v0/users.go @@ -455,7 +455,8 @@ func (g Graph) PatchUser(w http.ResponseWriter, r *http.Request) { e.Executant = currentUser.GetId() } g.publishEvent(e) - render.Status(r, http.StatusNoContent) + + render.Status(r, http.StatusOK) // TODO StatusNoContent when prefer=minimal is used render.JSON(w, r, u) } diff --git a/services/graph/pkg/service/v0/users_test.go b/services/graph/pkg/service/v0/users_test.go index 5a3d07bef0a..57a5308acad 100644 --- a/services/graph/pkg/service/v0/users_test.go +++ b/services/graph/pkg/service/v0/users_test.go @@ -516,7 +516,7 @@ var _ = Describe("Users", func() { r = r.WithContext(context.WithValue(revactx.ContextSetUser(ctx, currentUser), chi.RouteCtxKey, rctx)) svc.PatchUser(rr, r) - Expect(rr.Code).To(Equal(http.StatusNoContent)) + Expect(rr.Code).To(Equal(http.StatusOK)) data, err = io.ReadAll(rr.Body) Expect(err).ToNot(HaveOccurred()) diff --git a/tests/acceptance/features/bootstrap/GraphContext.php b/tests/acceptance/features/bootstrap/GraphContext.php index bcdc84a05cc..c17a7370f90 100644 --- a/tests/acceptance/features/bootstrap/GraphContext.php +++ b/tests/acceptance/features/bootstrap/GraphContext.php @@ -84,7 +84,7 @@ public function userHasBeenEditedUsingTheGraphApi( $displayName ); $this->featureContext->setResponse($response); - $this->featureContext->theHttpStatusCodeShouldBe(204); + $this->featureContext->theHttpStatusCodeShouldBe(200); // TODO 204 when prefer=minimal header was sent } /** From b563c1fac1db334d58afff7f719ef1ffa06e1a0a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Thu, 8 Dec 2022 22:19:04 +0000 Subject: [PATCH 09/10] revert user statusCreated change for now MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- services/graph/pkg/service/v0/users.go | 2 +- services/graph/pkg/service/v0/users_test.go | 2 +- tests/acceptance/features/apiGraph/createUser.feature | 6 +++--- tests/acceptance/features/bootstrap/GraphContext.php | 4 ++-- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/services/graph/pkg/service/v0/users.go b/services/graph/pkg/service/v0/users.go index d2494997337..b9803a79638 100644 --- a/services/graph/pkg/service/v0/users.go +++ b/services/graph/pkg/service/v0/users.go @@ -179,7 +179,7 @@ func (g Graph) PostUser(w http.ResponseWriter, r *http.Request) { } g.publishEvent(e) - render.Status(r, http.StatusCreated) + render.Status(r, http.StatusOK) render.JSON(w, r, u) } diff --git a/services/graph/pkg/service/v0/users_test.go b/services/graph/pkg/service/v0/users_test.go index 57a5308acad..ddc98a3ab52 100644 --- a/services/graph/pkg/service/v0/users_test.go +++ b/services/graph/pkg/service/v0/users_test.go @@ -393,7 +393,7 @@ var _ = Describe("Users", func() { r = r.WithContext(revactx.ContextSetUser(ctx, currentUser)) svc.PostUser(rr, r) - Expect(rr.Code).To(Equal(http.StatusCreated)) + Expect(rr.Code).To(Equal(http.StatusOK)) }) }) diff --git a/tests/acceptance/features/apiGraph/createUser.feature b/tests/acceptance/features/apiGraph/createUser.feature index ef6877bd971..1378a4be7de 100644 --- a/tests/acceptance/features/apiGraph/createUser.feature +++ b/tests/acceptance/features/apiGraph/createUser.feature @@ -20,9 +20,9 @@ Feature: create user And user "" exist Examples: | userName | displayName | email | password | code | shouldOrNot | - | SameDisplayName | Alice Hansen | new@example.org | containsCharacters(*:!;_+-&) | 201 | should | - | withoutPassSameEmail | without pass | alice@example.org | | 201 | should | - | name | pass with space | example@example.org | my pass | 201 | should | + | SameDisplayName | Alice Hansen | new@example.org | containsCharacters(*:!;_+-&) | 200 | should | + | withoutPassSameEmail | without pass | alice@example.org | | 200 | should | + | name | pass with space | example@example.org | my pass | 200 | should | | nameWithCharacters(*:!;_+-&) | user | new@example.org | 123 | 400 | should not | | withoutEmail | without email | | 123 | 400 | should not | | Alice | same userName | new@example.org | 123 | 400 | should | diff --git a/tests/acceptance/features/bootstrap/GraphContext.php b/tests/acceptance/features/bootstrap/GraphContext.php index c17a7370f90..80234ae7667 100644 --- a/tests/acceptance/features/bootstrap/GraphContext.php +++ b/tests/acceptance/features/bootstrap/GraphContext.php @@ -540,7 +540,7 @@ public function theAdminHasCreatedUser( $email, $displayName ); - if ($response->getStatusCode() !== 201) { + if ($response->getStatusCode() !== 200) { $this->throwHttpException($response, "Could not create user $user"); } else { $this->featureContext->setResponse($response); @@ -600,7 +600,7 @@ public function theUserHasCreatedANewUserUsingGraphapiWithTheFollowingSettings(s $rows = $table->getRowsHash(); $response = $this->featureContext->getResponse(); - if ($response->getStatusCode() !== 201) { + if ($response->getStatusCode() !== 200) { $this->throwHttpException($response, "Could not create user '$rows[userName]'"); } } From d681380f710dcb3789fe1f4d3ee503c62bfbb583 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Mon, 12 Dec 2022 09:35:34 +0000 Subject: [PATCH 10/10] revert return code changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- services/graph/pkg/service/v0/groups.go | 4 ++-- services/graph/pkg/service/v0/groups_test.go | 2 +- services/graph/pkg/service/v0/users.go | 2 +- tests/acceptance/features/apiGraph/createGroup.feature | 2 +- tests/acceptance/features/bootstrap/GraphContext.php | 4 ++-- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/services/graph/pkg/service/v0/groups.go b/services/graph/pkg/service/v0/groups.go index 1538f60d4f2..3dd8abbe860 100644 --- a/services/graph/pkg/service/v0/groups.go +++ b/services/graph/pkg/service/v0/groups.go @@ -92,7 +92,7 @@ func (g Graph) PostGroup(w http.ResponseWriter, r *http.Request) { currentUser := revactx.ContextMustGetUser(r.Context()) g.publishEvent(events.GroupCreated{Executant: currentUser.Id, GroupID: *grp.Id}) } - render.Status(r, http.StatusCreated) + render.Status(r, http.StatusOK) // FIXME 201 should return 201 created render.JSON(w, r, grp) } @@ -167,7 +167,7 @@ func (g Graph) PatchGroup(w http.ResponseWriter, r *http.Request) { } return } - render.Status(r, http.StatusNoContent) + render.Status(r, http.StatusNoContent) // TODO StatusNoContent when prefer=minimal is used, otherwise OK and the resource in the body render.NoContent(w, r) } diff --git a/services/graph/pkg/service/v0/groups_test.go b/services/graph/pkg/service/v0/groups_test.go index 770258ea68c..02cbe9d71fc 100644 --- a/services/graph/pkg/service/v0/groups_test.go +++ b/services/graph/pkg/service/v0/groups_test.go @@ -248,7 +248,7 @@ var _ = Describe("Groups", func() { svc.PostGroup(rr, r) - Expect(rr.Code).To(Equal(http.StatusCreated)) + Expect(rr.Code).To(Equal(http.StatusOK)) }) }) Describe("PatchGroup", func() { diff --git a/services/graph/pkg/service/v0/users.go b/services/graph/pkg/service/v0/users.go index b9803a79638..dc9253ffaaf 100644 --- a/services/graph/pkg/service/v0/users.go +++ b/services/graph/pkg/service/v0/users.go @@ -179,7 +179,7 @@ func (g Graph) PostUser(w http.ResponseWriter, r *http.Request) { } g.publishEvent(e) - render.Status(r, http.StatusOK) + render.Status(r, http.StatusOK) // FIXME 201 should return 201 created render.JSON(w, r, u) } diff --git a/tests/acceptance/features/apiGraph/createGroup.feature b/tests/acceptance/features/apiGraph/createGroup.feature index 7571dde2f77..78f0e6968fc 100644 --- a/tests/acceptance/features/apiGraph/createGroup.feature +++ b/tests/acceptance/features/apiGraph/createGroup.feature @@ -9,7 +9,7 @@ Feature: create group Scenario Outline: admin user creates a group When user "Alice" creates a group "" using the Graph API - Then the HTTP status code should be "201" + Then the HTTP status code should be "200" And group "" should exist Examples: | groupname | diff --git a/tests/acceptance/features/bootstrap/GraphContext.php b/tests/acceptance/features/bootstrap/GraphContext.php index 80234ae7667..c9e4276f87b 100644 --- a/tests/acceptance/features/bootstrap/GraphContext.php +++ b/tests/acceptance/features/bootstrap/GraphContext.php @@ -751,7 +751,7 @@ public function userCreatesGroupUsingTheGraphApi(string $group, ?string $user = $this->featureContext->setResponse($response); $this->featureContext->pushToLastHttpStatusCodesArray((string) $response->getStatusCode()); - if ($response->getStatusCode() === 201) { + if ($response->getStatusCode() === 200) { $groupId = $this->featureContext->getJsonDecodedResponse($response)["id"]; $this->featureContext->addGroupToCreatedGroupsList($group, true, true, $groupId); } @@ -768,7 +768,7 @@ public function userCreatesGroupUsingTheGraphApi(string $group, ?string $user = */ public function adminHasCreatedGroupUsingTheGraphApi(string $group): array { $result = $this->createGroup($group); - if ($result->getStatusCode() === 201) { + if ($result->getStatusCode() === 200) { return $this->featureContext->getJsonDecodedResponse($result); } else { $this->throwHttpException($result, "Could not create group '$group'.");