diff --git a/services/frontend/README.md b/services/frontend/README.md index 65b09be7a9..c493388722 100644 --- a/services/frontend/README.md +++ b/services/frontend/README.md @@ -134,3 +134,11 @@ For public accessible writable shares, a password can be enforced. To change the Note that changing this environment variable only makes sense if\ `OC_SHARING_PUBLIC_SHARE_MUST_HAVE_PASSWORD`\ is set to `false`. + +## Server Managed Spaces + +The `frontend` service can announce that the membership of spaces is managed by the server instead of by users. To enable this, set the following environment variable to `true`: + +`OC_CLAIM_MANAGED_SPACES_ENABLED` + +When enabled, the `spaces` capability contains `server_managed: true`. Clients like the web UI evaluate this capability and block adding or removing users to or from spaces. diff --git a/services/frontend/pkg/config/config.go b/services/frontend/pkg/config/config.go index bceda65970..1bc358f4d2 100644 --- a/services/frontend/pkg/config/config.go +++ b/services/frontend/pkg/config/config.go @@ -61,6 +61,8 @@ type Config struct { ConfigurableNotifications bool `yaml:"configurable_notifications" env:"FRONTEND_CONFIGURABLE_NOTIFICATIONS" desc:"Allow configuring notifications via web client." introductionVersion:"1.0.0"` + ServerManagedSpaces bool `yaml:"server_managed_spaces" env:"OC_CLAIM_MANAGED_SPACES_ENABLED" desc:"Mark spaces as managed by the server. Clients like the web UI then block adding or removing users to or from spaces. See the text description for more details." introductionVersion:"%%NEXT%%"` + Groupware Groupware `yaml:"groupware"` Context context.Context `yaml:"-"` diff --git a/services/frontend/pkg/revaconfig/config.go b/services/frontend/pkg/revaconfig/config.go index 537b14524c..989661adce 100644 --- a/services/frontend/pkg/revaconfig/config.go +++ b/services/frontend/pkg/revaconfig/config.go @@ -296,12 +296,13 @@ func FrontendConfigFromStruct(cfg *config.Config, logger log.Logger) (map[string }, }, "spaces": map[string]any{ - "version": "1.0.0", - "enabled": true, - "projects": true, - "share_jail": true, - "max_quota": cfg.MaxQuota, - "default_quota": parseQuota(cfg.DefaultQuota), + "version": "1.0.0", + "enabled": true, + "projects": true, + "share_jail": true, + "max_quota": cfg.MaxQuota, + "default_quota": parseQuota(cfg.DefaultQuota), + "server_managed": cfg.ServerManagedSpaces, }, "theme": capabilities.Default().Theme, "search": map[string]any{ diff --git a/services/frontend/pkg/revaconfig/config_test.go b/services/frontend/pkg/revaconfig/config_test.go index fbaf731150..df8656e4d8 100644 --- a/services/frontend/pkg/revaconfig/config_test.go +++ b/services/frontend/pkg/revaconfig/config_test.go @@ -24,6 +24,20 @@ func TestContentSearchCapabilityFollowsFullTextSearch(t *testing.T) { } } +func TestServerManagedCapabilityFollowsServerManagedSpaces(t *testing.T) { + for _, enabled := range []bool{true, false} { + cfg := defaults.FullDefaultConfig() + cfg.Commons = &shared.Commons{} + cfg.ServerManagedSpaces = enabled + + rcfg, err := FrontendConfigFromStruct(cfg, log.NopLogger()) + require.NoError(t, err) + + got := dig(t, rcfg, "http", "services", "ocs", "capabilities", "capabilities", "spaces")["server_managed"] + require.Equal(t, enabled, got) + } +} + func dig(t *testing.T, m map[string]any, keys ...string) map[string]any { t.Helper() for _, k := range keys { diff --git a/services/graph/mocks/drive_item_permissions_provider.go b/services/graph/mocks/drive_item_permissions_provider.go index 082ce69b9e..71b9e25cfb 100644 --- a/services/graph/mocks/drive_item_permissions_provider.go +++ b/services/graph/mocks/drive_item_permissions_provider.go @@ -310,6 +310,150 @@ func (_c *DriveItemPermissionsProvider_DeleteSpaceRootPermission_Call) RunAndRet return _c } +// GetPermission provides a mock function for the type DriveItemPermissionsProvider +func (_mock *DriveItemPermissionsProvider) GetPermission(ctx context.Context, itemID *providerv1beta1.ResourceId, permissionID string) (libregraph.Permission, error) { + ret := _mock.Called(ctx, itemID, permissionID) + + if len(ret) == 0 { + panic("no return value specified for GetPermission") + } + + var r0 libregraph.Permission + var r1 error + if returnFunc, ok := ret.Get(0).(func(context.Context, *providerv1beta1.ResourceId, string) (libregraph.Permission, error)); ok { + return returnFunc(ctx, itemID, permissionID) + } + if returnFunc, ok := ret.Get(0).(func(context.Context, *providerv1beta1.ResourceId, string) libregraph.Permission); ok { + r0 = returnFunc(ctx, itemID, permissionID) + } else { + r0 = ret.Get(0).(libregraph.Permission) + } + if returnFunc, ok := ret.Get(1).(func(context.Context, *providerv1beta1.ResourceId, string) error); ok { + r1 = returnFunc(ctx, itemID, permissionID) + } else { + r1 = ret.Error(1) + } + return r0, r1 +} + +// DriveItemPermissionsProvider_GetPermission_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'GetPermission' +type DriveItemPermissionsProvider_GetPermission_Call struct { + *mock.Call +} + +// GetPermission is a helper method to define mock.On call +// - ctx context.Context +// - itemID *providerv1beta1.ResourceId +// - permissionID string +func (_e *DriveItemPermissionsProvider_Expecter) GetPermission(ctx any, itemID any, permissionID any) *DriveItemPermissionsProvider_GetPermission_Call { + return &DriveItemPermissionsProvider_GetPermission_Call{Call: _e.mock.On("GetPermission", ctx, itemID, permissionID)} +} + +func (_c *DriveItemPermissionsProvider_GetPermission_Call) Run(run func(ctx context.Context, itemID *providerv1beta1.ResourceId, permissionID string)) *DriveItemPermissionsProvider_GetPermission_Call { + _c.Call.Run(func(args mock.Arguments) { + var arg0 context.Context + if args[0] != nil { + arg0 = args[0].(context.Context) + } + var arg1 *providerv1beta1.ResourceId + if args[1] != nil { + arg1 = args[1].(*providerv1beta1.ResourceId) + } + var arg2 string + if args[2] != nil { + arg2 = args[2].(string) + } + run( + arg0, + arg1, + arg2, + ) + }) + return _c +} + +func (_c *DriveItemPermissionsProvider_GetPermission_Call) Return(permission libregraph.Permission, err error) *DriveItemPermissionsProvider_GetPermission_Call { + _c.Call.Return(permission, err) + return _c +} + +func (_c *DriveItemPermissionsProvider_GetPermission_Call) RunAndReturn(run func(ctx context.Context, itemID *providerv1beta1.ResourceId, permissionID string) (libregraph.Permission, error)) *DriveItemPermissionsProvider_GetPermission_Call { + _c.Call.Return(run) + return _c +} + +// GetSpaceRootPermission provides a mock function for the type DriveItemPermissionsProvider +func (_mock *DriveItemPermissionsProvider) GetSpaceRootPermission(ctx context.Context, driveID *providerv1beta1.ResourceId, permissionID string) (libregraph.Permission, error) { + ret := _mock.Called(ctx, driveID, permissionID) + + if len(ret) == 0 { + panic("no return value specified for GetSpaceRootPermission") + } + + var r0 libregraph.Permission + var r1 error + if returnFunc, ok := ret.Get(0).(func(context.Context, *providerv1beta1.ResourceId, string) (libregraph.Permission, error)); ok { + return returnFunc(ctx, driveID, permissionID) + } + if returnFunc, ok := ret.Get(0).(func(context.Context, *providerv1beta1.ResourceId, string) libregraph.Permission); ok { + r0 = returnFunc(ctx, driveID, permissionID) + } else { + r0 = ret.Get(0).(libregraph.Permission) + } + if returnFunc, ok := ret.Get(1).(func(context.Context, *providerv1beta1.ResourceId, string) error); ok { + r1 = returnFunc(ctx, driveID, permissionID) + } else { + r1 = ret.Error(1) + } + return r0, r1 +} + +// DriveItemPermissionsProvider_GetSpaceRootPermission_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'GetSpaceRootPermission' +type DriveItemPermissionsProvider_GetSpaceRootPermission_Call struct { + *mock.Call +} + +// GetSpaceRootPermission is a helper method to define mock.On call +// - ctx context.Context +// - driveID *providerv1beta1.ResourceId +// - permissionID string +func (_e *DriveItemPermissionsProvider_Expecter) GetSpaceRootPermission(ctx any, driveID any, permissionID any) *DriveItemPermissionsProvider_GetSpaceRootPermission_Call { + return &DriveItemPermissionsProvider_GetSpaceRootPermission_Call{Call: _e.mock.On("GetSpaceRootPermission", ctx, driveID, permissionID)} +} + +func (_c *DriveItemPermissionsProvider_GetSpaceRootPermission_Call) Run(run func(ctx context.Context, driveID *providerv1beta1.ResourceId, permissionID string)) *DriveItemPermissionsProvider_GetSpaceRootPermission_Call { + _c.Call.Run(func(args mock.Arguments) { + var arg0 context.Context + if args[0] != nil { + arg0 = args[0].(context.Context) + } + var arg1 *providerv1beta1.ResourceId + if args[1] != nil { + arg1 = args[1].(*providerv1beta1.ResourceId) + } + var arg2 string + if args[2] != nil { + arg2 = args[2].(string) + } + run( + arg0, + arg1, + arg2, + ) + }) + return _c +} + +func (_c *DriveItemPermissionsProvider_GetSpaceRootPermission_Call) Return(permission libregraph.Permission, err error) *DriveItemPermissionsProvider_GetSpaceRootPermission_Call { + _c.Call.Return(permission, err) + return _c +} + +func (_c *DriveItemPermissionsProvider_GetSpaceRootPermission_Call) RunAndReturn(run func(ctx context.Context, driveID *providerv1beta1.ResourceId, permissionID string) (libregraph.Permission, error)) *DriveItemPermissionsProvider_GetSpaceRootPermission_Call { + _c.Call.Return(run) + return _c +} + // Invite provides a mock function for the type DriveItemPermissionsProvider func (_mock *DriveItemPermissionsProvider) Invite(ctx context.Context, resourceId *providerv1beta1.ResourceId, invite libregraph.DriveItemInvite) (libregraph.Permission, error) { ret := _mock.Called(ctx, resourceId, invite) diff --git a/services/graph/pkg/service/v0/api_driveitem_permissions.go b/services/graph/pkg/service/v0/api_driveitem_permissions.go index 7d43726744..c0a3164a67 100644 --- a/services/graph/pkg/service/v0/api_driveitem_permissions.go +++ b/services/graph/pkg/service/v0/api_driveitem_permissions.go @@ -28,6 +28,11 @@ import ( "github.com/opencloud-eu/reva/v2/pkg/share" "github.com/opencloud-eu/reva/v2/pkg/storagespace" "github.com/opencloud-eu/reva/v2/pkg/utils" + "github.com/rs/zerolog" + "go.opentelemetry.io/otel/codes" + semconv "go.opentelemetry.io/otel/semconv/v1.37.0" + "go.opentelemetry.io/otel/trace" + "go.opentelemetry.io/otel/trace/noop" "github.com/opencloud-eu/opencloud/pkg/conversions" "github.com/opencloud-eu/opencloud/pkg/l10n" @@ -46,6 +51,28 @@ const ( parseDriveIDErrMsg = "could not parse driveID" federatedRolesODataFilter = "@libre.graph.permissions.roles.allowedValues/rolePermissions/any(p:contains(p/condition, '@Subject.UserType==\"Federated\"'))" noLinksODataFilter = "grantedToV2 ne ''" + + // _tracerName is the name of the tracer that records the spans of the permission operations + _tracerName = "graph" + + // log keys of the permission operations + _logKeyItemID = "itemID" + _logKeyDriveID = "driveID" + _logKeyPermissionID = "permissionID" + _logKeyPermissionType = "permissionType" + _logKeyRoleID = "roleID" + _logKeyRecipientType = "recipientType" + _logKeySpaceType = "spaceType" + _logKeyUserID = "userID" + _logKeyErrorCode = "errorCode" + _logKeyLookupError = "lookupError" + _logKeyOCMError = "ocmError" + + // recipient types of an invitation + _recipientTypeUser = "user" + _recipientTypeGroup = "group" + _recipientTypeMail = "mail" + _recipientTypeOCM = "ocm" ) // DriveItemPermissionsProvider contains the methods related to handling permissions on drive items @@ -62,11 +89,14 @@ type DriveItemPermissionsProvider interface { CreateSpaceRootLink(ctx context.Context, driveID *storageprovider.ResourceId, createLink libregraph.DriveItemCreateLink) (libregraph.Permission, error) SetPublicLinkPassword(ctx context.Context, driveItemID *storageprovider.ResourceId, permissionID string, password string) (libregraph.Permission, error) SetPublicLinkPasswordOnSpaceRoot(ctx context.Context, driveID *storageprovider.ResourceId, permissionID string, password string) (libregraph.Permission, error) + GetPermission(ctx context.Context, itemID *storageprovider.ResourceId, permissionID string) (libregraph.Permission, error) + GetSpaceRootPermission(ctx context.Context, driveID *storageprovider.ResourceId, permissionID string) (libregraph.Permission, error) } // DriveItemPermissionsService contains the production business logic for everything that relates to permissions on drive items. type DriveItemPermissionsService struct { BaseGraphService + tp trace.TracerProvider } type permissionType int @@ -88,11 +118,14 @@ type ListPermissionsQueryOptions struct { } // NewDriveItemPermissionsService creates a new DriveItemPermissionsService -func NewDriveItemPermissionsService(logger log.Logger, gatewaySelector pool.Selectable[gateway.GatewayAPIClient], identityCache cache.IdentityCache, config *config.Config) (DriveItemPermissionsService, error) { +func NewDriveItemPermissionsService(logger log.Logger, gatewaySelector pool.Selectable[gateway.GatewayAPIClient], identityCache cache.IdentityCache, config *config.Config, tp trace.TracerProvider) (DriveItemPermissionsService, error) { publicBaseURL, err := url.Parse(config.Spaces.WebDavBase) if err != nil { return DriveItemPermissionsService{}, fmt.Errorf("could not parse graph.spaces.webdav_base: %w", err) } + if tp == nil { + tp = noop.NewTracerProvider() + } return DriveItemPermissionsService{ BaseGraphService: BaseGraphService{ logger: &log.Logger{Logger: logger.With().Str("graph api", "DrivesDriveItemService").Logger()}, @@ -102,25 +135,99 @@ func NewDriveItemPermissionsService(logger log.Logger, gatewaySelector pool.Sele availableRoles: unifiedrole.GetRoles(unifiedrole.RoleFilterIDs(config.UnifiedRoles.AvailableRoles...)), publicBaseURL: publicBaseURL, }, + tp: tp, }, nil } +// logLevelForError returns the log level matching the cause of err: +// expected client errors are logged at debug level, everything else at error level. +// Client errors are the codes errorcode.Error.Render maps to a 4xx status. This is a +// default only: call sites that know the operational context better (e.g. a +// NOT_FOUND that is part of a lookup chain) should choose their level explicitly. +func logLevelForError(err error) zerolog.Level { + if e, ok := errorcode.ToError(err); ok { + switch e.GetCode() { + case errorcode.ItemNotFound, errorcode.AccessDenied, errorcode.InvalidRequest, + errorcode.NameAlreadyExists, errorcode.InvalidRange, errorcode.ItemIsLocked, + errorcode.PreconditionFailed, errorcode.NotAllowed: + return zerolog.DebugLevel + } + } + return zerolog.ErrorLevel +} + +// errorCodeForError returns the graph error code err is rendered with. It classifies the failure +// in logs and spans. +func errorCodeForError(err error) string { + if e, ok := errorcode.ToError(err); ok { + return e.GetCode().String() + } + return errorcode.GeneralException.String() +} + +// errorEvent starts a log event for err at the level matching its cause. It carries the error +// code and the error text, so operators see the reason. +func errorEvent(logger *log.Logger, err error) *zerolog.Event { + return logger.WithLevel(logLevelForError(err)).Str(_logKeyErrorCode, errorCodeForError(err)).Err(err) +} + +// fallbackErrorEvent starts a log event for err, the error returned after a fallback attempt that +// failed with otherErr (which may be nil). The level matches the more severe of both causes, so an +// infrastructure failure is not hidden behind the not found of the other attempt. Like errorEvent, +// it carries the code and the text of err; those of otherErr are logged under otherKey+"Code" and +// otherKey. +func fallbackErrorEvent(logger *log.Logger, err, otherErr error, otherKey string) *zerolog.Event { + if otherErr == nil { + return errorEvent(logger, err) + } + level := max(logLevelForError(err), logLevelForError(otherErr)) + return logger.WithLevel(level). + Str(_logKeyErrorCode, errorCodeForError(err)).Err(err). + Str(otherKey+"Code", errorCodeForError(otherErr)).AnErr(otherKey, otherErr) +} + +// endSpan ends span, recording the error code of a failed operation. Like the log level, the span +// status only marks unexpected failures as error. Spans classify the failure by its code only, +// the error text is in the log entry of the same request. +func endSpan(span trace.Span, err error) { + if err != nil { + span.SetAttributes(semconv.ErrorTypeKey.String(errorCodeForError(err))) + setSpanErrorStatus(span, err) + } + span.End() +} + +// setSpanErrorStatus marks span as error when the cause of err is an unexpected failure, with the error +// code as description. Call sites that return a different error than the cause use it to keep the cause +// visible, endSpan never clears the status again. A nil err leaves the status untouched. +func setSpanErrorStatus(span trace.Span, err error) { + if err != nil && logLevelForError(err) == zerolog.ErrorLevel { + span.SetStatus(codes.Error, errorCodeForError(err)) + } +} + // Invite invites a user to a drive item. -func (s DriveItemPermissionsService) Invite(ctx context.Context, resourceId *storageprovider.ResourceId, invite libregraph.DriveItemInvite) (libregraph.Permission, error) { +func (s DriveItemPermissionsService) Invite(ctx context.Context, resourceId *storageprovider.ResourceId, invite libregraph.DriveItemInvite) (_ libregraph.Permission, err error) { + ctx, span := s.tp.Tracer(_tracerName).Start(ctx, "Invite") + defer func() { endSpan(span, err) }() + logger := s.logger.SubloggerWithRequestID(ctx) + tenantId := revactx.ContextMustGetUser(ctx).GetId().GetTenantId() gatewayClient, err := s.gatewaySelector.Next() if err != nil { + errorEvent(&logger, err).Str(_logKeyItemID, storagespace.FormatResourceID(resourceId)).Msg("could not select next gateway client") return libregraph.Permission{}, err } statResponse, err := gatewayClient.Stat(ctx, &storageprovider.StatRequest{Ref: &storageprovider.Reference{ResourceId: resourceId}}) if err := errorcode.FromStat(statResponse, err); err != nil { - s.logger.Warn().Err(err).Interface("stat.res", statResponse).Msg("stat failed") + errorEvent(&logger, err).Str(_logKeyItemID, storagespace.FormatResourceID(resourceId)).Msg("stat failed") return libregraph.Permission{}, err } var condition string if condition, err = roleConditionForResourceType(statResponse.GetInfo()); err != nil { + logger.Debug().Err(err).Str(_logKeyItemID, storagespace.FormatResourceID(resourceId)).Msg("unsupported resource type") return libregraph.Permission{}, err } @@ -128,18 +235,21 @@ func (s DriveItemPermissionsService) Invite(ctx context.Context, resourceId *sto for _, roleID := range invite.GetRoles() { // only allow roles that are enabled in the config if !slices.Contains(s.config.UnifiedRoles.AvailableRoles, roleID) { + logger.Debug().Err(unifiedrole.ErrUnknownRole).Str(_logKeyItemID, storagespace.FormatResourceID(resourceId)).Str(_logKeyRoleID, roleID).Msg("unknown role") return libregraph.Permission{}, unifiedrole.ErrUnknownRole } role, err := unifiedrole.GetRole(unifiedrole.RoleFilterIDs(roleID)) if err != nil { - s.logger.Debug().Err(err).Interface("role", invite.GetRoles()[0]).Msg("unable to convert requested role") + logger.Debug().Err(err).Str(_logKeyItemID, storagespace.FormatResourceID(resourceId)).Str(_logKeyRoleID, roleID).Msg("unable to convert requested role") return libregraph.Permission{}, err } allowedResourceActions := unifiedrole.GetAllowedResourceActions(role, condition) if len(allowedResourceActions) == 0 && role.GetId() != unifiedrole.UnifiedRoleDeniedID { - return libregraph.Permission{}, errorcode.New(errorcode.InvalidRequest, "role not applicable to this resource") + notApplicableErr := errorcode.New(errorcode.InvalidRequest, "role not applicable to this resource") + logger.Debug().Err(notApplicableErr).Str(_logKeyItemID, storagespace.FormatResourceID(resourceId)).Str(_logKeyRoleID, roleID).Msg("role not applicable to this resource") + return libregraph.Permission{}, notApplicableErr } unifiedRolePermissions = append(unifiedRolePermissions, conversions.ToPointerSlice(role.GetRolePermissions())...) @@ -165,7 +275,9 @@ func (s DriveItemPermissionsService) Invite(ctx context.Context, resourceId *sto var cTime *types.Timestamp if email := driveRecipient.GetEmail(); email != "" { if s.config.Commons == nil || !s.config.Commons.EnableGuestLinks { - return libregraph.Permission{}, errorcode.New(errorcode.NotSupported, "sharing with mail recipients is not enabled") + mailErr := errorcode.New(errorcode.NotSupported, "sharing with mail recipients is not enabled") + logger.Debug().Err(mailErr).Str(_logKeyItemID, storagespace.FormatResourceID(resourceId)).Msg("sharing with mail recipients is not enabled") + return libregraph.Permission{}, mailErr } createShareRequest := createShareRequestToMail(email, statResponse.GetInfo(), cs3ResourcePermissions) @@ -175,7 +287,7 @@ func (s DriveItemPermissionsService) Invite(ctx context.Context, resourceId *sto createShareResponse, err := gatewayClient.CreateShare(ctx, createShareRequest) if err := errorcode.FromCS3Status(createShareResponse.GetStatus(), err); err != nil { - s.logger.Debug().Err(err).Msg("share creation failed") + errorEvent(&logger, err).Str(_logKeyItemID, storagespace.FormatResourceID(resourceId)).Str(_logKeyRecipientType, _recipientTypeMail).Msg("share creation failed") return libregraph.Permission{}, err } shareid = createShareResponse.GetShare().GetId().GetOpaqueId() @@ -185,7 +297,7 @@ func (s DriveItemPermissionsService) Invite(ctx context.Context, resourceId *sto // the share provider stores the guest's mail address in its canonical form guestIdentity, err := cs3UserIdToIdentity(ctx, s.identityCache, createShareResponse.GetShare().GetGrantee().GetUserId()) if err != nil { - s.logger.Debug().Err(err).Msg("failed to convert guest user id to identity") + errorEvent(&logger, err).Str(_logKeyItemID, storagespace.FormatResourceID(resourceId)).Str(_logKeyRecipientType, _recipientTypeMail).Msg("failed to convert guest user id to identity") return libregraph.Permission{}, err } @@ -195,10 +307,11 @@ func (s DriveItemPermissionsService) Invite(ctx context.Context, resourceId *sto } else { switch driveRecipient.GetLibreGraphRecipientType() { - case "group": + case _recipientTypeGroup: group, err := s.identityCache.GetGroup(ctx, objectID) if err != nil { - s.logger.Debug().Err(err).Interface("groupId", objectID).Msg("failed group lookup") + errorEvent(&logger, err).Str(_logKeyItemID, storagespace.FormatResourceID(resourceId)).Str(_logKeyRecipientType, _recipientTypeGroup).Msg("recipient lookup failed") + setSpanErrorStatus(span, err) return libregraph.Permission{}, errorcode.New(errorcode.InvalidRequest, err.Error()) } permission.GrantedToV2 = &libregraph.SharePointIdentitySet{ @@ -213,7 +326,7 @@ func (s DriveItemPermissionsService) Invite(ctx context.Context, resourceId *sto } createShareResponse, err := gatewayClient.CreateShare(ctx, createShareRequest) if err := errorcode.FromCS3Status(createShareResponse.GetStatus(), err); err != nil { - s.logger.Debug().Err(err).Msg("share creation failed") + errorEvent(&logger, err).Str(_logKeyItemID, storagespace.FormatResourceID(resourceId)).Str(_logKeyRecipientType, _recipientTypeGroup).Msg("share creation failed") return libregraph.Permission{}, err } shareid = createShareResponse.GetShare().GetId().GetOpaqueId() @@ -224,11 +337,14 @@ func (s DriveItemPermissionsService) Invite(ctx context.Context, resourceId *sto if errors.Is(err, identity.ErrNotFound) && s.config.IncludeOCMSharees { user, err = s.identityCache.GetAcceptedCS3User(ctx, objectID) if err == nil && IsSpaceRoot(statResponse.GetInfo().GetId()) { - return libregraph.Permission{}, errorcode.New(errorcode.InvalidRequest, "federated user can not become a space member") + federatedErr := errorcode.New(errorcode.InvalidRequest, "federated user can not become a space member") + logger.Debug().Err(federatedErr).Str(_logKeyItemID, storagespace.FormatResourceID(resourceId)).Msg("federated user can not become a space member") + return libregraph.Permission{}, federatedErr } } if err != nil { - s.logger.Debug().Err(err).Interface("userId", objectID).Msg("failed user lookup") + errorEvent(&logger, err).Str(_logKeyItemID, storagespace.FormatResourceID(resourceId)).Str(_logKeyRecipientType, _recipientTypeUser).Msg("recipient lookup failed") + setSpanErrorStatus(span, err) return libregraph.Permission{}, errorcode.New(errorcode.InvalidRequest, err.Error()) } permission.GrantedToV2 = &libregraph.SharePointIdentitySet{ @@ -244,7 +360,7 @@ func (s DriveItemPermissionsService) Invite(ctx context.Context, resourceId *sto Domain: user.GetId().GetIdp(), }) if err = errorcode.FromCS3Status(providerInfoResp.GetStatus(), err); err != nil { - s.logger.Error().Err(err).Msg("getting provider info failed") + errorEvent(&logger, err).Str(_logKeyItemID, storagespace.FormatResourceID(resourceId)).Str(_logKeyRecipientType, _recipientTypeOCM).Msg("getting provider info failed") return libregraph.Permission{}, err } @@ -254,7 +370,7 @@ func (s DriveItemPermissionsService) Invite(ctx context.Context, resourceId *sto } createShareResponse, err := gatewayClient.CreateOCMShare(ctx, createShareRequest) if err = errorcode.FromCS3Status(createShareResponse.GetStatus(), err); err != nil { - s.logger.Error().Err(err).Msg("share creation failed") + errorEvent(&logger, err).Str(_logKeyItemID, storagespace.FormatResourceID(resourceId)).Str(_logKeyRecipientType, _recipientTypeOCM).Msg("share creation failed") return libregraph.Permission{}, err } shareid = createShareResponse.GetShare().GetId().GetOpaqueId() @@ -267,7 +383,7 @@ func (s DriveItemPermissionsService) Invite(ctx context.Context, resourceId *sto } createShareResponse, err := gatewayClient.CreateShare(ctx, createShareRequest) if err = errorcode.FromCS3Status(createShareResponse.GetStatus(), err); err != nil { - s.logger.Error().Err(err).Msg("share creation failed") + errorEvent(&logger, err).Str(_logKeyItemID, storagespace.FormatResourceID(resourceId)).Str(_logKeyRecipientType, _recipientTypeUser).Msg("share creation failed") return libregraph.Permission{}, err } shareid = createShareResponse.GetShare().GetId().GetOpaqueId() @@ -295,7 +411,8 @@ func (s DriveItemPermissionsService) Invite(ctx context.Context, resourceId *sto if user, ok := revactx.ContextGetUser(ctx); ok { userIdentity, err := userIdToIdentity(ctx, s.identityCache, tenantId, user.GetId().GetOpaqueId()) if err != nil { - s.logger.Error().Err(err).Msg("identity lookup failed") + logger.Error().Err(err).Str(_logKeyErrorCode, errorCodeForError(err)).Str(_logKeyItemID, storagespace.FormatResourceID(resourceId)).Msg("identity lookup failed") + setSpanErrorStatus(span, err) return libregraph.Permission{}, errorcode.New(errorcode.InvalidRequest, err.Error()) } permission.SetInvitation(libregraph.SharingInvitation{ @@ -385,19 +502,28 @@ func createShareRequestToMail(mail string, info *storageprovider.ResourceInfo, c } // SpaceRootInvite handles invitation request on project spaces -func (s DriveItemPermissionsService) SpaceRootInvite(ctx context.Context, driveID *storageprovider.ResourceId, invite libregraph.DriveItemInvite) (libregraph.Permission, error) { +func (s DriveItemPermissionsService) SpaceRootInvite(ctx context.Context, driveID *storageprovider.ResourceId, invite libregraph.DriveItemInvite) (_ libregraph.Permission, err error) { + ctx, span := s.tp.Tracer(_tracerName).Start(ctx, "SpaceRootInvite") + defer func() { endSpan(span, err) }() + logger := s.logger.SubloggerWithRequestID(ctx) + gatewayClient, err := s.gatewaySelector.Next() if err != nil { + errorEvent(&logger, err).Str(_logKeyDriveID, storagespace.FormatResourceID(driveID)).Msg("could not select next gateway client") return libregraph.Permission{}, err } space, err := utils.GetSpace(ctx, storagespace.FormatResourceID(driveID), gatewayClient) if err != nil { - return libregraph.Permission{}, errorcode.FromUtilsStatusCodeError(err) + spaceErr := errorcode.FromUtilsStatusCodeError(err) + errorEvent(&logger, spaceErr).Str(_logKeyDriveID, storagespace.FormatResourceID(driveID)).Msg("could not get space") + return libregraph.Permission{}, spaceErr } if space.SpaceType != _spaceTypeProject { - return libregraph.Permission{}, errorcode.New(errorcode.InvalidRequest, "unsupported space type") + spaceTypeErr := errorcode.New(errorcode.InvalidRequest, "unsupported space type") + logger.Debug().Err(spaceTypeErr).Str(_logKeyDriveID, storagespace.FormatResourceID(driveID)).Str(_logKeySpaceType, space.GetSpaceType()).Msg("unsupported space type") + return libregraph.Permission{}, spaceTypeErr } rootResourceID := space.GetRoot() @@ -405,21 +531,27 @@ func (s DriveItemPermissionsService) SpaceRootInvite(ctx context.Context, driveI } // ListPermissions lists the permissions of a driveItem -func (s DriveItemPermissionsService) ListPermissions(ctx context.Context, itemID *storageprovider.ResourceId, queryOptions ListPermissionsQueryOptions) (libregraph.CollectionOfPermissionsWithAllowedValues, error) { +func (s DriveItemPermissionsService) ListPermissions(ctx context.Context, itemID *storageprovider.ResourceId, queryOptions ListPermissionsQueryOptions) (_ libregraph.CollectionOfPermissionsWithAllowedValues, err error) { + ctx, span := s.tp.Tracer(_tracerName).Start(ctx, "ListPermissions") + defer func() { endSpan(span, err) }() + logger := s.logger.SubloggerWithRequestID(ctx) + collectionOfPermissions := libregraph.CollectionOfPermissionsWithAllowedValues{} gatewayClient, err := s.gatewaySelector.Next() if err != nil { + errorEvent(&logger, err).Str(_logKeyItemID, storagespace.FormatResourceID(itemID)).Msg("could not select next gateway client") return collectionOfPermissions, err } statResponse, err := gatewayClient.Stat(ctx, &storageprovider.StatRequest{Ref: &storageprovider.Reference{ResourceId: itemID}}) if err := errorcode.FromStat(statResponse, err); err != nil { - s.logger.Warn().Err(err).Interface("stat.res", statResponse).Msg("stat failed") + errorEvent(&logger, err).Str(_logKeyItemID, storagespace.FormatResourceID(itemID)).Msg("stat failed") return collectionOfPermissions, err } var condition string if condition, err = roleConditionForResourceType(statResponse.GetInfo()); err != nil { + logger.Debug().Err(err).Str(_logKeyItemID, storagespace.FormatResourceID(itemID)).Msg("unsupported resource type") return collectionOfPermissions, err } @@ -470,6 +602,7 @@ func (s DriveItemPermissionsService) ListPermissions(ctx context.Context, itemID share.ResourceIDFilter(itemID), }, driveItems) if err != nil { + errorEvent(&logger, err).Str(_logKeyItemID, storagespace.FormatResourceID(itemID)).Msg("could not list space root shares") return collectionOfPermissions, err } } else { @@ -478,6 +611,7 @@ func (s DriveItemPermissionsService) ListPermissions(ctx context.Context, itemID share.ResourceIDFilter(itemID), }, driveItems) if err != nil { + errorEvent(&logger, err).Str(_logKeyItemID, storagespace.FormatResourceID(itemID)).Msg("could not list user shares") return collectionOfPermissions, err } if s.config.IncludeOCMSharees { @@ -488,6 +622,7 @@ func (s DriveItemPermissionsService) ListPermissions(ctx context.Context, itemID }, }, driveItems) if err != nil { + errorEvent(&logger, err).Str(_logKeyItemID, storagespace.FormatResourceID(itemID)).Msg("could not list ocm shares") return collectionOfPermissions, err } } @@ -499,6 +634,7 @@ func (s DriveItemPermissionsService) ListPermissions(ctx context.Context, itemID publicshare.ResourceIDFilter(itemID), }, driveItems) if err != nil { + errorEvent(&logger, err).Str(_logKeyItemID, storagespace.FormatResourceID(itemID)).Msg("could not list public shares") return collectionOfPermissions, err } } @@ -518,21 +654,30 @@ func (s DriveItemPermissionsService) ListPermissions(ctx context.Context, itemID } // ListSpaceRootPermissions handles ListPermissions request on project spaces -func (s DriveItemPermissionsService) ListSpaceRootPermissions(ctx context.Context, driveID *storageprovider.ResourceId, queryOptions ListPermissionsQueryOptions) (libregraph.CollectionOfPermissionsWithAllowedValues, error) { +func (s DriveItemPermissionsService) ListSpaceRootPermissions(ctx context.Context, driveID *storageprovider.ResourceId, queryOptions ListPermissionsQueryOptions) (_ libregraph.CollectionOfPermissionsWithAllowedValues, err error) { + ctx, span := s.tp.Tracer(_tracerName).Start(ctx, "ListSpaceRootPermissions") + defer func() { endSpan(span, err) }() + logger := s.logger.SubloggerWithRequestID(ctx) + collectionOfPermissions := libregraph.CollectionOfPermissionsWithAllowedValues{} gatewayClient, err := s.gatewaySelector.Next() if err != nil { + errorEvent(&logger, err).Str(_logKeyDriveID, storagespace.FormatResourceID(driveID)).Msg("could not select next gateway client") return collectionOfPermissions, err } space, err := utils.GetSpace(ctx, storagespace.FormatResourceID(driveID), gatewayClient) if err != nil { - return collectionOfPermissions, errorcode.FromUtilsStatusCodeError(err) + spaceErr := errorcode.FromUtilsStatusCodeError(err) + errorEvent(&logger, spaceErr).Str(_logKeyDriveID, storagespace.FormatResourceID(driveID)).Msg("could not get space") + return collectionOfPermissions, spaceErr } isSupportedSpaceType := slices.Contains([]string{_spaceTypeProject, _spaceTypePersonal, _spaceTypeVirtual}, space.GetSpaceType()) if !isSupportedSpaceType { - return collectionOfPermissions, errorcode.New(errorcode.InvalidRequest, "unsupported space type") + spaceTypeErr := errorcode.New(errorcode.InvalidRequest, "unsupported space type") + logger.Debug().Err(spaceTypeErr).Str(_logKeyDriveID, storagespace.FormatResourceID(driveID)).Str(_logKeySpaceType, space.GetSpaceType()).Msg("unsupported space type") + return collectionOfPermissions, spaceTypeErr } rootResourceID := space.GetRoot() @@ -540,7 +685,11 @@ func (s DriveItemPermissionsService) ListSpaceRootPermissions(ctx context.Contex } // DeletePermission deletes a permission from a drive item -func (s DriveItemPermissionsService) DeletePermission(ctx context.Context, itemID *storageprovider.ResourceId, permissionID string) error { +func (s DriveItemPermissionsService) DeletePermission(ctx context.Context, itemID *storageprovider.ResourceId, permissionID string) (err error) { + ctx, span := s.tp.Tracer(_tracerName).Start(ctx, "DeletePermission") + defer func() { endSpan(span, err) }() + logger := s.logger.SubloggerWithRequestID(ctx) + var permissionType permissionType sharedResourceID, err := s.getLinkPermissionResourceID(ctx, permissionID) @@ -563,7 +712,10 @@ func (s DriveItemPermissionsService) DeletePermission(ctx context.Context, itemI } } + // the ocm lookup replaces the error of the share lookups, keep it for logging + var lookupErr error if sharedResourceID == nil && s.config.IncludeOCMSharees { + lookupErr = err sharedResourceID, err = s.getOCMPermissionResourceID(ctx, permissionID) if err == nil { permissionType = OCM @@ -572,47 +724,74 @@ func (s DriveItemPermissionsService) DeletePermission(ctx context.Context, itemI switch { case err != nil: + fallbackErrorEvent(&logger, err, lookupErr, _logKeyLookupError).Str(_logKeyItemID, storagespace.FormatResourceID(itemID)).Str(_logKeyPermissionID, permissionID).Msg("could not resolve permission") + setSpanErrorStatus(span, lookupErr) return err case permissionType == Unknown: + logger.Debug().Str(_logKeyItemID, storagespace.FormatResourceID(itemID)).Str(_logKeyPermissionID, permissionID).Msg("permission not found") return errorcode.New(errorcode.ItemNotFound, "permission not found") case sharedResourceID == nil: + logger.Debug().Str(_logKeyItemID, storagespace.FormatResourceID(itemID)).Str(_logKeyPermissionID, permissionID).Msg("could not resolve the shared resource of the permission") return errorcode.New(errorcode.ItemNotFound, "failed to resolve resource id for shared resource") } // The resourceID of the shared resource need to match the item ID from the Request Path // otherwise this is an invalid Request. if !utils.ResourceIDEqual(sharedResourceID, itemID) { - s.logger.Debug().Msg("resourceID of shared does not match itemID") + logger.Debug().Str(_logKeyItemID, storagespace.FormatResourceID(itemID)).Str(_logKeyPermissionID, permissionID).Msg("resourceID of shared does not match itemID") return errorcode.New(errorcode.InvalidRequest, "permissionID and itemID do not match") } switch permissionType { case User, Space: - return s.removeUserShare(ctx, permissionID) + if err := s.removeUserShare(ctx, permissionID); err != nil { + errorEvent(&logger, err).Str(_logKeyItemID, storagespace.FormatResourceID(itemID)).Str(_logKeyPermissionID, permissionID).Msg("could not remove permission") + return err + } + return nil case Public: - return s.removePublicShare(ctx, permissionID) + if err := s.removePublicShare(ctx, permissionID); err != nil { + errorEvent(&logger, err).Str(_logKeyItemID, storagespace.FormatResourceID(itemID)).Str(_logKeyPermissionID, permissionID).Msg("could not remove permission") + return err + } + return nil case OCM: - return s.removeOCMPermission(ctx, permissionID) + if err := s.removeOCMPermission(ctx, permissionID); err != nil { + errorEvent(&logger, err).Str(_logKeyItemID, storagespace.FormatResourceID(itemID)).Str(_logKeyPermissionID, permissionID).Msg("could not remove permission") + return err + } + return nil default: // This should never be reached - return errorcode.New(errorcode.GeneralException, "failed to delete permission") + unknownTypeErr := errorcode.New(errorcode.GeneralException, "failed to delete permission") + errorEvent(&logger, unknownTypeErr).Str(_logKeyItemID, storagespace.FormatResourceID(itemID)).Str(_logKeyPermissionID, permissionID).Int(_logKeyPermissionType, int(permissionType)).Msg("unknown permission type") + return unknownTypeErr } } // DeleteSpaceRootPermission deletes a permission on the root item of a project space -func (s DriveItemPermissionsService) DeleteSpaceRootPermission(ctx context.Context, driveID *storageprovider.ResourceId, permissionID string) error { +func (s DriveItemPermissionsService) DeleteSpaceRootPermission(ctx context.Context, driveID *storageprovider.ResourceId, permissionID string) (err error) { + ctx, span := s.tp.Tracer(_tracerName).Start(ctx, "DeleteSpaceRootPermission") + defer func() { endSpan(span, err) }() + logger := s.logger.SubloggerWithRequestID(ctx) + gatewayClient, err := s.gatewaySelector.Next() if err != nil { + errorEvent(&logger, err).Str(_logKeyDriveID, storagespace.FormatResourceID(driveID)).Str(_logKeyPermissionID, permissionID).Msg("could not select next gateway client") return err } space, err := utils.GetSpace(ctx, storagespace.FormatResourceID(driveID), gatewayClient) if err != nil { - return errorcode.FromUtilsStatusCodeError(err) + spaceErr := errorcode.FromUtilsStatusCodeError(err) + errorEvent(&logger, spaceErr).Str(_logKeyDriveID, storagespace.FormatResourceID(driveID)).Str(_logKeyPermissionID, permissionID).Msg("could not get space") + return spaceErr } if space.SpaceType != _spaceTypeProject { - return errorcode.New(errorcode.InvalidRequest, "unsupported space type") + spaceTypeErr := errorcode.New(errorcode.InvalidRequest, "unsupported space type") + logger.Debug().Err(spaceTypeErr).Str(_logKeyDriveID, storagespace.FormatResourceID(driveID)).Str(_logKeyPermissionID, permissionID).Str(_logKeySpaceType, space.GetSpaceType()).Msg("unsupported space type") + return spaceTypeErr } rootResourceID := space.GetRoot() @@ -620,23 +799,32 @@ func (s DriveItemPermissionsService) DeleteSpaceRootPermission(ctx context.Conte } // UpdatePermission updates a permission on a drive item -func (s DriveItemPermissionsService) UpdatePermission(ctx context.Context, itemID *storageprovider.ResourceId, permissionID string, newPermission libregraph.Permission) (libregraph.Permission, error) { +func (s DriveItemPermissionsService) UpdatePermission(ctx context.Context, itemID *storageprovider.ResourceId, permissionID string, newPermission libregraph.Permission) (_ libregraph.Permission, err error) { + ctx, span := s.tp.Tracer(_tracerName).Start(ctx, "UpdatePermission") + defer func() { endSpan(span, err) }() + logger := s.logger.SubloggerWithRequestID(ctx) + oldPermission, sharedResourceID, err := s.getPermissionByID(ctx, permissionID, itemID) - // try to get the permission from ocm if the permission was not found first place + // try to get the permission from ocm if the permission was not found first place, + // the ocm lookup replaces the error of the share lookup, keep it for logging + var lookupErr error if err != nil && s.config.IncludeOCMSharees { + lookupErr = err oldPermission, sharedResourceID, err = s.getOCMPermissionByID(ctx, permissionID, itemID) } // if we still can't find the permission, return an error if err != nil { + fallbackErrorEvent(&logger, err, lookupErr, _logKeyLookupError).Str(_logKeyItemID, storagespace.FormatResourceID(itemID)).Str(_logKeyPermissionID, permissionID).Msg("could not resolve permission") + setSpanErrorStatus(span, lookupErr) return libregraph.Permission{}, err } // The resourceID of the shared resource need to match the item ID from the Request Path // otherwise this is an invalid Request. if !utils.ResourceIDEqual(sharedResourceID, itemID) { - s.logger.Debug().Msg("resourceID of shared does not match itemID") + logger.Debug().Str(_logKeyItemID, storagespace.FormatResourceID(itemID)).Str(_logKeyPermissionID, permissionID).Msg("resourceID of shared does not match itemID") return libregraph.Permission{}, errorcode.New(errorcode.InvalidRequest, "permissionID and itemID do not match") } @@ -644,6 +832,7 @@ func (s DriveItemPermissionsService) UpdatePermission(ctx context.Context, itemI if _, ok := oldPermission.GetLinkOk(); ok { updatedPermission, err := s.updatePublicLinkPermission(ctx, permissionID, itemID, &newPermission) if err != nil { + errorEvent(&logger, err).Str(_logKeyItemID, storagespace.FormatResourceID(itemID)).Str(_logKeyPermissionID, permissionID).Msg("could not update permission") return libregraph.Permission{}, err } return *updatedPermission, nil @@ -656,37 +845,89 @@ func (s DriveItemPermissionsService) UpdatePermission(ctx context.Context, itemI } // This is an ocm share + var ocmErr error if s.config.IncludeOCMSharees { - updatePermission, err := s.updateOCMPermission(ctx, permissionID, itemID, &newPermission) - if err == nil { + var updatePermission *libregraph.Permission + updatePermission, ocmErr = s.updateOCMPermission(ctx, permissionID, itemID, &newPermission) + if ocmErr == nil { return *updatePermission, nil } } + if err != nil { + // the user share error is returned, but the ocm update may have failed for the more severe cause + fallbackErrorEvent(&logger, err, ocmErr, _logKeyOCMError).Str(_logKeyItemID, storagespace.FormatResourceID(itemID)).Str(_logKeyPermissionID, permissionID).Msg("could not update permission") + setSpanErrorStatus(span, ocmErr) + } return libregraph.Permission{}, err } // UpdateSpaceRootPermission updates a permission on the root item of a project space -func (s DriveItemPermissionsService) UpdateSpaceRootPermission(ctx context.Context, driveID *storageprovider.ResourceId, permissionID string, newPermission libregraph.Permission) (libregraph.Permission, error) { +func (s DriveItemPermissionsService) UpdateSpaceRootPermission(ctx context.Context, driveID *storageprovider.ResourceId, permissionID string, newPermission libregraph.Permission) (_ libregraph.Permission, err error) { + ctx, span := s.tp.Tracer(_tracerName).Start(ctx, "UpdateSpaceRootPermission") + defer func() { endSpan(span, err) }() + logger := s.logger.SubloggerWithRequestID(ctx) + gatewayClient, err := s.gatewaySelector.Next() if err != nil { + errorEvent(&logger, err).Str(_logKeyDriveID, storagespace.FormatResourceID(driveID)).Str(_logKeyPermissionID, permissionID).Msg("could not select next gateway client") return libregraph.Permission{}, err } space, err := utils.GetSpace(ctx, storagespace.FormatResourceID(driveID), gatewayClient) if err != nil { - return libregraph.Permission{}, errorcode.FromUtilsStatusCodeError(err) + spaceErr := errorcode.FromUtilsStatusCodeError(err) + errorEvent(&logger, spaceErr).Str(_logKeyDriveID, storagespace.FormatResourceID(driveID)).Str(_logKeyPermissionID, permissionID).Msg("could not get space") + return libregraph.Permission{}, spaceErr } if space.SpaceType != _spaceTypeProject { - return libregraph.Permission{}, errorcode.New(errorcode.InvalidRequest, "unsupported space type") + spaceTypeErr := errorcode.New(errorcode.InvalidRequest, "unsupported space type") + logger.Debug().Err(spaceTypeErr).Str(_logKeyDriveID, storagespace.FormatResourceID(driveID)).Str(_logKeyPermissionID, permissionID).Str(_logKeySpaceType, space.GetSpaceType()).Msg("unsupported space type") + return libregraph.Permission{}, spaceTypeErr } rootResourceID := space.GetRoot() return s.UpdatePermission(ctx, rootResourceID, permissionID, newPermission) } +// GetPermission returns a single permission of a drive item. +// It reuses ListPermissions, so only permissions visible in the listing of the item can be retrieved. +func (s DriveItemPermissionsService) GetPermission(ctx context.Context, itemID *storageprovider.ResourceId, permissionID string) (libregraph.Permission, error) { + collection, err := s.ListPermissions(ctx, itemID, ListPermissionsQueryOptions{}) + if err != nil { + return libregraph.Permission{}, err + } + + for _, permission := range collection.Value { + if permission.GetId() == permissionID { + return permission, nil + } + } + + return libregraph.Permission{}, errorcode.New(errorcode.ItemNotFound, "permission not found") +} + +// GetSpaceRootPermission returns a single permission on the root item of a project space +func (s DriveItemPermissionsService) GetSpaceRootPermission(ctx context.Context, driveID *storageprovider.ResourceId, permissionID string) (libregraph.Permission, error) { + gatewayClient, err := s.gatewaySelector.Next() + if err != nil { + return libregraph.Permission{}, err + } + + space, err := utils.GetSpace(ctx, storagespace.FormatResourceID(driveID), gatewayClient) + if err != nil { + return libregraph.Permission{}, errorcode.FromUtilsStatusCodeError(err) + } + + if space.GetSpaceType() != _spaceTypeProject { + return libregraph.Permission{}, errorcode.New(errorcode.InvalidRequest, "unsupported space type") + } + + return s.GetPermission(ctx, space.GetRoot(), permissionID) +} + // DriveItemPermissionsApi is the api that registers the http endpoints which expose needed operation to the graph api. // the business logic is delegated to the permissions service and further down to the cs3 client. type DriveItemPermissionsApi struct { @@ -1023,3 +1264,53 @@ func (api DriveItemPermissionsApi) UpdateSpaceRootPermission(w http.ResponseWrit render.Status(r, http.StatusOK) render.JSON(w, r, &updatedPermission) } + +// GetPermission handles GetPermission requests +func (api DriveItemPermissionsApi) GetPermission(w http.ResponseWriter, r *http.Request) { + _, itemID, err := GetDriveAndItemIDParam(r, &api.logger) + if err != nil { + api.logger.Debug().Err(err).Msg(invalidIdMsg) + errorcode.RenderError(w, r, err) + return + } + + permissionID, err := url.PathUnescape(chi.URLParam(r, "permissionID")) + if err != nil || permissionID == "" { + api.logger.Debug().Err(err).Msg("could not parse permissionID") + errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "invalid permissionID") + return + } + + permission, err := api.driveItemPermissionsService.GetPermission(r.Context(), itemID, permissionID) + if err != nil { + errorcode.RenderError(w, r, err) + return + } + render.Status(r, http.StatusOK) + render.JSON(w, r, &permission) +} + +// GetSpaceRootPermission handles GetPermission requests on a space root +func (api DriveItemPermissionsApi) GetSpaceRootPermission(w http.ResponseWriter, r *http.Request) { + driveID, err := parseIDParam(r, "driveID") + if err != nil { + api.logger.Debug().Err(err).Msg(parseDriveIDErrMsg) + errorcode.InvalidRequest.Render(w, r, http.StatusUnprocessableEntity, parseDriveIDErrMsg) + return + } + + permissionID, err := url.PathUnescape(chi.URLParam(r, "permissionID")) + if err != nil || permissionID == "" { + api.logger.Debug().Err(err).Msg("could not parse permissionID") + errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "invalid permissionID") + return + } + + permission, err := api.driveItemPermissionsService.GetSpaceRootPermission(r.Context(), &driveID, permissionID) + if err != nil { + errorcode.RenderError(w, r, err) + return + } + render.Status(r, http.StatusOK) + render.JSON(w, r, &permission) +} diff --git a/services/graph/pkg/service/v0/api_driveitem_permissions_links.go b/services/graph/pkg/service/v0/api_driveitem_permissions_links.go index 817cbc9515..aaf664239a 100644 --- a/services/graph/pkg/service/v0/api_driveitem_permissions_links.go +++ b/services/graph/pkg/service/v0/api_driveitem_permissions_links.go @@ -13,11 +13,11 @@ import ( types "github.com/cs3org/go-cs3apis/cs3/types/v1beta1" "github.com/go-chi/chi/v5" "github.com/go-chi/render" + libregraph "github.com/opencloud-eu/libre-graph-api-go" "github.com/opencloud-eu/opencloud/services/graph/pkg/errorcode" "github.com/opencloud-eu/opencloud/services/graph/pkg/linktype" "github.com/opencloud-eu/reva/v2/pkg/storagespace" "github.com/opencloud-eu/reva/v2/pkg/utils" - libregraph "github.com/opencloud-eu/libre-graph-api-go" ) func (s DriveItemPermissionsService) CreateLink(ctx context.Context, driveItemID *storageprovider.ResourceId, createLink libregraph.DriveItemCreateLink) (libregraph.Permission, error) { @@ -230,6 +230,8 @@ func (api DriveItemPermissionsApi) SetLinkPassword(w http.ResponseWriter, r *htt newPermission, err := api.driveItemPermissionsService.SetPublicLinkPassword(ctx, itemID, permissionID, password.GetPassword()) if err != nil { + logger := api.logger.SubloggerWithRequestID(ctx) + errorEvent(&logger, err).Str(_logKeyItemID, storagespace.FormatResourceID(itemID)).Str(_logKeyPermissionID, permissionID).Msg("could not set link password") errorcode.RenderError(w, r, err) return } @@ -264,6 +266,8 @@ func (api DriveItemPermissionsApi) SetSpaceRootLinkPassword(w http.ResponseWrite ctx := r.Context() newPermission, err := api.driveItemPermissionsService.SetPublicLinkPasswordOnSpaceRoot(ctx, &driveID, permissionID, password.GetPassword()) if err != nil { + logger := api.logger.SubloggerWithRequestID(ctx) + errorEvent(&logger, err).Str(_logKeyDriveID, storagespace.FormatResourceID(&driveID)).Str(_logKeyPermissionID, permissionID).Msg("could not set link password") errorcode.RenderError(w, r, err) return } @@ -275,7 +279,8 @@ func (api DriveItemPermissionsApi) SetSpaceRootLinkPassword(w http.ResponseWrite func (s DriveItemPermissionsService) updatePublicLinkPermission(ctx context.Context, permissionID string, itemID *storageprovider.ResourceId, newPermission *libregraph.Permission) (perm *libregraph.Permission, err error) { gatewayClient, err := s.gatewaySelector.Next() if err != nil { - s.logger.Error().Err(err).Msg("could not select next gateway client") + logger := s.logger.SubloggerWithRequestID(ctx) + logger.Error().Err(err).Str(_logKeyPermissionID, permissionID).Msg("could not select next gateway client") return nil, errorcode.New(errorcode.GeneralException, err.Error()) } diff --git a/services/graph/pkg/service/v0/api_driveitem_permissions_links_test.go b/services/graph/pkg/service/v0/api_driveitem_permissions_links_test.go index 47124eef0e..39616fdb23 100644 --- a/services/graph/pkg/service/v0/api_driveitem_permissions_links_test.go +++ b/services/graph/pkg/service/v0/api_driveitem_permissions_links_test.go @@ -54,7 +54,7 @@ var _ = Describe("createLinkTests", func() { cache := cache.NewIdentityCache(cache.IdentityCacheWithGatewaySelector(gatewaySelector)) cfg := defaults.FullDefaultConfig() - svc, err = service.NewDriveItemPermissionsService(logger, gatewaySelector, cache, cfg) + svc, err = service.NewDriveItemPermissionsService(logger, gatewaySelector, cache, cfg, nil) Expect(err).ToNot(HaveOccurred()) driveItemId = &provider.ResourceId{ StorageId: "1", diff --git a/services/graph/pkg/service/v0/api_driveitem_permissions_test.go b/services/graph/pkg/service/v0/api_driveitem_permissions_test.go index cf6ecd44c7..c5bce19f34 100644 --- a/services/graph/pkg/service/v0/api_driveitem_permissions_test.go +++ b/services/graph/pkg/service/v0/api_driveitem_permissions_test.go @@ -8,26 +8,42 @@ import ( "net/http" "net/http/httptest" "slices" + "strings" "time" gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" grouppb "github.com/cs3org/go-cs3apis/cs3/identity/group/v1beta1" userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" + invitepb "github.com/cs3org/go-cs3apis/cs3/ocm/invite/v1beta1" + ocmprovider "github.com/cs3org/go-cs3apis/cs3/ocm/provider/v1beta1" + cs3rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1" collaboration "github.com/cs3org/go-cs3apis/cs3/sharing/collaboration/v1beta1" link "github.com/cs3org/go-cs3apis/cs3/sharing/link/v1beta1" + ocm "github.com/cs3org/go-cs3apis/cs3/sharing/ocm/v1beta1" provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" types "github.com/cs3org/go-cs3apis/cs3/types/v1beta1" "github.com/go-chi/chi/v5" + "github.com/go-chi/chi/v5/middleware" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + gomegatypes "github.com/onsi/gomega/types" libregraph "github.com/opencloud-eu/libre-graph-api-go" "github.com/opencloud-eu/opencloud/services/graph/pkg/config" + "github.com/prometheus/client_golang/prometheus" + "github.com/rs/zerolog" "github.com/stretchr/testify/mock" "github.com/tidwall/gjson" + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/codes" + sdktrace "go.opentelemetry.io/otel/sdk/trace" + "go.opentelemetry.io/otel/sdk/trace/tracetest" + "go.opentelemetry.io/otel/trace" + "google.golang.org/grpc" roleconversions "github.com/opencloud-eu/reva/v2/pkg/conversions" revactx "github.com/opencloud-eu/reva/v2/pkg/ctx" "github.com/opencloud-eu/reva/v2/pkg/rgrpc/status" + "github.com/opencloud-eu/reva/v2/pkg/rgrpc/todo/pool" "github.com/opencloud-eu/reva/v2/pkg/storagespace" "github.com/opencloud-eu/reva/v2/pkg/utils" cs3mocks "github.com/opencloud-eu/reva/v2/tests/cs3mocks/mocks" @@ -39,10 +55,80 @@ import ( "github.com/opencloud-eu/opencloud/services/graph/pkg/errorcode" identitycache "github.com/opencloud-eu/opencloud/services/graph/pkg/identity/cache" "github.com/opencloud-eu/opencloud/services/graph/pkg/linktype" + "github.com/opencloud-eu/opencloud/services/graph/pkg/metrics" svc "github.com/opencloud-eu/opencloud/services/graph/pkg/service/v0" "github.com/opencloud-eu/opencloud/services/graph/pkg/unifiedrole" ) +// logEntries decodes the JSON lines written by a zerolog logger. +func logEntries(buf *bytes.Buffer) []map[string]any { + var entries []map[string]any + dec := json.NewDecoder(buf) + for dec.More() { + entry := map[string]any{} + Expect(dec.Decode(&entry)).To(Succeed()) + entries = append(entries, entry) + } + return entries +} + +// rawLog re-encodes the log entries, so a sentinel can be searched for in every field. +func rawLog(entries []map[string]any) string { + raw, err := json.Marshal(entries) + Expect(err).ToNot(HaveOccurred()) + return string(raw) +} + +// newSpanRecorder returns a span recorder and a TracerProvider recording into it. +func newSpanRecorder() (*tracetest.SpanRecorder, *sdktrace.TracerProvider) { + sr := tracetest.NewSpanRecorder() + tp := sdktrace.NewTracerProvider(sdktrace.WithSpanProcessor(sr)) + DeferCleanup(func() { + Expect(tp.Shutdown(context.Background())).To(Succeed()) + }) + return sr, tp +} + +// endedSpan returns the single ended span with the given name. It checks that the span carries no +// events and none of the sentinels in its status or attributes. +func endedSpan(sr *tracetest.SpanRecorder, name string, sentinels ...string) sdktrace.ReadOnlySpan { + var spans []sdktrace.ReadOnlySpan + for _, s := range sr.Ended() { + if s.Name() == name { + spans = append(spans, s) + } + } + Expect(spans).To(HaveLen(1)) + span := spans[0] + Expect(span.Events()).To(BeEmpty()) + raw, err := json.Marshal([]any{span.Status(), span.Attributes()}) + Expect(err).ToNot(HaveOccurred()) + for _, sentinel := range sentinels { + Expect(string(raw)).ToNot(ContainSubstring(sentinel)) + } + return span +} + +// gatewayHelperEntry matches the error entry a share helper logs when it cannot select a gateway. +func gatewayHelperEntry(message, errorText string) gomegatypes.GomegaMatcher { + return SatisfyAll( + HaveKeyWithValue("level", "error"), + HaveKeyWithValue("message", message), + HaveKeyWithValue("error", errorText), + ) +} + +// entriesAtLevel returns the log entries logged at the given level. +func entriesAtLevel(entries []map[string]any, level string) []map[string]any { + var filtered []map[string]any + for _, entry := range entries { + if entry["level"] == level { + filtered = append(filtered, entry) + } + } + return filtered +} + var _ = Describe("DriveItemPermissionsService", func() { var ( driveItemPermissionsService svc.DriveItemPermissionsService @@ -73,7 +159,7 @@ var _ = Describe("DriveItemPermissionsService", func() { cache = identitycache.NewIdentityCache(identitycache.IdentityCacheWithGatewaySelector(gatewaySelector)) cfg = defaults.FullDefaultConfig() - service, err := svc.NewDriveItemPermissionsService(logger, gatewaySelector, cache, cfg) + service, err := svc.NewDriveItemPermissionsService(logger, gatewaySelector, cache, cfg, nil) Expect(err).ToNot(HaveOccurred()) driveItemPermissionsService = service ctx = revactx.ContextSetUser(context.Background(), currentUser) @@ -99,6 +185,326 @@ var _ = Describe("DriveItemPermissionsService", func() { }) + // newLoggedService returns a service logging into a new buffer. It uses the mocks and the + // config of the current spec, so call it after the spec has adjusted them. + newLoggedService := func() (svc.DriveItemPermissionsService, *bytes.Buffer) { + logBuf := &bytes.Buffer{} + service, err := svc.NewDriveItemPermissionsService(log.Logger{Logger: zerolog.New(logBuf)}, gatewaySelector, cache, cfg, nil) + Expect(err).ToNot(HaveOccurred()) + return service, logBuf + } + + // A failing gateway selection is an unexpected failure of every operation, logged with its error text. + // Where a probing helper selects the gateway, the helper also logs the selector's error. + DescribeTable("reports a gateway selection failure once, as error log correlated with the request and as error span", + func(name string, call func(context.Context, svc.DriveItemPermissionsService) error, ids map[string]string, errorText gomegatypes.GomegaMatcher, helperEntries int) { + gatewayErr := errors.New("no gateway available at 10.0.0.42") + gatewaySelector.EXPECT().Next().Unset() + gatewaySelector.EXPECT().Next().Return(nil, gatewayErr) + var logBuf bytes.Buffer + sr, tp := newSpanRecorder() + service, err := svc.NewDriveItemPermissionsService(log.Logger{Logger: zerolog.New(&logBuf)}, gatewaySelector, cache, cfg, tp) + Expect(err).ToNot(HaveOccurred()) + + Expect(call(context.WithValue(ctx, middleware.RequestIDKey, "request-42"), service)).To(MatchError(gatewayErr)) + + // the error text is added once per entry: decoding into a map would hide a duplicate key + for line := range strings.SplitSeq(strings.TrimSpace(logBuf.String()), "\n") { + Expect(strings.Count(line, `"error":`)).To(BeNumerically("<=", 1), line) + } + entries := logEntries(&logBuf) + // the operation and the helpers log one entry each, all correlated with the request + const helperMessage = "selecting gatewaySelector failed" + var correlated, helpers []map[string]any + for _, entry := range entriesAtLevel(entries, "error") { + if entry["message"] == helperMessage { + helpers = append(helpers, entry) + } else { + correlated = append(correlated, entry) + } + } + Expect(correlated).To(HaveLen(1)) + Expect(helpers).To(HaveLen(helperEntries)) + for _, entry := range helpers { + Expect(entry).To(SatisfyAll( + gatewayHelperEntry(helperMessage, gatewayErr.Error()), + HaveKeyWithValue("request-id", "request-42"), + HaveKeyWithValue("permissionID", "permissionid"), + )) + } + expected := []gomegatypes.GomegaMatcher{ + HaveKeyWithValue("request-id", "request-42"), + HaveKeyWithValue("errorCode", "generalException"), + errorText, + } + for k, v := range ids { + expected = append(expected, HaveKeyWithValue(k, v)) + } + Expect(correlated[0]).To(SatisfyAll(expected...)) + // no serialized requests, responses or gateway clients + for _, payloadKey := range []string{"stat.res", "gateway.res", "Body", "body", "request", "response"} { + Expect(entries).ToNot(ContainElement(HaveKey(payloadKey))) + } + + // the span carries the error code only, the error message is not recorded + Expect(sr.Ended()).To(HaveLen(1)) + span := endedSpan(sr, name, "10.0.0.42") + Expect(span.Status().Code).To(Equal(codes.Error)) + Expect(span.Status().Description).To(Equal("generalException")) + Expect(span.Attributes()).To(ContainElement(attribute.String("error.type", "generalException"))) + }, + Entry("Invite", "Invite", func(ctx context.Context, s svc.DriveItemPermissionsService) error { + _, err := s.Invite(ctx, driveItemId, libregraph.DriveItemInvite{}) + return err + }, map[string]string{"itemID": "1$2!3"}, HaveKeyWithValue("error", "no gateway available at 10.0.0.42"), 0), + Entry("SpaceRootInvite", "SpaceRootInvite", func(ctx context.Context, s svc.DriveItemPermissionsService) error { + _, err := s.SpaceRootInvite(ctx, &provider.ResourceId{StorageId: "1", SpaceId: "2"}, libregraph.DriveItemInvite{}) + return err + }, map[string]string{"driveID": "1$2"}, HaveKeyWithValue("error", "no gateway available at 10.0.0.42"), 0), + Entry("ListPermissions", "ListPermissions", func(ctx context.Context, s svc.DriveItemPermissionsService) error { + _, err := s.ListPermissions(ctx, driveItemId, svc.ListPermissionsQueryOptions{}) + return err + }, map[string]string{"itemID": "1$2!3"}, HaveKeyWithValue("error", "no gateway available at 10.0.0.42"), 0), + Entry("ListSpaceRootPermissions", "ListSpaceRootPermissions", func(ctx context.Context, s svc.DriveItemPermissionsService) error { + _, err := s.ListSpaceRootPermissions(ctx, &provider.ResourceId{StorageId: "1", SpaceId: "2"}, svc.ListPermissionsQueryOptions{}) + return err + }, map[string]string{"driveID": "1$2"}, HaveKeyWithValue("error", "no gateway available at 10.0.0.42"), 0), + Entry("DeletePermission (selected by the probing helpers)", "DeletePermission", func(ctx context.Context, s svc.DriveItemPermissionsService) error { + return s.DeletePermission(ctx, driveItemId, "permissionid") + }, map[string]string{"itemID": "1$2!3", "permissionID": "permissionid"}, HaveKeyWithValue("error", "no gateway available at 10.0.0.42"), 2), + Entry("DeleteSpaceRootPermission", "DeleteSpaceRootPermission", func(ctx context.Context, s svc.DriveItemPermissionsService) error { + return s.DeleteSpaceRootPermission(ctx, &provider.ResourceId{StorageId: "1", SpaceId: "2"}, "permissionid") + }, map[string]string{"driveID": "1$2", "permissionID": "permissionid"}, HaveKeyWithValue("error", "no gateway available at 10.0.0.42"), 0), + Entry("UpdatePermission (selected by the probing helpers)", "UpdatePermission", func(ctx context.Context, s svc.DriveItemPermissionsService) error { + _, err := s.UpdatePermission(ctx, driveItemId, "permissionid", libregraph.Permission{}) + return err + }, map[string]string{"itemID": "1$2!3", "permissionID": "permissionid"}, HaveKeyWithValue("error", "no gateway available at 10.0.0.42"), 1), + Entry("UpdateSpaceRootPermission", "UpdateSpaceRootPermission", func(ctx context.Context, s svc.DriveItemPermissionsService) error { + _, err := s.UpdateSpaceRootPermission(ctx, &provider.ResourceId{StorageId: "1", SpaceId: "2"}, "permissionid", libregraph.Permission{}) + return err + }, map[string]string{"driveID": "1$2", "permissionID": "permissionid"}, HaveKeyWithValue("error", "no gateway available at 10.0.0.42"), 0), + ) + + It("does not mark the span of an expected client error as error, but records its error code", func() { + sr, tp := newSpanRecorder() + service, err := svc.NewDriveItemPermissionsService(log.NewLogger(), gatewaySelector, cache, cfg, tp) + Expect(err).ToNot(HaveOccurred()) + gatewayClient.On("ListStorageSpaces", mock.Anything, mock.Anything).Return( + &provider.ListStorageSpacesResponse{Status: status.NewNotFound(ctx, "space Project Alpha not found")}, nil) + + _, err = service.ListSpaceRootPermissions(ctx, &provider.ResourceId{StorageId: "1", SpaceId: "2"}, svc.ListPermissionsQueryOptions{}) + Expect(errorcode.IsErrorCode(err, errorcode.ItemNotFound)).To(BeTrue()) + + Expect(sr.Ended()).To(HaveLen(1)) + span := sr.Ended()[0] + Expect(span.Status().Code).To(Equal(codes.Unset)) + Expect(span.Status().Description).To(BeEmpty()) + Expect(span.Attributes()).To(ContainElement(attribute.String("error.type", "itemNotFound"))) + Expect(span.Events()).To(BeEmpty()) + }) + + // The stat failure tables use their own invite, the Invite specs share theirs between specs. + inviteAfterStat := func(s svc.DriveItemPermissionsService) error { + _, err := s.Invite(ctx, driveItemId, libregraph.DriveItemInvite{ + Roles: []string{unifiedrole.UnifiedRoleViewerID}, + Recipients: []libregraph.DriveRecipient{{ObjectId: libregraph.PtrString("1"), LibreGraphRecipientType: libregraph.PtrString("user")}}, + }) + return err + } + listAfterStat := func(s svc.DriveItemPermissionsService) error { + _, err := s.ListPermissions(ctx, driveItemId, svc.ListPermissionsQueryOptions{}) + return err + } + DescribeTable("logs stat failures by cause with the item id, without the stat payload", + func(call func(svc.DriveItemPermissionsService) error, newStatus func(context.Context, string) *cs3rpc.Status, code errorcode.ErrorCode, level string) { + service, logBuf := newLoggedService() + // decomposedfs reports a missing node as "/" + statMessage := "stat: 7c3e9a1b-parent/private-financial-report-2026.xlsx" + gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(&provider.StatResponse{ + Status: newStatus(ctx, statMessage), + Info: &provider.ResourceInfo{ + Id: driveItemId, + Path: "/private/project-plan.odt", + PermissionSet: roleconversions.NewViewerRole().CS3ResourcePermissions(), + }, + }, nil) + + Expect(call(service)).To(MatchError(errorcode.New(code, statMessage).WithOrigin(errorcode.ErrorOriginCS3))) + // nothing is shared or listed after the failed stat + gatewayClient.AssertCalled(GinkgoT(), "Stat", mock.Anything, mock.Anything) + for _, c := range gatewayClient.Calls { + Expect(c.Method).To(Equal("Stat")) + } + + entries := logEntries(logBuf) + var itemEntries []map[string]any + for _, entry := range entries { + if entry["itemID"] == storagespace.FormatResourceID(driveItemId) { + itemEntries = append(itemEntries, entry) + } + } + Expect(itemEntries).To(ConsistOf(HaveKeyWithValue("level", level))) + // the stat failure is the only entry at warn or error level + for _, entry := range entries { + if entry["level"] == "warn" || entry["level"] == "error" { + Expect(entry).To(Equal(itemEntries[0])) + } + } + // the failure is logged with its error text, whatever its level + Expect(itemEntries[0]).To(HaveKeyWithValue("error", errorcode.New(code, statMessage).WithOrigin(errorcode.ErrorOriginCS3).Error())) + for _, payloadKey := range []string{"stat.res", "res", "response", "request", "info", "Body", "body"} { + Expect(entries).ToNot(ContainElement(HaveKey(payloadKey))) + } + // the stat payload never reaches the log + raw := rawLog(entries) + for _, sentinel := range []string{"/private/project-plan.odt", "permission_set"} { + Expect(raw).ToNot(ContainSubstring(sentinel)) + } + }, + Entry("Invite: missing item as debug", inviteAfterStat, status.NewNotFound, errorcode.ItemNotFound, "debug"), + Entry("Invite: storage failure as error", inviteAfterStat, status.NewInternal, errorcode.GeneralException, "error"), + Entry("ListPermissions: missing item as debug", listAfterStat, status.NewNotFound, errorcode.ItemNotFound, "debug"), + Entry("ListPermissions: storage failure as error", listAfterStat, status.NewInternal, errorcode.GeneralException, "error"), + ) + + DescribeTable("logs unsupported space types as debug with the resource ids", + func(spaceType string, call func(svc.DriveItemPermissionsService) error, ids map[string]string) { + service, logBuf := newLoggedService() + gatewayClient.On("ListStorageSpaces", mock.Anything, mock.Anything).Return(&provider.ListStorageSpacesResponse{ + Status: status.NewOK(ctx), + StorageSpaces: []*provider.StorageSpace{{ + Id: &provider.StorageSpaceId{OpaqueId: "2"}, + SpaceType: spaceType, + Root: &provider.ResourceId{StorageId: "1", SpaceId: "2", OpaqueId: "2"}, + }}, + }, nil) + + Expect(call(service)).To(MatchError(errorcode.New(errorcode.InvalidRequest, "unsupported space type"))) + + // the request was rejected after resolving the space, before delegating to the item method + for _, c := range gatewayClient.Calls { + Expect(c.Method).To(Equal("ListStorageSpaces")) + } + + expected := []gomegatypes.GomegaMatcher{ + HaveKeyWithValue("level", "debug"), + HaveKeyWithValue("message", "unsupported space type"), + HaveKeyWithValue("spaceType", spaceType), + } + for _, k := range []string{"driveID", "permissionID", "itemID"} { + if v, ok := ids[k]; ok { + expected = append(expected, HaveKeyWithValue(k, v)) + } else { + expected = append(expected, Not(HaveKey(k))) + } + } + entries := logEntries(logBuf) + Expect(entries).To(ContainElement(SatisfyAll(expected...))) + Expect(entries).ToNot(ContainElement(HaveKeyWithValue("level", BeElementOf("error", "warn")))) + }, + Entry("SpaceRootInvite", "mountpoint", func(s svc.DriveItemPermissionsService) error { + _, err := s.SpaceRootInvite(ctx, &provider.ResourceId{StorageId: "1", SpaceId: "2"}, libregraph.DriveItemInvite{}) + return err + }, map[string]string{"driveID": "1$2"}), + Entry("ListSpaceRootPermissions", "mountpoint", func(s svc.DriveItemPermissionsService) error { + _, err := s.ListSpaceRootPermissions(ctx, &provider.ResourceId{StorageId: "1", SpaceId: "2"}, svc.ListPermissionsQueryOptions{}) + return err + }, map[string]string{"driveID": "1$2"}), + Entry("DeleteSpaceRootPermission", "personal", func(s svc.DriveItemPermissionsService) error { + return s.DeleteSpaceRootPermission(ctx, &provider.ResourceId{StorageId: "1", SpaceId: "2"}, "permissionid") + }, map[string]string{"driveID": "1$2", "permissionID": "permissionid"}), + Entry("UpdateSpaceRootPermission", "personal", func(s svc.DriveItemPermissionsService) error { + _, err := s.UpdateSpaceRootPermission(ctx, &provider.ResourceId{StorageId: "1", SpaceId: "2"}, "permissionid", libregraph.Permission{}) + return err + }, map[string]string{"driveID": "1$2", "permissionID": "permissionid"}), + ) + + var ( + spaceRootInvite = func(s svc.DriveItemPermissionsService) error { + _, err := s.SpaceRootInvite(ctx, &provider.ResourceId{StorageId: "1", SpaceId: "2"}, libregraph.DriveItemInvite{}) + return err + } + listSpaceRootPermissions = func(s svc.DriveItemPermissionsService) error { + _, err := s.ListSpaceRootPermissions(ctx, &provider.ResourceId{StorageId: "1", SpaceId: "2"}, svc.ListPermissionsQueryOptions{}) + return err + } + deleteSpaceRootPermission = func(s svc.DriveItemPermissionsService) error { + return s.DeleteSpaceRootPermission(ctx, &provider.ResourceId{StorageId: "1", SpaceId: "2"}, "permissionid") + } + updateSpaceRootPermission = func(s svc.DriveItemPermissionsService) error { + _, err := s.UpdateSpaceRootPermission(ctx, &provider.ResourceId{StorageId: "1", SpaceId: "2"}, "permissionid", libregraph.Permission{}) + return err + } + missingSpaceResponse = &provider.ListStorageSpacesResponse{Status: status.NewOK(context.Background())} + missingSpaceErr = errorcode.New(errorcode.ItemNotFound, "getting space").WithOrigin(errorcode.ErrorOriginCS3) + // storage providers may report space names or paths in their status messages + internalSpaceResponse = &provider.ListStorageSpacesResponse{Status: status.NewInternal(context.Background(), "storage provider unavailable for space Project Alpha")} + internalSpaceErr = errorcode.New(errorcode.GeneralException, "storage provider unavailable for space Project Alpha").WithOrigin(errorcode.ErrorOriginCS3) + transportSpaceErr = errorcode.New(errorcode.GeneralException, "connection refused").WithOrigin(errorcode.ErrorOriginCS3) + ) + DescribeTable("logs space lookup failures by cause with the resource ids", + func(call func(svc.DriveItemPermissionsService) error, ids map[string]string, res *provider.ListStorageSpacesResponse, expectedErr error, level string) { + service, logBuf := newLoggedService() + // a nil response stands for a failed request + var transportErr error + if res == nil { + transportErr = errors.New("connection refused") + } + gatewayClient.On("ListStorageSpaces", mock.Anything, mock.Anything).Return(res, transportErr) + + Expect(call(service)).To(MatchError(expectedErr)) + + // the request failed while resolving the space, before delegating to the item method + for _, c := range gatewayClient.Calls { + Expect(c.Method).To(Equal("ListStorageSpaces")) + } + + graphErr, _ := errorcode.ToError(expectedErr) + // the failure is logged with its error code and error text, whatever its level + expected := []gomegatypes.GomegaMatcher{ + HaveKeyWithValue("level", level), + HaveKeyWithValue("errorCode", graphErr.GetCode().String()), + HaveKeyWithValue("error", expectedErr.Error()), + } + for _, k := range []string{"driveID", "permissionID", "itemID"} { + if v, ok := ids[k]; ok { + expected = append(expected, HaveKeyWithValue(k, v)) + } else { + expected = append(expected, Not(HaveKey(k))) + } + } + entries := logEntries(logBuf) + Expect(entries).To(ContainElement(SatisfyAll(expected...))) + for _, payloadKey := range []string{"space", "spaces", "res", "response", "stat.res"} { + Expect(entries).ToNot(ContainElement(HaveKey(payloadKey))) + } + if level == "debug" { + Expect(entries).ToNot(ContainElement(HaveKeyWithValue("level", BeElementOf("error", "warn")))) + } else { + errorEntries := entriesAtLevel(entries, "error") + Expect(errorEntries).To(HaveLen(1)) + } + }, + Entry("SpaceRootInvite: missing space", spaceRootInvite, + map[string]string{"driveID": "1$2"}, missingSpaceResponse, missingSpaceErr, "debug"), + Entry("SpaceRootInvite: internal gateway error", spaceRootInvite, + map[string]string{"driveID": "1$2"}, internalSpaceResponse, internalSpaceErr, "error"), + Entry("SpaceRootInvite: transport error", spaceRootInvite, + map[string]string{"driveID": "1$2"}, nil, transportSpaceErr, "error"), + Entry("ListSpaceRootPermissions: missing space", listSpaceRootPermissions, + map[string]string{"driveID": "1$2"}, missingSpaceResponse, missingSpaceErr, "debug"), + Entry("ListSpaceRootPermissions: internal gateway error", listSpaceRootPermissions, + map[string]string{"driveID": "1$2"}, internalSpaceResponse, internalSpaceErr, "error"), + Entry("DeleteSpaceRootPermission: missing space", deleteSpaceRootPermission, + map[string]string{"driveID": "1$2", "permissionID": "permissionid"}, missingSpaceResponse, missingSpaceErr, "debug"), + Entry("DeleteSpaceRootPermission: internal gateway error", deleteSpaceRootPermission, + map[string]string{"driveID": "1$2", "permissionID": "permissionid"}, internalSpaceResponse, internalSpaceErr, "error"), + Entry("UpdateSpaceRootPermission: missing space", updateSpaceRootPermission, + map[string]string{"driveID": "1$2", "permissionID": "permissionid"}, missingSpaceResponse, missingSpaceErr, "debug"), + Entry("UpdateSpaceRootPermission: internal gateway error", updateSpaceRootPermission, + map[string]string{"driveID": "1$2", "permissionID": "permissionid"}, internalSpaceResponse, internalSpaceErr, "error"), + ) + Describe("Invite", func() { var ( createShareResponse *collaboration.CreateShareResponse @@ -205,6 +611,42 @@ var _ = Describe("DriveItemPermissionsService", func() { Expect(permission.GrantedToV2.User.GetId()).To(Equal("guest@xn--infocorp-o0e.com")) }) + It("logs a failed grantee lookup of a mail share by cause with the item id", func() { + cfg.Commons = &shared.Commons{EnableGuestLinks: true} + service, logBuf := newLoggedService() + + // the share provider returns a regular user as grantee, whose lookup fails in the identity backend + createShareResponse.Share = &collaboration.Share{ + Id: &collaboration.ShareId{OpaqueId: "guest123"}, + Grantee: &provider.Grantee{ + Type: provider.GranteeType_GRANTEE_TYPE_USER, + Id: &provider.Grantee_UserId{UserId: &userpb.UserId{ + OpaqueId: "grantee-id", Type: userpb.UserType_USER_TYPE_PRIMARY, + }}, + }, + } + gatewayClient.On("CreateShare", mock.Anything, mock.Anything).Return(createShareResponse, nil) + gatewayClient.On("GetUser", mock.Anything, mock.Anything).Return(nil, errors.New("connection to guest@example.com refused")) + invite := libregraph.DriveItemInvite{ + Roles: []string{unifiedrole.UnifiedRoleViewerID}, + Recipients: []libregraph.DriveRecipient{{Email: libregraph.PtrString("guest@example.com")}}, + } + + _, err := service.Invite(ctx, driveItemId, invite) + Expect(errorcode.IsErrorCode(err, errorcode.GeneralException)).To(BeTrue()) + + entries := logEntries(logBuf) + errorEntries := entriesAtLevel(entries, "error") + Expect(errorEntries).To(HaveLen(1)) + Expect(errorEntries[0]).To(SatisfyAll( + HaveKeyWithValue("message", "failed to convert guest user id to identity"), + HaveKeyWithValue("itemID", storagespace.FormatResourceID(driveItemId)), + HaveKeyWithValue("recipientType", "mail"), + HaveKeyWithValue("errorCode", "generalException"), + HaveKeyWithValue("error", err.Error()), + )) + }) + It("rejects guest shares when guest invites are disabled by default", func() { driveItemInvite.Recipients = []libregraph.DriveRecipient{ {Email: libregraph.PtrString("guest@example.com")}, @@ -247,6 +689,425 @@ var _ = Describe("DriveItemPermissionsService", func() { Expect(permission.GetRoles()[0]).To(Equal(unifiedrole.UnifiedRoleEditorID)) }) + DescribeTable("logs invite validation failures as debug with the resource ids", + func(setup func() *provider.ResourceId, expectedErr error, ids map[string]string) { + itemID := setup() + service, logBuf := newLoggedService() + + _, err := service.Invite(ctx, itemID, driveItemInvite) + Expect(err).To(MatchError(expectedErr)) + // validation failed before anything was shared + gatewayClient.AssertNotCalled(GinkgoT(), "CreateShare", mock.Anything, mock.Anything) + gatewayClient.AssertNotCalled(GinkgoT(), "CreateOCMShare", mock.Anything, mock.Anything) + + expected := []gomegatypes.GomegaMatcher{ + HaveKeyWithValue("level", "debug"), + HaveKeyWithValue("itemID", storagespace.FormatResourceID(itemID)), + } + for k, v := range ids { + expected = append(expected, HaveKeyWithValue(k, v)) + } + entries := logEntries(logBuf) + Expect(entries).To(ContainElement(SatisfyAll(expected...))) + Expect(entries).ToNot(ContainElement(HaveKeyWithValue("level", BeElementOf("error", "warn")))) + for _, payloadKey := range []string{"invite", "recipient", "recipients", "email", "mail", "Body", "body"} { + Expect(entries).ToNot(ContainElement(HaveKey(payloadKey))) + } + }, + Entry("unsupported resource type", + func() *provider.ResourceId { + statResponse.Info.Type = provider.ResourceType_RESOURCE_TYPE_SYMLINK + driveItemInvite.Roles = []string{unifiedrole.UnifiedRoleViewerID} + driveItemInvite.Recipients = []libregraph.DriveRecipient{ + {ObjectId: libregraph.PtrString("1"), LibreGraphRecipientType: libregraph.PtrString("user")}, + } + return driveItemId + }, + errorcode.New(errorcode.InvalidRequest, "unsupported resource type"), + map[string]string{}), + Entry("unknown or disabled role", + func() *provider.ResourceId { + driveItemInvite.Roles = []string{"unknown-role-id"} + driveItemInvite.Recipients = []libregraph.DriveRecipient{ + {ObjectId: libregraph.PtrString("1"), LibreGraphRecipientType: libregraph.PtrString("user")}, + } + return driveItemId + }, + unifiedrole.ErrUnknownRole, + map[string]string{"roleID": "unknown-role-id"}), + Entry("enabled role without a definition", + func() *provider.ResourceId { + // the failing role is not the first requested one + cfg.UnifiedRoles.AvailableRoles = append(cfg.UnifiedRoles.AvailableRoles, "undefined-role-id") + driveItemInvite.Roles = []string{unifiedrole.UnifiedRoleViewerID, "undefined-role-id"} + driveItemInvite.Recipients = []libregraph.DriveRecipient{ + {ObjectId: libregraph.PtrString("1"), LibreGraphRecipientType: libregraph.PtrString("user")}, + } + return driveItemId + }, + unifiedrole.ErrUnknownRole, + map[string]string{"roleID": "undefined-role-id"}), + Entry("role not applicable to the resource", + func() *provider.ResourceId { + driveItemInvite.Roles = []string{unifiedrole.UnifiedRoleManagerID} + driveItemInvite.Recipients = []libregraph.DriveRecipient{ + {ObjectId: libregraph.PtrString("1"), LibreGraphRecipientType: libregraph.PtrString("user")}, + } + return driveItemId + }, + errorcode.New(errorcode.InvalidRequest, "role not applicable to this resource"), + map[string]string{"roleID": unifiedrole.UnifiedRoleManagerID}), + Entry("mail sharing disabled", + func() *provider.ResourceId { + driveItemInvite.Roles = []string{unifiedrole.UnifiedRoleViewerID} + driveItemInvite.Recipients = []libregraph.DriveRecipient{ + {Email: libregraph.PtrString("guest@example.com")}, + } + return driveItemId + }, + errorcode.New(errorcode.NotSupported, "sharing with mail recipients is not enabled"), + map[string]string{}), + Entry("federated user invited as a space member", + func() *provider.ResourceId { + cfg.IncludeOCMSharees = true + spaceRootID := &provider.ResourceId{StorageId: "1", SpaceId: "2", OpaqueId: "2"} + statResponse.Info = &provider.ResourceInfo{ + Id: spaceRootID, + Type: provider.ResourceType_RESOURCE_TYPE_CONTAINER, + Space: &provider.StorageSpace{Root: spaceRootID}, + } + gatewayClient.On("GetUser", mock.Anything, mock.Anything).Return(&userpb.GetUserResponse{ + Status: status.NewNotFound(ctx, "user not found"), + }, nil) + gatewayClient.On("GetAcceptedUser", mock.Anything, mock.Anything).Return(&invitepb.GetAcceptedUserResponse{ + Status: status.NewOK(ctx), + RemoteUser: &userpb.User{ + Id: &userpb.UserId{OpaqueId: "remote", Idp: "ocm.example.org", Type: userpb.UserType_USER_TYPE_FEDERATED}, + }, + }, nil) + driveItemInvite.Roles = []string{unifiedrole.UnifiedRoleSpaceViewerID} + driveItemInvite.Recipients = []libregraph.DriveRecipient{ + {ObjectId: libregraph.PtrString("remote@ocm.example.org"), LibreGraphRecipientType: libregraph.PtrString("user")}, + } + return spaceRootID + }, + errorcode.New(errorcode.InvalidRequest, "federated user can not become a space member"), + map[string]string{}), + ) + + DescribeTable("logs invite recipient lookup failures by cause with the item id", + func(recipientType string, setup func(), expectedErr error, level string) { + setup() + service, logBuf := newLoggedService() + driveItemInvite.Roles = []string{unifiedrole.UnifiedRoleViewerID} + driveItemInvite.Recipients = []libregraph.DriveRecipient{ + {ObjectId: libregraph.PtrString("recipient-id-42"), LibreGraphRecipientType: libregraph.PtrString(recipientType)}, + } + + _, err := service.Invite(ctx, driveItemId, driveItemInvite) + Expect(err).To(MatchError(expectedErr)) + // nothing is shared after the failed lookup + gatewayClient.AssertNotCalled(GinkgoT(), "CreateShare", mock.Anything, mock.Anything) + gatewayClient.AssertNotCalled(GinkgoT(), "CreateOCMShare", mock.Anything, mock.Anything) + + entries := logEntries(logBuf) + // the failure is logged with the error text of the lookup, whatever its level + Expect(entries).To(ContainElement(SatisfyAll( + HaveKeyWithValue("level", level), + HaveKeyWithValue("itemID", storagespace.FormatResourceID(driveItemId)), + HaveKeyWithValue("recipientType", recipientType), + HaveKey("error"), + ))) + if level == "debug" { + Expect(entries).ToNot(ContainElement(HaveKeyWithValue("level", BeElementOf("error", "warn")))) + } else { + Expect(entriesAtLevel(entries, "error")).To(HaveLen(1)) + } + for _, payloadKey := range []string{"objectID", "groupId", "userId", "recipient", "recipients", "invite", "Body", "body"} { + Expect(entries).ToNot(ContainElement(HaveKey(payloadKey))) + } + }, + Entry("A: group not found", "group", + func() { + gatewayClient.On("GetGroup", mock.Anything, mock.Anything).Return(&grouppb.GetGroupResponse{ + Status: status.NewNotFound(ctx, "group recipient-id-42 not found"), + }, nil) + }, + errorcode.New(errorcode.InvalidRequest, "itemNotFound: not found"), "debug"), + Entry("B: group lookup backend failure", "group", + func() { + gatewayClient.On("GetGroup", mock.Anything, mock.Anything).Return(&grouppb.GetGroupResponse{ + Status: status.NewInternal(ctx, "lookup of recipient-id-42 failed"), + }, nil) + }, + errorcode.New(errorcode.InvalidRequest, "generalException: lookup of recipient-id-42 failed"), "error"), + Entry("C: user not found", "user", + func() { + gatewayClient.On("GetUser", mock.Anything, mock.Anything).Return(&userpb.GetUserResponse{ + Status: status.NewNotFound(ctx, "user recipient-id-42 not found"), + }, nil) + }, + errorcode.New(errorcode.InvalidRequest, "itemNotFound: not found"), "debug"), + Entry("D: user lookup backend failure", "user", + func() { + gatewayClient.On("GetUser", mock.Anything, mock.Anything).Return(&userpb.GetUserResponse{ + Status: status.NewInternal(ctx, "lookup of recipient-id-42 failed"), + }, nil) + }, + errorcode.New(errorcode.InvalidRequest, "generalException: unexpected status code while getting user: CODE_INTERNAL"), "error"), + ) + + DescribeTable("logs invite share creation failures by cause with the item id", + func(recipientType string, setup func(), expectedErr error, level string) { + setup() + service, logBuf := newLoggedService() + driveItemInvite.Roles = []string{unifiedrole.UnifiedRoleViewerID} + + _, err := service.Invite(ctx, driveItemId, driveItemInvite) + Expect(err).To(MatchError(expectedErr)) + // the share creation was attempted + gatewayClient.AssertCalled(GinkgoT(), "CreateShare", mock.Anything, mock.Anything) + + entries := logEntries(logBuf) + var creationEntries []map[string]any + for _, entry := range entries { + if entry["level"] == level && entry["itemID"] == storagespace.FormatResourceID(driveItemId) && entry["recipientType"] == recipientType { + creationEntries = append(creationEntries, entry) + } + } + Expect(creationEntries).To(HaveLen(1)) + // the failure is logged with its error text, whatever its level + Expect(creationEntries[0]).To(HaveKeyWithValue("error", expectedErr.Error())) + if level == "error" { + Expect(entriesAtLevel(entries, "error")).To(HaveLen(1)) + } else { + Expect(entries).ToNot(ContainElement(HaveKeyWithValue("level", BeElementOf("error", "warn")))) + } + for _, payloadKey := range []string{"objectID", "groupId", "userId", "email", "mail", "recipient", "recipients", "share", "invite", "Body", "body"} { + Expect(entries).ToNot(ContainElement(HaveKey(payloadKey))) + } + }, + Entry("A: mail recipient, internal error", "mail", + func() { + cfg.Commons = &shared.Commons{EnableGuestLinks: true} + driveItemInvite.Recipients = []libregraph.DriveRecipient{{Email: libregraph.PtrString("guest@example.com")}} + gatewayClient.On("CreateShare", mock.Anything, mock.Anything).Return(&collaboration.CreateShareResponse{ + Status: status.NewInternal(ctx, "could not share with guest@example.com"), + }, nil) + }, + errorcode.New(errorcode.GeneralException, "could not share with guest@example.com").WithOrigin(errorcode.ErrorOriginCS3), "error"), + Entry("B: group recipient, internal error", "group", + func() { + driveItemInvite.Recipients = []libregraph.DriveRecipient{ + {ObjectId: libregraph.PtrString("recipient-id-42"), LibreGraphRecipientType: libregraph.PtrString("group")}, + } + gatewayClient.On("GetGroup", mock.Anything, mock.Anything).Return(getGroupResponse, nil) + gatewayClient.On("CreateShare", mock.Anything, mock.Anything).Return(&collaboration.CreateShareResponse{ + Status: status.NewInternal(ctx, "could not share with recipient-id-42"), + }, nil) + }, + errorcode.New(errorcode.GeneralException, "could not share with recipient-id-42").WithOrigin(errorcode.ErrorOriginCS3), "error"), + Entry("C: user recipient, internal error", "user", + func() { + driveItemInvite.Recipients = []libregraph.DriveRecipient{ + {ObjectId: libregraph.PtrString("recipient-id-42"), LibreGraphRecipientType: libregraph.PtrString("user")}, + } + gatewayClient.On("GetUser", mock.Anything, mock.Anything).Return(getUserResponse, nil) + gatewayClient.On("CreateShare", mock.Anything, mock.Anything).Return(&collaboration.CreateShareResponse{ + Status: status.NewInternal(ctx, "could not share with recipient-id-42"), + }, nil) + }, + errorcode.New(errorcode.GeneralException, "could not share with recipient-id-42").WithOrigin(errorcode.ErrorOriginCS3), "error"), + Entry("D: user recipient, share already exists", "user", + func() { + driveItemInvite.Recipients = []libregraph.DriveRecipient{ + {ObjectId: libregraph.PtrString("recipient-id-42"), LibreGraphRecipientType: libregraph.PtrString("user")}, + } + gatewayClient.On("GetUser", mock.Anything, mock.Anything).Return(getUserResponse, nil) + gatewayClient.On("CreateShare", mock.Anything, mock.Anything).Return(&collaboration.CreateShareResponse{ + Status: status.NewAlreadyExists(ctx, nil, "share with recipient-id-42 already exists"), + }, nil) + }, + errorcode.New(errorcode.NameAlreadyExists, "share with recipient-id-42 already exists").WithOrigin(errorcode.ErrorOriginCS3), "debug"), + ) + + DescribeTable("logs invite ocm failures by cause with the item id", + func(setup func(), attempted string, expectedErr error, level string) { + // a federated recipient, found via the accepted user fallback, on a regular item + cfg.IncludeOCMSharees = true + gatewayClient.On("GetUser", mock.Anything, mock.Anything).Return(&userpb.GetUserResponse{ + Status: status.NewNotFound(ctx, "user not found"), + }, nil) + gatewayClient.On("GetAcceptedUser", mock.Anything, mock.Anything).Return(&invitepb.GetAcceptedUserResponse{ + Status: status.NewOK(ctx), + RemoteUser: &userpb.User{ + Id: &userpb.UserId{OpaqueId: "remote", Idp: "ocm.example.org", Type: userpb.UserType_USER_TYPE_FEDERATED}, + }, + }, nil) + setup() + service, logBuf := newLoggedService() + driveItemInvite.Roles = []string{unifiedrole.UnifiedRoleViewerID} + driveItemInvite.Recipients = []libregraph.DriveRecipient{ + {ObjectId: libregraph.PtrString("remote@ocm.example.org"), LibreGraphRecipientType: libregraph.PtrString("user")}, + } + + _, err := service.Invite(ctx, driveItemId, driveItemInvite) + Expect(err).To(MatchError(expectedErr)) + // the failing ocm operation was attempted, nothing else was shared + gatewayClient.AssertCalled(GinkgoT(), attempted, mock.Anything, mock.Anything) + gatewayClient.AssertNotCalled(GinkgoT(), "CreateShare", mock.Anything, mock.Anything) + + entries := logEntries(logBuf) + var ocmEntries []map[string]any + for _, entry := range entries { + if entry["level"] == level && entry["itemID"] == storagespace.FormatResourceID(driveItemId) && entry["recipientType"] == "ocm" { + ocmEntries = append(ocmEntries, entry) + } + } + Expect(ocmEntries).To(HaveLen(1)) + // the failure is logged with its error text, whatever its level + Expect(ocmEntries[0]).To(HaveKeyWithValue("error", expectedErr.Error())) + if level == "error" { + Expect(entriesAtLevel(entries, "error")).To(HaveLen(1)) + } else { + Expect(entries).ToNot(ContainElement(HaveKeyWithValue("level", BeElementOf("error", "warn")))) + } + for _, payloadKey := range []string{"objectID", "userId", "domain", "provider", "share", "recipient", "recipients", "invite", "Body", "body"} { + Expect(entries).ToNot(ContainElement(HaveKey(payloadKey))) + } + }, + Entry("A: provider info lookup, internal error", + func() { + gatewayClient.On("GetInfoByDomain", mock.Anything, mock.Anything).Return(&ocmprovider.GetInfoByDomainResponse{ + Status: status.NewInternal(ctx, "no provider info for ocm.example.org"), + }, nil) + }, + "GetInfoByDomain", + errorcode.New(errorcode.GeneralException, "no provider info for ocm.example.org").WithOrigin(errorcode.ErrorOriginCS3), "error"), + Entry("B: ocm share creation, internal error", + func() { + gatewayClient.On("GetInfoByDomain", mock.Anything, mock.Anything).Return(&ocmprovider.GetInfoByDomainResponse{ + Status: status.NewOK(ctx), + ProviderInfo: &ocmprovider.ProviderInfo{Domain: "ocm.example.org"}, + }, nil) + gatewayClient.On("CreateOCMShare", mock.Anything, mock.Anything).Return(&ocm.CreateOCMShareResponse{ + Status: status.NewInternal(ctx, "could not share with remote@ocm.example.org"), + }, nil) + }, + "CreateOCMShare", + errorcode.New(errorcode.GeneralException, "could not share with remote@ocm.example.org").WithOrigin(errorcode.ErrorOriginCS3), "error"), + Entry("C: ocm share creation, share already exists", + func() { + gatewayClient.On("GetInfoByDomain", mock.Anything, mock.Anything).Return(&ocmprovider.GetInfoByDomainResponse{ + Status: status.NewOK(ctx), + ProviderInfo: &ocmprovider.ProviderInfo{Domain: "ocm.example.org"}, + }, nil) + gatewayClient.On("CreateOCMShare", mock.Anything, mock.Anything).Return(&ocm.CreateOCMShareResponse{ + Status: status.NewAlreadyExists(ctx, nil, "share with remote@ocm.example.org already exists"), + }, nil) + }, + "CreateOCMShare", + errorcode.New(errorcode.NameAlreadyExists, "share with remote@ocm.example.org already exists").WithOrigin(errorcode.ErrorOriginCS3), "debug"), + ) + + DescribeTable("marks the Invite span by the cause of a failed identity lookup", + func(recipientType string, setup func(), wantStatus codes.Code, wantDescription string) { + sr, tp := newSpanRecorder() + service, err := svc.NewDriveItemPermissionsService(log.NewLogger(), gatewaySelector, cache, cfg, tp) + Expect(err).ToNot(HaveOccurred()) + setup() + invite := libregraph.DriveItemInvite{ + Roles: []string{unifiedrole.UnifiedRoleViewerID}, + Recipients: []libregraph.DriveRecipient{ + {ObjectId: libregraph.PtrString("1"), LibreGraphRecipientType: libregraph.PtrString(recipientType)}, + }, + } + + _, err = service.Invite(ctx, driveItemId, invite) + // the identity lookup failure is still returned as invalid request + Expect(errorcode.IsErrorCode(err, errorcode.InvalidRequest)).To(BeTrue()) + + var span sdktrace.ReadOnlySpan + for _, s := range sr.Ended() { + if s.Name() == "Invite" { + span = s + } + } + Expect(span).ToNot(BeNil()) + Expect(span.Status().Code).To(Equal(wantStatus)) + Expect(span.Status().Description).To(Equal(wantDescription)) + Expect(span.Attributes()).To(ContainElement(attribute.String("error.type", "invalidRequest"))) + Expect(span.Events()).To(BeEmpty()) + // backend identity text never reaches the span + raw, err := json.Marshal([]any{span.Status(), span.Attributes()}) + Expect(err).ToNot(HaveOccurred()) + Expect(string(raw)).ToNot(ContainSubstring("alice@example.com")) + }, + Entry("recipient not found", "user", func() { + gatewayClient.On("GetUser", mock.Anything, mock.Anything).Return(&userpb.GetUserResponse{ + Status: status.NewNotFound(ctx, "alice@example.com not found"), + }, nil) + }, codes.Unset, ""), + Entry("recipient lookup backend failure", "user", func() { + gatewayClient.On("GetUser", mock.Anything, mock.Anything).Return(nil, errors.New("connection to alice@example.com refused")) + }, codes.Error, "generalException"), + Entry("group recipient lookup backend failure", "group", func() { + gatewayClient.On("GetGroup", mock.Anything, mock.Anything).Return(&grouppb.GetGroupResponse{ + Status: status.NewInternal(ctx, "lookup of alice@example.com failed"), + }, nil) + }, codes.Error, "generalException"), + Entry("inviter lookup backend failure", "user", func() { + gatewayClient.On("GetUser", mock.Anything, mock.MatchedBy(func(req *userpb.GetUserRequest) bool { + return req.GetUserId().GetOpaqueId() == "1" + })).Return(getUserResponse, nil) + createShareResponse.Share = &collaboration.Share{Id: &collaboration.ShareId{OpaqueId: "123"}} + gatewayClient.On("CreateShare", mock.Anything, mock.Anything).Return(createShareResponse, nil) + gatewayClient.On("GetUser", mock.Anything, mock.MatchedBy(func(req *userpb.GetUserRequest) bool { + return req.GetUserId().GetOpaqueId() == currentUser.GetId().GetOpaqueId() + })).Return(nil, errors.New("connection to alice@example.com refused")) + }, codes.Error, "generalException"), + ) + + It("logs a failed inviter identity lookup as error with the item id and the error text", func() { + service, logBuf := newLoggedService() + + // the recipient is found and the share is created ... + gatewayClient.On("GetUser", mock.Anything, mock.MatchedBy(func(req *userpb.GetUserRequest) bool { + return req.GetUserId().GetOpaqueId() == "1" + })).Return(getUserResponse, nil) + createShareResponse.Share = &collaboration.Share{Id: &collaboration.ShareId{OpaqueId: "123"}} + gatewayClient.On("CreateShare", mock.Anything, mock.Anything).Return(createShareResponse, nil) + // ... but the lookup of the inviting user fails afterwards + gatewayClient.On("GetUser", mock.Anything, mock.MatchedBy(func(req *userpb.GetUserRequest) bool { + return req.GetUserId().GetOpaqueId() == currentUser.GetId().GetOpaqueId() + })).Return(nil, errors.New("connection to inviter@example.com refused")) + driveItemInvite.Roles = []string{unifiedrole.UnifiedRoleViewerID} + driveItemInvite.Recipients = []libregraph.DriveRecipient{ + {ObjectId: libregraph.PtrString("1"), LibreGraphRecipientType: libregraph.PtrString("user")}, + } + + _, err := service.Invite(ctx, driveItemId, driveItemInvite) + Expect(err).To(MatchError(errorcode.New(errorcode.InvalidRequest, "generalException: connection to inviter@example.com refused"))) + // the share was created before the inviter lookup, as in the existing control flow + gatewayClient.AssertCalled(GinkgoT(), "CreateShare", mock.Anything, mock.Anything) + gatewayClient.AssertCalled(GinkgoT(), "GetUser", mock.Anything, mock.MatchedBy(func(req *userpb.GetUserRequest) bool { + return req.GetUserId().GetOpaqueId() == currentUser.GetId().GetOpaqueId() + })) + + entries := logEntries(logBuf) + errorEntries := entriesAtLevel(entries, "error") + Expect(errorEntries).To(HaveLen(1)) + Expect(errorEntries[0]).To(SatisfyAll( + HaveKeyWithValue("itemID", storagespace.FormatResourceID(driveItemId)), + // the cause of the failed lookup, with its error text + HaveKeyWithValue("errorCode", "generalException"), + HaveKeyWithValue("error", "generalException: connection to inviter@example.com refused"), + )) + for _, payloadKey := range []string{"userId", "userID", "objectID", "user", "invite", "share", "Body", "body"} { + Expect(entries).ToNot(ContainElement(HaveKey(payloadKey))) + } + }) + It("fails with when trying to set a space role on a file", func() { driveItemInvite.Recipients = []libregraph.DriveRecipient{ {ObjectId: libregraph.PtrString("1"), LibreGraphRecipientType: libregraph.PtrString("user")}, @@ -313,7 +1174,7 @@ var _ = Describe("DriveItemPermissionsService", func() { // SecureViewer is enabled in ci, we need to remove it in the unit test return s != unifiedrole.UnifiedRoleSecureViewerID }) - service, err := svc.NewDriveItemPermissionsService(log.NewLogger(), gatewaySelector, cache, cfg) + service, err := svc.NewDriveItemPermissionsService(log.NewLogger(), gatewaySelector, cache, cfg, nil) Expect(err).ToNot(HaveOccurred()) driveItemInvite.Roles = []string{unifiedrole.UnifiedRoleViewerID, unifiedrole.UnifiedRoleSecureViewerID} @@ -393,6 +1254,49 @@ var _ = Describe("DriveItemPermissionsService", func() { Expect(permission.GrantedToV2.User.GetDisplayName()).To(Equal(getUserResponse.User.DisplayName)) Expect(permission.GrantedToV2.User.GetId()).To(Equal("1")) }) + It("records a SpaceRootInvite span as the parent of the Invite span", func() { + sr, tp := newSpanRecorder() + service, err := svc.NewDriveItemPermissionsService(log.NewLogger(), gatewaySelector, cache, cfg, tp) + Expect(err).ToNot(HaveOccurred()) + + listSpacesResponse.StorageSpaces[0].SpaceType = "project" + listSpacesResponse.StorageSpaces[0].Root = &provider.ResourceId{ + StorageId: "1", + SpaceId: "2", + OpaqueId: "3", + } + statResponse.Info.Id = listSpacesResponse.StorageSpaces[0].Root + statResponse.Info.Space = &provider.StorageSpace{ + Root: listSpacesResponse.StorageSpaces[0].Root, + } + gatewayClient.On("ListStorageSpaces", mock.Anything, mock.Anything).Return(listSpacesResponse, nil) + gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(statResponse, nil) + gatewayClient.On("GetUser", mock.Anything, mock.Anything).Return(getUserResponse, nil) + gatewayClient.On("CreateShare", mock.Anything, mock.Anything).Return(createShareResponse, nil) + driveItemInvite.Recipients = []libregraph.DriveRecipient{ + {ObjectId: libregraph.PtrString("1"), LibreGraphRecipientType: libregraph.PtrString("user")}, + } + createShareResponse.Share = &collaboration.Share{Id: &collaboration.ShareId{OpaqueId: "123"}} + + permission, err := service.SpaceRootInvite(ctx, driveId, driveItemInvite) + Expect(err).ToNot(HaveOccurred()) + // the space was resolved and the call was delegated to Invite, which created the share + Expect(permission.GetId()).To(Equal("123")) + gatewayClient.AssertCalled(GinkgoT(), "CreateShare", mock.Anything, mock.Anything) + + spans := map[string]sdktrace.ReadOnlySpan{} + for _, s := range sr.Ended() { + spans[s.Name()] = s + } + Expect(spans).To(HaveKey("SpaceRootInvite")) + Expect(spans).To(HaveKey("Invite")) + parent := spans["SpaceRootInvite"].SpanContext() + child := spans["Invite"] + Expect(parent.IsValid()).To(BeTrue()) + Expect(child.SpanContext().IsValid()).To(BeTrue()) + Expect(child.Parent().SpanID()).To(Equal(parent.SpanID())) + Expect(child.SpanContext().TraceID()).To(Equal(parent.TraceID())) + }) It("rejects to add a user to a personal space", func() { gatewayClient.On("ListStorageSpaces", mock.Anything, mock.Anything).Return(listSpacesResponse, nil) driveItemInvite.Recipients = []libregraph.DriveRecipient{ @@ -431,6 +1335,365 @@ var _ = Describe("DriveItemPermissionsService", func() { PermissionSet: roleconversions.NewViewerRole().CS3ResourcePermissions(), } }) + It("logs a share listing failure once as error with the item id", func() { + service, logBuf := newLoggedService() + + gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(statResponse, nil) + gatewayClient.On("ListShares", mock.Anything, mock.Anything).Return(&collaboration.ListSharesResponse{ + Status: status.NewInternal(context.Background(), "share manager unavailable"), + }, nil) + + _, err := service.ListPermissions(ctx, itemID, svc.ListPermissionsQueryOptions{}) + Expect(err).To(MatchError(errorcode.New(errorcode.GeneralException, "share manager unavailable"))) + // listing stops at the failed user share lookup + gatewayClient.AssertNotCalled(GinkgoT(), "ListPublicShares", mock.Anything, mock.Anything) + + entries := logEntries(logBuf) + errorEntries := entriesAtLevel(entries, "error") + Expect(errorEntries).To(HaveLen(1)) + Expect(errorEntries[0]).To(SatisfyAll( + HaveKeyWithValue("message", "could not list user shares"), + HaveKeyWithValue("itemID", storagespace.FormatResourceID(itemID)), + HaveKeyWithValue("error", err.Error()), + HaveKeyWithValue("errorCode", "generalException"), + )) + for _, payloadKey := range []string{"shares", "share", "res", "response", "stat.res"} { + Expect(entries).ToNot(ContainElement(HaveKey(payloadKey))) + } + }) + // spaceRootSharesRequest matches a share listing request for space root (or regular) shares only + spaceRootSharesRequest := func(spaceRoot bool) any { + return mock.MatchedBy(func(req *collaboration.ListSharesRequest) bool { + return slices.ContainsFunc(req.GetFilters(), func(f *collaboration.Filter) bool { + return f.GetType() == collaboration.Filter_TYPE_SPACE_ROOT && f.GetSpaceRoot() == spaceRoot + }) + }) + } + DescribeTable("logs the identity backend error when a share grantee lookup fails", + func(setup func(groupGrantee *provider.Grantee) *provider.ResourceId, listingMethod string) { + groupGrantee := &provider.Grantee{ + Type: provider.GranteeType_GRANTEE_TYPE_GROUP, + Id: &provider.Grantee_GroupId{GroupId: &grouppb.GroupId{OpaqueId: "groupid"}}, + } + listedItemID := setup(groupGrantee) + service, logBuf := newLoggedService() + + const identityBackendMessage = "ldap search failed for cn=private-group-dn-42,ou=groups" + gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(statResponse, nil) + gatewayClient.On("GetGroup", mock.Anything, mock.Anything).Return(&grouppb.GetGroupResponse{ + Status: status.NewInternal(context.Background(), identityBackendMessage), + }, nil) + + _, err := service.ListPermissions(ctx, listedItemID, svc.ListPermissionsQueryOptions{}) + // the returned error is unchanged: the share conversion error wrapped by the listing + Expect(err).To(MatchError(errorcode.New(errorcode.GeneralException, + errorcode.New(errorcode.GeneralException, + errorcode.New(errorcode.GeneralException, identityBackendMessage).Error()).Error()))) + // the shares were listed and the group grantee was looked up, listing stops before the public shares + gatewayClient.AssertCalled(GinkgoT(), listingMethod, mock.Anything, mock.Anything) + gatewayClient.AssertCalled(GinkgoT(), "GetGroup", mock.Anything, mock.MatchedBy(func(req *grouppb.GetGroupRequest) bool { + return req.GetGroupId().GetOpaqueId() == "groupid" + })) + gatewayClient.AssertNotCalled(GinkgoT(), "ListPublicShares", mock.Anything, mock.Anything) + + entries := logEntries(logBuf) + errorEntries := entriesAtLevel(entries, "error") + Expect(errorEntries).To(HaveLen(1)) + Expect(errorEntries[0]).To(SatisfyAll( + HaveKeyWithValue("itemID", storagespace.FormatResourceID(listedItemID)), + HaveKeyWithValue("error", err.Error()), + )) + for _, payloadKey := range []string{"shares", "share", "group", "res", "response", "stat.res"} { + Expect(entries).ToNot(ContainElement(HaveKey(payloadKey))) + } + }, + Entry("user share listing", + func(groupGrantee *provider.Grantee) *provider.ResourceId { + gatewayClient.On("ListShares", mock.Anything, spaceRootSharesRequest(false)).Return(&collaboration.ListSharesResponse{ + Status: status.NewOK(ctx), + Shares: []*collaboration.Share{{ + Id: &collaboration.ShareId{OpaqueId: "groupshareid"}, + ResourceId: itemID, + Grantee: groupGrantee, + Permissions: &collaboration.SharePermissions{Permissions: roleconversions.NewViewerRole().CS3ResourcePermissions()}, + }}, + }, nil) + return itemID + }, + "ListShares", + ), + Entry("space root share listing", + func(groupGrantee *provider.Grantee) *provider.ResourceId { + spaceRootID := &provider.ResourceId{StorageId: "1", SpaceId: "2", OpaqueId: "2"} + statResponse.Info.Id = spaceRootID + statResponse.Info.Type = provider.ResourceType_RESOURCE_TYPE_CONTAINER + statResponse.Info.Space = &provider.StorageSpace{Root: spaceRootID} + gatewayClient.On("ListShares", mock.Anything, spaceRootSharesRequest(true)).Return(&collaboration.ListSharesResponse{ + Status: status.NewOK(ctx), + Shares: []*collaboration.Share{{ + Id: &collaboration.ShareId{OpaqueId: "groupshareid"}, + ResourceId: spaceRootID, + Grantee: groupGrantee, + Permissions: &collaboration.SharePermissions{Permissions: roleconversions.NewSpaceViewerRole().CS3ResourcePermissions()}, + }}, + }, nil) + return spaceRootID + }, + "ListShares", + ), + Entry("ocm share listing", + func(groupGrantee *provider.Grantee) *provider.ResourceId { + cfg.IncludeOCMSharees = true + gatewayClient.On("ListShares", mock.Anything, mock.Anything).Return(listSharesResponse, nil) + gatewayClient.On("ListOCMShares", mock.Anything, mock.Anything).Return(&ocm.ListOCMSharesResponse{ + Status: status.NewOK(ctx), + Shares: []*ocm.Share{{ + Id: &ocm.ShareId{OpaqueId: "ocmgroupshareid"}, + ResourceId: itemID, + Grantee: groupGrantee, + }}, + }, nil) + return itemID + }, + "ListOCMShares", + ), + ) + It("logs an unsupported resource type as debug with the item id", func() { + service, logBuf := newLoggedService() + + // neither a space root, nor a folder, nor a file + statResponse.Info.Type = provider.ResourceType_RESOURCE_TYPE_SYMLINK + gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(statResponse, nil) + + _, err := service.ListPermissions(ctx, itemID, svc.ListPermissionsQueryOptions{}) + Expect(err).To(MatchError(errorcode.New(errorcode.InvalidRequest, "unsupported resource type"))) + // no shares are listed after the validation failed + for _, method := range []string{"ListShares", "ListOCMShares", "ListPublicShares"} { + gatewayClient.AssertNotCalled(GinkgoT(), method, mock.Anything, mock.Anything) + } + + entries := logEntries(logBuf) + var debugEntries []map[string]any + for _, entry := range entries { + if entry["level"] == "debug" && entry["itemID"] == storagespace.FormatResourceID(itemID) { + debugEntries = append(debugEntries, entry) + } + } + Expect(debugEntries).To(HaveLen(1)) + Expect(entries).ToNot(ContainElement(HaveKeyWithValue("level", BeElementOf("error", "warn")))) + }) + It("logs a public share listing failure once as error with the item id", func() { + service, logBuf := newLoggedService() + + gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(statResponse, nil) + gatewayClient.On("ListShares", mock.Anything, mock.Anything).Return(listSharesResponse, nil) + gatewayClient.On("ListPublicShares", mock.Anything, mock.Anything).Return(&link.ListPublicSharesResponse{ + Status: status.NewInternal(context.Background(), "listing failed for /Users/alice/Private/salary-2026.ods"), + }, nil) + + _, err := service.ListPermissions(ctx, itemID, svc.ListPermissionsQueryOptions{}) + Expect(err).To(MatchError(errorcode.New(errorcode.GeneralException, "listing failed for /Users/alice/Private/salary-2026.ods"))) + // user shares were listed before the public share lookup failed + gatewayClient.AssertCalled(GinkgoT(), "ListShares", mock.Anything, mock.Anything) + gatewayClient.AssertCalled(GinkgoT(), "ListPublicShares", mock.Anything, mock.Anything) + + entries := logEntries(logBuf) + errorEntries := entriesAtLevel(entries, "error") + Expect(errorEntries).To(HaveLen(1)) + Expect(errorEntries[0]).To(SatisfyAll( + HaveKeyWithValue("message", "could not list public shares"), + HaveKeyWithValue("itemID", storagespace.FormatResourceID(itemID)), + HaveKeyWithValue("errorCode", "generalException"), + HaveKeyWithValue("error", err.Error()), + )) + for _, payloadKey := range []string{"shares", "share", "Link", "res", "response"} { + Expect(entries).ToNot(ContainElement(HaveKey(payloadKey))) + } + }) + It("logs a gateway selection failure while listing public shares as error in the helper and the operation with the item id", func() { + gatewayErr := errors.New("no gateway available") + // ListPermissions and the user share listing get a client, the public share listing does not + gatewaySelector.EXPECT().Next().Unset() + gatewaySelector.EXPECT().Next().Return(gatewayClient, nil).Times(2) + gatewaySelector.EXPECT().Next().Return(nil, gatewayErr).Times(1) + service, logBuf := newLoggedService() + gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(statResponse, nil) + gatewayClient.On("ListShares", mock.Anything, mock.Anything).Return(listSharesResponse, nil) + + _, err := service.ListPermissions(ctx, itemID, svc.ListPermissionsQueryOptions{}) + Expect(err).To(MatchError(errorcode.New(errorcode.GeneralException, gatewayErr.Error()))) + // user shares were listed, listing stops at the public share lookup + gatewayClient.AssertCalled(GinkgoT(), "ListShares", mock.Anything, mock.Anything) + gatewayClient.AssertNotCalled(GinkgoT(), "ListPublicShares", mock.Anything, mock.Anything) + + // the helper and the operation both log the reason, the operation with the item id and the error code + Expect(entriesAtLevel(logEntries(logBuf), "error")).To(ConsistOf( + gatewayHelperEntry("could not select next gateway client", gatewayErr.Error()), + SatisfyAll( + HaveKeyWithValue("message", "could not list public shares"), + HaveKeyWithValue("itemID", storagespace.FormatResourceID(itemID)), + HaveKeyWithValue("errorCode", "generalException"), + HaveKeyWithValue("error", err.Error()), + ), + )) + }) + It("logs an ocm share listing failure once as error with the item id", func() { + cfg.IncludeOCMSharees = true + service, logBuf := newLoggedService() + + gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(statResponse, nil) + gatewayClient.On("ListShares", mock.Anything, mock.Anything).Return(listSharesResponse, nil) + gatewayClient.On("ListOCMShares", mock.Anything, mock.Anything).Return(&ocm.ListOCMSharesResponse{ + Status: status.NewInternal(context.Background(), "ocm share manager unavailable"), + }, nil) + + _, err := service.ListPermissions(ctx, itemID, svc.ListPermissionsQueryOptions{}) + Expect(err).To(MatchError(errorcode.New(errorcode.GeneralException, "ocm share manager unavailable"))) + // listing stops at the failed ocm share lookup + gatewayClient.AssertCalled(GinkgoT(), "ListOCMShares", mock.Anything, mock.Anything) + gatewayClient.AssertNotCalled(GinkgoT(), "ListPublicShares", mock.Anything, mock.Anything) + + entries := logEntries(logBuf) + errorEntries := entriesAtLevel(entries, "error") + Expect(errorEntries).To(HaveLen(1)) + Expect(errorEntries[0]).To(SatisfyAll( + HaveKeyWithValue("message", "could not list ocm shares"), + HaveKeyWithValue("itemID", storagespace.FormatResourceID(itemID)), + HaveKeyWithValue("error", err.Error()), + )) + for _, payloadKey := range []string{"shares", "share", "res", "response"} { + Expect(entries).ToNot(ContainElement(HaveKey(payloadKey))) + } + }) + It("logs a gateway selection failure while listing ocm shares as error in the helper and the operation with the item id", func() { + gatewayErr := errors.New("no gateway available") + cfg.IncludeOCMSharees = true + // ListPermissions and the user share listing get a client, the ocm share listing does not + gatewaySelector.EXPECT().Next().Unset() + gatewaySelector.EXPECT().Next().Return(gatewayClient, nil).Times(2) + gatewaySelector.EXPECT().Next().Return(nil, gatewayErr).Times(1) + service, logBuf := newLoggedService() + gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(statResponse, nil) + gatewayClient.On("ListShares", mock.Anything, mock.Anything).Return(listSharesResponse, nil) + + _, err := service.ListPermissions(ctx, itemID, svc.ListPermissionsQueryOptions{}) + Expect(err).To(MatchError(errorcode.New(errorcode.GeneralException, gatewayErr.Error()))) + // user shares were listed, listing stops at the ocm share lookup + gatewayClient.AssertCalled(GinkgoT(), "ListShares", mock.Anything, mock.Anything) + gatewayClient.AssertNotCalled(GinkgoT(), "ListOCMShares", mock.Anything, mock.Anything) + gatewayClient.AssertNotCalled(GinkgoT(), "ListPublicShares", mock.Anything, mock.Anything) + + // the helper and the operation both log the reason, the operation with the item id and the error code + Expect(entriesAtLevel(logEntries(logBuf), "error")).To(ConsistOf( + gatewayHelperEntry("could not select next gateway client", gatewayErr.Error()), + SatisfyAll( + HaveKeyWithValue("message", "could not list ocm shares"), + HaveKeyWithValue("itemID", storagespace.FormatResourceID(itemID)), + HaveKeyWithValue("error", err.Error()), + ), + )) + }) + It("logs a gateway selection failure while listing shares as error in the helper and the operation with the item id", func() { + gatewayErr := errors.New("no gateway available") + // ListPermissions gets a client, the share listing helper does not + gatewaySelector.EXPECT().Next().Unset() + gatewaySelector.EXPECT().Next().Return(gatewayClient, nil).Times(1) + gatewaySelector.EXPECT().Next().Return(nil, gatewayErr).Times(1) + service, logBuf := newLoggedService() + gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(statResponse, nil) + + _, err := service.ListPermissions(ctx, itemID, svc.ListPermissionsQueryOptions{}) + Expect(err).To(MatchError(errorcode.New(errorcode.GeneralException, gatewayErr.Error()))) + gatewayClient.AssertNotCalled(GinkgoT(), "ListShares", mock.Anything, mock.Anything) + + // the helper and the operation both log the reason, the operation with the item id and the error code + Expect(entriesAtLevel(logEntries(logBuf), "error")).To(ConsistOf( + gatewayHelperEntry("could not select next gateway client", gatewayErr.Error()), + SatisfyAll( + HaveKeyWithValue("message", "could not list user shares"), + HaveKeyWithValue("itemID", storagespace.FormatResourceID(itemID)), + HaveKeyWithValue("error", err.Error()), + ), + )) + }) + It("logs a gateway selection failure while listing space root shares as error in the helper and the operation with the item id", func() { + gatewayErr := errors.New("no gateway available") + spaceRootID := &provider.ResourceId{StorageId: "1", SpaceId: "2", OpaqueId: "2"} + // ListPermissions gets a client, the space root share listing helper does not + gatewaySelector.EXPECT().Next().Unset() + gatewaySelector.EXPECT().Next().Return(gatewayClient, nil).Times(1) + gatewaySelector.EXPECT().Next().Return(nil, gatewayErr).Times(1) + service, logBuf := newLoggedService() + statResponse.Info.Id = spaceRootID + gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(statResponse, nil) + + _, err := service.ListPermissions(ctx, spaceRootID, svc.ListPermissionsQueryOptions{}) + Expect(err).To(MatchError(errorcode.New(errorcode.GeneralException, gatewayErr.Error()))) + // listing stops at the failed space root share lookup + gatewayClient.AssertNotCalled(GinkgoT(), "ListShares", mock.Anything, mock.Anything) + gatewayClient.AssertNotCalled(GinkgoT(), "ListPublicShares", mock.Anything, mock.Anything) + + // the helper and the operation both log the reason, the operation with the item id and the error code + Expect(entriesAtLevel(logEntries(logBuf), "error")).To(ConsistOf( + gatewayHelperEntry("could not select next gateway client", gatewayErr.Error()), + SatisfyAll( + HaveKeyWithValue("message", "could not list space root shares"), + HaveKeyWithValue("itemID", storagespace.FormatResourceID(spaceRootID)), + HaveKeyWithValue("error", err.Error()), + ), + )) + }) + It("records a ListPermissions span", func() { + sr, tp := newSpanRecorder() + service, err := svc.NewDriveItemPermissionsService(log.NewLogger(), gatewaySelector, cache, cfg, tp) + Expect(err).ToNot(HaveOccurred()) + + gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(statResponse, nil) + gatewayClient.On("ListShares", mock.Anything, mock.Anything).Return(listSharesResponse, nil) + gatewayClient.On("ListPublicShares", mock.Anything, mock.Anything).Return(listPublicSharesResponse, nil) + _, err = service.ListPermissions(context.Background(), itemID, svc.ListPermissionsQueryOptions{}) + Expect(err).ToNot(HaveOccurred()) + + Expect(sr.Ended()).To(HaveLen(1)) + span := sr.Ended()[0] + Expect(span.Name()).To(Equal("ListPermissions")) + // a successful operation is neither marked as failed nor carries an error code + Expect(span.Status().Code).To(Equal(codes.Unset)) + for _, attr := range span.Attributes() { + Expect(attr.Key).ToNot(Equal(attribute.Key("error.type"))) + } + }) + It("does not panic and does not record spans without a TraceProvider", func() { + // a recording span from the caller, e.g. the http server span + callerTP := sdktrace.NewTracerProvider() + DeferCleanup(func() { + Expect(callerTP.Shutdown(context.Background())).To(Succeed()) + }) + callerCtx, callerSpan := callerTP.Tracer("test").Start(context.Background(), "caller") + defer callerSpan.End() + + var statCtx context.Context + gatewayClient.On("Stat", mock.Anything, mock.Anything).Run(func(args mock.Arguments) { + statCtx = args.Get(0).(context.Context) + }).Return(statResponse, nil) + gatewayClient.On("ListShares", mock.Anything, mock.Anything).Return(listSharesResponse, nil) + gatewayClient.On("ListPublicShares", mock.Anything, mock.Anything).Return(listPublicSharesResponse, nil) + + Expect(func() { + service, err := svc.NewDriveItemPermissionsService(log.NewLogger(), gatewaySelector, cache, cfg, nil) + Expect(err).ToNot(HaveOccurred()) + _, err = service.ListPermissions(callerCtx, itemID, svc.ListPermissionsQueryOptions{}) + Expect(err).ToNot(HaveOccurred()) + }).NotTo(Panic()) + + // the span active during the gateway call is a non-recording no-op span + // that still carries the caller's span context + Expect(statCtx).ToNot(BeNil()) + active := trace.SpanFromContext(statCtx) + Expect(active.IsRecording()).To(BeFalse()) + Expect(active.SpanContext().TraceID()).To(Equal(callerSpan.SpanContext().TraceID())) + Expect(active.SpanContext().SpanID()).To(Equal(callerSpan.SpanContext().SpanID())) + }) It("populates allowedValues for files that are not shared", func() { gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(statResponse, nil) gatewayClient.On("ListShares", mock.Anything, mock.Anything).Return(listSharesResponse, nil) @@ -537,7 +1800,7 @@ var _ = Describe("DriveItemPermissionsService", func() { cfg = defaults.FullDefaultConfig() cfg.UnifiedRoles.AvailableRoles = []string{unifiedrole.UnifiedRoleViewerID, unifiedrole.UnifiedRoleDeniedID, unifiedrole.UnifiedRoleManagerID} - service, err := svc.NewDriveItemPermissionsService(log.NewLogger(), gatewaySelector, cache, cfg) + service, err := svc.NewDriveItemPermissionsService(log.NewLogger(), gatewaySelector, cache, cfg, nil) gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(statResponse, nil) gatewayClient.On("ListShares", mock.Anything, mock.Anything).Return(listSharesResponse, nil) @@ -745,30 +2008,128 @@ var _ = Describe("DriveItemPermissionsService", func() { Expect(len(permissions.LibreGraphPermissionsActionsAllowedValues)).ToNot(BeZero()) }) - }) - Describe("DeletePermission", func() { - var ( - getShareResponse collaboration.GetShareResponse - getPublicShareResponse link.GetPublicShareResponse - ) - BeforeEach(func() { - getPublicShareResponse.Status = status.NewOK(context.Background()) - getShareResponse.Status = status.NewOK(context.Background()) - getShareResponse.Share = &collaboration.Share{ - Id: &collaboration.ShareId{ - OpaqueId: "permissionid", - }, - ResourceId: &provider.ResourceId{ - StorageId: "1", - SpaceId: "2", - OpaqueId: "3", - }, + It("records the ListPermissions span as a child of the ListSpaceRootPermissions span", func() { + sr, tp := newSpanRecorder() + service, err := svc.NewDriveItemPermissionsService(log.NewLogger(), gatewaySelector, cache, cfg, tp) + Expect(err).ToNot(HaveOccurred()) + + listSpacesResponse.StorageSpaces[0].SpaceType = "project" + listSpacesResponse.StorageSpaces[0].Root = &provider.ResourceId{ + StorageId: "1", + SpaceId: "2", + OpaqueId: "2", } - }) - It("fails to deletes a public link permission when it can be resolved", func() { - gatewayClient.On("GetPublicShare", mock.Anything, mock.Anything).Return(&getPublicShareResponse, nil) + gatewayClient.On("ListStorageSpaces", mock.Anything, mock.Anything).Return(listSpacesResponse, nil) + gatewayClient.On("ListShares", mock.Anything, mock.Anything).Return(&collaboration.ListSharesResponse{Status: status.NewOK(ctx)}, nil) + gatewayClient.On("ListPublicShares", mock.Anything, mock.Anything).Return(listPublicSharesResponse, nil) + statResponse.Info.Id = listSpacesResponse.StorageSpaces[0].Root + gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(statResponse, nil) + _, err = service.ListSpaceRootPermissions(context.Background(), driveId, svc.ListPermissionsQueryOptions{}) + Expect(err).ToNot(HaveOccurred()) - err := driveItemPermissionsService.DeletePermission(context.Background(), + spans := map[string]sdktrace.ReadOnlySpan{} + for _, s := range sr.Ended() { + spans[s.Name()] = s + } + Expect(spans).To(HaveKey("ListSpaceRootPermissions")) + Expect(spans).To(HaveKey("ListPermissions")) + parent := spans["ListSpaceRootPermissions"].SpanContext() + child := spans["ListPermissions"] + Expect(child.Parent().SpanID()).To(Equal(parent.SpanID())) + Expect(child.SpanContext().TraceID()).To(Equal(parent.TraceID())) + }) + }) + Describe("GetPermission", func() { + viewLink := func(id string) *link.PublicShare { + return &link.PublicShare{ + Id: &link.PublicShareId{OpaqueId: id}, + Token: id, + ResourceId: driveItemId, + Permissions: &link.PublicSharePermissions{Permissions: roleconversions.NewViewerRole().CS3ResourcePermissions()}, + } + } + + BeforeEach(func() { + statResponse.Info = &provider.ResourceInfo{ + Id: driveItemId, + Type: provider.ResourceType_RESOURCE_TYPE_FILE, + PermissionSet: roleconversions.NewEditorRole().CS3ResourcePermissions(), + } + }) + + It("returns not found for a permission that is not listed for the item", func() { + // shares are only listed for the requested item; permissions of other resources + // must not be resolved by ID (the strict gateway mock fails on GetShare/GetPublicShare) + gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(statResponse, nil) + gatewayClient.On("ListShares", mock.Anything, mock.MatchedBy(func(req *collaboration.ListSharesRequest) bool { + return slices.ContainsFunc(req.GetFilters(), func(f *collaboration.Filter) bool { + return utils.ResourceIDEqual(f.GetResourceId(), driveItemId) + }) + })).Return(&collaboration.ListSharesResponse{Status: status.NewOK(ctx)}, nil) + listPublicSharesResponse.Share = []*link.PublicShare{viewLink("aaaaaaaaaaaaaaa")} + gatewayClient.On("ListPublicShares", mock.Anything, mock.MatchedBy(func(req *link.ListPublicSharesRequest) bool { + return slices.ContainsFunc(req.GetFilters(), func(f *link.ListPublicSharesRequest_Filter) bool { + return utils.ResourceIDEqual(f.GetResourceId(), driveItemId) + }) + })).Return(listPublicSharesResponse, nil) + + _, err := driveItemPermissionsService.GetPermission(ctx, driveItemId, "ccccccccccccccc") + Expect(errorcode.IsErrorCode(err, errorcode.ItemNotFound)).To(BeTrue()) + }) + + It("propagates stat errors without listing permissions", func() { + statResponse.Status = status.NewPermissionDenied(ctx, nil, "denied") + gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(statResponse, nil) + + _, err := driveItemPermissionsService.GetPermission(ctx, driveItemId, "aaaaaaaaaaaaaaa") + Expect(errorcode.IsErrorCode(err, errorcode.AccessDenied)).To(BeTrue()) + }) + }) + + Describe("GetSpaceRootPermission", func() { + DescribeTable("rejects space types other than project", + func(spaceType string) { + gatewayClient.On("ListStorageSpaces", mock.Anything, mock.Anything).Return(&provider.ListStorageSpacesResponse{ + Status: status.NewOK(ctx), + StorageSpaces: []*provider.StorageSpace{{ + Id: &provider.StorageSpaceId{OpaqueId: "1$2"}, + SpaceType: spaceType, + Root: &provider.ResourceId{StorageId: "1", SpaceId: "2", OpaqueId: "2"}, + }}, + }, nil) + + _, err := driveItemPermissionsService.GetSpaceRootPermission(ctx, &provider.ResourceId{StorageId: "1", SpaceId: "2"}, "aaaaaaaaaaaaaaa") + Expect(err).To(MatchError(errorcode.New(errorcode.InvalidRequest, "unsupported space type"))) + }, + Entry("personal", "personal"), + Entry("virtual", "virtual"), + ) + }) + + Describe("DeletePermission", func() { + var ( + getShareResponse collaboration.GetShareResponse + getPublicShareResponse link.GetPublicShareResponse + ) + BeforeEach(func() { + getPublicShareResponse.Status = status.NewOK(context.Background()) + getPublicShareResponse.Share = nil + getShareResponse.Status = status.NewOK(context.Background()) + getShareResponse.Share = &collaboration.Share{ + Id: &collaboration.ShareId{ + OpaqueId: "permissionid", + }, + ResourceId: &provider.ResourceId{ + StorageId: "1", + SpaceId: "2", + OpaqueId: "3", + }, + } + }) + It("fails to deletes a public link permission when it can be resolved", func() { + gatewayClient.On("GetPublicShare", mock.Anything, mock.Anything).Return(&getPublicShareResponse, nil) + + err := driveItemPermissionsService.DeletePermission(context.Background(), getShareResponse.Share.ResourceId, "permissionid", ) @@ -830,6 +2191,287 @@ var _ = Describe("DriveItemPermissionsService", func() { ) Expect(err).ToNot(HaveOccurred()) }) + It("logs a share removal failure once as error with the item and permission ids", func() { + service, logBuf := newLoggedService() + + // not a public link, but a user share whose removal fails + getPublicShareResponse.Status = status.NewNotFound(context.Background(), "") + gatewayClient.On("GetPublicShare", mock.Anything, mock.Anything).Return(&getPublicShareResponse, nil) + gatewayClient.On("GetShare", mock.Anything, mock.Anything).Return(&getShareResponse, nil) + gatewayClient.On("RemoveShare", mock.Anything, mock.Anything).Return(&collaboration.RemoveShareResponse{ + Status: status.NewInternal(context.Background(), "removal failed for /Users/alice/Private/salary-2026.ods"), + }, nil) + + err := service.DeletePermission(ctx, getShareResponse.Share.ResourceId, "permissionid") + Expect(err).To(MatchError(errorcode.New(errorcode.GeneralException, "removal failed for /Users/alice/Private/salary-2026.ods").WithOrigin(errorcode.ErrorOriginCS3))) + gatewayClient.AssertCalled(GinkgoT(), "RemoveShare", mock.Anything, mock.MatchedBy(func(req *collaboration.RemoveShareRequest) bool { + return req.GetRef().GetId().GetOpaqueId() == "permissionid" + })) + + entries := logEntries(logBuf) + errorEntries := entriesAtLevel(entries, "error") + Expect(errorEntries).To(HaveLen(1)) + Expect(errorEntries[0]).To(SatisfyAll( + HaveKeyWithValue("itemID", storagespace.FormatResourceID(getShareResponse.Share.ResourceId)), + HaveKeyWithValue("permissionID", "permissionid"), + HaveKeyWithValue("errorCode", "generalException"), + // the error text tells operators the reason + HaveKeyWithValue("error", err.Error()), + )) + for _, payloadKey := range []string{"share", "res", "response", "request", "Body", "body"} { + Expect(entries).ToNot(ContainElement(HaveKey(payloadKey))) + } + }) + It("logs the error text when resolving a permission fails", func() { + service, logBuf := newLoggedService() + + // not a public link, the user share lookup fails in the share manager + getPublicShareResponse.Status = status.NewNotFound(context.Background(), "") + gatewayClient.On("GetPublicShare", mock.Anything, mock.Anything).Return(&getPublicShareResponse, nil) + gatewayClient.On("GetShare", mock.Anything, mock.Anything).Return(&collaboration.GetShareResponse{ + Status: status.NewInternal(context.Background(), "lookup failed for /Users/alice/Private/salary-2026.ods"), + }, nil) + + err := service.DeletePermission(ctx, getShareResponse.Share.ResourceId, "permissionid") + Expect(err).To(MatchError(errorcode.New(errorcode.GeneralException, "lookup failed for /Users/alice/Private/salary-2026.ods").WithOrigin(errorcode.ErrorOriginCS3))) + gatewayClient.AssertNotCalled(GinkgoT(), "RemoveShare", mock.Anything, mock.Anything) + + entries := logEntries(logBuf) + errorEntries := entriesAtLevel(entries, "error") + Expect(errorEntries).To(HaveLen(1)) + Expect(errorEntries[0]).To(SatisfyAll( + HaveKeyWithValue("message", "could not resolve permission"), + HaveKeyWithValue("itemID", storagespace.FormatResourceID(getShareResponse.Share.ResourceId)), + HaveKeyWithValue("permissionID", "permissionid"), + HaveKeyWithValue("errorCode", "generalException"), + HaveKeyWithValue("error", err.Error()), + )) + }) + DescribeTable("logs permission removal failures by cause with the item and permission ids", + func(setup func() string, expectedErr error, level string) { + removeMethod := setup() + service, logBuf := newLoggedService() + + err := service.DeletePermission(ctx, getShareResponse.Share.ResourceId, "permissionid") + Expect(err).To(MatchError(expectedErr)) + // the permission was resolved and its removal was attempted + gatewayClient.AssertCalled(GinkgoT(), removeMethod, mock.Anything, mock.Anything) + + graphErr, ok := errorcode.ToError(expectedErr) + Expect(ok).To(BeTrue()) + entries := logEntries(logBuf) + // the failure is logged with its error code and error text, whatever its level + Expect(entries).To(ContainElement(SatisfyAll( + HaveKeyWithValue("level", level), + HaveKeyWithValue("itemID", storagespace.FormatResourceID(getShareResponse.Share.ResourceId)), + HaveKeyWithValue("permissionID", "permissionid"), + HaveKeyWithValue("errorCode", graphErr.GetCode().String()), + HaveKeyWithValue("error", expectedErr.Error()), + ))) + errorEntries := entriesAtLevel(entries, "error") + if level == "error" { + Expect(errorEntries).To(HaveLen(1)) + } else { + Expect(entries).ToNot(ContainElement(HaveKeyWithValue("level", BeElementOf("error", "warn")))) + } + for _, payloadKey := range []string{"share", "res", "response", "request", "Body", "body"} { + Expect(entries).ToNot(ContainElement(HaveKey(payloadKey))) + } + }, + Entry("public link: internal error", + func() string { + // resolved as a public link on the item + getPublicShareResponse.Share = &link.PublicShare{ + Id: &link.PublicShareId{OpaqueId: "permissionid"}, + ResourceId: getShareResponse.Share.ResourceId, + } + gatewayClient.On("GetPublicShare", mock.Anything, mock.Anything).Return(&getPublicShareResponse, nil) + gatewayClient.On("RemovePublicShare", mock.Anything, mock.Anything).Return(&link.RemovePublicShareResponse{ + Status: status.NewInternal(context.Background(), "link manager unavailable"), + }, nil) + return "RemovePublicShare" + }, + errorcode.New(errorcode.GeneralException, "link manager unavailable").WithOrigin(errorcode.ErrorOriginCS3), "error"), + Entry("ocm share: internal error", + func() string { + // neither a public link nor a user share, but an ocm share on the item + cfg.IncludeOCMSharees = true + getPublicShareResponse.Status = status.NewNotFound(context.Background(), "") + gatewayClient.On("GetPublicShare", mock.Anything, mock.Anything).Return(&getPublicShareResponse, nil) + gatewayClient.On("GetShare", mock.Anything, mock.Anything).Return(&collaboration.GetShareResponse{ + Status: status.NewNotFound(context.Background(), ""), + }, nil) + gatewayClient.On("GetOCMShare", mock.Anything, mock.Anything).Return(&ocm.GetOCMShareResponse{ + Status: status.NewOK(context.Background()), + Share: &ocm.Share{ + Id: &ocm.ShareId{OpaqueId: "permissionid"}, + ResourceId: getShareResponse.Share.ResourceId, + }, + }, nil) + gatewayClient.On("RemoveOCMShare", mock.Anything, mock.Anything).Return(&ocm.RemoveOCMShareResponse{ + Status: status.NewInternal(context.Background(), "ocm share manager unavailable"), + }, nil) + return "RemoveOCMShare" + }, + errorcode.New(errorcode.GeneralException, "ocm share manager unavailable").WithOrigin(errorcode.ErrorOriginCS3), "error"), + Entry("user share: already removed", + func() string { + // resolved as a user share, which is gone by the time it is removed + getPublicShareResponse.Status = status.NewNotFound(context.Background(), "") + gatewayClient.On("GetPublicShare", mock.Anything, mock.Anything).Return(&getPublicShareResponse, nil) + gatewayClient.On("GetShare", mock.Anything, mock.Anything).Return(&getShareResponse, nil) + gatewayClient.On("RemoveShare", mock.Anything, mock.Anything).Return(&collaboration.RemoveShareResponse{ + Status: status.NewNotFound(context.Background(), "share already removed"), + }, nil) + return "RemoveShare" + }, + errorcode.New(errorcode.ItemNotFound, "share already removed").WithOrigin(errorcode.ErrorOriginCS3), "debug"), + ) + It("logs and traces a share lookup failure hidden by the ocm fallback once as error", func() { + var logBuf bytes.Buffer + sr, tp := newSpanRecorder() + cfg.IncludeOCMSharees = true + service, err := svc.NewDriveItemPermissionsService(log.Logger{Logger: zerolog.New(&logBuf)}, gatewaySelector, cache, cfg, tp) + Expect(err).ToNot(HaveOccurred()) + + // the user share lookup fails in the share manager, the ocm fallback does not know the permission + getPublicShareResponse.Status = status.NewNotFound(context.Background(), "") + gatewayClient.On("GetPublicShare", mock.Anything, mock.Anything).Return(&getPublicShareResponse, nil) + gatewayClient.On("GetShare", mock.Anything, mock.Anything).Return(&collaboration.GetShareResponse{ + Status: status.NewInternal(context.Background(), "lookup failed for /Users/alice/Private/salary-2026.ods"), + }, nil) + gatewayClient.On("GetOCMShare", mock.Anything, mock.Anything).Return(&ocm.GetOCMShareResponse{ + Status: status.NewNotFound(context.Background(), "not found"), + }, nil) + + err = service.DeletePermission(ctx, getShareResponse.Share.ResourceId, "permissionid") + // the ocm fallback result is still returned + Expect(err).To(MatchError(errorcode.New(errorcode.ItemNotFound, "not found").WithOrigin(errorcode.ErrorOriginCS3))) + gatewayClient.AssertCalled(GinkgoT(), "GetOCMShare", mock.Anything, mock.Anything) + + entries := logEntries(&logBuf) + errorEntries := entriesAtLevel(entries, "error") + Expect(errorEntries).To(HaveLen(1)) + Expect(errorEntries[0]).To(SatisfyAll( + HaveKeyWithValue("message", "could not resolve permission"), + HaveKeyWithValue("itemID", storagespace.FormatResourceID(getShareResponse.Share.ResourceId)), + HaveKeyWithValue("permissionID", "permissionid"), + HaveKeyWithValue("errorCode", "itemNotFound"), + HaveKeyWithValue("lookupErrorCode", "generalException"), + // the texts of both the hidden infrastructure failure and the returned not found are logged + HaveKeyWithValue("lookupError", errorcode.New(errorcode.GeneralException, "lookup failed for /Users/alice/Private/salary-2026.ods").WithOrigin(errorcode.ErrorOriginCS3).Error()), + HaveKeyWithValue("error", err.Error()), + )) + + // the span reports the hidden infrastructure failure by its code only + span := endedSpan(sr, "DeletePermission", "salary-2026.ods", "lookup failed") + Expect(span.Status().Code).To(Equal(codes.Error)) + Expect(span.Status().Description).To(Equal("generalException")) + Expect(span.Attributes()).To(ContainElement(attribute.String("error.type", "itemNotFound"))) + }) + It("does not mark the span when no share lookup finds the permission", func() { + sr, tp := newSpanRecorder() + cfg.IncludeOCMSharees = true + service, err := svc.NewDriveItemPermissionsService(log.NewLogger(), gatewaySelector, cache, cfg, tp) + Expect(err).ToNot(HaveOccurred()) + + getPublicShareResponse.Status = status.NewNotFound(context.Background(), "") + gatewayClient.On("GetPublicShare", mock.Anything, mock.Anything).Return(&getPublicShareResponse, nil) + gatewayClient.On("GetShare", mock.Anything, mock.Anything).Return(&collaboration.GetShareResponse{ + Status: status.NewNotFound(context.Background(), "not found"), + }, nil) + gatewayClient.On("GetOCMShare", mock.Anything, mock.Anything).Return(&ocm.GetOCMShareResponse{ + Status: status.NewNotFound(context.Background(), "not found"), + }, nil) + + err = service.DeletePermission(ctx, getShareResponse.Share.ResourceId, "permissionid") + Expect(err).To(MatchError(errorcode.New(errorcode.ItemNotFound, "not found").WithOrigin(errorcode.ErrorOriginCS3))) + + span := endedSpan(sr, "DeletePermission") + Expect(span.Status().Code).To(Equal(codes.Unset)) + Expect(span.Attributes()).To(ContainElement(attribute.String("error.type", "itemNotFound"))) + }) + It("does not mark the span of a permission that is not found without the ocm fallback", func() { + sr, tp := newSpanRecorder() + service, err := svc.NewDriveItemPermissionsService(log.NewLogger(), gatewaySelector, cache, cfg, tp) + Expect(err).ToNot(HaveOccurred()) + + getPublicShareResponse.Status = status.NewNotFound(context.Background(), "") + gatewayClient.On("GetPublicShare", mock.Anything, mock.Anything).Return(&getPublicShareResponse, nil) + gatewayClient.On("GetShare", mock.Anything, mock.Anything).Return(&collaboration.GetShareResponse{ + Status: status.NewNotFound(context.Background(), "not found"), + }, nil) + + err = service.DeletePermission(ctx, getShareResponse.Share.ResourceId, "permissionid") + Expect(err).To(MatchError(errorcode.New(errorcode.ItemNotFound, "not found").WithOrigin(errorcode.ErrorOriginCS3))) + gatewayClient.AssertNotCalled(GinkgoT(), "GetOCMShare", mock.Anything, mock.Anything) + + span := endedSpan(sr, "DeletePermission") + Expect(span.Status().Code).To(Equal(codes.Unset)) + Expect(span.Attributes()).To(ContainElement(attribute.String("error.type", "itemNotFound"))) + }) + It("does not mark the span when the ocm fallback succeeds after a share lookup failure", func() { + sr, tp := newSpanRecorder() + cfg.IncludeOCMSharees = true + service, err := svc.NewDriveItemPermissionsService(log.NewLogger(), gatewaySelector, cache, cfg, tp) + Expect(err).ToNot(HaveOccurred()) + + getPublicShareResponse.Status = status.NewNotFound(context.Background(), "") + gatewayClient.On("GetPublicShare", mock.Anything, mock.Anything).Return(&getPublicShareResponse, nil) + gatewayClient.On("GetShare", mock.Anything, mock.Anything).Return(&collaboration.GetShareResponse{ + Status: status.NewInternal(context.Background(), "share manager unavailable"), + }, nil) + gatewayClient.On("GetOCMShare", mock.Anything, mock.Anything).Return(&ocm.GetOCMShareResponse{ + Status: status.NewOK(context.Background()), + Share: &ocm.Share{ + Id: &ocm.ShareId{OpaqueId: "permissionid"}, + ResourceId: getShareResponse.Share.ResourceId, + }, + }, nil) + gatewayClient.On("RemoveOCMShare", mock.Anything, mock.Anything).Return(&ocm.RemoveOCMShareResponse{ + Status: status.NewOK(context.Background()), + }, nil) + + Expect(service.DeletePermission(ctx, getShareResponse.Share.ResourceId, "permissionid")).To(Succeed()) + gatewayClient.AssertCalled(GinkgoT(), "RemoveOCMShare", mock.Anything, mock.Anything) + + span := endedSpan(sr, "DeletePermission") + Expect(span.Status().Code).To(Equal(codes.Unset)) + Expect(span.Attributes()).ToNot(ContainElement(HaveField("Key", attribute.Key("error.type")))) + }) + It("logs a permission without a resolvable resource id as debug with the item and permission ids", func() { + service, logBuf := newLoggedService() + + // a public link without a resource id + gatewayClient.On("GetPublicShare", mock.Anything, mock.Anything).Return(&getPublicShareResponse, nil) + + err := service.DeletePermission(ctx, getShareResponse.Share.ResourceId, "permissionid") + Expect(err).To(MatchError(errorcode.New(errorcode.ItemNotFound, "failed to resolve resource id for shared resource"))) + + entries := logEntries(logBuf) + Expect(entries).To(ContainElement(SatisfyAll( + HaveKeyWithValue("level", "debug"), + HaveKeyWithValue("message", "could not resolve the shared resource of the permission"), + HaveKeyWithValue("itemID", storagespace.FormatResourceID(getShareResponse.Share.ResourceId)), + HaveKeyWithValue("permissionID", "permissionid"), + ))) + Expect(entries).ToNot(ContainElement(HaveKeyWithValue("level", BeElementOf("error", "warn")))) + }) + It("does not log the not found results of permission type probing as errors", func() { + service, logBuf := newLoggedService() + + // not a public link, but a user share + getPublicShareResponse.Status = status.NewNotFound(context.Background(), "") + gatewayClient.On("GetPublicShare", mock.Anything, mock.Anything).Return(&getPublicShareResponse, nil) + gatewayClient.On("GetShare", mock.Anything, mock.Anything).Return(&getShareResponse, nil) + gatewayClient.On("RemoveShare", mock.Anything, mock.Anything).Return(&collaboration.RemoveShareResponse{Status: status.NewOK(ctx)}, nil) + + err := service.DeletePermission(ctx, getShareResponse.Share.ResourceId, "permissionid") + Expect(err).ToNot(HaveOccurred()) + gatewayClient.AssertCalled(GinkgoT(), "RemoveShare", mock.Anything, mock.Anything) + + Expect(logEntries(logBuf)).ToNot(ContainElement(HaveKeyWithValue("level", BeElementOf("error", "warn")))) + }) It("deletes a space permission as expected", func() { getPublicShareResponse.Status = status.NewNotFound(context.Background(), "") gatewayClient.On("GetPublicShare", mock.Anything, mock.Anything).Return(&getPublicShareResponse, nil) @@ -885,6 +2527,72 @@ var _ = Describe("DriveItemPermissionsService", func() { Expect(err).To(MatchError(errorcode.New(errorcode.InvalidRequest, "permissionID and itemID do not match"))) }) + It("logs a permission and item id mismatch as debug with the item and permission ids", func() { + service, logBuf := newLoggedService() + + // not a public link, but a user share on another resource than the one in the request + getPublicShareResponse.Status = status.NewNotFound(context.Background(), "") + gatewayClient.On("GetPublicShare", mock.Anything, mock.Anything).Return(&getPublicShareResponse, nil) + getShareResponse.Share.ResourceId = &provider.ResourceId{StorageId: "3", SpaceId: "4", OpaqueId: "5"} + gatewayClient.On("GetShare", mock.Anything, mock.Anything).Return(&getShareResponse, nil) + + itemID := &provider.ResourceId{StorageId: "1", SpaceId: "2", OpaqueId: "3"} + err := service.DeletePermission(ctx, itemID, "permissionid") + Expect(err).To(MatchError(errorcode.New(errorcode.InvalidRequest, "permissionID and itemID do not match"))) + // nothing is removed after the mismatch + for _, method := range []string{"RemoveShare", "RemovePublicShare", "RemoveOCMShare"} { + gatewayClient.AssertNotCalled(GinkgoT(), method, mock.Anything, mock.Anything) + } + + entries := logEntries(logBuf) + var mismatchEntries []map[string]any + for _, entry := range entries { + if entry["level"] == "debug" && entry["itemID"] == storagespace.FormatResourceID(itemID) && entry["permissionID"] == "permissionid" { + mismatchEntries = append(mismatchEntries, entry) + } + } + Expect(mismatchEntries).To(HaveLen(1)) + Expect(entries).ToNot(ContainElement(HaveKeyWithValue("level", BeElementOf("error", "warn")))) + }) + }) + Describe("DeleteSpaceRootPermission", func() { + It("records a DeleteSpaceRootPermission span as the parent of the DeletePermission span", func() { + sr, tp := newSpanRecorder() + service, err := svc.NewDriveItemPermissionsService(log.NewLogger(), gatewaySelector, cache, cfg, tp) + Expect(err).ToNot(HaveOccurred()) + + spaceRoot := &provider.ResourceId{StorageId: "1", SpaceId: "2", OpaqueId: "2"} + gatewayClient.On("ListStorageSpaces", mock.Anything, mock.Anything).Return(&provider.ListStorageSpacesResponse{ + Status: status.NewOK(ctx), + StorageSpaces: []*provider.StorageSpace{ + {Id: &provider.StorageSpaceId{OpaqueId: "2"}, SpaceType: "project", Root: spaceRoot}, + }, + }, nil) + gatewayClient.On("GetPublicShare", mock.Anything, mock.Anything).Return(&link.GetPublicShareResponse{ + Status: status.NewNotFound(ctx, ""), + }, nil) + gatewayClient.On("RemoveShare", mock.Anything, mock.Anything).Return(&collaboration.RemoveShareResponse{Status: status.NewOK(ctx)}, nil) + + err = service.DeleteSpaceRootPermission(context.Background(), &provider.ResourceId{StorageId: "1", SpaceId: "2"}, "shareid") + Expect(err).ToNot(HaveOccurred()) + // the space was resolved and DeletePermission removed the space permission + gatewayClient.AssertCalled(GinkgoT(), "RemoveShare", mock.Anything, mock.MatchedBy(func(req *collaboration.RemoveShareRequest) bool { + return req.GetRef().GetId().GetOpaqueId() == "shareid" + })) + + spans := map[string]sdktrace.ReadOnlySpan{} + for _, s := range sr.Ended() { + spans[s.Name()] = s + } + Expect(spans).To(HaveKey("DeleteSpaceRootPermission")) + Expect(spans).To(HaveKey("DeletePermission")) + parent := spans["DeleteSpaceRootPermission"].SpanContext() + child := spans["DeletePermission"] + Expect(parent.IsValid()).To(BeTrue()) + Expect(child.SpanContext().IsValid()).To(BeTrue()) + Expect(child.Parent().SpanID()).To(Equal(parent.SpanID())) + Expect(child.SpanContext().TraceID()).To(Equal(parent.TraceID())) + }) }) Describe("UpdatePermission", func() { var ( @@ -994,6 +2702,418 @@ var _ = Describe("DriveItemPermissionsService", func() { Expect(err).To(HaveOccurred()) Expect(res).To(BeZero()) }) + It("logs a gateway selection failure while listing space root shares as error in the helper and the operation with the item and permission ids", func() { + gatewayErr := errors.New("no gateway available") + spaceRootID := &provider.ResourceId{StorageId: "1", SpaceId: "2", OpaqueId: "2"} + // getPermissionByID and the public link lookup get a client, the space root share listing does not + gatewaySelector.EXPECT().Next().Unset() + gatewaySelector.EXPECT().Next().Return(gatewayClient, nil).Times(2) + gatewaySelector.EXPECT().Next().Return(nil, gatewayErr).Times(1) + service, logBuf := newLoggedService() + getPublicShareMockResponse.Share = nil + getPublicShareMockResponse.Status = status.NewNotFound(ctx, "not found") + gatewayClient.On("GetPublicShare", mock.Anything, mock.Anything).Return(getPublicShareMockResponse, nil) + + _, err := service.UpdatePermission(ctx, spaceRootID, "permissionid", driveItemPermission) + Expect(err).To(MatchError(errorcode.New(errorcode.GeneralException, gatewayErr.Error()))) + // the lookup stops at the failed space root share listing, nothing is updated + for _, method := range []string{"ListShares", "UpdateShare", "UpdatePublicShare", "Stat"} { + gatewayClient.AssertNotCalled(GinkgoT(), method, mock.Anything, mock.Anything) + } + + entries := logEntries(logBuf) + // the helper and the operation both log the reason, the operation with the item and permission ids + Expect(entriesAtLevel(entries, "error")).To(ConsistOf( + gatewayHelperEntry("could not select next gateway client", gatewayErr.Error()), + SatisfyAll( + HaveKeyWithValue("itemID", storagespace.FormatResourceID(spaceRootID)), + HaveKeyWithValue("permissionID", "permissionid"), + HaveKeyWithValue("error", err.Error()), + ), + )) + for _, payloadKey := range []string{"share", "shares", "res", "response", "request", "Body", "body"} { + Expect(entries).ToNot(ContainElement(HaveKey(payloadKey))) + } + }) + It("logs the identity backend error when resolving a user share grantee fails", func() { + service, logBuf := newLoggedService() + + const identityBackendMessage = "ldap search failed for cn=private-group-dn-42,ou=groups" + getPublicShareMockResponse.Share = nil + getPublicShareMockResponse.Status = status.NewNotFound(ctx, "not found") + gatewayClient.On("GetPublicShare", mock.Anything, mock.Anything).Return(getPublicShareMockResponse, nil) + getShareMockResponse.Share.Grantee = &provider.Grantee{ + Type: provider.GranteeType_GRANTEE_TYPE_GROUP, + Id: &provider.Grantee_GroupId{GroupId: &grouppb.GroupId{OpaqueId: "groupid"}}, + } + gatewayClient.On("GetShare", mock.Anything, mock.Anything).Return(getShareMockResponse, nil) + gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(statResponse, nil) + gatewayClient.On("GetGroup", mock.Anything, mock.Anything).Return(&grouppb.GetGroupResponse{ + Status: status.NewInternal(context.Background(), identityBackendMessage), + }, nil) + + _, err := service.UpdatePermission(ctx, driveItemId, "permissionid", driveItemPermission) + // the returned error is unchanged: the share conversion error + Expect(err).To(MatchError(errorcode.New(errorcode.GeneralException, + errorcode.New(errorcode.GeneralException, identityBackendMessage).Error()))) + // the user share grantee was looked up, nothing is updated + gatewayClient.AssertCalled(GinkgoT(), "GetGroup", mock.Anything, mock.MatchedBy(func(req *grouppb.GetGroupRequest) bool { + return req.GetGroupId().GetOpaqueId() == "groupid" + })) + for _, method := range []string{"UpdateShare", "UpdatePublicShare"} { + gatewayClient.AssertNotCalled(GinkgoT(), method, mock.Anything, mock.Anything) + } + + entries := logEntries(logBuf) + errorEntries := entriesAtLevel(entries, "error") + Expect(errorEntries).To(HaveLen(1)) + Expect(errorEntries[0]).To(SatisfyAll( + HaveKeyWithValue("itemID", storagespace.FormatResourceID(driveItemId)), + HaveKeyWithValue("permissionID", "permissionid"), + HaveKeyWithValue("error", err.Error()), + )) + for _, payloadKey := range []string{"share", "shares", "group", "res", "response", "request", "Body", "body"} { + Expect(entries).ToNot(ContainElement(HaveKey(payloadKey))) + } + }) + DescribeTable("logs public link update failures as error with the item and permission ids", + func(setup func(), expectedErr error, helperEntries int) { + setup() + service, logBuf := newLoggedService() + + sharingLink := libregraph.NewSharingLink() + sharingLink.SetLibreGraphDisplayName(TestLinkName) + driveItemPermission.SetLink(*sharingLink) + _, err := service.UpdatePermission(ctx, driveItemId, "permissionid", driveItemPermission) + Expect(err).To(MatchError(expectedErr)) + // the permission was resolved as a public link, user and ocm shares are not touched + gatewayClient.AssertCalled(GinkgoT(), "GetPublicShare", mock.Anything, mock.Anything) + for _, method := range []string{"GetShare", "UpdateShare", "GetOCMShare"} { + gatewayClient.AssertNotCalled(GinkgoT(), method, mock.Anything, mock.Anything) + } + + entries := logEntries(logBuf) + // the operation's entry carries the ids, a helper that cannot select a gateway logs the reason + var operationEntries, helpers []map[string]any + for _, entry := range entriesAtLevel(entries, "error") { + if _, ok := entry["itemID"]; ok { + operationEntries = append(operationEntries, entry) + } else { + helpers = append(helpers, entry) + } + } + Expect(operationEntries).To(HaveLen(1)) + Expect(operationEntries[0]).To(SatisfyAll( + HaveKeyWithValue("itemID", storagespace.FormatResourceID(driveItemId)), + HaveKeyWithValue("permissionID", "permissionid"), + HaveKeyWithValue("error", expectedErr.Error()), + )) + Expect(helpers).To(HaveLen(helperEntries)) + for _, entry := range helpers { + Expect(entry).To(gatewayHelperEntry("could not select next gateway client", "no gateway available")) + } + for _, payloadKey := range []string{"share", "link", "Link", "res", "response", "request", "Body", "body", "password"} { + Expect(entries).ToNot(ContainElement(HaveKey(payloadKey))) + } + // the public link token must never be logged + raw, err := json.Marshal(entries) + Expect(err).ToNot(HaveOccurred()) + Expect(string(raw)).ToNot(ContainSubstring(`"token"`)) + }, + Entry("UpdatePublicShare returns an internal error", + func() { + gatewayClient.On("GetPublicShare", mock.Anything, mock.Anything).Return(getPublicShareMockResponse, nil) + gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(statResponse, nil) + gatewayClient.On("UpdatePublicShare", mock.Anything, mock.Anything).Return(&link.UpdatePublicShareResponse{ + Status: status.NewInternal(ctx, "link manager unavailable"), + }, nil) + }, + errorcode.New(errorcode.GeneralException, "link manager unavailable").WithOrigin(errorcode.ErrorOriginCS3), 0), + Entry("gateway selection fails in the public link update", + func() { + // getPermissionByID and the public link lookup get a client, the link update does not + gatewaySelector.EXPECT().Next().Unset() + gatewaySelector.EXPECT().Next().Return(gatewayClient, nil).Times(2) + gatewaySelector.EXPECT().Next().Return(nil, errors.New("no gateway available")).Times(1) + gatewayClient.On("GetPublicShare", mock.Anything, mock.Anything).Return(getPublicShareMockResponse, nil) + }, + errorcode.New(errorcode.GeneralException, "no gateway available"), 1), + ) + It("logs a missing public link item at debug level with the backend stat message", func() { + service, logBuf := newLoggedService() + + // the permission resolves to a public link, but its item can no longer be stat'ed; + // decomposedfs reports a missing node as "/" + statMessage := "stat: 7c3e9a1b-parent/private-financial-report-2026.xlsx" + gatewayClient.On("GetPublicShare", mock.Anything, mock.Anything).Return(getPublicShareMockResponse, nil) + gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(&provider.StatResponse{ + Status: status.NewNotFound(ctx, statMessage), + }, nil) + + sharingLink := libregraph.NewSharingLink() + sharingLink.SetLibreGraphDisplayName(TestLinkName) + driveItemPermission.SetLink(*sharingLink) + _, err := service.UpdatePermission(ctx, driveItemId, "permissionid", driveItemPermission) + Expect(err).To(MatchError(errorcode.New(errorcode.ItemNotFound, statMessage).WithOrigin(errorcode.ErrorOriginCS3))) + gatewayClient.AssertCalled(GinkgoT(), "Stat", mock.Anything, mock.Anything) + // nothing is updated after the failed stat + for _, method := range []string{"UpdatePublicShare", "UpdateShare", "UpdateOCMShare"} { + gatewayClient.AssertNotCalled(GinkgoT(), method, mock.Anything, mock.Anything) + } + + entries := logEntries(logBuf) + var debugEntries []map[string]any + for _, entry := range entries { + if entry["level"] == "debug" && entry["itemID"] == storagespace.FormatResourceID(driveItemId) && entry["permissionID"] == "permissionid" { + debugEntries = append(debugEntries, entry) + } + } + Expect(debugEntries).To(HaveLen(1)) + // the debug entry carries the backend message, so the missing item can be identified + Expect(debugEntries[0]).To(HaveKeyWithValue("error", ContainSubstring(statMessage))) + Expect(entries).ToNot(ContainElement(HaveKeyWithValue("level", BeElementOf("error", "warn")))) + }) + It("logs a user share update failure once as error with the item and permission ids", func() { + service, logBuf := newLoggedService() + + // resolved as a user share on the item, whose update fails + gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(statResponse, nil) + gatewayClient.On("GetUser", mock.Anything, mock.Anything).Return(getUserResponse, nil) + getPublicShareMockResponse.Share = nil + getPublicShareMockResponse.Status = status.NewNotFound(ctx, "not found") + gatewayClient.On("GetPublicShare", mock.Anything, mock.Anything).Return(getPublicShareMockResponse, nil) + gatewayClient.On("GetShare", mock.Anything, mock.Anything).Return(getShareMockResponse, nil) + gatewayClient.On("UpdateShare", mock.Anything, mock.Anything).Return(&collaboration.UpdateShareResponse{ + Status: status.NewInternal(ctx, "share manager unavailable"), + }, nil) + + driveItemPermission.SetExpirationDateTime(time.Now().Add(time.Hour)) + _, err := service.UpdatePermission(ctx, driveItemId, "permissionid", driveItemPermission) + Expect(err).To(MatchError(errorcode.New(errorcode.GeneralException, "share manager unavailable").WithOrigin(errorcode.ErrorOriginCS3))) + gatewayClient.AssertCalled(GinkgoT(), "UpdateShare", mock.Anything, mock.Anything) + + entries := logEntries(logBuf) + errorEntries := entriesAtLevel(entries, "error") + Expect(errorEntries).To(HaveLen(1)) + Expect(errorEntries[0]).To(SatisfyAll( + HaveKeyWithValue("itemID", storagespace.FormatResourceID(driveItemId)), + HaveKeyWithValue("permissionID", "permissionid"), + HaveKeyWithValue("errorCode", "generalException"), + HaveKeyWithValue("error", err.Error()), + )) + for _, payloadKey := range []string{"share", "shares", "res", "response", "request", "Body", "body"} { + Expect(entries).ToNot(ContainElement(HaveKey(payloadKey))) + } + }) + It("logs and traces a failed ocm share update once with the cause of both update attempts", func() { + var logBuf bytes.Buffer + sr, tp := newSpanRecorder() + cfg.IncludeOCMSharees = true + service, err := svc.NewDriveItemPermissionsService(log.Logger{Logger: zerolog.New(&logBuf)}, gatewaySelector, cache, cfg, tp) + Expect(err).ToNot(HaveOccurred()) + + // the permission is neither a public link nor a user share, but an ocm share + getPublicShareMockResponse.Share = nil + getPublicShareMockResponse.Status = status.NewNotFound(ctx, "not found") + gatewayClient.On("GetPublicShare", mock.Anything, mock.Anything).Return(getPublicShareMockResponse, nil) + getShareMockResponse.Share = nil + getShareMockResponse.Status = status.NewNotFound(ctx, "not found") + gatewayClient.On("GetShare", mock.Anything, mock.Anything).Return(getShareMockResponse, nil) + gatewayClient.On("GetOCMShare", mock.Anything, mock.Anything).Return(&ocm.GetOCMShareResponse{ + Status: status.NewOK(ctx), + Share: &ocm.Share{Id: &ocm.ShareId{OpaqueId: "permissionid"}, ResourceId: driveItemId}, + }, nil) + gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(statResponse, nil) + // the user share update attempt does not find the share, the ocm share update fails + gatewayClient.On("UpdateShare", mock.Anything, mock.Anything).Return(&collaboration.UpdateShareResponse{ + Status: status.NewNotFound(ctx, "not found"), + }, nil) + gatewayClient.On("UpdateOCMShare", mock.Anything, mock.Anything).Return(&ocm.UpdateOCMShareResponse{ + Status: status.NewInternal(ctx, "ocm share manager unavailable"), + }, nil) + + driveItemPermission.SetExpirationDateTime(time.Now().Add(time.Hour)) + _, err = service.UpdatePermission(ctx, driveItemId, "permissionid", driveItemPermission) + // the returned error is unchanged: the error of the user share update attempt + Expect(err).To(MatchError(errorcode.New(errorcode.ItemNotFound, "not found").WithOrigin(errorcode.ErrorOriginCS3))) + gatewayClient.AssertCalled(GinkgoT(), "UpdateOCMShare", mock.Anything, mock.Anything) + + entries := logEntries(&logBuf) + errorEntries := entriesAtLevel(entries, "error") + Expect(errorEntries).To(HaveLen(1)) + Expect(errorEntries[0]).To(SatisfyAll( + HaveKeyWithValue("message", "could not update permission"), + HaveKeyWithValue("itemID", storagespace.FormatResourceID(driveItemId)), + HaveKeyWithValue("permissionID", "permissionid"), + HaveKeyWithValue("errorCode", "itemNotFound"), + HaveKeyWithValue("ocmErrorCode", "generalException"), + // the texts of both the failed ocm update and the returned not found are logged + HaveKeyWithValue("ocmError", errorcode.New(errorcode.GeneralException, "ocm share manager unavailable").WithOrigin(errorcode.ErrorOriginCS3).Error()), + HaveKeyWithValue("error", err.Error()), + )) + + // the span reports the failed ocm update by its code only + span := endedSpan(sr, "UpdatePermission", "ocm share manager unavailable") + Expect(span.Status().Code).To(Equal(codes.Error)) + Expect(span.Status().Description).To(Equal("generalException")) + Expect(span.Attributes()).To(ContainElement(attribute.String("error.type", "itemNotFound"))) + }) + It("does not mark the span when neither update attempt finds the share", func() { + sr, tp := newSpanRecorder() + cfg.IncludeOCMSharees = true + service, err := svc.NewDriveItemPermissionsService(log.NewLogger(), gatewaySelector, cache, cfg, tp) + Expect(err).ToNot(HaveOccurred()) + + getPublicShareMockResponse.Share = nil + getPublicShareMockResponse.Status = status.NewNotFound(ctx, "not found") + gatewayClient.On("GetPublicShare", mock.Anything, mock.Anything).Return(getPublicShareMockResponse, nil) + getShareMockResponse.Share = nil + getShareMockResponse.Status = status.NewNotFound(ctx, "not found") + gatewayClient.On("GetShare", mock.Anything, mock.Anything).Return(getShareMockResponse, nil) + gatewayClient.On("GetOCMShare", mock.Anything, mock.Anything).Return(&ocm.GetOCMShareResponse{ + Status: status.NewOK(ctx), + Share: &ocm.Share{Id: &ocm.ShareId{OpaqueId: "permissionid"}, ResourceId: driveItemId}, + }, nil) + gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(statResponse, nil) + gatewayClient.On("UpdateShare", mock.Anything, mock.Anything).Return(&collaboration.UpdateShareResponse{ + Status: status.NewNotFound(ctx, "not found"), + }, nil) + gatewayClient.On("UpdateOCMShare", mock.Anything, mock.Anything).Return(&ocm.UpdateOCMShareResponse{ + Status: status.NewNotFound(ctx, "not found"), + }, nil) + + driveItemPermission.SetExpirationDateTime(time.Now().Add(time.Hour)) + _, err = service.UpdatePermission(ctx, driveItemId, "permissionid", driveItemPermission) + Expect(err).To(MatchError(errorcode.New(errorcode.ItemNotFound, "not found").WithOrigin(errorcode.ErrorOriginCS3))) + gatewayClient.AssertCalled(GinkgoT(), "UpdateOCMShare", mock.Anything, mock.Anything) + + span := endedSpan(sr, "UpdatePermission") + Expect(span.Status().Code).To(Equal(codes.Unset)) + Expect(span.Attributes()).To(ContainElement(attribute.String("error.type", "itemNotFound"))) + }) + It("logs the identity backend error when converting the updated user share fails", func() { + service, logBuf := newLoggedService() + + // resolved as a user share on the item, the updated share is returned with a + // group grantee that has not been looked up (and cached) before + const identityBackendMessage = "ldap search failed for cn=private-group-dn-42,ou=groups" + gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(statResponse, nil) + gatewayClient.On("GetUser", mock.Anything, mock.Anything).Return(getUserResponse, nil) + getPublicShareMockResponse.Share = nil + getPublicShareMockResponse.Status = status.NewNotFound(ctx, "not found") + gatewayClient.On("GetPublicShare", mock.Anything, mock.Anything).Return(getPublicShareMockResponse, nil) + gatewayClient.On("GetShare", mock.Anything, mock.Anything).Return(getShareMockResponse, nil) + updateShareMockResponse.Share = &collaboration.Share{ + Id: &collaboration.ShareId{OpaqueId: "permissionid"}, + ResourceId: driveItemId, + Grantee: &provider.Grantee{ + Type: provider.GranteeType_GRANTEE_TYPE_GROUP, + Id: &provider.Grantee_GroupId{GroupId: &grouppb.GroupId{OpaqueId: "groupid"}}, + }, + } + gatewayClient.On("UpdateShare", mock.Anything, mock.Anything).Return(updateShareMockResponse, nil) + gatewayClient.On("GetGroup", mock.Anything, mock.Anything).Return(&grouppb.GetGroupResponse{ + Status: status.NewInternal(ctx, identityBackendMessage), + }, nil) + + driveItemPermission.SetExpirationDateTime(time.Now().Add(time.Hour)) + _, err := service.UpdatePermission(ctx, driveItemId, "permissionid", driveItemPermission) + // the returned error is unchanged: the identity error wrapped by the share conversion + Expect(err).To(MatchError(errorcode.New(errorcode.GeneralException, + errorcode.New(errorcode.GeneralException, identityBackendMessage).Error()))) + gatewayClient.AssertCalled(GinkgoT(), "GetGroup", mock.Anything, mock.Anything) + + entries := logEntries(logBuf) + errorEntries := entriesAtLevel(entries, "error") + Expect(errorEntries).To(HaveLen(1)) + Expect(errorEntries[0]).To(SatisfyAll( + HaveKeyWithValue("itemID", storagespace.FormatResourceID(driveItemId)), + HaveKeyWithValue("permissionID", "permissionid"), + HaveKeyWithValue("errorCode", "generalException"), + HaveKeyWithValue("error", err.Error()), + )) + }) + It("does not log a permission that is not found by any lookup as error", func() { + service, logBuf := newLoggedService() + + getPublicShareMockResponse.Share = nil + getPublicShareMockResponse.Status = status.NewNotFound(ctx, "not found") + gatewayClient.On("GetPublicShare", mock.Anything, mock.Anything).Return(getPublicShareMockResponse, nil) + getShareMockResponse.Share = nil + getShareMockResponse.Status = status.NewNotFound(ctx, "not found") + gatewayClient.On("GetShare", mock.Anything, mock.Anything).Return(getShareMockResponse, nil) + + _, err := service.UpdatePermission(ctx, driveItemId, "permissionid", driveItemPermission) + Expect(errorcode.IsErrorCode(err, errorcode.ItemNotFound)).To(BeTrue()) + + Expect(logEntries(logBuf)).ToNot(ContainElement(HaveKeyWithValue("level", BeElementOf("error", "warn")))) + }) + It("logs and traces a share lookup failure hidden by the ocm fallback once as error", func() { + var logBuf bytes.Buffer + sr, tp := newSpanRecorder() + cfg.IncludeOCMSharees = true + service, err := svc.NewDriveItemPermissionsService(log.Logger{Logger: zerolog.New(&logBuf)}, gatewaySelector, cache, cfg, tp) + Expect(err).ToNot(HaveOccurred()) + + // the user share lookup fails in the share manager, the ocm fallback does not know the permission + getPublicShareMockResponse.Share = nil + getPublicShareMockResponse.Status = status.NewNotFound(ctx, "not found") + gatewayClient.On("GetPublicShare", mock.Anything, mock.Anything).Return(getPublicShareMockResponse, nil) + getShareMockResponse.Share = nil + getShareMockResponse.Status = status.NewInternal(ctx, "lookup failed for /Users/alice/Private/salary-2026.ods") + gatewayClient.On("GetShare", mock.Anything, mock.Anything).Return(getShareMockResponse, nil) + gatewayClient.On("GetOCMShare", mock.Anything, mock.Anything).Return(&ocm.GetOCMShareResponse{ + Status: status.NewNotFound(ctx, "not found"), + }, nil) + + _, err = service.UpdatePermission(ctx, driveItemId, "permissionid", driveItemPermission) + // the ocm fallback result is still returned + Expect(err).To(MatchError(errorcode.New(errorcode.ItemNotFound, "not found").WithOrigin(errorcode.ErrorOriginCS3))) + gatewayClient.AssertCalled(GinkgoT(), "GetOCMShare", mock.Anything, mock.Anything) + + entries := logEntries(&logBuf) + errorEntries := entriesAtLevel(entries, "error") + Expect(errorEntries).To(HaveLen(1)) + Expect(errorEntries[0]).To(SatisfyAll( + HaveKeyWithValue("message", "could not resolve permission"), + HaveKeyWithValue("itemID", storagespace.FormatResourceID(driveItemId)), + HaveKeyWithValue("permissionID", "permissionid"), + HaveKeyWithValue("errorCode", "itemNotFound"), + HaveKeyWithValue("lookupErrorCode", "generalException"), + // the texts of both the hidden infrastructure failure and the returned not found are logged + HaveKeyWithValue("lookupError", errorcode.New(errorcode.GeneralException, "lookup failed for /Users/alice/Private/salary-2026.ods").WithOrigin(errorcode.ErrorOriginCS3).Error()), + HaveKeyWithValue("error", err.Error()), + )) + + // the span reports the hidden infrastructure failure by its code only + span := endedSpan(sr, "UpdatePermission", "salary-2026.ods", "lookup failed") + Expect(span.Status().Code).To(Equal(codes.Error)) + Expect(span.Status().Description).To(Equal("generalException")) + Expect(span.Attributes()).To(ContainElement(attribute.String("error.type", "itemNotFound"))) + }) + It("does not mark the span when neither permission lookup finds the permission", func() { + sr, tp := newSpanRecorder() + cfg.IncludeOCMSharees = true + service, err := svc.NewDriveItemPermissionsService(log.NewLogger(), gatewaySelector, cache, cfg, tp) + Expect(err).ToNot(HaveOccurred()) + + getPublicShareMockResponse.Share = nil + getPublicShareMockResponse.Status = status.NewNotFound(ctx, "not found") + gatewayClient.On("GetPublicShare", mock.Anything, mock.Anything).Return(getPublicShareMockResponse, nil) + getShareMockResponse.Share = nil + getShareMockResponse.Status = status.NewNotFound(ctx, "not found") + gatewayClient.On("GetShare", mock.Anything, mock.Anything).Return(getShareMockResponse, nil) + gatewayClient.On("GetOCMShare", mock.Anything, mock.Anything).Return(&ocm.GetOCMShareResponse{ + Status: status.NewNotFound(ctx, "not found"), + }, nil) + + _, err = service.UpdatePermission(ctx, driveItemId, "permissionid", driveItemPermission) + Expect(err).To(MatchError(errorcode.New(errorcode.ItemNotFound, "not found").WithOrigin(errorcode.ErrorOriginCS3))) + + span := endedSpan(sr, "UpdatePermission") + Expect(span.Status().Code).To(Equal(codes.Unset)) + Expect(span.Attributes()).To(ContainElement(attribute.String("error.type", "itemNotFound"))) + }) It("fails to update permission when the resourceID mismatches with the shared resource's id", func() { getShareMockResponse.Share = nil getShareMockResponse.Status = status.NewNotFound(ctx, "not found") @@ -1011,6 +3131,31 @@ var _ = Describe("DriveItemPermissionsService", func() { Expect(err).To(MatchError(errorcode.New(errorcode.InvalidRequest, "permissionID and itemID do not match"))) Expect(res).To(BeZero()) }) + It("logs a permission and item id mismatch as debug with the item and permission ids", func() { + service, logBuf := newLoggedService() + + // the link belongs to another item than the one in the request + getPublicShareMockResponse.Share.ResourceId = &provider.ResourceId{StorageId: "1", SpaceId: "2", OpaqueId: "4"} + gatewayClient.On("GetPublicShare", mock.Anything, mock.Anything).Return(getPublicShareMockResponse, nil) + + driveItemPermission.SetExpirationDateTime(time.Now().Add(time.Hour)) + _, err := service.UpdatePermission(ctx, driveItemId, "permissionid", driveItemPermission) + Expect(err).To(MatchError(errorcode.New(errorcode.InvalidRequest, "permissionID and itemID do not match"))) + // nothing is updated after the mismatch + for _, method := range []string{"Stat", "UpdatePublicShare", "UpdateShare", "UpdateOCMShare"} { + gatewayClient.AssertNotCalled(GinkgoT(), method, mock.Anything, mock.Anything) + } + + entries := logEntries(logBuf) + var mismatchEntries []map[string]any + for _, entry := range entries { + if entry["level"] == "debug" && entry["itemID"] == storagespace.FormatResourceID(driveItemId) && entry["permissionID"] == "permissionid" { + mismatchEntries = append(mismatchEntries, entry) + } + } + Expect(mismatchEntries).To(HaveLen(1)) + Expect(entries).ToNot(ContainElement(HaveKeyWithValue("level", BeElementOf("error", "warn")))) + }) It("succeeds when trying to update a link permission with displayname", func() { gatewayClient.On("GetPublicShare", mock.Anything, mock.MatchedBy(func(req *link.GetPublicShareRequest) bool { return req.GetRef().GetId().GetOpaqueId() == "permissionid" @@ -1039,6 +3184,46 @@ var _ = Describe("DriveItemPermissionsService", func() { Expect(res.Link).ToNot(BeNil()) Expect(res.Link.GetLibreGraphDisplayName() == TestLinkName) }) + It("records an UpdateSpaceRootPermission span as the parent of the UpdatePermission span", func() { + sr, tp := newSpanRecorder() + service, err := svc.NewDriveItemPermissionsService(log.NewLogger(), gatewaySelector, cache, cfg, tp) + Expect(err).ToNot(HaveOccurred()) + + spaceRoot := &provider.ResourceId{StorageId: "1", SpaceId: "2", OpaqueId: "2"} + listSpacesResponse.StorageSpaces[0].SpaceType = "project" + listSpacesResponse.StorageSpaces[0].Root = spaceRoot + getPublicShareMockResponse.Share.ResourceId = spaceRoot + statResponse.Info.Id = spaceRoot + gatewayClient.On("ListStorageSpaces", mock.Anything, mock.Anything).Return(listSpacesResponse, nil) + gatewayClient.On("GetPublicShare", mock.Anything, mock.Anything).Return(getPublicShareMockResponse, nil) + gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(statResponse, nil) + gatewayClient.On("UpdatePublicShare", mock.Anything, mock.Anything).Return(updatePublicShareMockResponse, nil) + + sharingLink := libregraph.NewSharingLink() + sharingLink.SetLibreGraphDisplayName(TestLinkName) + driveItemPermission.SetLink(*sharingLink) + res, err := service.UpdateSpaceRootPermission(context.Background(), &provider.ResourceId{StorageId: "1", SpaceId: "2"}, "permissionid", driveItemPermission) + Expect(err).ToNot(HaveOccurred()) + // the space was resolved and UpdatePermission updated the link on the space root + Expect(res.Link).ToNot(BeNil()) + Expect(res.Link.GetLibreGraphDisplayName()).To(Equal(TestLinkName)) + gatewayClient.AssertCalled(GinkgoT(), "UpdatePublicShare", mock.Anything, mock.MatchedBy(func(req *link.UpdatePublicShareRequest) bool { + return req.GetRef().GetId().GetOpaqueId() == "permissionid" && req.GetUpdate().GetDisplayName() == TestLinkName + })) + + spans := map[string]sdktrace.ReadOnlySpan{} + for _, s := range sr.Ended() { + spans[s.Name()] = s + } + Expect(spans).To(HaveKey("UpdateSpaceRootPermission")) + Expect(spans).To(HaveKey("UpdatePermission")) + parent := spans["UpdateSpaceRootPermission"].SpanContext() + child := spans["UpdatePermission"] + Expect(parent.IsValid()).To(BeTrue()) + Expect(child.SpanContext().IsValid()).To(BeTrue()) + Expect(child.Parent().SpanID()).To(Equal(parent.SpanID())) + Expect(child.SpanContext().TraceID()).To(Equal(parent.TraceID())) + }) It("updates the expiration date", func() { gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(statResponse, nil) gatewayClient.On("GetUser", mock.Anything, mock.Anything).Return(getUserResponse, nil) @@ -1328,6 +3513,81 @@ var _ = Describe("DriveItemPermissionsApi", func() { } }) + DescribeTable("logs link password service failures by cause with the resource and permission ids", + func(spaceRoot bool, serviceErr error, expectedStatus int, level string) { + var logBuf bytes.Buffer + api, err := svc.NewDriveItemPermissionsApi(mockProvider, log.Logger{Logger: zerolog.New(&logBuf)}, defaults.FullDefaultConfig()) + Expect(err).ToNot(HaveOccurred()) + + rCTX.URLParams.Add("permissionID", "permissionid") + handler, method, ids := api.SetLinkPassword, "SetPublicLinkPassword", map[string]string{"itemID": "1$2!3"} + if spaceRoot { + handler, method, ids = api.SetSpaceRootLinkPassword, "SetPublicLinkPasswordOnSpaceRoot", map[string]string{"driveID": "1$2"} + } else { + rCTX.URLParams.Add("itemID", "1$2!3") + } + mockProvider.On(method, mock.Anything, mock.Anything, "permissionid", "s3cr3t-P4ss!"). + Return(libregraph.Permission{}, serviceErr).Once() + + responseRecorder := httptest.NewRecorder() + request := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(`{"password":"s3cr3t-P4ss!"}`)). + WithContext(context.WithValue(context.WithValue(context.Background(), chi.RouteCtxKey, rCTX), middleware.RequestIDKey, "request-42")) + request.Header.Set("Authorization", "Bearer secret-token-xyz") + handler(responseRecorder, request) + + // the HTTP response is the rendered service error, as before + Expect(responseRecorder.Code).To(Equal(expectedStatus)) + graphErr, ok := errorcode.ToError(serviceErr) + Expect(ok).To(BeTrue()) + jsonData := gjson.Get(responseRecorder.Body.String(), "error") + Expect(jsonData.Get("code").String()).To(Equal(graphErr.GetCode().String())) + Expect(jsonData.Get("message").String()).To(Equal(strings.TrimPrefix(serviceErr.Error(), graphErr.GetCode().String()+": "))) + // the service operation was attempted with the parsed ids and password + mockProvider.AssertExpectations(GinkgoT()) + + entries := logEntries(&logBuf) + var failureEntries []map[string]any + for _, entry := range entries { + matches := entry["level"] == level && entry["permissionID"] == "permissionid" + for k, v := range ids { + matches = matches && entry[k] == v + } + if matches { + failureEntries = append(failureEntries, entry) + } + } + Expect(failureEntries).To(HaveLen(1)) + // the failure is logged with its error code and error text, whatever its level + Expect(failureEntries[0]).To(SatisfyAll( + HaveKeyWithValue("request-id", "request-42"), + HaveKeyWithValue("errorCode", graphErr.GetCode().String()), + HaveKeyWithValue("error", serviceErr.Error()), + )) + if level == "error" { + Expect(entriesAtLevel(entries, "error")).To(HaveLen(1)) + } else { + Expect(entries).ToNot(ContainElement(HaveKeyWithValue("level", BeElementOf("error", "warn")))) + } + for _, payloadKey := range []string{"password", "Body", "body", "request", "response", "token", "Authorization"} { + Expect(entries).ToNot(ContainElement(HaveKey(payloadKey))) + } + // secrets are never logged: neither the password nor the bearer token + raw, err := json.Marshal(entries) + Expect(err).ToNot(HaveOccurred()) + for _, secret := range []string{"s3cr3t-P4ss!", "secret-token-xyz"} { + Expect(string(raw)).ToNot(ContainSubstring(secret)) + } + }, + Entry("SetLinkPassword: internal error", false, + errorcode.New(errorcode.GeneralException, "link manager unavailable"), http.StatusInternalServerError, "error"), + Entry("SetLinkPassword: link not found", false, + errorcode.New(errorcode.ItemNotFound, "link not found"), http.StatusNotFound, "debug"), + Entry("SetSpaceRootLinkPassword: internal error", true, + errorcode.New(errorcode.GeneralException, "link manager unavailable"), http.StatusInternalServerError, "error"), + Entry("SetSpaceRootLinkPassword: link not found", true, + errorcode.New(errorcode.ItemNotFound, "link not found"), http.StatusNotFound, "debug"), + ) + checkDriveIDAndItemIDValidation := func(handler http.HandlerFunc) { rCTX.URLParams.Add("itemID", "3$4!5") @@ -1494,6 +3754,175 @@ var _ = Describe("DriveItemPermissionsApi", func() { Expect(responseRecorder.Code).To(Equal(http.StatusBadRequest)) }) }) + + Describe("GetPermission", func() { + get := func() *httptest.ResponseRecorder { + responseRecorder := httptest.NewRecorder() + request := httptest.NewRequest(http.MethodGet, "/", nil). + WithContext(context.WithValue(context.Background(), chi.RouteCtxKey, rCTX)) + httpAPI.GetPermission(responseRecorder, request) + return responseRecorder + } + + It("passes the item and permission ID to the provider and renders the permission", func() { + rCTX.URLParams.Add("itemID", "1$2!3") + rCTX.URLParams.Add("permissionID", "perm%2Fid") + mockProvider.On("GetPermission", mock.Anything, mock.Anything, mock.Anything). + Return(func(ctx context.Context, itemID *provider.ResourceId, permissionID string) (libregraph.Permission, error) { + Expect(storagespace.FormatResourceID(itemID)).To(Equal("1$2!3")) + Expect(permissionID).To(Equal("perm/id")) + return libregraph.Permission{Id: libregraph.PtrString("perm/id")}, nil + }).Once() + + rr := get() + Expect(rr.Code).To(Equal(http.StatusOK)) + Expect(gjson.Get(rr.Body.String(), "id").String()).To(Equal("perm/id")) + }) + + It("rejects an itemID that does not belong to the drive", func() { + rCTX.URLParams.Add("itemID", "3$4!5") + rCTX.URLParams.Add("permissionID", "permid") + Expect(get().Code).To(Equal(http.StatusNotFound)) + }) + + It("rejects an empty permissionID", func() { + rCTX.URLParams.Add("itemID", "1$2!3") + Expect(get().Code).To(Equal(http.StatusBadRequest)) + }) + + It("renders a not found error of the provider", func() { + rCTX.URLParams.Add("itemID", "1$2!3") + rCTX.URLParams.Add("permissionID", "permid") + mockProvider.On("GetPermission", mock.Anything, mock.Anything, mock.Anything). + Return(libregraph.Permission{}, errorcode.New(errorcode.ItemNotFound, "permission not found")).Once() + + Expect(get().Code).To(Equal(http.StatusNotFound)) + }) + }) + + Describe("GetSpaceRootPermission", func() { + It("rejects an invalid driveID", func() { + rCTX = chi.NewRouteContext() + rCTX.URLParams.Add("permissionID", "permid") + responseRecorder := httptest.NewRecorder() + request := httptest.NewRequest(http.MethodGet, "/", nil). + WithContext(context.WithValue(context.Background(), chi.RouteCtxKey, rCTX)) + httpAPI.GetSpaceRootPermission(responseRecorder, request) + + Expect(responseRecorder.Code).To(Equal(http.StatusUnprocessableEntity)) + }) + }) +}) + +var _ = Describe("DriveItemPermissions routes", func() { + var ( + graphService svc.Service + gatewayClient *cs3mocks.GatewayAPIClient + ctx context.Context + rr *httptest.ResponseRecorder + ) + + BeforeEach(func() { + ctx = revactx.ContextSetUser(context.Background(), &userpb.User{Id: &userpb.UserId{OpaqueId: "user"}}) + rr = httptest.NewRecorder() + + cfg := defaults.FullDefaultConfig() + cfg.Identity.LDAP.CACert = "" + cfg.TokenManager.JWTSecret = "loremipsum" + cfg.Commons = &shared.Commons{} + cfg.GRPCClientTLS = &shared.GRPCClientTLS{} + + pool.RemoveSelector("GatewaySelector" + "eu.opencloud.api.gateway") + gatewayClient = &cs3mocks.GatewayAPIClient{} + gatewaySelector := pool.GetSelector[gateway.GatewayAPIClient]( + "GatewaySelector", + "eu.opencloud.api.gateway", + func(cc grpc.ClientConnInterface) gateway.GatewayAPIClient { + return gatewayClient + }, + ) + + logger := log.NewLogger() + m, _ := metrics.New(prometheus.NewRegistry(), &logger, func([]string) (string, string) { return "", "" }) + + var err error + graphService, err = svc.NewService( + svc.Config(cfg), + svc.Metrics(m), + svc.WithGatewaySelector(gatewaySelector), + svc.EventsPublisher(&mocks.Publisher{}), + ) + Expect(err).ToNot(HaveOccurred()) + }) + + It("GET .../items/{itemID}/permissions/{permissionID} returns the matching permission", func() { + itemID := &provider.ResourceId{StorageId: "1", SpaceId: "2", OpaqueId: "3"} + gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(&provider.StatResponse{ + Status: status.NewOK(ctx), + Info: &provider.ResourceInfo{ + Id: itemID, + Type: provider.ResourceType_RESOURCE_TYPE_CONTAINER, + PermissionSet: roleconversions.NewEditorRole().CS3ResourcePermissions(), + }, + }, nil) + gatewayClient.On("ListShares", mock.Anything, mock.Anything).Return(&collaboration.ListSharesResponse{Status: status.NewOK(ctx)}, nil) + viewLink := func(id string) *link.PublicShare { + return &link.PublicShare{ + Id: &link.PublicShareId{OpaqueId: id}, + Token: id, + ResourceId: itemID, + Permissions: &link.PublicSharePermissions{Permissions: roleconversions.NewViewerRole().CS3ResourcePermissions()}, + Ctime: &types.Timestamp{Seconds: uint64(time.Now().Unix())}, + } + } + gatewayClient.On("ListPublicShares", mock.Anything, mock.Anything).Return(&link.ListPublicSharesResponse{ + Status: status.NewOK(ctx), + Share: []*link.PublicShare{viewLink("aaaaaaaaaaaaaaa"), viewLink("bbbbbbbbbbbbbbb")}, + }, nil) + + r := httptest.NewRequest(http.MethodGet, "/graph/v1beta1/drives/1$2/items/1$2!3/permissions/bbbbbbbbbbbbbbb", nil).WithContext(ctx) + graphService.ServeHTTP(rr, r) + + Expect(rr.Code).To(Equal(http.StatusOK)) + Expect(gjson.Get(rr.Body.String(), "id").String()).To(Equal("bbbbbbbbbbbbbbb")) + Expect(gjson.Get(rr.Body.String(), "link.type").String()).To(Equal("view")) + }) + + It("GET .../root/permissions/{permissionID} returns the matching permission of a project space", func() { + rootID := &provider.ResourceId{StorageId: "1", SpaceId: "2", OpaqueId: "2"} + gatewayClient.On("ListStorageSpaces", mock.Anything, mock.Anything).Return(&provider.ListStorageSpacesResponse{ + Status: status.NewOK(ctx), + StorageSpaces: []*provider.StorageSpace{{ + Id: &provider.StorageSpaceId{OpaqueId: "1$2"}, + SpaceType: "project", + Root: rootID, + }}, + }, nil) + gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(&provider.StatResponse{ + Status: status.NewOK(ctx), + Info: &provider.ResourceInfo{ + Id: rootID, + Type: provider.ResourceType_RESOURCE_TYPE_CONTAINER, + PermissionSet: roleconversions.NewManagerRole().CS3ResourcePermissions(), + }, + }, nil) + gatewayClient.On("ListShares", mock.Anything, mock.Anything).Return(&collaboration.ListSharesResponse{Status: status.NewOK(ctx)}, nil) + gatewayClient.On("ListPublicShares", mock.Anything, mock.Anything).Return(&link.ListPublicSharesResponse{ + Status: status.NewOK(ctx), + Share: []*link.PublicShare{{ + Id: &link.PublicShareId{OpaqueId: "aaaaaaaaaaaaaaa"}, + Token: "aaaaaaaaaaaaaaa", + ResourceId: rootID, + Permissions: &link.PublicSharePermissions{Permissions: roleconversions.NewViewerRole().CS3ResourcePermissions()}, + }}, + }, nil) + + r := httptest.NewRequest(http.MethodGet, "/graph/v1beta1/drives/1$2/root/permissions/aaaaaaaaaaaaaaa", nil).WithContext(ctx) + graphService.ServeHTTP(rr, r) + + Expect(rr.Code).To(Equal(http.StatusOK)) + Expect(gjson.Get(rr.Body.String(), "id").String()).To(Equal("aaaaaaaaaaaaaaa")) + }) }) func guestGrantee(mail string) *provider.Grantee { diff --git a/services/graph/pkg/service/v0/base.go b/services/graph/pkg/service/v0/base.go index cc25d30da1..7571c64627 100644 --- a/services/graph/pkg/service/v0/base.go +++ b/services/graph/pkg/service/v0/base.go @@ -267,7 +267,8 @@ func (g BaseGraphService) listSpaceRootUserShares(ctx context.Context, filters [ func (g BaseGraphService) listSharesWithSpaceRootFilter(ctx context.Context, spaceRoot bool, filters []*collaboration.Filter, driveItems driveItemsByResourceID) (driveItemsByResourceID, error) { gatewayClient, err := g.gatewaySelector.Next() if err != nil { - g.logger.Error().Err(err).Msg("could not select next gateway client") + logger := g.logger.SubloggerWithRequestID(ctx) + logger.Error().Err(err).Msg("could not select next gateway client") return driveItems, errorcode.New(errorcode.GeneralException, err.Error()) } @@ -299,7 +300,8 @@ func (g BaseGraphService) listSharesWithSpaceRootFilter(ctx context.Context, spa func (g BaseGraphService) listOCMShares(ctx context.Context, filters []*ocm.ListOCMSharesRequest_Filter, driveItems driveItemsByResourceID) (driveItemsByResourceID, error) { gatewayClient, err := g.gatewaySelector.Next() if err != nil { - g.logger.Error().Err(err).Msg("could not select next gateway client") + logger := g.logger.SubloggerWithRequestID(ctx) + logger.Error().Err(err).Msg("could not select next gateway client") return driveItems, errorcode.New(errorcode.GeneralException, err.Error()) } @@ -328,7 +330,8 @@ func (g BaseGraphService) listPublicShares(ctx context.Context, filters []*link. gatewayClient, err := g.gatewaySelector.Next() if err != nil { - g.logger.Error().Err(err).Msg("could not select next gateway client") + logger := g.logger.SubloggerWithRequestID(ctx) + logger.Error().Err(err).Msg("could not select next gateway client") return driveItems, errorcode.New(errorcode.GeneralException, err.Error()) } @@ -815,7 +818,8 @@ func (g BaseGraphService) getLinkPermissionResourceID(ctx context.Context, permi func (g BaseGraphService) getCS3PublicShareByID(ctx context.Context, permissionID string) (*link.PublicShare, error) { gatewayClient, err := g.gatewaySelector.Next() if err != nil { - g.logger.Debug().Err(err).Msg("selecting gatewaySelector failed") + logger := g.logger.SubloggerWithRequestID(ctx) + logger.Error().Err(err).Str(_logKeyPermissionID, permissionID).Msg("selecting gatewaySelector failed") return nil, err } @@ -840,7 +844,8 @@ func (g BaseGraphService) getCS3PublicShareByID(ctx context.Context, permissionI func (g BaseGraphService) removeOCMPermission(ctx context.Context, permissionID string) error { gatewayClient, err := g.gatewaySelector.Next() if err != nil { - g.logger.Debug().Err(err).Msg("selecting gatewaySelector failed") + logger := g.logger.SubloggerWithRequestID(ctx) + logger.Error().Err(err).Str(_logKeyPermissionID, permissionID).Msg("selecting gatewaySelector failed") return err } @@ -866,7 +871,8 @@ func (g BaseGraphService) removeOCMPermission(ctx context.Context, permissionID func (g BaseGraphService) removePublicShare(ctx context.Context, permissionID string) error { gatewayClient, err := g.gatewaySelector.Next() if err != nil { - g.logger.Debug().Err(err).Msg("selecting gatewaySelector failed") + logger := g.logger.SubloggerWithRequestID(ctx) + logger.Error().Err(err).Str(_logKeyPermissionID, permissionID).Msg("selecting gatewaySelector failed") return err } @@ -892,7 +898,8 @@ func (g BaseGraphService) removePublicShare(ctx context.Context, permissionID st func (g BaseGraphService) removeUserShare(ctx context.Context, permissionID string) error { gatewayClient, err := g.gatewaySelector.Next() if err != nil { - g.logger.Debug().Err(err).Msg("selecting gatewaySelector failed") + logger := g.logger.SubloggerWithRequestID(ctx) + logger.Error().Err(err).Str(_logKeyPermissionID, permissionID).Msg("selecting gatewaySelector failed") return err } @@ -927,7 +934,8 @@ func (g BaseGraphService) getOCMPermissionResourceID(ctx context.Context, permis func (g BaseGraphService) getCS3OCMShareByID(ctx context.Context, permissionID string) (*ocm.Share, error) { gatewayClient, err := g.gatewaySelector.Next() if err != nil { - g.logger.Debug().Err(err).Msg("selecting gatewaySelector failed") + logger := g.logger.SubloggerWithRequestID(ctx) + logger.Error().Err(err).Str(_logKeyPermissionID, permissionID).Msg("selecting gatewaySelector failed") return nil, err } @@ -961,7 +969,8 @@ func (g BaseGraphService) getUserPermissionResourceID(ctx context.Context, permi func (g BaseGraphService) getCS3UserShareByID(ctx context.Context, permissionID string) (*collaboration.Share, error) { gatewayClient, err := g.gatewaySelector.Next() if err != nil { - g.logger.Debug().Err(err).Msg("selecting gatewaySelector failed") + logger := g.logger.SubloggerWithRequestID(ctx) + logger.Error().Err(err).Str(_logKeyPermissionID, permissionID).Msg("selecting gatewaySelector failed") return nil, err } @@ -986,7 +995,8 @@ func (g BaseGraphService) getCS3UserShareByID(ctx context.Context, permissionID func (g BaseGraphService) getOCMPermissionByID(ctx context.Context, permissionID string, itemID *storageprovider.ResourceId) (*libregraph.Permission, *storageprovider.ResourceId, error) { gatewayClient, err := g.gatewaySelector.Next() if err != nil { - g.logger.Debug().Err(err).Msg("selecting gatewaySelevtor failed") + logger := g.logger.SubloggerWithRequestID(ctx) + logger.Error().Err(err).Str(_logKeyPermissionID, permissionID).Msg("selecting gatewaySelector failed") return nil, nil, err } @@ -1016,7 +1026,8 @@ func (g BaseGraphService) getOCMPermissionByID(ctx context.Context, permissionID func (g BaseGraphService) getPermissionByID(ctx context.Context, permissionID string, itemID *storageprovider.ResourceId) (*libregraph.Permission, *storageprovider.ResourceId, error) { gatewayClient, err := g.gatewaySelector.Next() if err != nil { - g.logger.Debug().Err(err).Msg("selecting gatewaySelector failed") + logger := g.logger.SubloggerWithRequestID(ctx) + logger.Error().Err(err).Str(_logKeyPermissionID, permissionID).Msg("selecting gatewaySelector failed") return nil, nil, err } publicShare, err := g.getCS3PublicShareByID(ctx, permissionID) @@ -1075,7 +1086,8 @@ func (g BaseGraphService) getPermissionByID(ctx context.Context, permissionID st func (g BaseGraphService) updateOCMPermission(ctx context.Context, permissionID string, itemID *storageprovider.ResourceId, newPermission *libregraph.Permission) (*libregraph.Permission, error) { gatewayClient, err := g.gatewaySelector.Next() if err != nil { - g.logger.Debug().Err(err).Msg("selecting gatewaySelector failed") + logger := g.logger.SubloggerWithRequestID(ctx) + logger.Error().Err(err).Str(_logKeyPermissionID, permissionID).Msg("selecting gatewaySelector failed") return nil, err } @@ -1181,7 +1193,8 @@ func (g BaseGraphService) updateOCMPermission(ctx context.Context, permissionID func (g BaseGraphService) updateUserShare(ctx context.Context, permissionID string, itemID *storageprovider.ResourceId, newPermission *libregraph.Permission) (*libregraph.Permission, error) { gatewayClient, err := g.gatewaySelector.Next() if err != nil { - g.logger.Debug().Err(err).Msg("selecting gatewaySelector failed") + logger := g.logger.SubloggerWithRequestID(ctx) + logger.Error().Err(err).Str(_logKeyPermissionID, permissionID).Msg("selecting gatewaySelector failed") return nil, err } diff --git a/services/graph/pkg/service/v0/driveitems.go b/services/graph/pkg/service/v0/driveitems.go index d20e8cf53f..9d35a437e0 100644 --- a/services/graph/pkg/service/v0/driveitems.go +++ b/services/graph/pkg/service/v0/driveitems.go @@ -493,9 +493,12 @@ func cs3ResourceToDriveItem(logger *log.Logger, publicBaseURL *url.URL, res *sto webURL.Path = path.Join(webURL.Path, "f", storagespace.FormatResourceID(res.GetId())) driveItem.WebUrl = libregraph.PtrString(webURL.String()) - if name := path.Base(res.GetPath()); name != "" { - driveItem.Name = &name + name := res.GetName() + if name == "" { + name = path.Base(res.GetPath()) } + driveItem.Name = &name + if res.GetEtag() != "" { driveItem.ETag = &res.Etag } diff --git a/services/graph/pkg/service/v0/drives.go b/services/graph/pkg/service/v0/drives.go index 954cd0791d..2351e4a6af 100644 --- a/services/graph/pkg/service/v0/drives.go +++ b/services/graph/pkg/service/v0/drives.go @@ -12,6 +12,7 @@ import ( "sort" "strconv" "strings" + "unicode" "github.com/CiscoM31/godata" gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" @@ -20,6 +21,7 @@ import ( storageprovider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" types "github.com/cs3org/go-cs3apis/cs3/types/v1beta1" "github.com/go-chi/render" + "github.com/google/uuid" libregraph "github.com/opencloud-eu/libre-graph-api-go" revactx "github.com/opencloud-eu/reva/v2/pkg/ctx" "github.com/opencloud-eu/reva/v2/pkg/storagespace" @@ -403,7 +405,7 @@ func (g Graph) CreateDrive(w http.ResponseWriter, r *http.Request) { errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "invalid body schema definition") return } - spaceName := strings.TrimSpace(drive.Name) + spaceName := trimSpaceName(drive.Name) if err := validateSpaceName(spaceName); err != nil { log.Debug().Str("name", spaceName).Err(err).Msg("could not create drive: name validation failed") errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, fmt.Sprintf("invalid spacename: %s", err.Error())) @@ -429,6 +431,28 @@ func (g Graph) CreateDrive(w http.ResponseWriter, r *http.Request) { Quota: getQuota(drive.Quota, g.config.Spaces.DefaultQuota), } + if id := drive.GetId(); id != "" { + // the id ends up in storage paths and in "storageid$spaceid!opaqueid" references, + // so only accept the canonical form of the uuids the storage drivers generate themselves + if u, err := uuid.Parse(id); err != nil || u.String() != id { + log.Debug().Str("id", id).Msg("could not create drive: id is not a canonical uuid") + errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "invalid drive id: must be a canonical UUID") + return + } + // personal spaces may use their owner's user id as space id, so an id that belongs to a user + // must not be taken by another space, or that user's personal space could not be created anymore + if _, err := g.identityBackend.GetUser(r.Context(), id, &godata.GoDataRequest{}); err == nil { + log.Debug().Str("id", id).Msg("could not create drive: id belongs to a user") + errorcode.NameAlreadyExists.Render(w, r, http.StatusConflict, "drive already exists") + return + } else if !errorcode.IsErrorCode(err, errorcode.ItemNotFound) { + log.Error().Err(err).Str("id", id).Msg("could not create drive: could not check whether the id belongs to a user") + errorcode.RenderError(w, r, err) + return + } + csr.Opaque = utils.AppendPlainToOpaque(csr.Opaque, "spaceid", id) + } + if drive.Description != nil { csr.Opaque = utils.AppendPlainToOpaque(csr.Opaque, "description", *drive.Description) } @@ -463,6 +487,11 @@ func (g Graph) CreateDrive(w http.ResponseWriter, r *http.Request) { errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, resp.GetStatus().GetMessage()) return } + if resp.GetStatus().GetCode() == cs3rpc.Code_CODE_ALREADY_EXISTS { + log.Debug().Str("grpcmessage", resp.GetStatus().GetMessage()).Msg("could not create drive: already exists") + errorcode.NameAlreadyExists.Render(w, r, http.StatusConflict, "drive already exists") + return + } log.Debug().Interface("grpcmessage", csr).Str("grpc", resp.GetStatus().GetMessage()).Msg("could not create drive: grpc error") errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, resp.GetStatus().GetMessage()) return @@ -580,8 +609,8 @@ func (g Graph) UpdateDrive(w http.ResponseWriter, r *http.Request) { } } - if drive.GetName() != "" { - spacename := strings.TrimSpace(drive.GetName()) + if _, ok := drive.GetNameOk(); ok { + spacename := trimSpaceName(drive.GetName()) if err := validateSpaceName(spacename); err != nil { log.Info().Err(err).Msg("could not update drive: spacename invalid") errorcode.GeneralException.Render(w, r, http.StatusBadRequest, err.Error()) @@ -1178,6 +1207,18 @@ func sortSpaces(req *godata.GoDataRequest, spaces []*libregraph.Drive) ([]*libre return spaces, nil } +// trimSpaceName removes whitespace and invisible formatting characters from both ends of a +// space name. strings.TrimSpace alone isn't enough: characters such as the zero-width space +// (U+200B), the word joiner (U+2060) or the byte order mark (U+FEFF) aren't whitespace in +// Unicode, but they are invisible, so a name made only of them would look empty in every client. +// Formatting characters inside a name are kept, since some of them are needed there, e.g. the +// zero-width joiner (U+200D) in emoji sequences. +func trimSpaceName(name string) string { + return strings.TrimFunc(name, func(r rune) bool { + return unicode.IsSpace(r) || unicode.Is(unicode.Cf, r) + }) +} + func validateSpaceName(name string) error { if name == "" { return ErrNameEmpty diff --git a/services/graph/pkg/service/v0/drives_test.go b/services/graph/pkg/service/v0/drives_test.go index cfd4fcc0e8..289eeb15c2 100644 --- a/services/graph/pkg/service/v0/drives_test.go +++ b/services/graph/pkg/service/v0/drives_test.go @@ -195,3 +195,31 @@ func TestSpaceNameValidation(t *testing.T) { // set max length back to protect other tests _maxSpaceNameLength = 255 } + +func TestTrimSpaceName(t *testing.T) { + // the names are written as escape sequences on purpose: written directly, these characters + // would be invisible or look like ordinary spaces in the source code + testCases := []struct { + Alias string + SpaceName string + Expected string + }{ + {"Unchanged", "Space", "Space"}, + {"Spaces around", " Space ", "Space"}, + {"Spaces inside are kept", "My Space", "My Space"}, + // U+00A0 no-break space, U+3000 ideographic space + {"Unicode spaces around", "\u00a0Space\u3000", "Space"}, + // U+200B zero-width space, U+FEFF byte order mark + {"Invisible characters around", "\u200bSpace\ufeff", "Space"}, + // U+2060 word joiner, U+2003 em space, U+200E left-to-right mark + {"Mixed around", " \u2060\u2003Space\u200e ", "Space"}, + // U+200D zero-width joiner, as in the emoji sequence of a family + {"Zero-width joiner inside is kept", "Team \U0001F468\u200d\U0001F469\u200d\U0001F467", "Team \U0001F468\u200d\U0001F469\u200d\U0001F467"}, + // U+200B zero-width space, U+200C zero-width non-joiner, U+00A0 no-break space + {"Only invisible characters", "\u200b\u200c\u00a0", ""}, + } + + for _, tc := range testCases { + require.Equal(t, tc.Expected, trimSpaceName(tc.SpaceName), tc.Alias) + } +} diff --git a/services/graph/pkg/service/v0/graph_test.go b/services/graph/pkg/service/v0/graph_test.go index 0b2d2a2d3f..8d0bff30a2 100644 --- a/services/graph/pkg/service/v0/graph_test.go +++ b/services/graph/pkg/service/v0/graph_test.go @@ -8,6 +8,7 @@ import ( "io" "net/http" "net/http/httptest" + "strings" "time" gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" @@ -28,6 +29,8 @@ import ( "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/mock" "github.com/tidwall/gjson" + sdktrace "go.opentelemetry.io/otel/sdk/trace" + "go.opentelemetry.io/otel/sdk/trace/tracetest" "google.golang.org/grpc" "github.com/opencloud-eu/opencloud/pkg/log" @@ -38,6 +41,8 @@ import ( "github.com/opencloud-eu/opencloud/services/graph/pkg/config" "github.com/opencloud-eu/opencloud/services/graph/pkg/config/defaults" "github.com/opencloud-eu/opencloud/services/graph/pkg/errorcode" + "github.com/opencloud-eu/opencloud/services/graph/pkg/identity" + identitymocks "github.com/opencloud-eu/opencloud/services/graph/pkg/identity/mocks" "github.com/opencloud-eu/opencloud/services/graph/pkg/metrics" service "github.com/opencloud-eu/opencloud/services/graph/pkg/service/v0" "github.com/opencloud-eu/opencloud/services/graph/pkg/unifiedrole" @@ -50,6 +55,7 @@ var _ = Describe("Graph", func() { gatewaySelector pool.Selectable[gateway.GatewayAPIClient] eventsPublisher mocks.Publisher permissionService mocks.Permissions + identityBackend *identitymocks.Backend ctx context.Context cfg *config.Config rr *httptest.ResponseRecorder @@ -86,6 +92,7 @@ var _ = Describe("Graph", func() { eventsPublisher = mocks.Publisher{} permissionService = mocks.Permissions{} + identityBackend = &identitymocks.Backend{} var err error svc, err = service.NewService( @@ -94,6 +101,7 @@ var _ = Describe("Graph", func() { service.WithGatewaySelector(gatewaySelector), service.EventsPublisher(&eventsPublisher), service.PermissionService(&permissionService), + service.WithIdentityBackend(identityBackend), ) Expect(err).ToNot(HaveOccurred()) }) @@ -104,6 +112,48 @@ var _ = Describe("Graph", func() { }) }) + Describe("permissions tracing", func() { + It("records a span for the permissions service when a TraceProvider is configured", func() { + sr := tracetest.NewSpanRecorder() + tp := sdktrace.NewTracerProvider(sdktrace.WithSpanProcessor(sr)) + DeferCleanup(func() { + Expect(tp.Shutdown(context.Background())).To(Succeed()) + }) + + logger := log.NewLogger() + m, err := metrics.New(prometheus.NewRegistry(), &logger, func([]string) (string, string) { return "", "" }) + Expect(err).ToNot(HaveOccurred()) + tracedSvc, err := service.NewService( + service.Config(cfg), + service.Metrics(m), + service.WithGatewaySelector(gatewaySelector), + service.EventsPublisher(&eventsPublisher), + service.PermissionService(&permissionService), + service.TraceProvider(tp), + ) + Expect(err).ToNot(HaveOccurred()) + + gatewayClient.On("ListStorageSpaces", mock.Anything, mock.Anything).Return(&provider.ListStorageSpacesResponse{ + Status: status.NewOK(ctx), + }, nil) + + r := httptest.NewRequest(http.MethodGet, "/graph/v1beta1/drives/1$2/root/permissions", nil).WithContext(ctx) + tracedSvc.ServeHTTP(rr, r) + + // the request was routed to the permissions service, which looked up the space + Expect(rr.Code).To(Equal(http.StatusNotFound)) + gatewayClient.AssertCalled(GinkgoT(), "ListStorageSpaces", mock.Anything, mock.Anything) + // http tracing works with this provider + Expect(sr.Ended()).ToNot(BeEmpty()) + + var names []string + for _, s := range sr.Ended() { + names = append(names, s.Name()) + } + Expect(names).To(ContainElement("ListSpaceRootPermissions")) + }) + }) + Describe("Drives", func() { Describe("GetDrivesV1 and GetAllDrivesV1", func() { BeforeEach(func() { @@ -843,6 +893,156 @@ var _ = Describe("Graph", func() { Expect(*response.Description).To(Equal("This space is for testing")) Expect(*response.LibreGraphContentType).To(Equal("application/vnd.opencloud.test")) }) + Context("drive id handling", func() { + const spaceID = "0b9a7b84-3e71-4a4e-9c34-0e6a4c3f5f21" + + // csr captures the request the handler sends to the gateway + var csr *provider.CreateStorageSpaceRequest + mockSuccessfulCreate := func(createdSpaceID string) { + gatewayClient.On("CreateStorageSpace", mock.Anything, mock.Anything).Run(func(args mock.Arguments) { + csr = args.Get(1).(*provider.CreateStorageSpaceRequest) + }).Return(&provider.CreateStorageSpaceResponse{ + Status: status.NewOK(ctx), + StorageSpace: &provider.StorageSpace{ + Id: &provider.StorageSpaceId{OpaqueId: "pro-1$" + createdSpaceID}, + Name: "Test Space", + SpaceType: "project", + Root: &provider.ResourceId{StorageId: "pro-1", SpaceId: createdSpaceID, OpaqueId: createdSpaceID}, + }, + }, nil) + gatewayClient.On("GetQuota", mock.Anything, mock.Anything).Return(&provider.GetQuotaResponse{ + Status: status.NewOK(ctx), + TotalBytes: 500, + }, nil) + } + + BeforeEach(func() { + csr = nil + permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Return(&settingssvc.GetPermissionByIDResponse{ + Permission: &v0.Permission{ + Operation: v0.Permission_OPERATION_READWRITE, + Constraint: v0.Permission_CONSTRAINT_ALL, + }, + }, nil) + }) + + DescribeTable("lets the storage provider generate the id when none is supplied, without looking up users", + func(jsonBody string) { + mockSuccessfulCreate("generated-id") + + r := httptest.NewRequest(http.MethodPost, "/graph/v1.0/drives", bytes.NewBufferString(jsonBody)).WithContext(ctx) + rr := httptest.NewRecorder() + svc.CreateDrive(rr, r) + Expect(rr.Code).To(Equal(http.StatusCreated)) + Expect(csr).ToNot(BeNil()) + Expect(csr.GetOpaque().GetMap()).ToNot(HaveKey("spaceid")) + identityBackend.AssertNotCalled(GinkgoT(), "GetUser", mock.Anything, mock.Anything, mock.Anything) + }, + Entry("absent id", `{"name": "Test Space"}`), + Entry("empty id", `{"name": "Test Space", "id": ""}`), + ) + + It("forwards the id to the storage provider when it does not belong to a user", func() { + identityBackend.On("GetUser", mock.Anything, spaceID, mock.Anything).Return(nil, identity.ErrNotFound) + mockSuccessfulCreate(spaceID) + + jsonBody := []byte(`{"name": "Test Space", "id": "` + spaceID + `"}`) + r := httptest.NewRequest(http.MethodPost, "/graph/v1.0/drives", bytes.NewBuffer(jsonBody)).WithContext(ctx) + rr := httptest.NewRecorder() + svc.CreateDrive(rr, r) + Expect(rr.Code).To(Equal(http.StatusCreated)) + Expect(utils.ReadPlainFromOpaque(csr.GetOpaque(), "spaceid")).To(Equal(spaceID)) + + var response libregraph.Drive + Expect(json.Unmarshal(rr.Body.Bytes(), &response)).To(Succeed()) + Expect(response.GetId()).To(Equal("pro-1$" + spaceID)) + identityBackend.AssertCalled(GinkgoT(), "GetUser", mock.Anything, spaceID, mock.Anything) + }) + + // In the next two tests a successful CreateStorageSpace is mocked on purpose: a handler that + // skips the user lookup then fails on the status and AssertNotCalled checks, not on an + // unexpected mock call. + It("returns 409 Conflict without creating the space when the id belongs to a user", func() { + identityBackend.On("GetUser", mock.Anything, spaceID, mock.Anything).Return(&libregraph.User{Id: libregraph.PtrString(spaceID)}, nil) + mockSuccessfulCreate(spaceID) + + jsonBody := []byte(`{"name": "Test Space", "id": "` + spaceID + `"}`) + r := httptest.NewRequest(http.MethodPost, "/graph/v1.0/drives", bytes.NewBuffer(jsonBody)).WithContext(ctx) + rr := httptest.NewRecorder() + svc.CreateDrive(rr, r) + Expect(rr.Code).To(Equal(http.StatusConflict)) + + var libreError libregraph.OdataError + Expect(json.Unmarshal(rr.Body.Bytes(), &libreError)).To(Succeed()) + Expect(libreError.Error.Code).To(Equal(errorcode.NameAlreadyExists.String())) + Expect(libreError.Error.Message).To(Equal("drive already exists")) + gatewayClient.AssertNotCalled(GinkgoT(), "CreateStorageSpace", mock.Anything, mock.Anything) + }) + + It("does not create the space when the identity backend fails to look up the id", func() { + identityBackend.On("GetUser", mock.Anything, spaceID, mock.Anything).Return(nil, errorcode.New(errorcode.ServiceNotAvailable, "identity backend unavailable")) + mockSuccessfulCreate(spaceID) + + jsonBody := []byte(`{"name": "Test Space", "id": "` + spaceID + `"}`) + r := httptest.NewRequest(http.MethodPost, "/graph/v1.0/drives", bytes.NewBuffer(jsonBody)).WithContext(ctx) + rr := httptest.NewRecorder() + svc.CreateDrive(rr, r) + Expect(rr.Code).To(Equal(http.StatusInternalServerError)) + + var libreError libregraph.OdataError + Expect(json.Unmarshal(rr.Body.Bytes(), &libreError)).To(Succeed()) + Expect(libreError.Error.Code).To(Equal(errorcode.ServiceNotAvailable.String())) + gatewayClient.AssertNotCalled(GinkgoT(), "CreateStorageSpace", mock.Anything, mock.Anything) + }) + + DescribeTable("rejects ids that are not canonical UUIDs before looking up users or calling the storage provider", + func(id string) { + gatewayClient.On("CreateStorageSpace", mock.Anything, mock.Anything).Return(&provider.CreateStorageSpaceResponse{ + Status: status.NewInternal(ctx, "storage provider must not be called"), + }, nil).Maybe() + + body, err := json.Marshal(map[string]string{"name": "Test Space", "id": id}) + Expect(err).ToNot(HaveOccurred()) + r := httptest.NewRequest(http.MethodPost, "/graph/v1.0/drives", bytes.NewBuffer(body)).WithContext(ctx) + rr := httptest.NewRecorder() + svc.CreateDrive(rr, r) + Expect(rr.Code).To(Equal(http.StatusBadRequest)) + + var libreError libregraph.OdataError + Expect(json.Unmarshal(rr.Body.Bytes(), &libreError)).To(Succeed()) + Expect(libreError.Error.Code).To(Equal(errorcode.InvalidRequest.String())) + Expect(libreError.Error.Message).To(Equal("invalid drive id: must be a canonical UUID")) + identityBackend.AssertNotCalled(GinkgoT(), "GetUser", mock.Anything, mock.Anything, mock.Anything) + gatewayClient.AssertNotCalled(GinkgoT(), "CreateStorageSpace", mock.Anything, mock.Anything) + }, + Entry("path traversal", "../../x"), + Entry("storage id delimiter", "pro-1$"+spaceID), + Entry("opaque id delimiter", spaceID+"!"+spaceID), + Entry("arbitrary string", "not-a-uuid"), + Entry("uppercase", strings.ToUpper(spaceID)), + Entry("braces", "{"+spaceID+"}"), + Entry("urn", "urn:uuid:"+spaceID), + Entry("no hyphens", strings.ReplaceAll(spaceID, "-", "")), + ) + + It("returns 409 Conflict when the storage provider reports that the space already exists", func() { + identityBackend.On("GetUser", mock.Anything, spaceID, mock.Anything).Return(nil, identity.ErrNotFound) + gatewayClient.On("CreateStorageSpace", mock.Anything, mock.Anything).Return(&provider.CreateStorageSpaceResponse{ + Status: status.NewAlreadyExists(ctx, nil, "decomposedfs: spaces: space already exists"), + }, nil) + + jsonBody := []byte(`{"name": "Test Space", "id": "` + spaceID + `"}`) + r := httptest.NewRequest(http.MethodPost, "/graph/v1.0/drives", bytes.NewBuffer(jsonBody)).WithContext(ctx) + rr := httptest.NewRecorder() + svc.CreateDrive(rr, r) + Expect(rr.Code).To(Equal(http.StatusConflict)) + + var libreError libregraph.OdataError + Expect(json.Unmarshal(rr.Body.Bytes(), &libreError)).To(Succeed()) + Expect(libreError.Error.Code).To(Equal(errorcode.NameAlreadyExists.String())) + Expect(libreError.Error.Message).To(Equal("drive already exists")) + }) + }) It("Incomplete space", func() { gatewayClient.On("CreateStorageSpace", mock.Anything, mock.Anything).Return(&provider.CreateStorageSpaceResponse{ Status: status.NewOK(ctx), @@ -1177,6 +1377,67 @@ var _ = Describe("Graph", func() { })) }) + DescribeTable("rejects an invalid name without updating the space", + func(name string) { + body, err := json.Marshal(map[string]string{"name": name}) + Expect(err).ToNot(HaveOccurred()) + r := httptest.NewRequest(http.MethodPatch, "/graph/v1.0/drives/{driveID}/", bytes.NewBuffer(body)) + rctx := chi.NewRouteContext() + rctx.URLParams.Add("driveID", "spaceid") + r = r.WithContext(context.WithValue(revactx.ContextSetUser(ctx, nil), chi.RouteCtxKey, rctx)) + svc.UpdateDrive(rr, r) + Expect(rr.Code).To(Equal(http.StatusBadRequest)) + + gatewayClient.AssertNotCalled(GinkgoT(), "UpdateStorageSpace", mock.Anything, mock.Anything) + }, + // the names are written as escape sequences on purpose: written directly, these + // characters would be invisible or look like ordinary spaces in the source code + Entry("empty name", ""), + Entry("name of only spaces", " "), + Entry("name of only tabs and line breaks", "\t\n\r"), + // U+00A0 no-break space, U+202F narrow no-break space + Entry("name of only no-break spaces", "\u00a0\u202f"), + // U+2003 em space, U+2009 thin space, U+3000 ideographic space + Entry("name of only wide spaces", "\u2003\u2009\u3000"), + // U+200B zero-width space: invisible, but not whitespace in Unicode + Entry("name of only zero-width spaces", "\u200b\u200b"), + // U+200C zero-width non-joiner, U+200D zero-width joiner + Entry("name of only zero-width joiners", "\u200c\u200d"), + // U+2060 word joiner, U+FEFF byte order mark (zero-width no-break space) + Entry("name of only a word joiner and a byte order mark", "\u2060\ufeff"), + // space, U+200B zero-width space, U+00A0 no-break space, U+FEFF byte order mark, space + Entry("name of mixed invisible characters", " \u200b\u00a0\ufeff "), + ) + + DescribeTable("keeps the name when the request doesn't set one", + func(jsonBody string) { + gatewayClient.On("UpdateStorageSpace", mock.Anything, mock.Anything).Return(func(_ context.Context, req *provider.UpdateStorageSpaceRequest, _ ...grpc.CallOption) *provider.UpdateStorageSpaceResponse { + return &provider.UpdateStorageSpaceResponse{ + Status: status.NewOK(ctx), + StorageSpace: req.StorageSpace, + } + }, nil) + gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(&provider.StatResponse{ + Status: status.NewNotFound(ctx, "no special files found"), + }, nil).Maybe() + + r := httptest.NewRequest(http.MethodPatch, "/graph/v1.0/drives/{driveID}/", bytes.NewBufferString(jsonBody)) + rctx := chi.NewRouteContext() + rctx.URLParams.Add("driveID", "spaceid") + r = r.WithContext(context.WithValue(revactx.ContextSetUser(ctx, nil), chi.RouteCtxKey, rctx)) + svc.UpdateDrive(rr, r) + Expect(rr.Code).To(Equal(http.StatusOK)) + + gatewayClient.AssertCalled(GinkgoT(), "UpdateStorageSpace", mock.Anything, mock.MatchedBy(func(req *provider.UpdateStorageSpaceRequest) bool { + return req.StorageSpace.Id.OpaqueId == "spaceid" && + req.StorageSpace.Name == "" && + utils.ReadPlainFromOpaque(req.StorageSpace.Opaque, "description") == "thedescription" + })) + }, + Entry("no name", `{"description": "thedescription"}`), + Entry("null name", `{"name": null, "description": "thedescription"}`), + ) + It("restores", func() { drive := libregraph.NewDrive("thename") driveJson, err := json.Marshal(drive) diff --git a/services/graph/pkg/service/v0/loglevel_test.go b/services/graph/pkg/service/v0/loglevel_test.go new file mode 100644 index 0000000000..3fcdfd402f --- /dev/null +++ b/services/graph/pkg/service/v0/loglevel_test.go @@ -0,0 +1,259 @@ +package svc + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "net/http" + "net/http/httptest" + "testing" + + gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" + storageprovider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" + "github.com/go-chi/chi/v5/middleware" + libregraph "github.com/opencloud-eu/libre-graph-api-go" + "github.com/opencloud-eu/reva/v2/pkg/rgrpc/todo/pool" + "github.com/rs/zerolog" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/opencloud-eu/opencloud/pkg/log" + "github.com/opencloud-eu/opencloud/services/graph/pkg/errorcode" +) + +// logEntry logs one event with fn into a buffer and returns the decoded entry. +func logEntry(t *testing.T, fn func(logger *log.Logger) *zerolog.Event) map[string]any { + t.Helper() + var buf bytes.Buffer + logger := &log.Logger{Logger: zerolog.New(&buf)} + fn(logger).Msg("test") + var entry map[string]any + require.NoError(t, json.Unmarshal(buf.Bytes(), &entry)) + return entry +} + +func TestErrorEvent(t *testing.T) { + t.Run("unexpected failure: error level with the error text", func(t *testing.T) { + err := errorcode.New(errorcode.GeneralException, "share manager unavailable") + entry := logEntry(t, func(l *log.Logger) *zerolog.Event { return errorEvent(l, err) }) + assert.Equal(t, "error", entry["level"]) + assert.Equal(t, "generalException", entry["errorCode"]) + assert.Equal(t, err.Error(), entry["error"]) + }) + t.Run("expected client error: debug level with the error text", func(t *testing.T) { + err := errorcode.New(errorcode.ItemNotFound, "share not found") + entry := logEntry(t, func(l *log.Logger) *zerolog.Event { return errorEvent(l, err) }) + assert.Equal(t, "debug", entry["level"]) + assert.Equal(t, "itemNotFound", entry["errorCode"]) + assert.Equal(t, err.Error(), entry["error"]) + }) +} + +func TestFallbackErrorEvent(t *testing.T) { + notFound := errorcode.New(errorcode.ItemNotFound, "not found") + failure := errorcode.New(errorcode.GeneralException, "share manager unavailable") + + t.Run("the other attempt failed unexpectedly: error level, both texts", func(t *testing.T) { + entry := logEntry(t, func(l *log.Logger) *zerolog.Event { return fallbackErrorEvent(l, notFound, failure, "lookupError") }) + assert.Equal(t, "error", entry["level"]) + assert.Equal(t, "itemNotFound", entry["errorCode"]) + assert.Equal(t, notFound.Error(), entry["error"]) + assert.Equal(t, "generalException", entry["lookupErrorCode"]) + assert.Equal(t, failure.Error(), entry["lookupError"]) + }) + t.Run("the returned error is unexpected: error level, both texts", func(t *testing.T) { + entry := logEntry(t, func(l *log.Logger) *zerolog.Event { return fallbackErrorEvent(l, failure, notFound, "lookupError") }) + assert.Equal(t, "error", entry["level"]) + assert.Equal(t, failure.Error(), entry["error"]) + assert.Equal(t, "itemNotFound", entry["lookupErrorCode"]) + assert.Equal(t, notFound.Error(), entry["lookupError"]) + }) + t.Run("both attempts failed expectedly: debug level, both texts", func(t *testing.T) { + entry := logEntry(t, func(l *log.Logger) *zerolog.Event { return fallbackErrorEvent(l, notFound, notFound, "lookupError") }) + assert.Equal(t, "debug", entry["level"]) + assert.Equal(t, notFound.Error(), entry["error"]) + assert.Equal(t, notFound.Error(), entry["lookupError"]) + }) + t.Run("without another attempt: like errorEvent", func(t *testing.T) { + entry := logEntry(t, func(l *log.Logger) *zerolog.Event { return fallbackErrorEvent(l, failure, nil, "lookupError") }) + assert.Equal(t, "error", entry["level"]) + assert.Equal(t, failure.Error(), entry["error"]) + assert.NotContains(t, entry, "lookupErrorCode") + }) +} + +// failingGatewaySelector is a gateway selector that never finds a gateway. +type failingGatewaySelector struct{ err error } + +func (s failingGatewaySelector) Next(...pool.Option) (gateway.GatewayAPIClient, error) { + return nil, s.err +} + +// TestHelperGatewaySelectionFailureIsLoggedAsError checks that every helper that selects a gateway on its +// own logs a failed selection at error level with the selector's error, whichever operation calls it. +// The entry carries the request id, so it can be matched with the entry of the calling operation, +// and the permission id when the helper works on a single permission. +func TestHelperGatewaySelectionFailureIsLoggedAsError(t *testing.T) { + const listMsg = "could not select next gateway client" + const lookupMsg = "selecting gatewaySelector failed" + const requestID = "request-42" + const permissionID = "permissionid" + ctx := context.WithValue(context.Background(), middleware.RequestIDKey, requestID) + itemID := &storageprovider.ResourceId{StorageId: "1", SpaceId: "2", OpaqueId: "3"} + + tests := []struct { + name string + message string + permissionID string + call func(s DriveItemPermissionsService) error + }{ + {"listSharesWithSpaceRootFilter", listMsg, "", func(s DriveItemPermissionsService) error { + _, err := s.listSharesWithSpaceRootFilter(ctx, false, nil, driveItemsByResourceID{}) + return err + }}, + {"listOCMShares", listMsg, "", func(s DriveItemPermissionsService) error { + _, err := s.listOCMShares(ctx, nil, driveItemsByResourceID{}) + return err + }}, + {"listPublicShares", listMsg, "", func(s DriveItemPermissionsService) error { + _, err := s.listPublicShares(ctx, nil, driveItemsByResourceID{}) + return err + }}, + {"updatePublicLinkPermission", listMsg, permissionID, func(s DriveItemPermissionsService) error { + _, err := s.updatePublicLinkPermission(ctx, permissionID, itemID, &libregraph.Permission{}) + return err + }}, + {"getCS3PublicShareByID", lookupMsg, permissionID, func(s DriveItemPermissionsService) error { + _, err := s.getCS3PublicShareByID(ctx, permissionID) + return err + }}, + {"getCS3UserShareByID", lookupMsg, permissionID, func(s DriveItemPermissionsService) error { + _, err := s.getCS3UserShareByID(ctx, permissionID) + return err + }}, + {"getCS3OCMShareByID", lookupMsg, permissionID, func(s DriveItemPermissionsService) error { + _, err := s.getCS3OCMShareByID(ctx, permissionID) + return err + }}, + {"getPermissionByID", lookupMsg, permissionID, func(s DriveItemPermissionsService) error { + _, _, err := s.getPermissionByID(ctx, permissionID, itemID) + return err + }}, + {"getOCMPermissionByID", lookupMsg, permissionID, func(s DriveItemPermissionsService) error { + _, _, err := s.getOCMPermissionByID(ctx, permissionID, itemID) + return err + }}, + {"removeUserShare", lookupMsg, permissionID, func(s DriveItemPermissionsService) error { + return s.removeUserShare(ctx, permissionID) + }}, + {"removePublicShare", lookupMsg, permissionID, func(s DriveItemPermissionsService) error { + return s.removePublicShare(ctx, permissionID) + }}, + {"removeOCMPermission", lookupMsg, permissionID, func(s DriveItemPermissionsService) error { + return s.removeOCMPermission(ctx, permissionID) + }}, + {"updateUserShare", lookupMsg, permissionID, func(s DriveItemPermissionsService) error { + _, err := s.updateUserShare(ctx, permissionID, itemID, &libregraph.Permission{}) + return err + }}, + {"updateOCMPermission", lookupMsg, permissionID, func(s DriveItemPermissionsService) error { + _, err := s.updateOCMPermission(ctx, permissionID, itemID, &libregraph.Permission{}) + return err + }}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + var buf bytes.Buffer + s := DriveItemPermissionsService{BaseGraphService: BaseGraphService{ + logger: &log.Logger{Logger: zerolog.New(&buf)}, + gatewaySelector: failingGatewaySelector{err: errors.New("no gateway available")}, + }} + + require.Error(t, tc.call(s)) + + var entry map[string]any + require.NoError(t, json.Unmarshal(buf.Bytes(), &entry), "exactly one log entry") + assert.Equal(t, "error", entry["level"]) + assert.Equal(t, tc.message, entry["message"]) + assert.Equal(t, "no gateway available", entry["error"]) + assert.Equal(t, requestID, entry[log.RequestIDString]) + if tc.permissionID != "" { + assert.Equal(t, tc.permissionID, entry[_logKeyPermissionID]) + } else { + assert.NotContains(t, entry, _logKeyPermissionID) + } + }) + } +} + +func TestLogLevelForError(t *testing.T) { + tests := []struct { + name string + err error + want zerolog.Level + }{ + // expected client errors, rendered as 4xx by errorcode.Error.Render + {"item not found", errorcode.New(errorcode.ItemNotFound, "x"), zerolog.DebugLevel}, + {"access denied", errorcode.New(errorcode.AccessDenied, "x"), zerolog.DebugLevel}, + {"invalid request", errorcode.New(errorcode.InvalidRequest, "x"), zerolog.DebugLevel}, + {"name already exists", errorcode.New(errorcode.NameAlreadyExists, "x"), zerolog.DebugLevel}, + {"invalid range", errorcode.New(errorcode.InvalidRange, "x"), zerolog.DebugLevel}, + {"item is locked", errorcode.New(errorcode.ItemIsLocked, "x"), zerolog.DebugLevel}, + {"precondition failed", errorcode.New(errorcode.PreconditionFailed, "x"), zerolog.DebugLevel}, + {"not allowed", errorcode.New(errorcode.NotAllowed, "x"), zerolog.DebugLevel}, + {"wrapped item not found", fmt.Errorf("lookup: %w", errorcode.New(errorcode.ItemNotFound, "x")), zerolog.DebugLevel}, + {"wrapped access denied", fmt.Errorf("lookup: %w", errorcode.New(errorcode.AccessDenied, "x")), zerolog.DebugLevel}, + + // unexpected server-side failures + {"general exception", errorcode.New(errorcode.GeneralException, "x"), zerolog.ErrorLevel}, + {"service not available", errorcode.New(errorcode.ServiceNotAvailable, "x"), zerolog.ErrorLevel}, + {"unauthenticated", errorcode.New(errorcode.Unauthenticated, "x"), zerolog.ErrorLevel}, + {"quota limit reached", errorcode.New(errorcode.QuotaLimitReached, "x"), zerolog.ErrorLevel}, + {"not supported", errorcode.New(errorcode.NotSupported, "x"), zerolog.ErrorLevel}, + {"wrapped general exception", fmt.Errorf("stat: %w", errorcode.New(errorcode.GeneralException, "x")), zerolog.ErrorLevel}, + {"plain error", errors.New("connection refused"), zerolog.ErrorLevel}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, logLevelForError(tc.err)) + }) + } +} + +func TestErrorCodeForError(t *testing.T) { + tests := []struct { + name string + err error + want string + }{ + {"graph error", errorcode.New(errorcode.ItemNotFound, "private-file.txt"), "itemNotFound"}, + {"wrapped graph error", fmt.Errorf("stat: %w", errorcode.New(errorcode.ServiceNotAvailable, "x")), "serviceNotAvailable"}, + // errorcode.RenderError renders untyped errors as generalException + {"plain error", errors.New("connection refused"), "generalException"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, errorCodeForError(tc.err)) + }) + } +} + +// TestLogLevelForErrorMatchesRenderedStatus keeps logLevelForError in sync with errorcode.Error.Render: +// every code rendered as a 4xx is an expected client error, everything else is unexpected. +func TestLogLevelForErrorMatchesRenderedStatus(t *testing.T) { + // ItemIsLocked is the last declared error code + for code := errorcode.AccessDenied; code <= errorcode.ItemIsLocked; code++ { + t.Run(code.String(), func(t *testing.T) { + rr := httptest.NewRecorder() + errorcode.New(code, "x").Render(rr, httptest.NewRequest(http.MethodGet, "/", nil)) + + want := zerolog.ErrorLevel + if rr.Code >= http.StatusBadRequest && rr.Code < http.StatusInternalServerError { + want = zerolog.DebugLevel + } + assert.Equal(t, want, logLevelForError(errorcode.New(code, "x")), "rendered status %d", rr.Code) + }) + } +} diff --git a/services/graph/pkg/service/v0/service.go b/services/graph/pkg/service/v0/service.go index 65e0d39305..9dd651f346 100644 --- a/services/graph/pkg/service/v0/service.go +++ b/services/graph/pkg/service/v0/service.go @@ -167,7 +167,7 @@ func NewService(opts ...Option) (Graph, error) { //nolint:maintidx return Graph{}, err } - driveItemPermissionsService, err := NewDriveItemPermissionsService(options.Logger, options.GatewaySelector, identityCache, options.Config) + driveItemPermissionsService, err := NewDriveItemPermissionsService(options.Logger, options.GatewaySelector, identityCache, options.Config, options.TraceProvider) if err != nil { return Graph{}, err } @@ -264,6 +264,7 @@ func NewService(opts ...Option) (Graph, error) { //nolint:maintidx r.Route("/permissions", func(r chi.Router) { r.Get("/", driveItemPermissionsApi.ListSpaceRootPermissions) r.Route("/{permissionID}", func(r chi.Router) { + r.Get("/", driveItemPermissionsApi.GetSpaceRootPermission) r.Delete("/", driveItemPermissionsApi.DeleteSpaceRootPermission) r.Patch("/", driveItemPermissionsApi.UpdateSpaceRootPermission) r.Post("/setPassword", driveItemPermissionsApi.SetSpaceRootLinkPassword) @@ -279,6 +280,7 @@ func NewService(opts ...Option) (Graph, error) { //nolint:maintidx r.Route("/permissions", func(r chi.Router) { r.Get("/", driveItemPermissionsApi.ListPermissions) r.Route("/{permissionID}", func(r chi.Router) { + r.Get("/", driveItemPermissionsApi.GetPermission) r.Delete("/", driveItemPermissionsApi.DeletePermission) r.Patch("/", driveItemPermissionsApi.UpdatePermission) r.Post("/setPassword", driveItemPermissionsApi.SetLinkPassword) diff --git a/services/graph/pkg/service/v0/sharedbyme.go b/services/graph/pkg/service/v0/sharedbyme.go index 011bf94d80..14afeec381 100644 --- a/services/graph/pkg/service/v0/sharedbyme.go +++ b/services/graph/pkg/service/v0/sharedbyme.go @@ -7,7 +7,9 @@ import ( rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1" "github.com/go-chi/render" libregraph "github.com/opencloud-eu/libre-graph-api-go" + revactx "github.com/opencloud-eu/reva/v2/pkg/ctx" + "github.com/opencloud-eu/opencloud/pkg/log" "github.com/opencloud-eu/opencloud/services/graph/pkg/errorcode" ) @@ -15,11 +17,15 @@ type driveItemsByResourceID map[string]libregraph.DriveItem // GetSharedByMe implements the Service interface (/me/drives/sharedByMe endpoint) func (g Graph) GetSharedByMe(w http.ResponseWriter, r *http.Request) { - g.logger.Debug().Msg("Calling GetRootDriveChildren") ctx := r.Context() + logger := g.logger.SubloggerWithRequestID(ctx) + if user, ok := revactx.ContextGetUser(ctx); ok { + logger = log.Logger{Logger: logger.With().Str(_logKeyUserID, user.GetId().GetOpaqueId()).Logger()} + } driveItems, err := g.listUserShares(ctx, nil, make(driveItemsByResourceID)) if err != nil { + errorEvent(&logger, err).Msg("could not list user shares") errorcode.RenderError(w, r, err) return } @@ -27,6 +33,7 @@ func (g Graph) GetSharedByMe(w http.ResponseWriter, r *http.Request) { if g.config.IncludeOCMSharees { driveItems, err = g.listOCMShares(ctx, nil, driveItems) if err != nil { + errorEvent(&logger, err).Msg("could not list ocm shares") errorcode.RenderError(w, r, err) return } @@ -34,6 +41,7 @@ func (g Graph) GetSharedByMe(w http.ResponseWriter, r *http.Request) { driveItems, err = g.listPublicShares(ctx, nil, driveItems) if err != nil { + errorEvent(&logger, err).Msg("could not list public shares") errorcode.RenderError(w, r, err) return } diff --git a/services/graph/pkg/service/v0/sharedbyme_test.go b/services/graph/pkg/service/v0/sharedbyme_test.go index a42b8177f8..a33c116cd4 100644 --- a/services/graph/pkg/service/v0/sharedbyme_test.go +++ b/services/graph/pkg/service/v0/sharedbyme_test.go @@ -1,6 +1,7 @@ package svc_test import ( + "bytes" "context" "encoding/json" "errors" @@ -14,17 +15,21 @@ import ( userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" collaboration "github.com/cs3org/go-cs3apis/cs3/sharing/collaboration/v1beta1" link "github.com/cs3org/go-cs3apis/cs3/sharing/link/v1beta1" + ocm "github.com/cs3org/go-cs3apis/cs3/sharing/ocm/v1beta1" provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" + "github.com/go-chi/chi/v5/middleware" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" libregraph "github.com/opencloud-eu/libre-graph-api-go" "github.com/opencloud-eu/reva/v2/pkg/conversions" + revactx "github.com/opencloud-eu/reva/v2/pkg/ctx" "github.com/opencloud-eu/reva/v2/pkg/rgrpc/status" "github.com/opencloud-eu/reva/v2/pkg/rgrpc/todo/pool" "github.com/opencloud-eu/reva/v2/pkg/storagespace" "github.com/opencloud-eu/reva/v2/pkg/utils" cs3mocks "github.com/opencloud-eu/reva/v2/tests/cs3mocks/mocks" "github.com/prometheus/client_golang/prometheus" + "github.com/rs/zerolog" "github.com/stretchr/testify/mock" "google.golang.org/grpc" @@ -33,6 +38,7 @@ import ( "github.com/opencloud-eu/opencloud/services/graph/mocks" "github.com/opencloud-eu/opencloud/services/graph/pkg/config" "github.com/opencloud-eu/opencloud/services/graph/pkg/config/defaults" + "github.com/opencloud-eu/opencloud/services/graph/pkg/errorcode" identitymocks "github.com/opencloud-eu/opencloud/services/graph/pkg/identity/mocks" "github.com/opencloud-eu/opencloud/services/graph/pkg/linktype" "github.com/opencloud-eu/opencloud/services/graph/pkg/metrics" @@ -279,6 +285,260 @@ var _ = Describe("sharedbyme", func() { nil, ) } + Describe("GetSharedByMe logging", func() { + var logBuf *bytes.Buffer + + newLoggedService := func(selector pool.Selectable[gateway.GatewayAPIClient]) service.Service { + logBuf = &bytes.Buffer{} + logger := log.NewLogger() + m, err := metrics.New(prometheus.NewRegistry(), &logger, func([]string) (string, string) { return "", "" }) + Expect(err).ToNot(HaveOccurred()) + s, err := service.NewService( + service.Config(cfg), + service.Metrics(m), + service.WithGatewaySelector(selector), + service.EventsPublisher(&eventsPublisher), + service.WithIdentityBackend(identityBackend), + service.Logger(log.Logger{Logger: zerolog.New(logBuf)}), + ) + Expect(err).ToNot(HaveOccurred()) + return s + } + errorEntries := func() []map[string]any { + return entriesAtLevel(logEntries(logBuf), "error") + } + odataError := func() libregraph.OdataError { + var odataErr libregraph.OdataError + Expect(json.Unmarshal(rr.Body.Bytes(), &odataErr)).To(Succeed()) + return odataErr + } + + It("logs a gateway selection failure while listing shares as error in the helper and the operation", func() { + failingSelector := mocks.NewSelectable[gateway.GatewayAPIClient](GinkgoT()) + failingSelector.EXPECT().Next().Return(nil, errors.New("no gateway available")) + s := newLoggedService(failingSelector) + + s.GetSharedByMe(rr, httptest.NewRequest(http.MethodGet, "/graph/v1.0/me/drives/sharedByMe", nil)) + + Expect(rr.Code).To(Equal(http.StatusInternalServerError)) + Expect(odataError().Error.Code).To(Equal("generalException")) + Expect(odataError().Error.Message).To(Equal("no gateway available")) + // the helper and the handler both log the reason, the handler with the error code + Expect(errorEntries()).To(ConsistOf( + gatewayHelperEntry("could not select next gateway client", "no gateway available"), + SatisfyAll( + HaveKeyWithValue("message", "could not list user shares"), + HaveKeyWithValue("errorCode", "generalException"), + HaveKeyWithValue("error", errorcode.New(errorcode.GeneralException, "no gateway available").Error()), + ), + )) + }) + + It("logs a share listing failure once as error", func() { + emptyListPublicSharesMock() + gatewayClient.On("ListShares", mock.Anything, mock.Anything).Return( + &collaboration.ListSharesResponse{Status: status.NewInternal(ctx, "error listing shares")}, + nil, + ) + s := newLoggedService(gatewaySelector) + + r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/me/drives/sharedByMe", nil) + reqCtx := context.WithValue(r.Context(), middleware.RequestIDKey, "request-42") + reqCtx = revactx.ContextSetUser(reqCtx, &userpb.User{Id: &userpb.UserId{OpaqueId: "user-42"}}) + s.GetSharedByMe(rr, r.WithContext(reqCtx)) + + Expect(rr.Code).To(Equal(http.StatusInternalServerError)) + Expect(odataError().Error.Code).To(Equal("generalException")) + Expect(odataError().Error.Message).To(Equal("error listing shares")) + entries := errorEntries() + Expect(entries).To(HaveLen(1)) + Expect(entries[0]).To(SatisfyAll( + HaveKeyWithValue("message", "could not list user shares"), + HaveKeyWithValue("error", errorcode.New(errorcode.GeneralException, "error listing shares").Error()), + HaveKeyWithValue("errorCode", "generalException"), + HaveKeyWithValue("request-id", "request-42"), + HaveKeyWithValue("userID", "user-42"), + )) + }) + + DescribeTable("logs the identity backend error when a share grantee lookup fails", + func(setup func(identityClient *cs3mocks.GatewayAPIClient, resourceID *provider.ResourceId, groupGrantee *provider.Grantee), listingMethod string) { + const identityBackendMessage = "ldap search failed for cn=private-group-dn-42,ou=groups" + identityClient := cs3mocks.NewGatewayAPIClient(GinkgoT()) + selector := mocks.NewSelectable[gateway.GatewayAPIClient](GinkgoT()) + selector.EXPECT().Next().Return(identityClient, nil) + resourceID := &provider.ResourceId{StorageId: "storageid", SpaceId: "spaceid", OpaqueId: "opaqueid"} + groupGrantee := &provider.Grantee{ + Type: provider.GranteeType_GRANTEE_TYPE_GROUP, + Id: &provider.Grantee_GroupId{GroupId: &grouppb.GroupId{OpaqueId: "groupid"}}, + } + setup(identityClient, resourceID, groupGrantee) + identityClient.On("Stat", mock.Anything, mock.Anything).Return(&provider.StatResponse{ + Status: status.NewOK(ctx), + Info: &provider.ResourceInfo{Id: resourceID, Type: provider.ResourceType_RESOURCE_TYPE_CONTAINER}, + }, nil) + identityClient.On("GetGroup", mock.Anything, mock.Anything).Return(&grouppb.GetGroupResponse{ + Status: status.NewInternal(ctx, identityBackendMessage), + }, nil) + s := newLoggedService(selector) + + s.GetSharedByMe(rr, httptest.NewRequest(http.MethodGet, "/graph/v1.0/me/drives/sharedByMe", nil)) + + // the rendered error is unchanged + Expect(rr.Code).To(Equal(http.StatusInternalServerError)) + Expect(odataError().Error.Code).To(Equal("generalException")) + Expect(odataError().Error.Message).To(Equal( + errorcode.New(errorcode.GeneralException, + errorcode.New(errorcode.GeneralException, identityBackendMessage).Error()).Error())) + // the shares were listed and the group grantee was looked up, listing stops before the public shares + identityClient.AssertCalled(GinkgoT(), listingMethod, mock.Anything, mock.Anything) + identityClient.AssertCalled(GinkgoT(), "GetGroup", mock.Anything, mock.MatchedBy(func(req *grouppb.GetGroupRequest) bool { + return req.GetGroupId().GetOpaqueId() == "groupid" + })) + identityClient.AssertNotCalled(GinkgoT(), "ListPublicShares", mock.Anything, mock.Anything) + + entries := logEntries(logBuf) + errorEntries := entriesAtLevel(entries, "error") + Expect(errorEntries).To(HaveLen(1)) + // the handler logs the error text, which carries the identity backend message + Expect(errorEntries[0]).To(HaveKeyWithValue("error", ContainSubstring(identityBackendMessage))) + for _, payloadKey := range []string{"shares", "share", "group", "res", "response"} { + Expect(entries).ToNot(ContainElement(HaveKey(payloadKey))) + } + }, + Entry("user share listing", + func(identityClient *cs3mocks.GatewayAPIClient, resourceID *provider.ResourceId, groupGrantee *provider.Grantee) { + identityClient.On("ListShares", mock.Anything, mock.Anything).Return(&collaboration.ListSharesResponse{ + Status: status.NewOK(ctx), + Shares: []*collaboration.Share{{ + Id: &collaboration.ShareId{OpaqueId: "groupshareid"}, + ResourceId: resourceID, + Grantee: groupGrantee, + Permissions: &collaboration.SharePermissions{Permissions: editorResourcePermissions}, + }}, + }, nil) + }, + "ListShares", + ), + Entry("ocm share listing", + func(identityClient *cs3mocks.GatewayAPIClient, resourceID *provider.ResourceId, groupGrantee *provider.Grantee) { + cfg.IncludeOCMSharees = true + identityClient.On("ListShares", mock.Anything, mock.Anything).Return(&collaboration.ListSharesResponse{ + Status: status.NewOK(ctx), + Shares: []*collaboration.Share{}, + }, nil) + identityClient.On("ListOCMShares", mock.Anything, mock.Anything).Return(&ocm.ListOCMSharesResponse{ + Status: status.NewOK(ctx), + Shares: []*ocm.Share{{ + Id: &ocm.ShareId{OpaqueId: "ocmgroupshareid"}, + ResourceId: resourceID, + Grantee: groupGrantee, + }}, + }, nil) + }, + "ListOCMShares", + ), + ) + + It("logs an ocm share listing failure once as error", func() { + cfg.IncludeOCMSharees = true + emptyListSharesMock() + gatewayClient.On("ListOCMShares", mock.Anything, mock.Anything).Return( + &ocm.ListOCMSharesResponse{Status: status.NewInternal(ctx, "error listing ocm shares")}, + nil, + ) + s := newLoggedService(gatewaySelector) + + s.GetSharedByMe(rr, httptest.NewRequest(http.MethodGet, "/graph/v1.0/me/drives/sharedByMe", nil)) + + Expect(rr.Code).To(Equal(http.StatusInternalServerError)) + Expect(odataError().Error.Code).To(Equal("generalException")) + Expect(odataError().Error.Message).To(Equal("error listing ocm shares")) + gatewayClient.AssertNotCalled(GinkgoT(), "ListPublicShares", mock.Anything, mock.Anything) + entries := errorEntries() + Expect(entries).To(HaveLen(1)) + Expect(entries[0]).To(SatisfyAll( + HaveKeyWithValue("message", "could not list ocm shares"), + HaveKeyWithValue("error", errorcode.New(errorcode.GeneralException, "error listing ocm shares").Error()), + )) + }) + + It("logs a gateway selection failure while listing ocm shares as error in the helper and the operation", func() { + cfg.IncludeOCMSharees = true + emptyListSharesMock() + // the user share listing gets a client, the ocm share listing does not + selector := mocks.NewSelectable[gateway.GatewayAPIClient](GinkgoT()) + selector.EXPECT().Next().Return(gatewayClient, nil).Times(1) + selector.EXPECT().Next().Return(nil, errors.New("no gateway available")).Times(1) + s := newLoggedService(selector) + + s.GetSharedByMe(rr, httptest.NewRequest(http.MethodGet, "/graph/v1.0/me/drives/sharedByMe", nil)) + + Expect(rr.Code).To(Equal(http.StatusInternalServerError)) + Expect(odataError().Error.Code).To(Equal("generalException")) + Expect(odataError().Error.Message).To(Equal("no gateway available")) + gatewayClient.AssertCalled(GinkgoT(), "ListShares", mock.Anything, mock.Anything) + gatewayClient.AssertNotCalled(GinkgoT(), "ListOCMShares", mock.Anything, mock.Anything) + // the helper and the handler both log the reason, the handler with the error code + Expect(errorEntries()).To(ConsistOf( + gatewayHelperEntry("could not select next gateway client", "no gateway available"), + SatisfyAll( + HaveKeyWithValue("message", "could not list ocm shares"), + HaveKeyWithValue("error", errorcode.New(errorcode.GeneralException, "no gateway available").Error()), + ), + )) + }) + + It("logs a public share listing failure once as error", func() { + emptyListSharesMock() + gatewayClient.On("ListPublicShares", mock.Anything, mock.Anything).Return( + &link.ListPublicSharesResponse{Status: status.NewInternal(ctx, "listing failed for /Users/alice/Private/salary-2026.ods")}, + nil, + ) + s := newLoggedService(gatewaySelector) + + s.GetSharedByMe(rr, httptest.NewRequest(http.MethodGet, "/graph/v1.0/me/drives/sharedByMe", nil)) + + Expect(rr.Code).To(Equal(http.StatusInternalServerError)) + Expect(odataError().Error.Code).To(Equal("generalException")) + // the response is unchanged + Expect(odataError().Error.Message).To(Equal("listing failed for /Users/alice/Private/salary-2026.ods")) + gatewayClient.AssertCalled(GinkgoT(), "ListShares", mock.Anything, mock.Anything) + entries := errorEntries() + Expect(entries).To(HaveLen(1)) + Expect(entries[0]).To(SatisfyAll( + HaveKeyWithValue("message", "could not list public shares"), + HaveKeyWithValue("errorCode", "generalException"), + HaveKeyWithValue("error", errorcode.New(errorcode.GeneralException, "listing failed for /Users/alice/Private/salary-2026.ods").Error()), + )) + }) + + It("logs a gateway selection failure while listing public shares as error in the helper and the operation", func() { + emptyListSharesMock() + // the user share listing gets a client, the public share listing does not + selector := mocks.NewSelectable[gateway.GatewayAPIClient](GinkgoT()) + selector.EXPECT().Next().Return(gatewayClient, nil).Times(1) + selector.EXPECT().Next().Return(nil, errors.New("no gateway available")).Times(1) + s := newLoggedService(selector) + + s.GetSharedByMe(rr, httptest.NewRequest(http.MethodGet, "/graph/v1.0/me/drives/sharedByMe", nil)) + + Expect(rr.Code).To(Equal(http.StatusInternalServerError)) + Expect(odataError().Error.Code).To(Equal("generalException")) + Expect(odataError().Error.Message).To(Equal("no gateway available")) + gatewayClient.AssertCalled(GinkgoT(), "ListShares", mock.Anything, mock.Anything) + gatewayClient.AssertNotCalled(GinkgoT(), "ListPublicShares", mock.Anything, mock.Anything) + // the helper and the handler both log the reason, the handler with the error code + Expect(errorEntries()).To(ConsistOf( + gatewayHelperEntry("could not select next gateway client", "no gateway available"), + SatisfyAll( + HaveKeyWithValue("message", "could not list public shares"), + HaveKeyWithValue("error", errorcode.New(errorcode.GeneralException, "no gateway available").Error()), + ), + )) + }) + }) + Describe("GetSharedByMe", func() { It("handles a failing ListShares", func() { emptyListPublicSharesMock() diff --git a/services/graph/pkg/service/v0/users.go b/services/graph/pkg/service/v0/users.go index 5575db07b3..e66dfc2e63 100644 --- a/services/graph/pkg/service/v0/users.go +++ b/services/graph/pkg/service/v0/users.go @@ -42,7 +42,6 @@ import ( // GetMe implements the Service interface. func (g Graph) GetMe(w http.ResponseWriter, r *http.Request) { logger := g.logger.SubloggerWithRequestID(r.Context()) - logger.Debug().Msg("calling get user in /me") sanitizedPath := strings.TrimPrefix(r.URL.Path, "/graph/v1.0/") odataReq, err := godata.ParseRequest(r.Context(), sanitizedPath, r.URL.Query()) @@ -66,22 +65,23 @@ func (g Graph) GetMe(w http.ResponseWriter, r *http.Request) { return } - var me *libregraph.User - // We can just return the user from context unless we need to expand the group memberships - if !slices.Contains(exp, "memberOf") { - me = identity.CreateUserModelFromCS3(u) - } else { - var err error - logger.Debug().Msg("calling get user on backend") - me, err = g.identityBackend.GetUser(r.Context(), u.GetId().GetOpaqueId(), odataReq) - if err != nil { - logger.Debug().Err(err).Interface("user", u).Msg("could not get user from backend") - errorcode.RenderError(w, r, err) - return - } - if me.MemberOf == nil { - me.MemberOf = []libregraph.Group{} - } + // The user is always read from the identity backend: the CS3 user of the request + // context doesn't know the identities the user signed in with, and a model built + // from it would report the internal user ID as identities[].issuerAssignedId + // instead of the subject assigned by the identity provider. The group memberships + // are only looked up when $expand=memberOf asks for them. + me, err := g.identityBackend.GetUser(r.Context(), u.GetId().GetOpaqueId(), odataReq) + if err != nil { + // a user the backend doesn't know is logged at debug level, a failing backend as error + errorEvent(&logger, err).Str(_logKeyUserID, u.GetId().GetOpaqueId()).Msg("could not get user from backend") + errorcode.RenderError(w, r, err) + return + } + // An expanded relation without entries is returned as an empty array rather than + // left out, so that clients that asked for it can rely on the attribute being there. + // The JSON encoding leaves out a nil slice, but keeps an empty one. + if slices.Contains(exp, "memberOf") && me.MemberOf == nil { + me.MemberOf = []libregraph.Group{} } // expand appRoleAssignments if requested diff --git a/services/graph/pkg/service/v0/users_test.go b/services/graph/pkg/service/v0/users_test.go index 55aac117db..42fa7ff5ea 100644 --- a/services/graph/pkg/service/v0/users_test.go +++ b/services/graph/pkg/service/v0/users_test.go @@ -40,6 +40,7 @@ import ( "github.com/opencloud-eu/opencloud/services/graph/mocks" "github.com/opencloud-eu/opencloud/services/graph/pkg/config" "github.com/opencloud-eu/opencloud/services/graph/pkg/config/defaults" + "github.com/opencloud-eu/opencloud/services/graph/pkg/errorcode" identitymocks "github.com/opencloud-eu/opencloud/services/graph/pkg/identity/mocks" service "github.com/opencloud-eu/opencloud/services/graph/pkg/service/v0" ) @@ -131,6 +132,14 @@ var _ = Describe("Users", func() { }) It("gets the information", func() { + user := &libregraph.User{ + Id: libregraph.PtrString("user"), + Identities: []libregraph.ObjectIdentity{{ + Issuer: libregraph.PtrString("https://idp.example.org"), + IssuerAssignedId: libregraph.PtrString("subject-of-the-idp"), + }}, + } + identityBackend.On("GetUser", mock.Anything, "user", mock.Anything).Return(user, nil) valueService.On("GetValueByUniqueIdentifiers", mock.Anything, mock.Anything, mock.Anything). Return(&settings.GetValueResponse{ Value: &settingsmsg.ValueWithIdentifier{ @@ -155,6 +164,32 @@ var _ = Describe("Users", func() { svc.GetMe(rr, r) Expect(rr.Code).To(Equal(http.StatusOK)) + + data, err := io.ReadAll(rr.Body) + Expect(err).ToNot(HaveOccurred()) + + responseUser := &libregraph.User{} + err = json.Unmarshal(data, &responseUser) + Expect(err).ToNot(HaveOccurred()) + + // the identities come from the identity backend, not from the user ID + Expect(responseUser.GetIdentities()).To(HaveLen(1)) + Expect(responseUser.GetIdentities()[0].GetIssuer()).To(Equal("https://idp.example.org")) + Expect(responseUser.GetIdentities()[0].GetIssuerAssignedId()).To(Equal("subject-of-the-idp")) + // without $expand=memberOf, the group memberships are left out + Expect(responseUser.HasMemberOf()).To(BeFalse()) + }) + + It("renders the error of the identity backend", func() { + identityBackend.On("GetUser", mock.Anything, "user", mock.Anything). + Return(nil, errorcode.New(errorcode.ItemNotFound, "user not found")) + + r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/me", nil) + r = r.WithContext(revactx.ContextSetUser(ctx, currentUser)) + svc.GetMe(rr, r) + + Expect(rr.Code).To(Equal(http.StatusNotFound)) + valueService.AssertNotCalled(GinkgoT(), "GetValueByUniqueIdentifiers", mock.Anything, mock.Anything, mock.Anything) }) It("expands the memberOf", func() { @@ -209,6 +244,7 @@ var _ = Describe("Users", func() { RoleId: "some-appRole-ID", }, } + identityBackend.On("GetUser", mock.Anything, "user", mock.Anything).Return(&libregraph.User{Id: libregraph.PtrString("user")}, nil) roleService.On("ListRoleAssignments", mock.Anything, mock.Anything, mock.Anything).Return(&settings.ListRoleAssignmentsResponse{Assignments: assignments}, nil) valueService.On("GetValueByUniqueIdentifiers", mock.Anything, mock.Anything, mock.Anything). Return(&settings.GetValueResponse{ diff --git a/tests/acceptance/TestHelpers/GraphHelper.php b/tests/acceptance/TestHelpers/GraphHelper.php index acee0c4ac0..052dcf699d 100644 --- a/tests/acceptance/TestHelpers/GraphHelper.php +++ b/tests/acceptance/TestHelpers/GraphHelper.php @@ -2183,6 +2183,35 @@ public static function getDrivePermissionsList( ); } + /** + * @param string $baseUrl + * @param string $xRequestId + * @param string $user + * @param string $password + * @param string $spaceId + * @param string $permissionId + * + * @return ResponseInterface + * @throws GuzzleException + */ + public static function getDrivePermission( + string $baseUrl, + string $xRequestId, + string $user, + string $password, + string $spaceId, + string $permissionId + ): ResponseInterface { + $url = self::getBetaFullUrl($baseUrl, "drives/$spaceId/root/permissions/$permissionId"); + return HttpRequestHelper::get( + $url, + $xRequestId, + $user, + $password, + self::getRequestHeaders() + ); + } + /** * @param string $baseUrl * @param string $xRequestId diff --git a/tests/acceptance/bootstrap/SharingNgContext.php b/tests/acceptance/bootstrap/SharingNgContext.php index 9479ae42a8..eb446050d1 100644 --- a/tests/acceptance/bootstrap/SharingNgContext.php +++ b/tests/acceptance/bootstrap/SharingNgContext.php @@ -1724,6 +1724,27 @@ public function userListsThePermissionsOfDriveUsingRootEndPointOFTheGraphApi(str $this->featureContext->setResponse($this->getDrivePermissionsList($user, $space)); } + /** + * @param string $user + * @param string $space + * + * @return void + * @throws GuzzleException + */ + #[When('user :user gets the last link permission of space :space using root endpoint of the Graph API')] + public function userGetsTheLastLinkPermissionOfSpaceUsingRootEndpointOfTheGraphApi(string $user, string $space): void { + $this->featureContext->setResponse( + GraphHelper::getDrivePermission( + $this->featureContext->getBaseUrl(), + $this->featureContext->getStepLineRef(), + $user, + $this->featureContext->getPasswordForUser($user), + ($this->spacesContext->getSpaceByName($user, $space))["id"], + $this->featureContext->shareNgGetLastCreatedLinkShareID() + ) + ); + } + /** * * @param string $user diff --git a/tests/acceptance/bootstrap/SpacesContext.php b/tests/acceptance/bootstrap/SpacesContext.php index 1d422b2f0c..3af2560fbe 100644 --- a/tests/acceptance/bootstrap/SpacesContext.php +++ b/tests/acceptance/bootstrap/SpacesContext.php @@ -805,6 +805,30 @@ public function theUserCreatesASpaceWithQuotaUsingTheGraphApi( ); } + /** + * @param string $user + * @param string $spaceName + * @param string $spaceType + * @param string $spaceId + * + * @return void + * + * @throws GuzzleException + * @throws Exception + */ + #[When('/^user "([^"]*)" (?:creates|tries to create) a space "([^"]*)" of type "([^"]*)" with id "([^"]*)" using the Graph API$/')] + public function theUserCreatesASpaceWithIdUsingTheGraphApi( + string $user, + string $spaceName, + string $spaceType, + string $spaceId + ): void { + $space = ["name" => $spaceName, "driveType" => $spaceType, "id" => $spaceId]; + $this->featureContext->setResponse( + $this->createSpace($user, $space) + ); + } + /** * Remember the available Spaces * @@ -1636,6 +1660,30 @@ public function updateSpaceName( ); } + /** + * Like updateSpaceName, but the new name is written with JSON escape sequences, + * e.g. "\u200b" for a zero-width space. Characters such as invisible or unusual + * spaces can't be told apart from ordinary spaces, or can't be seen at all, when + * written directly into a feature file. + * + * @param string $user + * @param string $spaceName + * @param string $escapedNewName + * + * @return void + * @throws GuzzleException + * @throws JsonException + */ + #[When('/^user "([^"]*)" (?:changes|tries to change) the name of the "([^"]*)" space to the escaped name "([^"]*)"$/')] + public function updateSpaceNameEscaped( + string $user, + string $spaceName, + string $escapedNewName + ): void { + $newName = json_decode('"' . $escapedNewName . '"', false, 512, JSON_THROW_ON_ERROR); + $this->updateSpaceName($user, $spaceName, $newName); + } + /** * * @param string $user diff --git a/tests/acceptance/features/apiSharingNg1/listPermissions.feature b/tests/acceptance/features/apiSharingNg1/listPermissions.feature index 3ab389e37a..59a425ef6e 100644 --- a/tests/acceptance/features/apiSharingNg1/listPermissions.feature +++ b/tests/acceptance/features/apiSharingNg1/listPermissions.feature @@ -2926,3 +2926,60 @@ Feature: List a sharing permissions } } """ + + + Scenario: user gets a single link permission of a project space using root endpoint + Given using spaces DAV path + And the administrator has assigned the role "Space Admin" to user "Alice" using the Graph API + And user "Alice" has created a space "new-space" with the default quota using the Graph API + And user "Alice" has created the following space link share: + | space | new-space | + | permissionsRole | view | + | password | %public% | + When user "Alice" gets the last link permission of space "new-space" using root endpoint of the Graph API + Then the HTTP status code should be "200" + And the JSON data of the response should match + """ + { + "type": "object", + "required": ["createdDateTime", "hasPassword", "id", "link"], + "properties": { + "createdDateTime": { + "format": "date-time" + }, + "hasPassword": { + "const": true + }, + "id": { + "pattern": "^[a-zA-Z]{15}$" + }, + "link": { + "type": "object", + "required": [ + "@libre.graph.displayName", + "@libre.graph.quickLink", + "preventsDownload", + "type", + "webUrl" + ], + "properties": { + "@libre.graph.displayName": { + "const": "" + }, + "@libre.graph.quickLink": { + "const": false + }, + "preventsDownload": { + "const": false + }, + "type": { + "const": "view" + }, + "webUrl": { + "pattern": "^%base_url%/s/[a-zA-Z]{15}$" + } + } + } + } + } + """ diff --git a/tests/acceptance/features/apiSharingNg1/sharedByMe.feature b/tests/acceptance/features/apiSharingNg1/sharedByMe.feature index 1223b03c23..782cd63ebf 100644 --- a/tests/acceptance/features/apiSharingNg1/sharedByMe.feature +++ b/tests/acceptance/features/apiSharingNg1/sharedByMe.feature @@ -3378,7 +3378,7 @@ Feature: resources shared by user } """ - @issue-8355 + Scenario: sharer lists the link share of a project space Given using spaces DAV path And the administrator has assigned the role "Space Admin" to user "Alice" using the Graph API @@ -3413,7 +3413,7 @@ Feature: resources shared by user ], "properties": { "name": { - "const": "." + "const": "NewSpace" }, "root": { "const": {} diff --git a/tests/acceptance/features/apiSpaces/changeSpaces.feature b/tests/acceptance/features/apiSpaces/changeSpaces.feature index 3a1a62384c..438c27c1ff 100644 --- a/tests/acceptance/features/apiSpaces/changeSpaces.feature +++ b/tests/acceptance/features/apiSpaces/changeSpaces.feature @@ -58,6 +58,41 @@ Feature: Change data of space | Bob | + Scenario: user with space manager role can't change the name of a space to an empty name via the Graph API + When user "Alice" tries to change the name of the "Project Jupiter" space to "" + Then the HTTP status code should be "400" + And the user "Alice" should have a space called "Project Jupiter" + + + Scenario: user with space manager role can't change the name of a space to a name of only spaces via the Graph API + When user "Alice" tries to change the name of the "Project Jupiter" space to " " + Then the HTTP status code should be "400" + And the user "Alice" should have a space called "Project Jupiter" + + + Scenario: user with space manager role can't change the name of a space to a name of only invisible or unusual spaces via the Graph API + # \u00a0 no-break space, \u202f narrow no-break space + When user "Alice" tries to change the name of the "Project Jupiter" space to the escaped name "\u00a0\u202f" + Then the HTTP status code should be "400" + # \u2003 em space, \u2009 thin space, \u3000 ideographic space + When user "Alice" tries to change the name of the "Project Jupiter" space to the escaped name "\u2003\u2009\u3000" + Then the HTTP status code should be "400" + # \u200b zero-width space, \u200c zero-width non-joiner, \u200d zero-width joiner + When user "Alice" tries to change the name of the "Project Jupiter" space to the escaped name "\u200b\u200c\u200d" + Then the HTTP status code should be "400" + # \u2060 word joiner, \ufeff byte order mark, mixed with a tab and ordinary spaces + When user "Alice" tries to change the name of the "Project Jupiter" space to the escaped name " \u2060\t\ufeff " + Then the HTTP status code should be "400" + And the user "Alice" should have a space called "Project Jupiter" + + + Scenario: user with space manager role can change the name of a space to a name with invisible characters around it via the Graph API + # \u200b zero-width space, \ufeff byte order mark + When user "Alice" changes the name of the "Project Jupiter" space to the escaped name "\u200bProject Saturn\ufeff" + Then the HTTP status code should be "200" + And the user "Alice" should have a space called "Project Saturn" + + Scenario: user with space manager role can change the description(subtitle) of a space via the Graph API When user "Alice" changes the description of the "Project Jupiter" space to "The Death Star is a fictional mobile space station" Then the HTTP status code should be "200" diff --git a/tests/acceptance/features/apiSpaces/createSpace.feature b/tests/acceptance/features/apiSpaces/createSpace.feature index c463560a31..348cae4a11 100644 --- a/tests/acceptance/features/apiSpaces/createSpace.feature +++ b/tests/acceptance/features/apiSpaces/createSpace.feature @@ -153,3 +153,44 @@ Feature: create space | user-role | | Admin | | Space Admin | + + + Scenario: space admin creates a project space with a client-supplied id via the Graph API + Given the administrator has assigned the role "Space Admin" to user "Alice" using the Graph API + When user "Alice" creates a space "Project Mars" of type "project" with id "4e3b1a52-8f0d-4c6e-9a71-2d5f6b8c9e10" using the Graph API + Then the HTTP status code should be "201" + And the JSON response should contain space called "Project Mars" and match + """ + { + "type": "object", + "required": ["id"], + "properties": { + "id": { + "type": "string", + "pattern": "\\$4e3b1a52-8f0d-4c6e-9a71-2d5f6b8c9e10$" + } + } + } + """ + When user "Alice" looks up the single space "Project Mars" via the Graph API by using its id + Then the HTTP status code should be "200" + And the JSON response should contain space called "Project Mars" and match + """ + { + "type": "object", + "required": ["id", "name"], + "properties": { + "id": { + "type": "string", + "pattern": "\\$4e3b1a52-8f0d-4c6e-9a71-2d5f6b8c9e10$" + }, + "name": { + "type": "string", + "enum": ["Project Mars"] + } + } + } + """ + When user "Alice" tries to create a space "Project Venus" of type "project" with id "4e3b1a52-8f0d-4c6e-9a71-2d5f6b8c9e10" using the Graph API + Then the HTTP status code should be "409" + And the user "Alice" should not have a space called "Project Venus"