diff --git a/internal/eventstest/events_test_helpers.go b/internal/eventstest/events_test_helpers.go new file mode 100644 index 0000000000..ce8ef22ad6 --- /dev/null +++ b/internal/eventstest/events_test_helpers.go @@ -0,0 +1,44 @@ +package eventstest + +import ( + "encoding/json" + "reflect" + + "github.com/google/uuid" + + rev "github.com/opencloud-eu/reva/v2/pkg/events" + microevents "go-micro.dev/v4/events" +) + +func NewTestBus() TestBus { + return TestBus(make(chan rev.Event)) +} + +type TestBus chan rev.Event + +func (tb TestBus) Consume(_ string, _ ...microevents.ConsumeOption) (<-chan microevents.Event, error) { + ch := make(chan microevents.Event) + go func() { + for ev := range tb { + b, _ := json.Marshal(ev.Event) + ch <- microevents.Event{ + Payload: b, + Metadata: map[string]string{ + rev.MetadatakeyEventID: ev.ID, + rev.MetadatakeyEventType: ev.Type, + }, + } + } + }() + return ch, nil +} + +func (tb TestBus) Publish(e any) string { + ev := rev.Event{ + ID: uuid.New().String(), + Type: reflect.TypeOf(e).String(), + Event: e, + } + tb <- ev + return ev.ID +} diff --git a/internal/metricstest/metrics_test_helpers.go b/internal/metricstest/metrics_test_helpers.go new file mode 100644 index 0000000000..2a47285cd0 --- /dev/null +++ b/internal/metricstest/metrics_test_helpers.go @@ -0,0 +1,143 @@ +package metricstest + +import ( + "fmt" + + "github.com/prometheus/client_golang/prometheus" + dto "github.com/prometheus/client_model/go" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// copied and adapted from Prometheus testutil.ToFloat64(), since we don't import that package +func collect(c prometheus.Collector) []prometheus.Metric { + result := []prometheus.Metric{} + ch := make(chan prometheus.Metric) + done := make(chan struct{}) + go func() { + for m := range ch { + result = append(result, m) + } + close(done) + }() + c.Collect(ch) + close(ch) + <-done + return result +} + +func RequireIsNotSet(t require.TestingT, c prometheus.Collector, msgAndArgs ...any) { + if h, ok := t.(interface{ Helper() }); ok { + h.Helper() + } + if !IsNotSet(t, c, msgAndArgs) { + t.FailNow() + } +} + +func IsNotSet(t assert.TestingT, c prometheus.Collector, msgAndArgs ...any) bool { + if h, ok := t.(interface{ Helper() }); ok { + h.Helper() + } + + m := collect(c) + if len(m) > 0 { + return assert.Fail(t, "Metric exists while expected to not exist", msgAndArgs) + } else { + return true + } +} + +func RequireEqual(t require.TestingT, expected float64, c prometheus.Collector, msgAndArgs ...any) { + if h, ok := t.(interface{ Helper() }); ok { + h.Helper() + } + if !Equal(t, expected, c, msgAndArgs) { + t.FailNow() + } +} + +// copied and adapted from Prometheus testutil.ToFloat64(), since we don't import that package +func Equal(t assert.TestingT, expected float64, c prometheus.Collector, msgAndArgs ...any) bool { + if h, ok := t.(interface{ Helper() }); ok { + h.Helper() + } + + m := collect(c) + if !assert.Len(t, m, 1, msgAndArgs...) { + return false + } + pb := &dto.Metric{} + err := m[0].Write(pb) + if !assert.NoError(t, err, msgAndArgs...) { + return false + } + if pb.Gauge != nil { + return assert.Equal(t, expected, pb.Gauge.GetValue(), msgAndArgs...) + } else if pb.Counter != nil { + return assert.Equal(t, expected, pb.Counter.GetValue(), msgAndArgs...) + } else if pb.Untyped != nil { + return assert.Equal(t, expected, pb.Untyped.GetValue(), msgAndArgs...) + } else { + return assert.Fail(t, fmt.Sprintf("collected a non-gauge/counter/untyped metric: %s", pb), msgAndArgs...) + } +} + +func RequireEqualWithLabels(t require.TestingT, expectedValue float64, expectedLabels map[string]string, c prometheus.Collector, msgAndArgs ...any) { + if h, ok := t.(interface{ Helper() }); ok { + h.Helper() + } + if !EqualWithLabels(t, expectedValue, expectedLabels, c, msgAndArgs) { + t.FailNow() + } +} + +func EqualWithLabels(t assert.TestingT, expectedValue float64, expectedLabels map[string]string, c prometheus.Collector, msgAndArgs ...any) bool { + if h, ok := t.(interface{ Helper() }); ok { + h.Helper() + } + + m := collect(c) + if !assert.Len(t, m, 1, "collected %d metrics instead of exactly 1", len(m)) { + return false + } + pb := &dto.Metric{} + err := m[0].Write(pb) + if !assert.NoError(t, err) { + return false + } + if pb.Gauge != nil { + if !assert.Equal(t, expectedValue, pb.Gauge.GetValue()) { + return false + } + } else if pb.Counter != nil { + if !assert.Equal(t, expectedValue, pb.Counter.GetValue()) { + return false + } + } else if pb.Untyped != nil { + if !assert.Equal(t, expectedValue, pb.Untyped.GetValue()) { + return false + } + } else { + return assert.Fail(t, "collected a non-gauge/counter/untyped metric: %s", pb) + } + + if !assert.NotNil(t, pb.Label) { + return false + } + actualLabels := map[string]string{} + for _, label := range pb.Label { + if !assert.NotNil(t, label) { + return false + } + if !assert.NotNil(t, label.Name) { + return false + } + if !assert.NotNil(t, label.Value) { + return false + } + actualLabels[*label.Name] = *label.Value + } + return assert.Equal(t, expectedLabels, actualLabels, msgAndArgs) +} diff --git a/services/graph/README.md b/services/graph/README.md index cbf84ebdd0..f6ea85fe3b 100644 --- a/services/graph/README.md +++ b/services/graph/README.md @@ -168,7 +168,7 @@ The output of this command includes the following information for each role: * `Condition` * `Allowed resource actions` -**Example output (shortned)** +**Example output (shortened)** ```bash +--------------------------------------+----------+--------------------------------+--------------------------------+------------------------------------------+ @@ -184,3 +184,22 @@ The output of this command includes the following information for each role: +--------------------------------------+----------+--------------------------------+--------------------------------+------------------------------------------+ ``` +## API Handlers + +To specialize `graph` service instances in order to scale them independently, it is possible to disable its API handlers: + +* `GRAPH_HTTP_DISABLE`: when set to `true`, the service does not listen on HTTP and only consumes events (defaults to `false`) +* `GRAPH_EVENTS_DISABLE_CONSUMER`: when set to `true`, the service does not consome events and only listens on HTTP (defaults to `false`) + +## Metrics + +The `graph` service provides the following metrics: + +| Name | Description | +| ---- | ----------- | +| `opencloud_graph_build_info{version=...}` | Contains a label `version` that is set to the current version of the service, and always has a value of `1` | +| `opencloud_graph_events_enabled` | Is set to `1` if the Events API handler is enabled, or `0` if not | +| `opencloud_graph_http_enabled` | Is set to `1` if the HTTP API handler is enabled, or `0` if not | +| `opencloud_graph_events{event=...,result=...}` | Counts the number of events that have been consumed, with a `event` label that contains the name of the event, and a `result` label that is set to `success` or `failure` | +| `opencloud_graph_events_invalid` | Counts the number of invalid events that are malformed or are missing required data | +| `opencloud_graph_events_unsupported` | Counts the numbef of consumed events that cannot be processes by this service, should always be `0` | diff --git a/services/graph/pkg/command/server.go b/services/graph/pkg/command/server.go index b8cdc7f977..e6690f84b1 100644 --- a/services/graph/pkg/command/server.go +++ b/services/graph/pkg/command/server.go @@ -7,6 +7,7 @@ import ( "strings" "github.com/opencloud-eu/opencloud/pkg/config/configlog" + "github.com/opencloud-eu/opencloud/pkg/generators" "github.com/opencloud-eu/opencloud/pkg/log" natspkg "github.com/opencloud-eu/opencloud/pkg/nats" "github.com/opencloud-eu/opencloud/pkg/runner" @@ -14,9 +15,15 @@ import ( "github.com/opencloud-eu/opencloud/pkg/version" "github.com/opencloud-eu/opencloud/services/graph/pkg/config" "github.com/opencloud-eu/opencloud/services/graph/pkg/config/parser" + "github.com/opencloud-eu/opencloud/services/graph/pkg/identity" "github.com/opencloud-eu/opencloud/services/graph/pkg/metrics" "github.com/opencloud-eu/opencloud/services/graph/pkg/server/debug" "github.com/opencloud-eu/opencloud/services/graph/pkg/server/http" + evc "github.com/opencloud-eu/opencloud/services/graph/pkg/service/events" + svc "github.com/opencloud-eu/opencloud/services/graph/pkg/service/v0" + "github.com/opencloud-eu/reva/v2/pkg/events" + "github.com/opencloud-eu/reva/v2/pkg/events/stream" + "github.com/prometheus/client_golang/prometheus" "github.com/nats-io/nats.go" "github.com/nats-io/nats.go/jetstream" @@ -46,7 +53,14 @@ func Server(cfg *config.Config) *cobra.Command { } ctx := cfg.Context - mtrcs := metrics.New() + prom := prometheus.DefaultRegisterer + + // note that the function we pass here is tasked with decomposing Graph HTTP API + // request URL patterns into information that is then used for labels in metrics + // to track HTTP request processing durations, and it is located there to be close + // to the HTTP API route definitions, to improve chances of adapting it accordingly + // whenever those routes should change in the future + mtrcs := metrics.New(prom, svc.DecomposeGraphApiRequestPattern) mtrcs.BuildInfo.WithLabelValues(version.GetString()).Set(1) var kv jetstream.KeyValue @@ -78,9 +92,45 @@ func Server(cfg *config.Config) *cobra.Command { } } + identityBackendName := cfg.Identity.Backend // contains the name of the backend implementation to use + + // since the identity backend in use is of prime importance to understand issues through logs, every + // log entry should contain a 'backend' entry with the name of the backend in use from here on: + logger = log.Logger{Logger: logger.With().Str("backend", identityBackendName).Logger()} + + identityBackend, eduBackend, err := identity.CreateIdentityBackends( + identityBackendName, + cfg, + &logger, + prom, + traceProvider, + ) + + if err != nil { + logger.Error().Err(err).Msg("Error initializing the identity backend") + return fmt.Errorf("could not initialize identity backend: %w", err) + } + + var eventsStream events.Stream + if cfg.Events.Endpoint != "" { + var err error + connName := generators.GenerateConnectionName(cfg.Service.Name, generators.NTypeBus) + eventsStream, err = stream.NatsFromConfig(connName, false, cfg.Events.ToNatsConfig()) + if err != nil { + logger.Error().Err(err).Msg("Error initializing events publisher") + return fmt.Errorf("could not initialize events publisher: %w", err) + } + } + gr := runner.NewGroup() - { + + if !cfg.HTTP.Disabled { + mtrcs.HttpEnabled.Set(1) + server, err := http.Server( + identityBackend, + eduBackend, + eventsStream, http.Logger(logger), http.Context(ctx), http.Config(cfg), @@ -92,8 +142,37 @@ func Server(cfg *config.Config) *cobra.Command { logger.Error().Err(err).Str("transport", "http").Msg("Failed to initialize server") return err } - gr.Add(runner.NewGoMicroHttpServerRunner(cfg.Service.Name+".http", server)) + } else { + mtrcs.HttpEnabled.Set(0) + logger.Info().Str("transport", "http").Msg("HTTP server is disabled") + } + + if !cfg.Events.DisabledConsumer { + mtrcs.EventsEnabled.Set(1) + + // even if events are enabled, we still need to differentiate between whether this process + // show be consuming events or not (and even when that is disabled, we still need to be + // able to produce events), which is why this is a separate setting; + // for context, see https://github.com/opencloud-eu/opencloud/issues/1312 + + logger := &log.Logger{Logger: logger.With().Str("transport", "events").Logger()} + eventConsumer, err := evc.NewService(cfg.Context, eventsStream, identityBackend, mtrcs, logger) + if err != nil { + return fmt.Errorf("could not initialize events consumer: %w", err) + } + + gr.Add(runner.New(cfg.Service.Name+".svc", func() error { + return eventConsumer.Start() + }, func() { + err := eventConsumer.Close() + if err != nil { + logger.Error().Err(err).Msg("failed to stop event consumer") + } + })) + } else { + mtrcs.EventsEnabled.Set(0) + logger.Info().Str("transport", "events").Msg("event consumer is disabled") } { diff --git a/services/graph/pkg/config/config.go b/services/graph/pkg/config/config.go index 6734152be2..cf244c4865 100644 --- a/services/graph/pkg/config/config.go +++ b/services/graph/pkg/config/config.go @@ -5,6 +5,7 @@ import ( "time" "github.com/opencloud-eu/opencloud/pkg/shared" + "github.com/opencloud-eu/reva/v2/pkg/events/stream" ) // Config combines all available configuration parts. @@ -57,6 +58,10 @@ type Spaces struct { TranslationPath string `yaml:"translation_path" env:"OC_TRANSLATION_PATH;GRAPH_TRANSLATION_PATH" desc:"(optional) Set this to a path with custom translations to overwrite the builtin translations. Note that file and folder naming rules apply, see the documentation for more details." introductionVersion:"1.0.0"` } +type LDAPMetrics struct { + Disabled bool `yaml:"disabled" env:"GRAPH_LDAP_METRICS_DISABLE" desc:"Disables the metrics for outbound LDAP operations." introductionVersion:"%NEXT%"` +} + type LDAP struct { URI string `yaml:"uri" env:"OC_LDAP_URI;GRAPH_LDAP_URI" desc:"URI of the LDAP Server to connect to. Supported URI schemes are 'ldaps://' and 'ldap://'" introductionVersion:"1.0.0"` CACert string `yaml:"cacert" env:"OC_LDAP_CACERT;GRAPH_LDAP_CACERT" desc:"Path/File name for the root CA certificate (in PEM format) used to validate TLS server certificates of the LDAP service. If not defined, the root directory derives from $OC_BASE_DATA_PATH/idm." introductionVersion:"1.0.0"` @@ -94,6 +99,8 @@ type LDAP struct { EducationResourcesEnabled bool `yaml:"education_resources_enabled" env:"GRAPH_LDAP_EDUCATION_RESOURCES_ENABLED" desc:"Enable LDAP support for managing education related resources." introductionVersion:"1.0.0"` EducationConfig LDAPEducationConfig + + Metrics LDAPMetrics `yaml:"metrics"` } // LDAPEducationConfig represents the LDAP configuration for education related resources @@ -127,6 +134,7 @@ type API struct { // Events combines the configuration options for the event bus. type Events struct { + DisabledConsumer bool `yaml:"disabled_consumer" env:"GRAPH_EVENTS_DISABLE_CONSUMER" desc:"Disables consuming events. Set this to true if the service should only handle HTTP requests." introductionVersion:"%NEXT%"` Endpoint string `yaml:"endpoint" env:"OC_EVENTS_ENDPOINT;GRAPH_EVENTS_ENDPOINT" desc:"The address of the event system. The event system is the message queuing service. It is used as message broker for the microservice architecture. Set to a empty string to disable emitting events." introductionVersion:"1.0.0"` Cluster string `yaml:"cluster" env:"OC_EVENTS_CLUSTER;GRAPH_EVENTS_CLUSTER" desc:"The clusterID of the event system. The event system is the message queuing service. It is used as message broker for the microservice architecture." introductionVersion:"1.0.0"` TLSInsecure bool `yaml:"tls_insecure" env:"OC_INSECURE;OC_EVENTS_TLS_INSECURE;GRAPH_EVENTS_TLS_INSECURE" desc:"Whether to verify the server TLS certificates." introductionVersion:"1.0.0"` @@ -136,6 +144,18 @@ type Events struct { AuthPassword string `yaml:"password" env:"OC_EVENTS_AUTH_PASSWORD;GRAPH_EVENTS_AUTH_PASSWORD" desc:"The password to authenticate with the events broker. The events broker is the OpenCloud service which receives and delivers events between the services." introductionVersion:"1.0.0"` } +func (e Events) ToNatsConfig() stream.NatsConfig { + return stream.NatsConfig{ + Endpoint: e.Endpoint, + Cluster: e.Cluster, + TLSInsecure: e.TLSInsecure, + TLSRootCACertificate: e.TLSRootCACertificate, + EnableTLS: e.EnableTLS, + AuthUsername: e.AuthUsername, + AuthPassword: e.AuthPassword, + } +} + // CORS defines the available cors configuration. type CORS struct { AllowedOrigins []string `yaml:"allow_origins" env:"OC_CORS_ALLOW_ORIGINS;GRAPH_CORS_ALLOW_ORIGINS" desc:"A list of allowed CORS origins. See following chapter for more details: *Access-Control-Allow-Origin* at https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Origin. See the Environment Variable Types description for more details." introductionVersion:"1.0.0"` diff --git a/services/graph/pkg/config/defaults/defaultconfig.go b/services/graph/pkg/config/defaults/defaultconfig.go index f27bcce186..3ae60b3fad 100644 --- a/services/graph/pkg/config/defaults/defaultconfig.go +++ b/services/graph/pkg/config/defaults/defaultconfig.go @@ -43,6 +43,7 @@ func DefaultConfig() *config.Config { Token: "", }, HTTP: config.HTTP{ + Disabled: false, Addr: "127.0.0.1:9120", Namespace: "eu.opencloud.web", Root: "/graph", @@ -52,6 +53,11 @@ func DefaultConfig() *config.Config { AllowedHeaders: []string{"Authorization", "Origin", "Content-Type", "Accept", "X-Requested-With", "X-Request-Id", "Purge", "Restore"}, AllowCredentials: true, }, + Metrics: config.HTTPMetrics{ + // disabling inbound HTTP metrics collection by default for now, since the runtime performance impact is currently unclear; + // it is most likely to be negligible, but has not been measured yet to confirm + Disabled: true, + }, }, Service: config.Service{ Name: "graph", @@ -109,6 +115,11 @@ func DefaultConfig() *config.Config { GroupMemberAttribute: "member", GroupIDAttribute: "openCloudUUID", EducationResourcesEnabled: false, + Metrics: config.LDAPMetrics{ + // disabling inbound HTTP metrics collection by default for now, since the runtime performance impact is currently unclear; + // it is most likely to be negligible, but has not been measured yet to confirm + Disabled: true, + }, }, }, Cache: &config.Cache{ @@ -118,9 +129,10 @@ func DefaultConfig() *config.Config { TTL: time.Hour * 24, }, Events: config.Events{ - Endpoint: "127.0.0.1:9233", - Cluster: "opencloud-cluster", - EnableTLS: false, + DisabledConsumer: false, + Endpoint: "127.0.0.1:9233", + Cluster: "opencloud-cluster", + EnableTLS: false, }, MaxConcurrency: 20, UnifiedRoles: config.UnifiedRoles{ diff --git a/services/graph/pkg/config/http.go b/services/graph/pkg/config/http.go index dca2a55cfd..98f49f3f40 100644 --- a/services/graph/pkg/config/http.go +++ b/services/graph/pkg/config/http.go @@ -2,12 +2,18 @@ package config import "github.com/opencloud-eu/opencloud/pkg/shared" +type HTTPMetrics struct { + Disabled bool `yaml:"disabled" env:"GRAPH_HTTP_METRICS_DISABLE" desc:"Disables the metrics for the HTTP service." introductionVersion:"%NEXT%"` +} + // HTTP defines the available http configuration. type HTTP struct { + Disabled bool `yaml:"disabled" env:"GRAPH_HTTP_DISABLE" desc:"Disables the HTTP service. Set this to true if the service should only consume events." introductionVersion:"%NEXT%"` Addr string `yaml:"addr" env:"GRAPH_HTTP_ADDR" desc:"The bind address of the HTTP service." introductionVersion:"1.0.0"` Namespace string `yaml:"-"` Root string `yaml:"root" env:"GRAPH_HTTP_ROOT" desc:"Subdirectory that serves as the root for this HTTP service." introductionVersion:"1.0.0"` TLS shared.HTTPServiceTLS `yaml:"tls"` APIToken string `yaml:"apitoken" env:"GRAPH_HTTP_API_TOKEN" desc:"An optional API bearer token" introductionVersion:"1.0.0"` CORS CORS `yaml:"cors"` + Metrics HTTPMetrics `yaml:"metrics"` } diff --git a/services/graph/pkg/config/parser/parse.go b/services/graph/pkg/config/parser/parse.go index eb400899b1..58f6a77173 100644 --- a/services/graph/pkg/config/parser/parse.go +++ b/services/graph/pkg/config/parser/parse.go @@ -39,6 +39,14 @@ func ParseConfig(cfg *config.Config) error { } func Validate(cfg *config.Config) error { + if cfg.HTTP.Disabled && cfg.Events.DisabledConsumer { + // might be debatable, but this situation should be treated as an error, + // as the process wouldn't be able to serve either API and would thus be + // completely useless -- in that case, just don't start this service + // in the first place (especially since it's optional) + return errors.New("both HTTP and events consumption APIs are disabled by configuration; at least one must be enabled") + } + if cfg.TokenManager.JWTSecret == "" { return shared.MissingJWTTokenError(cfg.Service.Name) } diff --git a/services/graph/pkg/errorcode/errorcode.go b/services/graph/pkg/errorcode/errorcode.go index cc9cce63aa..2a9433f17d 100644 --- a/services/graph/pkg/errorcode/errorcode.go +++ b/services/graph/pkg/errorcode/errorcode.go @@ -50,6 +50,8 @@ const ( InvalidRequest // ItemNotFound defines the error if the resource could not be found. ItemNotFound + // TooManyResults defines the error if multiple results are found for a unique resource. + TooManyResults // MalwareDetected defines the error if malware was detected in the requested resource. MalwareDetected // NameAlreadyExists defines the error if the specified item name already exists. diff --git a/services/graph/pkg/identity/backend.go b/services/graph/pkg/identity/backend.go index 342af7ade9..39f0b8bfce 100644 --- a/services/graph/pkg/identity/backend.go +++ b/services/graph/pkg/identity/backend.go @@ -18,6 +18,8 @@ var ( ErrReadOnly = errorcode.New(errorcode.NotAllowed, "server is configured read-only") // ErrNotFound signals that the requested resource was not found. ErrNotFound = errorcode.New(errorcode.ItemNotFound, "not found") + // ErrTooManyResults signals that multiple results were found when only one was expected + ErrTooManyResults = errorcode.New(errorcode.TooManyResults, "too many results") // ErrUnsupportedFilter signals that the requested filter is not supported by the backend. ErrUnsupportedFilter = godata.NotImplementedError("unsupported filter") ) @@ -33,21 +35,28 @@ type Backend interface { // CreateUser creates a given user in the identity backend. CreateUser(ctx context.Context, user libregraph.User) (*libregraph.User, error) // DeleteUser deletes a given user, identified by username or id, from the backend - DeleteUser(ctx context.Context, nameOrID string) error + DeleteUser(ctx context.Context, nameOrID string) (bool, error) // UpdateUser applies changes to given user, identified by username or id UpdateUser(ctx context.Context, nameOrID string, user libregraph.UserUpdate) (*libregraph.User, error) GetUser(ctx context.Context, nameOrID string, oreq *godata.GoDataRequest) (*libregraph.User, error) GetUsers(ctx context.Context, oreq *godata.GoDataRequest) ([]*libregraph.User, error) // FilterUsers returns a list of users that match the filter FilterUsers(ctx context.Context, oreq *godata.GoDataRequest, filter *godata.ParseNode) ([]*libregraph.User, error) - UpdateLastSignInDate(ctx context.Context, userID string, timestamp time.Time) error - + // Update the last sign-in date of a given user. + // + // Returns a boolean which is set to true if the sign-in date was updated, or false if not. + // + // Cases in which it may return no error but false for the boolean may be: + // - the backend does not support write operations + // - the backend does not support last sign-in dates + // - the user could not be found in the backend storage + UpdateLastSignInDate(ctx context.Context, userID string, timestamp time.Time) (bool, error) // CreateGroup creates the supplied group in the identity backend. CreateGroup(ctx context.Context, group libregraph.Group) (*libregraph.Group, error) // DeleteGroup deletes a given group, identified by id - DeleteGroup(ctx context.Context, id string) error + DeleteGroup(ctx context.Context, id string) (foundGroup bool, err error) // UpdateGroupName updates the group name - UpdateGroupName(ctx context.Context, groupID string, groupName string) error + UpdateGroupName(ctx context.Context, groupID string, groupName string) (foundGroup bool, err error) GetGroup(ctx context.Context, nameOrID string, queryParam url.Values) (*libregraph.Group, error) GetGroups(ctx context.Context, oreq *godata.GoDataRequest) ([]*libregraph.Group, error) // GetGroupMembers list all members of a group @@ -55,7 +64,7 @@ type Backend interface { // AddMembersToGroup adds new members (reference by a slice of IDs) to supplied group in the identity backend. AddMembersToGroup(ctx context.Context, groupID string, memberID []string) error // RemoveMemberFromGroup removes a single member (by ID) from a group - RemoveMemberFromGroup(ctx context.Context, groupID string, memberID string) error + RemoveMemberFromGroup(ctx context.Context, groupID string, memberID string) (foundGroup bool, foundMember bool, foundMemberInGroup bool, err error) } // EducationBackend defines the Interface for an EducationBackend implementation @@ -63,7 +72,7 @@ type EducationBackend interface { // CreateEducationSchool creates the supplied school in the identity backend. CreateEducationSchool(ctx context.Context, group libregraph.EducationSchool) (*libregraph.EducationSchool, error) // DeleteEducationSchool deletes a given school, identified by id - DeleteEducationSchool(ctx context.Context, id string) error + DeleteEducationSchool(ctx context.Context, id string) (found bool, err error) // GetEducationSchool reads a given school by id GetEducationSchool(ctx context.Context, nameOrID string) (*libregraph.EducationSchool, error) // GetEducationSchools lists all schools @@ -75,9 +84,9 @@ type EducationBackend interface { // GetEducationSchoolUsers lists all members of a school GetEducationSchoolUsers(ctx context.Context, id string) ([]*libregraph.EducationUser, error) // AddUsersToEducationSchool adds new members (reference by a slice of IDs) to supplied school in the identity backend. - AddUsersToEducationSchool(ctx context.Context, schoolID string, memberID []string) error + AddUsersToEducationSchool(ctx context.Context, schoolID string, memberID []string) (found bool, err error) // RemoveUserFromEducationSchool removes a single member (by ID) from a school - RemoveUserFromEducationSchool(ctx context.Context, schoolID string, memberID string) error + RemoveUserFromEducationSchool(ctx context.Context, schoolID string, memberID string) (foundSchool, foundUser, foundUserInSchool bool, err error) // GetEducationSchoolClasses lists all classes in a school GetEducationSchoolClasses(ctx context.Context, schoolNumberOrID string) ([]*libregraph.EducationClass, error) diff --git a/services/graph/pkg/identity/cs3.go b/services/graph/pkg/identity/cs3.go index 93de4ecb3d..cba871ced3 100644 --- a/services/graph/pkg/identity/cs3.go +++ b/services/graph/pkg/identity/cs3.go @@ -28,14 +28,30 @@ type CS3 struct { GatewaySelector pool.Selectable[gateway.GatewayAPIClient] } +var _ Backend = &CS3{} + +func NewCS3Backend(config *shared.Reva, gatewaySelector pool.Selectable[gateway.GatewayAPIClient], logger *log.Logger) (*CS3, error) { + logger = &log.Logger{Logger: logger.With(). + // Str("backend", "cs3"). // already added upstream + Str("gateway", config.Address). + Logger(), + } + + return &CS3{ + Config: config, + GatewaySelector: gatewaySelector, + Logger: logger, + }, nil +} + // CreateUser implements the Backend Interface. It's currently not supported for the CS3 backend func (i *CS3) CreateUser(ctx context.Context, user libregraph.User) (*libregraph.User, error) { return nil, errNotImplemented } // DeleteUser implements the Backend Interface. It's currently not supported for the CS3 backend -func (i *CS3) DeleteUser(ctx context.Context, nameOrID string) error { - return errNotImplemented +func (i *CS3) DeleteUser(ctx context.Context, nameOrID string) (bool, error) { + return false, errNotImplemented } // UpdateUser implements the Backend Interface. It's currently not supported for the CS3 backend @@ -147,8 +163,8 @@ func (i *CS3) FilterUsers(_ context.Context, _ *godata.GoDataRequest, _ *godata. } // UpdateLastSignInDate implements the Backend Interface. It's currently not supported for the CS3 backend -func (i *CS3) UpdateLastSignInDate(ctx context.Context, userID string, timestamp time.Time) error { - return errNotImplemented +func (i *CS3) UpdateLastSignInDate(ctx context.Context, userID string, timestamp time.Time) (bool, error) { + return false, nil } // GetGroups implements the Backend Interface. @@ -236,13 +252,13 @@ func (i *CS3) GetGroup(ctx context.Context, groupID string, queryParam url.Value } // DeleteGroup implements the Backend Interface. It's currently not supported for the CS3 backend -func (i *CS3) DeleteGroup(ctx context.Context, id string) error { - return errNotImplemented +func (i *CS3) DeleteGroup(ctx context.Context, id string) (bool, error) { + return false, errNotImplemented } // UpdateGroupName implements the Backend Interface. It's currently not supported for the CS3 backend -func (i *CS3) UpdateGroupName(ctx context.Context, groupID string, groupName string) error { - return errNotImplemented +func (i *CS3) UpdateGroupName(ctx context.Context, groupID string, groupName string) (bool, error) { + return false, errNotImplemented } // GetGroupMembers implements the Backend Interface. It's currently not supported for the CS3 backend @@ -256,6 +272,6 @@ func (i *CS3) AddMembersToGroup(ctx context.Context, groupID string, memberID [] } // RemoveMemberFromGroup implements the Backend Interface. It's currently not supported for the CS3 backend -func (i *CS3) RemoveMemberFromGroup(ctx context.Context, groupID string, memberID string) error { - return errNotImplemented +func (i *CS3) RemoveMemberFromGroup(ctx context.Context, groupID string, memberID string) (bool, bool, bool, error) { + return false, false, false, errNotImplemented } diff --git a/services/graph/pkg/identity/err_education.go b/services/graph/pkg/identity/err_education.go index 4138299035..8d0cfb47c0 100644 --- a/services/graph/pkg/identity/err_education.go +++ b/services/graph/pkg/identity/err_education.go @@ -9,14 +9,16 @@ import ( // ErrEducationBackend is a dummy EducationBackend, doing nothing type ErrEducationBackend struct{} +var _ EducationBackend = &ErrEducationBackend{} + // CreateEducationSchool creates the supplied school in the identity backend. func (i *ErrEducationBackend) CreateEducationSchool(ctx context.Context, school libregraph.EducationSchool) (*libregraph.EducationSchool, error) { return nil, errNotImplemented } // DeleteEducationSchool deletes a given school, identified by id -func (i *ErrEducationBackend) DeleteEducationSchool(ctx context.Context, id string) error { - return errNotImplemented +func (i *ErrEducationBackend) DeleteEducationSchool(ctx context.Context, id string) (bool, error) { + return false, errNotImplemented } // GetEducationSchool implements the EducationBackend interface for the ErrEducationBackend backend. @@ -60,13 +62,13 @@ func (i *ErrEducationBackend) RemoveClassFromEducationSchool(ctx context.Context } // AddUsersToEducationSchool adds new members (reference by a slice of IDs) to supplied school in the identity backend. -func (i *ErrEducationBackend) AddUsersToEducationSchool(ctx context.Context, schoolID string, memberID []string) error { - return errNotImplemented +func (i *ErrEducationBackend) AddUsersToEducationSchool(ctx context.Context, schoolID string, memberID []string) (bool, error) { + return false, errNotImplemented } // RemoveUserFromEducationSchool removes a single member (by ID) from a school -func (i *ErrEducationBackend) RemoveUserFromEducationSchool(ctx context.Context, schoolID string, memberID string) error { - return errNotImplemented +func (i *ErrEducationBackend) RemoveUserFromEducationSchool(ctx context.Context, schoolID string, memberID string) (bool, bool, bool, error) { + return false, false, false, errNotImplemented } // GetEducationClasses implements the EducationBackend interface diff --git a/services/graph/pkg/identity/factory.go b/services/graph/pkg/identity/factory.go new file mode 100644 index 0000000000..fc23d9985b --- /dev/null +++ b/services/graph/pkg/identity/factory.go @@ -0,0 +1,151 @@ +package identity + +import ( + "crypto/tls" + "crypto/x509" + "errors" + "fmt" + "os" + "strings" + + ldapv3 "github.com/go-ldap/ldap/v3" + ocldap "github.com/opencloud-eu/opencloud/pkg/ldap" + "github.com/opencloud-eu/opencloud/pkg/log" + "github.com/opencloud-eu/opencloud/pkg/registry" + "github.com/opencloud-eu/opencloud/services/graph/pkg/config" + "github.com/opencloud-eu/opencloud/services/graph/pkg/metrics" + "github.com/opencloud-eu/reva/v2/pkg/rgrpc/todo/pool" + "github.com/opencloud-eu/reva/v2/pkg/utils/ldap" + "github.com/prometheus/client_golang/prometheus" + "go.opentelemetry.io/otel/trace" +) + +const ( + cs3Backend = "cs3" + ldapBackend = "ldap" +) + +var supportedBackends = []string{cs3Backend, ldapBackend} + +func CreateIdentityBackends(name string, cfg *config.Config, logger *log.Logger, registrer prometheus.Registerer, traceProvider trace.TracerProvider) (Backend, EducationBackend, error) { + switch name { + case cs3Backend: + gatewaySelector, err := pool.GatewaySelector( + cfg.Reva.Address, + append( + cfg.Reva.GetRevaOptions(), + pool.WithRegistry(registry.GetRegistry()), + pool.WithTracerProvider(traceProvider), + )..., + ) + if err != nil { + return nil, nil, err + } + + if cs3, err := NewCS3Backend(cfg.Reva, gatewaySelector, logger); err != nil { + return nil, nil, err + } else { + return cs3, nil, nil + } + case ldapBackend: + var err error + + var tlsConf *tls.Config + if cfg.Identity.LDAP.Insecure { + // When insecure is set to true then we don't need a certificate. + cfg.Identity.LDAP.CACert = "" + tlsConf = &tls.Config{ + MinVersion: tls.VersionTLS12, + + //nolint:gosec // We need the ability to run with "insecure" (dev/testing) + InsecureSkipVerify: cfg.Identity.LDAP.Insecure, + } + } + + if cfg.Identity.LDAP.CACert != "" { + if err := ocldap.WaitForCA(*logger, + cfg.Identity.LDAP.Insecure, + cfg.Identity.LDAP.CACert); err != nil { + logger.Fatal().Err(err).Msg("The configured LDAP CA cert does not exist") + } + if tlsConf == nil { + tlsConf = &tls.Config{ + MinVersion: tls.VersionTLS12, + } + } + certs := x509.NewCertPool() + pemData, err := os.ReadFile(cfg.Identity.LDAP.CACert) + if err != nil { + logger.Error().Err(err).Msg("Error initializing LDAP Backend") + return nil, nil, err + } + if !certs.AppendCertsFromPEM(pemData) { + logger.Error().Msg("Error initializing LDAP Backend. Adding CA cert failed") + return nil, nil, err + } + tlsConf.RootCAs = certs + } + + ldapConfig := ldap.Config{ + URI: cfg.Identity.LDAP.URI, + BindDN: cfg.Identity.LDAP.BindDN, + BindPassword: cfg.Identity.LDAP.BindPassword, + TLSConfig: tlsConf, + } + + logger = &log.Logger{Logger: logger.With(). + Str("ldap-uri", ldapConfig.URI). + Logger(), + } + + conn := ldap.NewLDAPWithReconnect(ldapConfig) + conn.SetLogger(&logger.Logger) + lb, err := NewLDAPBackend(conn, cfg.Identity.LDAP, logger, metrics.Namespace, metrics.Subsystem, registrer) + if err != nil { + logger.Error().Err(err).Msg("Error initializing LDAP Backend") + return nil, nil, err + } + + identityBackend := lb + var eduBackend EducationBackend = lb + + if !cfg.Identity.LDAP.EducationResourcesEnabled { + eduBackend = &ErrEducationBackend{} + } + + disableMechanismType, err := ParseDisableMechanismType(cfg.Identity.LDAP.DisableUserMechanism) + if err != nil { + logger.Error().Err(err).Msg("Error initializing LDAP Backend") + return nil, nil, err + } + + if disableMechanismType == DisableMechanismGroup { + logger.Info().Msg("LocalUserDisable is true, will create group if not exists") + err := lb.CreateLDAPGroupByDN(cfg.Identity.LDAP.LdapDisabledUsersGroupDN) + if err != nil { + isAnError := false + var lerr *ldapv3.Error + if errors.As(err, &lerr) { + if lerr.ResultCode != ldapv3.LDAPResultEntryAlreadyExists { + isAnError = true + } + } else { + isAnError = true + } + + if isAnError { + msg := "error adding group for disabling users" + logger.Error().Err(err).Str("local_user_disable", cfg.Identity.LDAP.LdapDisabledUsersGroupDN).Msg(msg) + return nil, nil, err + } + } + } + + return identityBackend, eduBackend, nil + + default: + err := fmt.Errorf("unknown identity backend: %q, must be one of [%s]", name, strings.Join(supportedBackends, ", ")) + logger.Error().Err(err).Send() + return nil, nil, err + } +} diff --git a/services/graph/pkg/identity/ldap.go b/services/graph/pkg/identity/ldap.go index 0cd28b23f0..d4ad0e36c2 100644 --- a/services/graph/pkg/identity/ldap.go +++ b/services/graph/pkg/identity/ldap.go @@ -15,10 +15,12 @@ import ( "github.com/google/uuid" "github.com/libregraph/idm/pkg/ldapdn" libregraph "github.com/opencloud-eu/libre-graph-api-go" + "github.com/prometheus/client_golang/prometheus" "github.com/opencloud-eu/opencloud/pkg/log" "github.com/opencloud-eu/opencloud/services/graph/pkg/config" "github.com/opencloud-eu/opencloud/services/graph/pkg/errorcode" + "github.com/opencloud-eu/opencloud/services/graph/pkg/metrics" "github.com/opencloud-eu/opencloud/services/graph/pkg/odata" ) @@ -73,10 +75,15 @@ type LDAP struct { educationConfig educationConfig - logger *log.Logger - conn ldap.Client + logger *log.Logger + metrics *ldapMetrics + + client LdapClient } +var _ Backend = &LDAP{} +var _ EducationBackend = &LDAP{} + type userAttributeMap struct { displayName string id string @@ -107,11 +114,82 @@ func ParseDisableMechanismType(disableMechanism string) (DisableUserMechanismTyp return t, nil } -func NewLDAPBackend(lc ldap.Client, config config.LDAP, logger *log.Logger) (*LDAP, error) { +type ldapMetrics struct { + ApiOperationDuration *prometheus.HistogramVec + LdapEgressDuration *prometheus.HistogramVec +} + +const ( + MetricOpCreateUser = "create-user" + MetricOpDeleteUser = "delete-user" + MetricOpUpdateUser = "update-user" + MetricOpGetUser = "get-user" + MetricOpFilterUsers = "filter-users" + MetricOpUpdateLastSignInDate = "update-last-signin-date" + MetricOpGetGroup = "get-group" + MetricOpGetGroups = "get-groups" + MetricOpCreateGroup = "create-group" + MetricOpDeleteGroup = "delete-group" + MetricOpUpdateGroupName = "update-group-name" + MetricOpAddMembersToGroup = "add-members-to-group" + MetricOpRemoveMemberFromGroup = "remove-member-from-group" + + MetricOpCreateEducationSchool = "create-school" + MetricOpUpdateEducationSchool = "update-school" + MetricOpDeleteEducationSchool = "delete-school" + MetricOpGetEducationSchool = "get-school" + MetricOpGetEducationSchools = "get-schools" + MetricOpFilterEducationSchoolsByAttribute = "filter-schools-byattr" + MetricOpAddUsersToEducationSchool = "add-users-to-school" + MetricOpRemoveUserFromEducationSchool = "remove-user-from-school" + MetricOpGetEducationSchoolClasses = "get-school-classes" + MetricOpAddClassesToEducationSchool = "add-classes-to-school" + MetricOpRemoveClassFromEducationSchool = "remove-class-from-school" + + MetricResultSuccess = "success" + MetricResultFailure = "failure" + MetricResultNotFound = "not-found" + + MetricLabelOperation = "operation" +) + +func newLdapMetrics(namespace string, subsystem string, registry prometheus.Registerer) *ldapMetrics { + m := &ldapMetrics{ + ApiOperationDuration: prometheus.NewHistogramVec(prometheus.HistogramOpts{ + Namespace: namespace, + Subsystem: subsystem, + Name: "ldap_api_duration_seconds", + Help: "Duration of LDAP operations performed by the Graph service in seconds.", + Buckets: prometheus.DefBuckets, + }, []string{MetricLabelOperation, metrics.LabelResult}), + LdapEgressDuration: prometheus.NewHistogramVec(prometheus.HistogramOpts{ + Namespace: namespace, + Subsystem: subsystem, + Name: "ldap_client_operation_duration_seconds", + Help: "Duration of LDAP operations performed by the Graph service in seconds.", + Buckets: prometheus.DefBuckets, + }, []string{MetricLabelOperation, metrics.LabelResult}), + } + + _ = registry.Register(m.ApiOperationDuration) + _ = registry.Register(m.LdapEgressDuration) + + return m +} + +func NewLDAPBackend(lc ldap.Client, config config.LDAP, logger *log.Logger, namespace string, subsystem string, registry prometheus.Registerer) (*LDAP, error) { if config.UserDisplayNameAttribute == "" || config.UserIDAttribute == "" || config.UserEmailAttribute == "" || config.UserNameAttribute == "" { return nil, errors.New("invalid user attribute mappings") } + + logger = &log.Logger{Logger: logger.With(). + // Str("backend", "ldap"). // already added upstream + Bool("write", config.WriteEnabled). + Bool("refint", config.RefintEnabled). + Logger(), + } + uam := userAttributeMap{ displayName: config.UserDisplayNameAttribute, id: config.UserIDAttribute, @@ -154,6 +232,14 @@ func NewLDAPBackend(lc ldap.Client, config config.LDAP, logger *log.Logger) (*LD return nil, fmt.Errorf("error configuring disable user mechanism: %w", err) } + metrics := newLdapMetrics(namespace, subsystem, registry) + + var client LdapClient + client = NewGoLdapLdapClient(lc) + if !config.Metrics.Disabled { + client = NewMetricsLdapClient(client, metrics.LdapEgressDuration) + } + return &LDAP{ useServerUUID: config.UseServerUUID, usePwModifyExOp: config.UsePasswordModExOp, @@ -174,7 +260,8 @@ func NewLDAPBackend(lc ldap.Client, config config.LDAP, logger *log.Logger) (*LD disableUserMechanism: disableMechanismType, localUserDisableGroupDN: config.LdapDisabledUsersGroupDN, logger: logger, - conn: lc, + metrics: metrics, + client: client, writeEnabled: config.WriteEnabled, refintEnabled: config.RefintEnabled, }, nil @@ -183,62 +270,97 @@ func NewLDAPBackend(lc ldap.Client, config config.LDAP, logger *log.Logger) (*LD // CreateUser implements the Backend Interface. It converts the libregraph.User into an // LDAP User Entry (using the inetOrgPerson LDAP Objectclass) add adds that to the // configured LDAP server -func (i *LDAP) CreateUser(ctx context.Context, user libregraph.User) (*libregraph.User, error) { +func (i *LDAP) CreateUser(ctx context.Context, user libregraph.User) (result *libregraph.User, err error) { logger := i.logger.SubloggerWithRequestID(ctx) - logger.Debug().Str("backend", "ldap").Msg("CreateUser") + logger.Debug().Msg("CreateUser") if !i.writeEnabled { return nil, ErrReadOnly } + timer := prometheus.NewTimer(prometheus.ObserverFunc(func(seconds float64) { + result := MetricResultSuccess + if err != nil { + result = MetricResultFailure + } + i.metrics.ApiOperationDuration.WithLabelValues(MetricOpCreateUser, result).Observe(seconds) + })) + defer timer.ObserveDuration() + if user.AccountEnabled != nil && i.disableUserMechanism == DisableMechanismNone { - return nil, errors.New("accountEnabled option not compatible with backend disable user mechanism") + err = errors.New("accountEnabled option not compatible with backend disable user mechanism") + return } ar, err := i.userToAddRequest(user) if err != nil { - return nil, err + return } - if err := i.conn.Add(ar); err != nil { + if addErr := i.client.Add(ar); addErr != nil { msg := "failed to add user" - logger.Error().Err(err).Msg(msg) + logger.Error().Err(addErr).Msg(msg) errMap := ldapResultToErrMap{ ldap.LDAPResultEntryAlreadyExists: errorcode.New(errorcode.NameAlreadyExists, "a user with that name already exists"), ldap.LDAPResultUnwillingToPerform: errorcode.New(errorcode.NotAllowed, msg), ldap.LDAPResultInsufficientAccessRights: errorcode.New(errorcode.NotAllowed, msg), ldapGenericErr: errorcode.New(errorcode.GeneralException, msg), } - return nil, i.mapLDAPError(err, errMap) + err = i.mapLDAPError(addErr, errMap) + return } if i.usePwModifyExOp && user.PasswordProfile != nil && user.PasswordProfile.Password != nil { - if err := i.updateUserPassword(ctx, ar.DN, user.PasswordProfile.GetPassword()); err != nil { - return nil, err + if updateErr := i.updateUserPassword(ctx, ar.DN, user.PasswordProfile.GetPassword()); updateErr != nil { + err = updateErr + return } } // Read back user from LDAP to get the generated UUID e, err := i.getUserByDN(ar.DN, "") if err != nil { - return nil, err + return } - return i.createUserModelFromLDAP(e), nil + result, err = i.createUserModelFromLDAP(e) + return } // DeleteUser implements the Backend Interface. It permanently deletes a User identified // by name or id from the LDAP server -func (i *LDAP) DeleteUser(ctx context.Context, nameOrID string) error { +func (i *LDAP) DeleteUser(ctx context.Context, nameOrID string) (found bool, err error) { logger := i.logger.SubloggerWithRequestID(ctx) - logger.Debug().Str("backend", "ldap").Msg("DeleteUser") + logger.Debug().Msg("DeleteUser") if !i.writeEnabled { - return ErrReadOnly + return false, ErrReadOnly } + + found = false + timer := prometheus.NewTimer(prometheus.ObserverFunc(func(seconds float64) { + result := MetricResultSuccess + if err != nil { + result = MetricResultFailure + } else if !found { + result = MetricResultNotFound + } + i.metrics.ApiOperationDuration.WithLabelValues(MetricOpDeleteUser, result).Observe(seconds) + })) + defer timer.ObserveDuration() + e, err := i.getLDAPUserByNameOrID(nameOrID) if err != nil { - return err + return } + if e == nil { + // user does not exist in LDAP: debatable whether that should be an error, or whether + // it should be silently treated as successful, which is something only the caller can + // decide + return + } + + found = true + dr := ldap.DelRequest{DN: e.DN} - if err = i.conn.Del(&dr); err != nil { + if err = i.client.Del(&dr); err != nil { msg := "error deleting user" logger.Error().Err(err).Msg(msg) errMap := ldapResultToErrMap{ @@ -247,63 +369,87 @@ func (i *LDAP) DeleteUser(ctx context.Context, nameOrID string) error { ldap.LDAPResultInsufficientAccessRights: errorcode.New(errorcode.NotAllowed, msg), ldapGenericErr: errorcode.New(errorcode.GeneralException, msg), } - return i.mapLDAPError(err, errMap) + err = i.mapLDAPError(err, errMap) + return } if !i.refintEnabled { // Find all the groups that this user was a member of and remove it from there - groupEntries, err := i.getLDAPGroupsByFilter(fmt.Sprintf("(%s=%s)", i.groupAttributeMap.member, e.DN), true, false) + var groupEntries []*ldap.Entry + groupEntries, err = i.getLDAPGroupsByFilter(fmt.Sprintf("(%s=%s)", i.groupAttributeMap.member, e.DN), true, false) if err != nil { - return err + return } for _, group := range groupEntries { logger.Debug().Str("group", group.DN).Str("user", e.DN).Msg("Cleaning up group membership") - if err := i.removeEntryByDNAndAttributeFromEntry(group, e.DN, i.groupAttributeMap.member); err != nil { + if err = i.removeEntryByDNAndAttributeFromEntry(group, e.DN, i.groupAttributeMap.member); err != nil { // Errors when deleting the memberships are only logged as warnings but not returned // to the user as we already successfully deleted the users itself logger.Warn().Str("group", group.DN).Str("user", e.DN).Err(err).Msg("failed to remove member") } } } - return nil + return } // UpdateUser implements the Backend Interface for the LDAP Backend -func (i *LDAP) UpdateUser(ctx context.Context, nameOrID string, user libregraph.UserUpdate) (*libregraph.User, error) { +func (i *LDAP) UpdateUser(ctx context.Context, nameOrID string, user libregraph.UserUpdate) (returnUser *libregraph.User, err error) { logger := i.logger.SubloggerWithRequestID(ctx) - logger.Debug().Str("backend", "ldap").Msg("UpdateUser") + logger.Debug().Msg("UpdateUser") if !i.writeEnabled { // still allow to enable/disable user when using DisableMechanismGroup if i.disableUserMechanism == DisableMechanismGroup && isUserEnabledUpdate(user) { - logger.Error().Str("backend", "ldap").Msg("Allowing accountEnabled Update on read-only backend") + logger.Error().Msg("Allowing accountEnabled Update on read-only backend") } else { - return nil, ErrReadOnly + err = ErrReadOnly + return } } + + timer := prometheus.NewTimer(prometheus.ObserverFunc(func(seconds float64) { + r := MetricResultSuccess + if err != nil { + r = MetricResultFailure + } else if returnUser == nil { + r = MetricResultNotFound + } + i.metrics.ApiOperationDuration.WithLabelValues(MetricOpDeleteUser, r).Observe(seconds) + })) + defer timer.ObserveDuration() + e, err := i.getLDAPUserByNameOrID(nameOrID) if err != nil { - return nil, err + return + } + if e == nil { + // user does not exist in LDAP: debatable whether that should be an error, or whether + // it should be treated differently, which is something only the caller can decide; + // instead of returning an error, we return nil for the User result + return } var updateNeeded bool // Don't allow updates of the ID if user.GetId() != "" { - id, err := i.ldapUUIDtoString(e, i.userAttributeMap.id, i.userIDisOctetString) + var id string + id, err = i.ldapUUIDtoString(e, i.userAttributeMap.id, i.userIDisOctetString) if err != nil { i.logger.Warn().Str("dn", e.DN).Str(i.userAttributeMap.id, e.GetEqualFoldAttributeValue(i.userAttributeMap.id)).Msg("Invalid User. Cannot convert UUID") - return nil, errorcode.New(errorcode.GeneralException, "error converting uuid") + err = errorcode.New(errorcode.GeneralException, "error converting uuid") + return } if id != user.GetId() { - return nil, errorcode.New(errorcode.NotAllowed, "changing the UserId is not allowed") + err = errorcode.New(errorcode.NotAllowed, "changing the UserId is not allowed") + return } } if user.GetOnPremisesSamAccountName() != "" { if eu := e.GetEqualFoldAttributeValue(i.userAttributeMap.userName); eu != user.GetOnPremisesSamAccountName() { e, err = i.changeUserName(ctx, e.DN, eu, user.GetOnPremisesSamAccountName()) if err != nil { - return nil, err + return } } } @@ -328,7 +474,7 @@ func (i *LDAP) UpdateUser(ctx context.Context, nameOrID string, user libregraph. if user.PasswordProfile != nil && user.PasswordProfile.GetPassword() != "" { if i.usePwModifyExOp { - if err := i.updateUserPassword(ctx, e.DN, user.PasswordProfile.GetPassword()); err != nil { + if err = i.updateUserPassword(ctx, e.DN, user.PasswordProfile.GetPassword()); err != nil { msg := "error updating user password" logger.Error().Err(err).Msg(msg) errMap := ldapResultToErrMap{ @@ -337,7 +483,8 @@ func (i *LDAP) UpdateUser(ctx context.Context, nameOrID string, user libregraph. ldap.LDAPResultInsufficientAccessRights: errorcode.New(errorcode.NotAllowed, msg), ldapGenericErr: errorcode.New(errorcode.GeneralException, msg), } - return nil, i.mapLDAPError(err, errMap) + err = i.mapLDAPError(err, errMap) + return } } else { // password are hashed server side there is no need to check if the new password @@ -350,9 +497,10 @@ func (i *LDAP) UpdateUser(ctx context.Context, nameOrID string, user libregraph. if identities, ok := user.GetIdentitiesOk(); ok { attrValues := make([]string, 0, len(identities)) for _, identity := range identities { - identityStr, err := i.identityToLDAPAttrValue(identity) + var identityStr string + identityStr, err = i.identityToLDAPAttrValue(identity) if err != nil { - return nil, err + return } attrValues = append(attrValues, identityStr) } @@ -365,19 +513,18 @@ func (i *LDAP) UpdateUser(ctx context.Context, nameOrID string, user libregraph. // "attribute": For the upstream user management service which modifies accountEnabled on the user entry // "group": Makes it possible for local admins to disable users by adding them to a special group if user.AccountEnabled != nil { - un, err := i.updateAccountEnabledState(logger, user.GetAccountEnabled(), e, &mr) - + var un bool + un, err = i.updateAccountEnabledState(logger, user.GetAccountEnabled(), e, &mr) if err != nil { - return nil, err + return } - if un { updateNeeded = true } } if updateNeeded { - if err := i.conn.Modify(&mr); err != nil { + if err = i.client.Modify(&mr); err != nil { msg := "error updating user" logger.Error().Err(err).Msg(msg) errMap := ldapResultToErrMap{ @@ -386,25 +533,30 @@ func (i *LDAP) UpdateUser(ctx context.Context, nameOrID string, user libregraph. ldap.LDAPResultInsufficientAccessRights: errorcode.New(errorcode.NotAllowed, msg), ldapGenericErr: errorcode.New(errorcode.GeneralException, msg), } - return nil, i.mapLDAPError(err, errMap) + err = i.mapLDAPError(err, errMap) + return } } // Read back user from LDAP to get the generated UUID e, err = i.getUserByDN(e.DN, "") if err != nil { - return nil, err + return } - returnUser := i.createUserModelFromLDAP(e) - - // To avoid a ldap lookup for group membership, set the enabled flag to same as input value - // since this would have been updated with group membership from the input anyway. - if user.AccountEnabled != nil && i.disableUserMechanism == DisableMechanismGroup { - returnUser.AccountEnabled = user.AccountEnabled + returnUser, err = i.createUserModelFromLDAP(e) + if err != nil { + return + } + if returnUser != nil { + // To avoid a ldap lookup for group membership, set the enabled flag to same as input value + // since this would have been updated with group membership from the input anyway. + if user.AccountEnabled != nil && i.disableUserMechanism == DisableMechanismGroup { + returnUser.AccountEnabled = user.AccountEnabled + } } - return returnUser, nil + return } func (i *LDAP) getUserByDN(dn, searchTerm string) (*ldap.Entry, error) { @@ -445,19 +597,38 @@ func (i *LDAP) getEntryByDN(dn string, attrs []string, filter string) (*ldap.Ent nil, ) - i.logger.Debug().Str("backend", "ldap"). + i.logger.Debug(). Str("base", searchRequest.BaseDN). Str("filter", searchRequest.Filter). Int("scope", searchRequest.Scope). Int("sizelimit", searchRequest.SizeLimit). Interface("attributes", searchRequest.Attributes). Msg("getEntryByDN") - res, err := i.conn.Search(searchRequest) + res, err := i.client.Search(searchRequest) if err != nil { - i.logger.Error().Err(err).Str("backend", "ldap").Str("dn", dn).Msg("Search ldap by DN failed") - return nil, errorcode.New(errorcode.ItemNotFound, "user lookup failed") + i.logger.Error().Err(err).Str("dn", dn).Msg("Search ldap by DN failed") + msg := "user search failed" + errMap := ldapResultToErrMap{ + ldap.LDAPResultNoSuchObject: ErrNotFound, + ldap.LDAPResultUnwillingToPerform: errorcode.New(errorcode.NotAllowed, msg), + ldap.LDAPResultInsufficientAccessRights: errorcode.New(errorcode.NotAllowed, msg), + ldapGenericErr: errorcode.New(errorcode.GeneralException, msg), + } + err = i.mapLDAPError(err, errMap) + if err == ErrNotFound { + // TODO: XXX returning nil instead of an error is not implemented yet, need to check all callers first + // when the entry could not be found, including due to a missing parent, instead of + // returning a ErrNotFound, we return nil for the entry, but no error, to allow for + // callers to deal with that accordingly + // return nil, nil + return nil, err + } else { + return nil, i.mapLDAPError(err, errMap) + } } if len(res.Entries) == 0 { + // TODO: XXX same as above, return nil instead of error + // return nil, nil return nil, ErrNotFound } @@ -478,23 +649,34 @@ func (i *LDAP) searchLDAPEntryByFilter(basedn string, attrs []string, filter str nil, ) - i.logger.Debug().Str("backend", "ldap"). + i.logger.Debug(). Str("base", searchRequest.BaseDN). Str("filter", searchRequest.Filter). Int("scope", searchRequest.Scope). Int("sizelimit", searchRequest.SizeLimit). Interface("attributes", searchRequest.Attributes). Msg("getEntryByFilter") - res, err := i.conn.Search(searchRequest) + res, err := i.client.Search(searchRequest) if err != nil { - i.logger.Error().Err(err).Str("backend", "ldap").Str("dn", basedn).Str("filter", filter).Msg("Search user by filter failed") - return nil, errorcode.New(errorcode.ItemNotFound, "user search failed") + i.logger.Error().Err(err).Str("dn", basedn).Str("filter", filter).Msg("Search user by filter failed") + msg := "user search failed" + errMap := ldapResultToErrMap{ + ldap.LDAPResultNoSuchObject: ErrNotFound, + ldap.LDAPResultUnwillingToPerform: errorcode.New(errorcode.NotAllowed, msg), + ldap.LDAPResultInsufficientAccessRights: errorcode.New(errorcode.NotAllowed, msg), + ldapGenericErr: errorcode.New(errorcode.GeneralException, msg), + } + return nil, i.mapLDAPError(err, errMap) } - if len(res.Entries) == 0 { - return nil, ErrNotFound + switch len(res.Entries) { + case 0: + // this situation used to be treated as an error, by returning ErrNotFound + return nil, nil + case 1: + return res.Entries[0], nil + default: + return nil, ErrTooManyResults } - - return res.Entries[0], nil } func filterEscapeAttribute(attribute string, binary bool, id string) (string, error) { @@ -570,18 +752,38 @@ func (i *LDAP) getLDAPUserByFilter(filter string) (*ldap.Entry, error) { } // GetUser implements the Backend Interface. -func (i *LDAP) GetUser(ctx context.Context, nameOrID string, oreq *godata.GoDataRequest) (*libregraph.User, error) { +func (i *LDAP) GetUser(ctx context.Context, nameOrID string, oreq *godata.GoDataRequest) (u *libregraph.User, err error) { logger := i.logger.SubloggerWithRequestID(ctx) - logger.Debug().Str("backend", "ldap").Msg("GetUser") + logger.Debug().Msg("GetUser") + + timer := prometheus.NewTimer(prometheus.ObserverFunc(func(seconds float64) { + r := MetricResultSuccess + if err != nil { + r = MetricResultFailure + } else if u == nil { + r = MetricResultNotFound + } + i.metrics.ApiOperationDuration.WithLabelValues(MetricOpGetUser, r).Observe(seconds) + })) + defer timer.ObserveDuration() e, err := i.getLDAPUserByNameOrID(nameOrID) if err != nil { - return nil, err + return + } + if e == nil { + // this used to be treated as an error (ErrNotFound), but only the caller can really decide whether + // this situation is an error or not, and react appropriately + return } - u := i.createUserModelFromLDAP(e) + u, err = i.createUserModelFromLDAP(e) + if err != nil { + return + } if u == nil { - return nil, ErrNotFound + err = ErrNotFound + return } if i.disableUserMechanism != DisableMechanismNone { @@ -593,17 +795,18 @@ func (i *LDAP) GetUser(ctx context.Context, nameOrID string, oreq *godata.GoData exp, err := odata.GetExpandValues(oreq.Query) if err != nil { - return nil, err + return } if slices.Contains(exp, "memberOf") { - userGroups, err := i.getGroupsForUser(e.DN) - if err != nil { - return nil, err + userGroups, geterr := i.getGroupsForUser(e.DN) + if geterr != nil { + err = geterr + return } u.MemberOf = i.groupsFromLDAPEntries(userGroups) } - return u, nil + return } // GetUsers implements the Backend Interface. @@ -612,23 +815,32 @@ func (i *LDAP) GetUsers(ctx context.Context, oreq *godata.GoDataRequest) ([]*lib } // FilterUsers implements the Backend Interface. -func (i *LDAP) FilterUsers(ctx context.Context, oreq *godata.GoDataRequest, filter *godata.ParseNode) ([]*libregraph.User, error) { +func (i *LDAP) FilterUsers(ctx context.Context, oreq *godata.GoDataRequest, filter *godata.ParseNode) (result []*libregraph.User, err error) { logger := i.logger.SubloggerWithRequestID(ctx) - logger.Debug().Str("backend", "ldap").Msg("GetUsers") + logger.Debug().Msg("GetUsers") + + timer := prometheus.NewTimer(prometheus.ObserverFunc(func(seconds float64) { + result := MetricResultSuccess + if err != nil { + result = MetricResultFailure + } + i.metrics.ApiOperationDuration.WithLabelValues(MetricOpGetUser, result).Observe(seconds) + })) + defer timer.ObserveDuration() queryFilter, err := i.oDataFilterToLDAPFilter(filter) if err != nil { - return nil, err + return } search, err := odata.GetSearchValues(oreq.Query) if err != nil { - return nil, err + return } exp, err := odata.GetExpandValues(oreq.Query) if err != nil { - return nil, err + return } var userFilter string @@ -648,14 +860,14 @@ func (i *LDAP) FilterUsers(ctx context.Context, oreq *godata.GoDataRequest, filt i.getUserAttrTypesForSearch(), nil, ) - logger.Debug().Str("backend", "ldap"). + logger.Debug(). Str("base", searchRequest.BaseDN). Str("filter", searchRequest.Filter). Int("scope", searchRequest.Scope). Int("sizelimit", searchRequest.SizeLimit). Interface("attributes", searchRequest.Attributes). Msg("GetUsers") - res, err := i.conn.Search(searchRequest) + res, err := i.client.Search(searchRequest) if err != nil { msg := "error listing users" logger.Error().Err(err).Msg(msg) @@ -663,10 +875,12 @@ func (i *LDAP) FilterUsers(ctx context.Context, oreq *godata.GoDataRequest, filt ldap.LDAPResultInsufficientAccessRights: errorcode.New(errorcode.AccessDenied, msg), ldapGenericErr: errorcode.New(errorcode.GeneralException, msg), } - return nil, i.mapLDAPError(err, errMap) + err = i.mapLDAPError(err, errMap) + return } - return i.usersFromLDAPEntries(res.Entries, exp) + result, err = i.usersFromLDAPEntries(res.Entries, exp) + return } func (i *LDAP) usersFromLDAPEntries(entries []*ldap.Entry, exp []string) ([]*libregraph.User, error) { @@ -676,9 +890,9 @@ func (i *LDAP) usersFromLDAPEntries(entries []*ldap.Entry, exp []string) ([]*lib } users := make([]*libregraph.User, 0, len(entries)) for _, e := range entries { - u := i.createUserModelFromLDAP(e) - // Skip invalid LDAP users - if u == nil { + u, err := i.createUserModelFromLDAP(e) + if u == nil || err != nil { + // Skip invalid LDAP users continue } @@ -700,23 +914,37 @@ func (i *LDAP) usersFromLDAPEntries(entries []*ldap.Entry, exp []string) ([]*lib } // UpdateLastSignInDate implements the Backend Interface. -func (i *LDAP) UpdateLastSignInDate(ctx context.Context, userID string, timestamp time.Time) error { +func (i *LDAP) UpdateLastSignInDate(ctx context.Context, userID string, timestamp time.Time) (supported bool, err error) { if !i.writeEnabled { - i.logger.Debug().Str("backend", "ldap").Msg("The LDAP Server is readonly. Skipping update of last sign in date") - return nil + i.logger.Debug().Msg("The LDAP Server is readonly. Skipping update of last sign in date") + return false, nil } + + timer := prometheus.NewTimer(prometheus.ObserverFunc(func(seconds float64) { + result := MetricResultSuccess + if err != nil { + result = MetricResultFailure + } + i.metrics.ApiOperationDuration.WithLabelValues(MetricOpGetUser, result).Observe(seconds) + })) + defer timer.ObserveDuration() + + supported = true + e, err := i.getLDAPUserByID(userID) switch { case errors.Is(err, ErrNotFound): i.logger.Warn().Err(err).Str("userID", userID).Msg("Failed to update last sign in date for user") - return nil + err = nil + return case err != nil: - return err + return } mr := ldap.ModifyRequest{DN: e.DN} mr.Replace(lastSignAttribute, []string{timestamp.UTC().Format(ldapDateFormat)}) - if err := i.conn.Modify(&mr); err != nil { + if moderr := i.client.Modify(&mr); moderr != nil { + err = moderr msg := "error updating last sign in date for user" i.logger.Error().Err(err).Str("userid", userID).Msg(msg) errMap := ldapResultToErrMap{ @@ -725,10 +953,11 @@ func (i *LDAP) UpdateLastSignInDate(ctx context.Context, userID string, timestam ldap.LDAPResultInsufficientAccessRights: errorcode.New(errorcode.NotAllowed, msg), ldapGenericErr: errorcode.New(errorcode.GeneralException, msg), } - return i.mapLDAPError(err, errMap) + err = i.mapLDAPError(err, errMap) + return } - return nil + return } func (i *LDAP) changeUserName(ctx context.Context, dn, originalUserName, newUserName string) (*ldap.Entry, error) { @@ -743,7 +972,7 @@ func (i *LDAP) changeUserName(ctx context.Context, dn, originalUserName, newUser logger.Debug().Str("originalDN", dn).Str("newDN", newDNString).Msg("Modifying DN") mrdn := ldap.NewModifyDNRequest(dn, newDNString, true, "") - if err := i.conn.ModifyDN(mrdn); err != nil { + if err := i.client.ModifyDN(mrdn); err != nil { msg := "error renaming user" logger.Error().Err(err).Msg(msg) errMap := ldapResultToErrMap{ @@ -797,7 +1026,7 @@ func (i *LDAP) renameMemberInGroup(ctx context.Context, group *ldap.Entry, oldMe mr := ldap.NewModifyRequest(group.DN, nil) mr.Delete(i.groupAttributeMap.member, []string{oldMember}) mr.Add(i.groupAttributeMap.member, []string{newMember}) - if err := i.conn.Modify(mr); err != nil { + if err := i.client.Modify(mr); err != nil { logger.Warn().Err(err). Str("oldMember", oldMember). Str("newMember", newMember). @@ -819,14 +1048,14 @@ func (i *LDAP) renameMemberInGroup(ctx context.Context, group *ldap.Entry, oldMe func (i *LDAP) updateUserPassword(ctx context.Context, dn, password string) error { logger := i.logger.SubloggerWithRequestID(ctx) - logger.Debug().Str("backend", "ldap").Msg("updateUserPassword") + logger.Debug().Msg("updateUserPassword") pwMod := ldap.PasswordModifyRequest{ UserIdentity: dn, NewPassword: password, } // Note: We can ignore the result message here, as it were only relevant if we requested // the server to generate a new Password - _, err := i.conn.PasswordModify(&pwMod) + _, err := i.client.PasswordModify(&pwMod) if err != nil { var lerr *ldap.Error logger.Debug().Err(err).Msg("error setting password for user") @@ -860,9 +1089,9 @@ func (i *LDAP) ldapUUIDtoString(e *ldap.Entry, attribute string, binary bool) (s return e.GetEqualFoldAttributeValue(attribute), nil } -func (i *LDAP) createUserModelFromLDAP(e *ldap.Entry) *libregraph.User { +func (i *LDAP) createUserModelFromLDAP(e *ldap.Entry) (*libregraph.User, error) { if e == nil { - return nil + return nil, nil } opsan := e.GetEqualFoldAttributeValue(i.userAttributeMap.userName) @@ -910,10 +1139,12 @@ func (i *LDAP) createUserModelFromLDAP(e *ldap.Entry) *libregraph.User { case !errors.Is(err, errNotSet): i.logger.Warn().Err(err).Str("dn", e.DN).Msg("Error getting last signin timestamp") } - return user + return user, nil } + + err = errorcode.New(errorcode.GeneralException, "Invalid User. Missing username or id attribute") i.logger.Warn().Str("dn", e.DN).Str("id", id).Str("username", opsan).Msg("Invalid User. Missing username or id attribute") - return nil + return nil, err } func (i *LDAP) userToLDAPAttrValues(user libregraph.User) (map[string][]string, error) { @@ -1082,7 +1313,7 @@ func (i *LDAP) removeEntryByDNAndAttributeFromEntry(entry *ldap.Entry, dn string } } if !found { - i.logger.Error().Str("backend", "ldap").Str("entry", entry.DN).Str("target", dn). + i.logger.Error().Str("entry", entry.DN).Str("target", dn). Msg("The target value is not present in the attribute list") return ErrNotFound } @@ -1093,7 +1324,7 @@ func (i *LDAP) removeEntryByDNAndAttributeFromEntry(entry *ldap.Entry, dn string } mr.Delete(attribute, []string{dn}) - err = i.conn.Modify(mr) + err = i.client.Modify(mr) var lerr *ldap.Error if err != nil && errors.As(err, &lerr) { if lerr.ResultCode == ldap.LDAPResultObjectClassViolation { @@ -1103,7 +1334,7 @@ func (i *LDAP) removeEntryByDNAndAttributeFromEntry(entry *ldap.Entry, dn string i.logger.Debug().Err(err). Msg("Failed to remove last group member. Retrying once. Replacing last group member with an empty member value.") mr.Add(attribute, []string{""}) - err = i.conn.Modify(mr) + err = i.client.Modify(mr) } } @@ -1126,7 +1357,7 @@ func (i *LDAP) removeEntryByDNAndAttributeFromEntry(entry *ldap.Entry, dn string // expandLDAPAttributeEntries reads an attribute from a ldap entry and expands to users func (i *LDAP) expandLDAPAttributeEntries(ctx context.Context, e *ldap.Entry, attribute, searchTerm string) ([]*ldap.Entry, error) { logger := i.logger.SubloggerWithRequestID(ctx) - logger.Debug().Str("backend", "ldap").Msg("ExpandLDAPAttributeEntries") + logger.Debug().Msg("ExpandLDAPAttributeEntries") result := []*ldap.Entry{} for _, entryDN := range e.GetEqualFoldAttributeValues(attribute) { @@ -1176,21 +1407,23 @@ func (i *LDAP) CreateLDAPGroupByDN(dn string) error { ar.Attribute(attrType, values) } - return i.conn.Add(ar) + return i.client.Add(ar) } func (i *LDAP) addUserToDisableGroup(logger log.Logger, userDN string) (err error) { groupFilter := fmt.Sprintf("(objectClass=%s)", i.groupObjectClass) group, err := i.getEntryByDN(i.localUserDisableGroupDN, []string{i.groupAttributeMap.member}, groupFilter) - if err != nil { return err } + if group == nil { + return ErrNotFound + } mr := ldap.ModifyRequest{DN: group.DN} mr.Add(i.groupAttributeMap.member, []string{userDN}) - err = i.conn.Modify(&mr) + err = i.client.Modify(&mr) var lerr *ldap.Error if errors.As(err, &lerr) { // If the user is already in the group, just log a message and return @@ -1206,15 +1439,17 @@ func (i *LDAP) addUserToDisableGroup(logger log.Logger, userDN string) (err erro func (i *LDAP) removeUserFromDisableGroup(logger log.Logger, userDN string) (err error) { groupFilter := fmt.Sprintf("(objectClass=%s)", i.groupObjectClass) group, err := i.getEntryByDN(i.localUserDisableGroupDN, []string{i.groupAttributeMap.member}, groupFilter) - if err != nil { return err } + if group == nil { + return ErrNotFound + } mr := ldap.ModifyRequest{DN: group.DN} mr.Delete(i.groupAttributeMap.member, []string{userDN}) - err = i.conn.Modify(&mr) + err = i.client.Modify(&mr) var lerr *ldap.Error if errors.As(err, &lerr) { // If the user is not in the group, just log a message and return @@ -1240,10 +1475,12 @@ func (i *LDAP) userEnabledByAttribute(user *ldap.Entry) bool { func (i *LDAP) usersEnabledStateFromGroup(users []string) (usersEnabledState map[string]bool, err error) { groupFilter := fmt.Sprintf("(objectClass=%s)", i.groupObjectClass) group, err := i.getEntryByDN(i.localUserDisableGroupDN, []string{i.groupAttributeMap.member}, groupFilter) - if err != nil { return nil, err } + if group == nil { + return nil, ErrNotFound + } usersEnabledState = make(map[string]bool, len(users)) for _, user := range users { diff --git a/services/graph/pkg/identity/ldap_client.go b/services/graph/pkg/identity/ldap_client.go new file mode 100644 index 0000000000..bee64b03a6 --- /dev/null +++ b/services/graph/pkg/identity/ldap_client.go @@ -0,0 +1,14 @@ +package identity + +import ( + "github.com/go-ldap/ldap/v3" +) + +type LdapClient interface { + Add(*ldap.AddRequest) error + Del(*ldap.DelRequest) error + Modify(*ldap.ModifyRequest) error + ModifyDN(*ldap.ModifyDNRequest) error + PasswordModify(*ldap.PasswordModifyRequest) (*ldap.PasswordModifyResult, error) + Search(*ldap.SearchRequest) (*ldap.SearchResult, error) +} diff --git a/services/graph/pkg/identity/ldap_client_goldap.go b/services/graph/pkg/identity/ldap_client_goldap.go new file mode 100644 index 0000000000..3b3950db9e --- /dev/null +++ b/services/graph/pkg/identity/ldap_client_goldap.go @@ -0,0 +1,36 @@ +package identity + +import ( + "github.com/go-ldap/ldap/v3" +) + +// implementation that adapts the go-ldap ldap.Client interface +// and delegates everything to a proper LDAP client +type GoLdapLdapClient struct { + delegate ldap.Client +} + +var _ LdapClient = &GoLdapLdapClient{} + +func NewGoLdapLdapClient(delegate ldap.Client) *GoLdapLdapClient { + return &GoLdapLdapClient{delegate: delegate} +} + +func (c *GoLdapLdapClient) Add(r *ldap.AddRequest) error { + return c.delegate.Add(r) +} +func (c *GoLdapLdapClient) Del(r *ldap.DelRequest) error { + return c.delegate.Del(r) +} +func (c *GoLdapLdapClient) Modify(r *ldap.ModifyRequest) error { + return c.delegate.Modify(r) +} +func (c *GoLdapLdapClient) ModifyDN(r *ldap.ModifyDNRequest) error { + return c.delegate.ModifyDN(r) +} +func (c *GoLdapLdapClient) PasswordModify(r *ldap.PasswordModifyRequest) (*ldap.PasswordModifyResult, error) { + return c.delegate.PasswordModify(r) +} +func (c *GoLdapLdapClient) Search(r *ldap.SearchRequest) (*ldap.SearchResult, error) { + return c.delegate.Search(r) +} diff --git a/services/graph/pkg/identity/ldap_client_metrics.go b/services/graph/pkg/identity/ldap_client_metrics.go new file mode 100644 index 0000000000..cb19ee9880 --- /dev/null +++ b/services/graph/pkg/identity/ldap_client_metrics.go @@ -0,0 +1,100 @@ +package identity + +import ( + "time" + + "github.com/go-ldap/ldap/v3" + "github.com/opencloud-eu/opencloud/services/graph/pkg/metrics" + "github.com/prometheus/client_golang/prometheus" +) + +// decorator implementation that wraps another LdapClient and delegates +// everything to it, measuring the time taken for each operation +// and recording it into a histogram metric +type MetricsLdapClient struct { + delegate LdapClient + metric *prometheus.HistogramVec +} + +var _ LdapClient = &MetricsLdapClient{} + +// Create an LdapClient that decorates another LdapClient delegate with metrics recording. +// +// The histogram metric is expected to take two value labels: +// - the name of the LDAP operation ('add', 'delete', ...) +// - the result of the operation ('success' or 'failure') +func NewMetricsLdapClient(delegate LdapClient, metric *prometheus.HistogramVec) *MetricsLdapClient { + return &MetricsLdapClient{delegate: delegate, metric: metric} +} + +func (c *MetricsLdapClient) Add(r *ldap.AddRequest) error { + start := time.Now() + err := c.delegate.Add(r) + duration := time.Since(start).Seconds() + result := metrics.ResultSuccess + if err != nil { + result = metrics.ResultFailure + } + c.metric.WithLabelValues("add", result).Observe(duration) + return err +} + +func (c *MetricsLdapClient) Del(r *ldap.DelRequest) error { + start := time.Now() + err := c.delegate.Del(r) + duration := time.Since(start).Seconds() + result := metrics.ResultSuccess + if err != nil { + result = metrics.ResultFailure + } + c.metric.WithLabelValues("delete", result).Observe(duration) + return err +} + +func (c *MetricsLdapClient) Modify(r *ldap.ModifyRequest) error { + start := time.Now() + err := c.delegate.Modify(r) + duration := time.Since(start).Seconds() + result := metrics.ResultSuccess + if err != nil { + result = metrics.ResultFailure + } + c.metric.WithLabelValues("modify", result).Observe(duration) + return err +} + +func (c *MetricsLdapClient) ModifyDN(r *ldap.ModifyDNRequest) error { + start := time.Now() + err := c.delegate.ModifyDN(r) + duration := time.Since(start).Seconds() + result := metrics.ResultSuccess + if err != nil { + result = metrics.ResultFailure + } + c.metric.WithLabelValues("modify-dn", result).Observe(duration) + return err +} + +func (c *MetricsLdapClient) PasswordModify(r *ldap.PasswordModifyRequest) (*ldap.PasswordModifyResult, error) { + start := time.Now() + pmr, err := c.delegate.PasswordModify(r) + duration := time.Since(start).Seconds() + result := metrics.ResultSuccess + if err != nil { + result = metrics.ResultFailure + } + c.metric.WithLabelValues("modify-password", result).Observe(duration) + return pmr, err +} + +func (c *MetricsLdapClient) Search(r *ldap.SearchRequest) (*ldap.SearchResult, error) { + start := time.Now() + sr, err := c.delegate.Search(r) + duration := time.Since(start).Seconds() + result := metrics.ResultSuccess + if err != nil { + result = metrics.ResultFailure + } + c.metric.WithLabelValues("search", result).Observe(duration) + return sr, err +} diff --git a/services/graph/pkg/identity/ldap_education_class.go b/services/graph/pkg/identity/ldap_education_class.go index f9c3d47cbc..da484a0362 100644 --- a/services/graph/pkg/identity/ldap_education_class.go +++ b/services/graph/pkg/identity/ldap_education_class.go @@ -47,7 +47,7 @@ func (i *LDAP) GetEducationClasses(ctx context.Context) ([]*libregraph.Education Int("sizelimit", searchRequest.SizeLimit). Interface("attributes", searchRequest.Attributes). Msg("GetEducationClasses") - res, err := i.conn.Search(searchRequest) + res, err := i.client.Search(searchRequest) if err != nil { return nil, errorcode.New(errorcode.ItemNotFound, err.Error()) } @@ -78,7 +78,7 @@ func (i *LDAP) CreateEducationClass(ctx context.Context, class libregraph.Educat return nil, err } - if err := i.conn.Add(ar); err != nil { + if err := i.client.Add(ar); err != nil { var lerr *ldap.Error logger.Debug().Err(err).Msg("error adding class") if errors.As(err, &lerr) { @@ -125,7 +125,7 @@ func (i *LDAP) DeleteEducationClass(ctx context.Context, id string) error { } dr := ldap.DelRequest{DN: e.DN} - if err = i.conn.Del(&dr); err != nil { + if err = i.client.Del(&dr); err != nil { return err } @@ -146,6 +146,15 @@ func (i *LDAP) UpdateEducationClass(ctx context.Context, id string, class libreg if err != nil { return nil, err } + if g == nil { + // group does not exist in LDAP: debatable whether that should be an error, or whether + // it should be silently treated as successful, which is something only the caller can + // decide + // + // currently, for full backwards compatibility of the EducationBackend interface, + // this is still treated as an error: + return nil, errorcode.New(errorcode.ItemNotFound, "group not found") + } var updateNeeded bool @@ -193,7 +202,7 @@ func (i *LDAP) UpdateEducationClass(ctx context.Context, id string, class libreg } if updateNeeded { - if err := i.conn.Modify(&mr); err != nil { + if err := i.client.Modify(&mr); err != nil { return nil, err } } @@ -216,7 +225,7 @@ func (i *LDAP) updateClassExternalID(ctx context.Context, dn, externalID string) Str("newrdn", mrdn.NewRDN). Msg("updating class external ID") - if err := i.conn.ModifyDN(mrdn); err != nil { + if err := i.client.ModifyDN(mrdn); err != nil { var lerr *ldap.Error logger.Debug().Err(err).Msg("error updating class external ID") if errors.As(err, &lerr) { @@ -245,7 +254,7 @@ func (i *LDAP) GetEducationClassMembers(ctx context.Context, id string) ([]*libr return nil, err } for _, member := range memberEntries { - if u := i.createEducationUserModelFromLDAP(member); u != nil { + if u, err := i.createEducationUserModelFromLDAP(member); u != nil && err == nil { result = append(result, u) } } @@ -372,7 +381,7 @@ func (i *LDAP) GetEducationClassTeachers(ctx context.Context, classID string) ([ return nil, err } for _, teacher := range teacherEntries { - if u := i.createEducationUserModelFromLDAP(teacher); u != nil { + if u, err := i.createEducationUserModelFromLDAP(teacher); u != nil && err == nil { result = append(result, u) } } @@ -437,7 +446,7 @@ func (i *LDAP) AddTeacherToEducationClass(ctx context.Context, classID string, t if len(newTeacherDN) > 0 { mr.Add(i.educationConfig.classAttributeMap.teachers, newTeacherDN) - if err := i.conn.Modify(&mr); err != nil { + if err := i.client.Modify(&mr); err != nil { return err } } diff --git a/services/graph/pkg/identity/ldap_education_school.go b/services/graph/pkg/identity/ldap_education_school.go index 68ee855b61..32ee9b3040 100644 --- a/services/graph/pkg/identity/ldap_education_school.go +++ b/services/graph/pkg/identity/ldap_education_school.go @@ -13,6 +13,7 @@ import ( "github.com/opencloud-eu/opencloud/pkg/log" "github.com/opencloud-eu/opencloud/services/graph/pkg/config" "github.com/opencloud-eu/opencloud/services/graph/pkg/errorcode" + "github.com/prometheus/client_golang/prometheus" ) type educationConfig struct { @@ -114,27 +115,39 @@ func newSchoolAttributeMap() schoolAttributeMap { } // CreateEducationSchool creates the supplied school in the identity backend. -func (i *LDAP) CreateEducationSchool(ctx context.Context, school libregraph.EducationSchool) (*libregraph.EducationSchool, error) { +func (i *LDAP) CreateEducationSchool(ctx context.Context, school libregraph.EducationSchool) (result *libregraph.EducationSchool, err error) { logger := i.logger.SubloggerWithRequestID(ctx) - logger.Debug().Str("backend", "ldap").Msg("CreateEducationSchool") + logger.Debug().Msg("CreateEducationSchool") if !i.writeEnabled { return nil, ErrReadOnly } + timer := prometheus.NewTimer(prometheus.ObserverFunc(func(seconds float64) { + r := MetricResultSuccess + if err != nil { + r = MetricResultFailure + } + i.metrics.ApiOperationDuration.WithLabelValues(MetricOpCreateEducationSchool, r).Observe(seconds) + })) + defer timer.ObserveDuration() + // Check that the school number is not already used if school.HasSchoolNumber() { - _, err := i.getSchoolByNumber(school.GetSchoolNumber()) + _, err = i.getSchoolByNumber(school.GetSchoolNumber()) switch { case err == nil: logger.Debug().Err(errSchoolNumberExists).Str("schoolNumber", school.GetSchoolNumber()).Msg("duplicate school number") - return nil, errSchoolNumberExists + err = errSchoolNumberExists + return case errors.Is(err, ErrNotFound): break default: logger.Error().Err(err).Str("schoolNumber", school.GetSchoolNumber()).Msg("error looking up school by number") - return nil, errorcode.New(errorcode.GeneralException, "error looking up school by number") + err = errorcode.New(errorcode.GeneralException, "error looking up school by number") + return } } + err = nil attributeTypeAndValue := ldap.AttributeTypeAndValue{ Type: i.educationConfig.schoolAttributeMap.displayName, @@ -159,7 +172,7 @@ func (i *LDAP) CreateEducationSchool(ctx context.Context, school libregraph.Educ objectClasses := []string{"organizationalUnit", i.educationConfig.schoolObjectClass, "top"} ar.Attribute("objectClass", objectClasses) - if err := i.conn.Add(ar); err != nil { + if err = i.client.Add(ar); err != nil { var lerr *ldap.Error logger.Debug().Err(err).Msg("error adding school") if errors.As(err, &lerr) { @@ -167,15 +180,21 @@ func (i *LDAP) CreateEducationSchool(ctx context.Context, school libregraph.Educ err = errSchoolNameExists } } - return nil, err + return } // Read back school from LDAP to get the generated UUID e, err := i.getSchoolByDN(ar.DN) if err != nil { - return nil, err + return } - return i.createSchoolModelFromLDAP(e), nil + if e == nil { + logger.Error().Str("dn", ar.DN).Str("school-number", school.GetSchoolNumber()).Msg("failed to find the school that was just created") + err = errorcode.New(errorcode.ItemNotFound, fmt.Sprintf("failed to find the school in LDAP that was just created, with DN=%q", ar.DN)) + return + } + result = i.createSchoolModelFromLDAP(e) + return } // UpdateEducationSchoolOperation contains the logic for which update operation to apply to a school @@ -183,7 +202,6 @@ func (i *LDAP) updateEducationSchoolOperation( schoolUpdate libregraph.EducationSchool, currentSchool libregraph.EducationSchool, ) schoolUpdateOperation { - providedDisplayName, displayNameIsSet := schoolUpdate.GetDisplayNameOk() if displayNameIsSet { if *providedDisplayName == "" || *providedDisplayName == currentSchool.GetDisplayName() { @@ -229,12 +247,12 @@ func (i *LDAP) updateDisplayName(ctx context.Context, dn string, providedDisplay } mrdn := ldap.NewModifyDNRequest(dn, attributeTypeAndValue.String(), true, "") - i.logger.Debug().Str("backend", "ldap"). + i.logger.Debug(). Str("dn", mrdn.DN). Str("newrdn", mrdn.NewRDN). Msg("updateDisplayName") - if err := i.conn.ModifyDN(mrdn); err != nil { + if err := i.client.ModifyDN(mrdn); err != nil { var lerr *ldap.Error logger.Debug().Err(err).Msg("error updating school name") if errors.As(err, &lerr) { @@ -275,7 +293,7 @@ func (i *LDAP) updateSchoolProperties(ctx context.Context, dn string, currentSch } } - if err := i.conn.Modify(mr); err != nil { + if err := i.client.Modify(mr); err != nil { logger.Debug().Err(err).Msg("error updating school number") return err } @@ -284,96 +302,175 @@ func (i *LDAP) updateSchoolProperties(ctx context.Context, dn string, currentSch } // UpdateEducationSchool updates the supplied school in the identity backend -func (i *LDAP) UpdateEducationSchool(ctx context.Context, numberOrID string, school libregraph.EducationSchool) (*libregraph.EducationSchool, error) { +func (i *LDAP) UpdateEducationSchool(ctx context.Context, numberOrID string, school libregraph.EducationSchool) (updated *libregraph.EducationSchool, err error) { logger := i.logger.SubloggerWithRequestID(ctx) - logger.Debug().Str("backend", "ldap").Msg("UpdateEducationSchool") + logger.Debug().Msg("UpdateEducationSchool") if !i.writeEnabled { return nil, ErrReadOnly } + timer := prometheus.NewTimer(prometheus.ObserverFunc(func(seconds float64) { + r := MetricResultSuccess + if err != nil { + r = MetricResultFailure + } else if updated == nil { + r = MetricResultNotFound + } + i.metrics.ApiOperationDuration.WithLabelValues(MetricOpUpdateEducationSchool, r).Observe(seconds) + })) + defer timer.ObserveDuration() + e, err := i.getSchoolByNumberOrID(numberOrID) if err != nil { - return nil, err + return + } + if e == nil { + // don't treat this as an error, just return nil for the updated school instead, + // caller must deal with this and decide whether it's an error or not + return } currentSchool := i.createSchoolModelFromLDAP(e) switch i.updateEducationSchoolOperation(school, *currentSchool) { case tooManyValues: - return nil, fmt.Errorf("school name and school number cannot be updated in the same request") + err = fmt.Errorf("school name and school number cannot be updated in the same request") + return case schoolUnchanged: - logger.Debug().Str("backend", "ldap").Msg("UpdateEducationSchool: Nothing changed") - return currentSchool, nil + logger.Debug().Msg("UpdateEducationSchool: Nothing changed") + updated = currentSchool + err = nil + return case schoolRenamed: - if err := i.updateDisplayName(ctx, e.DN, school.GetDisplayName()); err != nil { - return nil, err + if err = i.updateDisplayName(ctx, e.DN, school.GetDisplayName()); err != nil { + return } case schoolPropertiesUpdated: - if err := i.updateSchoolProperties(ctx, e.DN, *currentSchool, school); err != nil { - return nil, err + if err = i.updateSchoolProperties(ctx, e.DN, *currentSchool, school); err != nil { + return } } // Read back school from LDAP - e, err = i.getSchoolByNumberOrID(i.getID(e)) + id := i.getID(e) + e, err = i.getSchoolByNumberOrID(id) if err != nil { return nil, err } - return i.createSchoolModelFromLDAP(e), nil + if e == nil { + logger.Error().Str("id", id).Str("school-number", currentSchool.GetSchoolNumber()).Msg("failed to find the school that was just updated") + err = errorcode.New(errorcode.ItemNotFound, fmt.Sprintf("failed to find the school in LDAP that was just updated, with id %q", id)) + return + } + updated = i.createSchoolModelFromLDAP(e) + return } // DeleteEducationSchool deletes a given school, identified by id -func (i *LDAP) DeleteEducationSchool(ctx context.Context, id string) error { +func (i *LDAP) DeleteEducationSchool(ctx context.Context, id string) (found bool, err error) { logger := i.logger.SubloggerWithRequestID(ctx) - logger.Debug().Str("backend", "ldap").Msg("DeleteEducationSchool") + logger.Debug().Msg("DeleteEducationSchool") if !i.writeEnabled { - return ErrReadOnly + err = ErrReadOnly + return } + + timer := prometheus.NewTimer(prometheus.ObserverFunc(func(seconds float64) { + result := MetricResultSuccess + if err != nil { + result = MetricResultFailure + } else if !found { + result = MetricResultNotFound + } + i.metrics.ApiOperationDuration.WithLabelValues(MetricOpDeleteEducationSchool, result).Observe(seconds) + })) + defer timer.ObserveDuration() + e, err := i.getSchoolByNumberOrID(id) if err != nil { - return err + return + } + if e == nil { + return } + found = true dr := ldap.DelRequest{DN: e.DN} - if err = i.conn.Del(&dr); err != nil { - return err + if err = i.client.Del(&dr); err != nil { + return } // TODO update any users that are member of this school - return nil + return } // GetEducationSchool implements the EducationBackend interface for the LDAP backend. -func (i *LDAP) GetEducationSchool(ctx context.Context, numberOrID string) (*libregraph.EducationSchool, error) { +func (i *LDAP) GetEducationSchool(ctx context.Context, numberOrID string) (school *libregraph.EducationSchool, err error) { logger := i.logger.SubloggerWithRequestID(ctx) - logger.Debug().Str("backend", "ldap").Msg("GetEducationSchool") + logger.Debug().Msg("GetEducationSchool") + + timer := prometheus.NewTimer(prometheus.ObserverFunc(func(seconds float64) { + result := MetricResultSuccess + if err != nil { + result = MetricResultFailure + } else if school == nil { + result = MetricResultNotFound + } + i.metrics.ApiOperationDuration.WithLabelValues(MetricOpGetEducationSchool, result).Observe(seconds) + })) + defer timer.ObserveDuration() + e, err := i.getSchoolByNumberOrID(numberOrID) if err != nil { - return nil, err + return + } + if e == nil { + return } - return i.createSchoolModelFromLDAP(e), nil + school = i.createSchoolModelFromLDAP(e) + return } // GetEducationSchools implements the EducationBackend interface for the LDAP backend. -func (i *LDAP) GetEducationSchools(ctx context.Context) ([]*libregraph.EducationSchool, error) { +func (i *LDAP) GetEducationSchools(ctx context.Context) (schools []*libregraph.EducationSchool, err error) { + timer := prometheus.NewTimer(prometheus.ObserverFunc(func(seconds float64) { + result := MetricResultSuccess + if err != nil { + result = MetricResultFailure + } + i.metrics.ApiOperationDuration.WithLabelValues(MetricOpGetEducationSchools, result).Observe(seconds) + })) + defer timer.ObserveDuration() + filter := fmt.Sprintf("(objectClass=%s)", i.educationConfig.schoolObjectClass) if i.educationConfig.schoolFilter != "" { filter = fmt.Sprintf("(&%s%s)", i.educationConfig.schoolFilter, filter) } - return i.searchEducationSchools(ctx, filter) + schools, err = i.searchEducationSchools(ctx, filter) + return } // FilterEducationSchoolsByAttribute implements the EducationBackend interface for the LDAP backend. -func (i *LDAP) FilterEducationSchoolsByAttribute(ctx context.Context, attr, value string) ([]*libregraph.EducationSchool, error) { +func (i *LDAP) FilterEducationSchoolsByAttribute(ctx context.Context, attr, value string) (schools []*libregraph.EducationSchool, err error) { logger := i.logger.SubloggerWithRequestID(ctx).With().Str("func", "FilterEducationSchoolsByAttribute").Logger() - logger.Debug().Str("backend", "ldap").Str("attribute", attr).Str("value", value).Msg("") + logger.Debug().Str("attribute", attr).Str("value", value).Msg("") + + timer := prometheus.NewTimer(prometheus.ObserverFunc(func(seconds float64) { + result := MetricResultSuccess + if err != nil { + result = MetricResultFailure + } + i.metrics.ApiOperationDuration.WithLabelValues(MetricOpFilterEducationSchoolsByAttribute, result).Observe(seconds) + })) + defer timer.ObserveDuration() var ldapAttr string switch attr { case "externalId": ldapAttr = i.educationConfig.schoolAttributeMap.externalId default: - return nil, errorcode.New(errorcode.InvalidRequest, fmt.Sprintf("filtering by attribute '%s' is not supported", attr)) + err = errorcode.New(errorcode.InvalidRequest, fmt.Sprintf("filtering by attribute '%s' is not supported", attr)) + return } filter := fmt.Sprintf("(&%s(objectClass=%s)(%s=%s))", i.educationConfig.schoolFilter, @@ -381,7 +478,8 @@ func (i *LDAP) FilterEducationSchoolsByAttribute(ctx context.Context, attr, valu ldap.EscapeFilter(ldapAttr), ldap.EscapeFilter(value), ) - return i.searchEducationSchools(ctx, filter) + schools, err = i.searchEducationSchools(ctx, filter) + return } // searchEducationSchools builds and executes an LDAP search for education schools and converts the results to EducationSchool models. @@ -403,9 +501,19 @@ func (i *LDAP) searchEducationSchools(ctx context.Context, filter string) ([]*li Interface("attributes", searchRequest.Attributes). Msg("searchEducationSchools") - res, err := i.conn.Search(searchRequest) + res, err := i.client.Search(searchRequest) if err != nil { - return nil, errorcode.New(errorcode.ItemNotFound, err.Error()) + msg := "school search failed" + errMap := ldapResultToErrMap{ + ldap.LDAPResultNoSuchObject: ErrNotFound, + ldap.LDAPResultUnwillingToPerform: errorcode.New(errorcode.NotAllowed, msg), + ldap.LDAPResultInsufficientAccessRights: errorcode.New(errorcode.NotAllowed, msg), + ldapGenericErr: errorcode.New(errorcode.GeneralException, msg), + } + return nil, i.mapLDAPError(err, errMap) + } + if res == nil { + return nil, ErrNotFound } schools := make([]*libregraph.EducationSchool, 0, len(res.Entries)) @@ -435,49 +543,64 @@ func (i *LDAP) GetEducationSchoolUsers(ctx context.Context, schoolNumberOrID str users := make([]*libregraph.EducationUser, 0, len(entries)) for _, e := range entries { - u := i.createEducationUserModelFromLDAP(e) - // Skip invalid LDAP users - if u == nil { - continue + if u, err := i.createEducationUserModelFromLDAP(e); u != nil && err == nil { + users = append(users, u) + } else { + // Skip invalid LDAP users } - users = append(users, u) } return users, nil } // AddUsersToEducationSchool adds new members (reference by a slice of IDs) to supplied school in the identity backend. -func (i *LDAP) AddUsersToEducationSchool(ctx context.Context, schoolNumberOrID string, memberIDs []string) error { +func (i *LDAP) AddUsersToEducationSchool(ctx context.Context, schoolNumberOrID string, memberIDs []string) (foundSchool bool, err error) { logger := i.logger.SubloggerWithRequestID(ctx) - logger.Debug().Str("backend", "ldap").Msg("AddUsersToEducationSchool") + logger.Debug().Msg("AddUsersToEducationSchool") + + timer := prometheus.NewTimer(prometheus.ObserverFunc(func(seconds float64) { + result := MetricResultSuccess + if err != nil { + result = MetricResultFailure + } + i.metrics.ApiOperationDuration.WithLabelValues(MetricOpAddUsersToEducationSchool, result).Observe(seconds) + })) + defer timer.ObserveDuration() schoolEntry, err := i.getSchoolByNumberOrID(schoolNumberOrID) if err != nil { - return err + return } - if schoolEntry == nil { - return ErrNotFound + return } + foundSchool = true schoolID := schoolEntry.GetEqualFoldAttributeValue(i.educationConfig.schoolAttributeMap.id) userEntries := make([]*ldap.Entry, 0, len(memberIDs)) for _, memberID := range memberIDs { - user, err := i.getEducationUserByNameOrID(memberID) + var user *ldap.Entry + user, err = i.getEducationUserByNameOrID(memberID) if err != nil { + i.logger.Warn().Err(err).Str("userid", memberID).Msg("User does not exist") + err = errorcode.New(errorcode.ItemNotFound, fmt.Sprintf("user '%s' not found", memberID)) + return + } + if user == nil { i.logger.Warn().Str("userid", memberID).Msg("User does not exist") - return errorcode.New(errorcode.ItemNotFound, fmt.Sprintf("user '%s' not found", memberID)) + err = errorcode.New(errorcode.ItemNotFound, fmt.Sprintf("user '%s' not found", memberID)) + return } userEntries = append(userEntries, user) } for _, userEntry := range userEntries { - if err := i.addEntryToSchool(userEntry, schoolID); err != nil { - return err + if err = i.addEntryToSchool(userEntry, schoolID); err != nil { + return } } - return nil + return } // addEntryToSchool adds the schoolID to the entry's memberOfSchool attribute if not already present. @@ -488,66 +611,93 @@ func (i *LDAP) addEntryToSchool(entry *ldap.Entry, schoolID string) error { } mr := ldap.ModifyRequest{DN: entry.DN} mr.Add(i.educationConfig.memberOfSchoolAttribute, []string{schoolID}) - return i.conn.Modify(&mr) + return i.client.Modify(&mr) } // RemoveUserFromEducationSchool removes a single member (by ID) from a school -func (i *LDAP) RemoveUserFromEducationSchool(ctx context.Context, schoolNumberOrID string, memberID string) error { +func (i *LDAP) RemoveUserFromEducationSchool(ctx context.Context, schoolNumberOrID string, memberID string) (foundSchool, foundUser, foundUserInSchool bool, err error) { logger := i.logger.SubloggerWithRequestID(ctx) - logger.Debug().Str("backend", "ldap").Msg("RemoveUserFromEducationSchool") + logger.Debug().Msg("RemoveUserFromEducationSchool") + + timer := prometheus.NewTimer(prometheus.ObserverFunc(func(seconds float64) { + result := MetricResultSuccess + if err != nil { + result = MetricResultFailure + } else if !foundSchool || !foundUser || !foundUserInSchool { + result = MetricResultNotFound + } + i.metrics.ApiOperationDuration.WithLabelValues(MetricOpRemoveUserFromEducationSchool, result).Observe(seconds) + })) + defer timer.ObserveDuration() schoolEntry, err := i.getSchoolByNumberOrID(schoolNumberOrID) if err != nil { - return err + return } - if schoolEntry == nil { - return ErrNotFound + // let the caller decide whether this is an error or not + return } + foundSchool = true schoolID := schoolEntry.GetEqualFoldAttributeValue(i.educationConfig.schoolAttributeMap.id) user, err := i.getEducationUserByNameOrID(memberID) - if err != nil { + if err != nil || user == nil { i.logger.Warn().Str("userid", memberID).Msg("User does not exist") - return err + err = nil + // let the caller decide whether this is an error or not + return } + foundUser = true + currentSchools := user.GetEqualFoldAttributeValues(i.educationConfig.memberOfSchoolAttribute) for _, currentSchool := range currentSchools { if currentSchool == schoolID { + foundUserInSchool = true mr := ldap.ModifyRequest{DN: user.DN} mr.Delete(i.educationConfig.memberOfSchoolAttribute, []string{schoolID}) - if err := i.conn.Modify(&mr); err != nil { - return err + if err = i.client.Modify(&mr); err != nil { + return } break } } - return nil + return } // GetEducationSchoolClasses implements the EducationBackend interface for the LDAP backend. -func (i *LDAP) GetEducationSchoolClasses(ctx context.Context, schoolNumberOrID string) ([]*libregraph.EducationClass, error) { +func (i *LDAP) GetEducationSchoolClasses(ctx context.Context, schoolNumberOrID string) (classes []*libregraph.EducationClass, err error) { logger := i.logger.SubloggerWithRequestID(ctx) - logger.Debug().Str("backend", "ldap").Msg("GetEducationSchoolClasses") + logger.Debug().Msg("GetEducationSchoolClasses") + + timer := prometheus.NewTimer(prometheus.ObserverFunc(func(seconds float64) { + result := MetricResultSuccess + if err != nil { + result = MetricResultFailure + } + i.metrics.ApiOperationDuration.WithLabelValues(MetricOpGetEducationSchoolClasses, result).Observe(seconds) + })) + defer timer.ObserveDuration() entries, err := i.getEducationSchoolEntries( schoolNumberOrID, i.groupFilter, i.educationConfig.classObjectClass, i.groupBaseDN, i.groupScope, i.getEducationClassAttrTypes(false), logger, ) if err != nil { - return nil, err + return } - classes := make([]*libregraph.EducationClass, 0, len(entries)) + classes = make([]*libregraph.EducationClass, 0, len(entries)) for _, e := range entries { class := i.createEducationClassModelFromLDAP(e) // Skip invalid LDAP classes if class == nil { + logger.Warn().Str("school-number", schoolNumberOrID).Interface("entry", e).Msg("failed to create class model from LDAP") continue } classes = append(classes, class) } - return classes, nil + return } func (i *LDAP) getEducationSchoolEntries( @@ -578,87 +728,116 @@ func (i *LDAP) getEducationSchoolEntries( attributes, nil, ) - logger.Debug().Str("backend", "ldap"). + logger.Debug(). Str("base", searchRequest.BaseDN). Str("filter", searchRequest.Filter). Int("scope", searchRequest.Scope). Int("sizelimit", searchRequest.SizeLimit). Interface("attributes", searchRequest.Attributes). Msg("GetEducationClasses") - res, err := i.conn.Search(searchRequest) + res, err := i.client.Search(searchRequest) if err != nil { - return nil, errorcode.New(errorcode.ItemNotFound, err.Error()) + msg := "school search failed" + errMap := ldapResultToErrMap{ + ldap.LDAPResultNoSuchObject: ErrNotFound, + ldap.LDAPResultUnwillingToPerform: errorcode.New(errorcode.NotAllowed, msg), + ldap.LDAPResultInsufficientAccessRights: errorcode.New(errorcode.NotAllowed, msg), + ldapGenericErr: errorcode.New(errorcode.GeneralException, msg), + } + return nil, i.mapLDAPError(err, errMap) + } + if res == nil { + return nil, ErrNotFound } return res.Entries, nil } // AddClassesToEducationSchool adds new members (reference by a slice of IDs) to supplied school in the identity backend. -func (i *LDAP) AddClassesToEducationSchool(ctx context.Context, schoolNumberOrID string, memberIDs []string) error { +func (i *LDAP) AddClassesToEducationSchool(ctx context.Context, schoolNumberOrID string, memberIDs []string) (err error) { logger := i.logger.SubloggerWithRequestID(ctx) - logger.Debug().Str("backend", "ldap").Msg("AddClassesToEducationSchool") + logger.Debug().Msg("AddClassesToEducationSchool") + + timer := prometheus.NewTimer(prometheus.ObserverFunc(func(seconds float64) { + result := MetricResultSuccess + if err != nil { + result = MetricResultFailure + } + i.metrics.ApiOperationDuration.WithLabelValues(MetricOpAddClassesToEducationSchool, result).Observe(seconds) + })) + defer timer.ObserveDuration() schoolEntry, err := i.getSchoolByNumberOrID(schoolNumberOrID) if err != nil { - return err + return } - if schoolEntry == nil { - return ErrNotFound + err = ErrNotFound + return } schoolID := schoolEntry.GetEqualFoldAttributeValue(i.educationConfig.schoolAttributeMap.id) classEntries := make([]*ldap.Entry, 0, len(memberIDs)) for _, memberID := range memberIDs { - class, err := i.getEducationClassByID(memberID, false) + var class *ldap.Entry + class, err = i.getEducationClassByID(memberID, false) if err != nil { i.logger.Warn().Str("userid", memberID).Msg("Class does not exist") - return err + return } classEntries = append(classEntries, class) } for _, classEntry := range classEntries { - if err := i.addEntryToSchool(classEntry, schoolID); err != nil { - return err + if err = i.addEntryToSchool(classEntry, schoolID); err != nil { + return } } - return nil + return } // RemoveClassFromEducationSchool removes a single member (by ID) from a school -func (i *LDAP) RemoveClassFromEducationSchool(ctx context.Context, schoolNumberOrID string, memberID string) error { +func (i *LDAP) RemoveClassFromEducationSchool(ctx context.Context, schoolNumberOrID string, memberID string) (err error) { logger := i.logger.SubloggerWithRequestID(ctx) - logger.Debug().Str("backend", "ldap").Msg("RemoveClassFromEducationSchool") + logger.Debug().Msg("RemoveClassFromEducationSchool") + + timer := prometheus.NewTimer(prometheus.ObserverFunc(func(seconds float64) { + result := MetricResultSuccess + if err != nil { + result = MetricResultFailure + } + i.metrics.ApiOperationDuration.WithLabelValues(MetricOpRemoveClassFromEducationSchool, result).Observe(seconds) + })) + defer timer.ObserveDuration() schoolEntry, err := i.getSchoolByNumberOrID(schoolNumberOrID) if err != nil { - return err + return } - if schoolEntry == nil { - return ErrNotFound + err = ErrNotFound + return } schoolID := schoolEntry.GetEqualFoldAttributeValue(i.educationConfig.schoolAttributeMap.id) class, err := i.getEducationClassByID(memberID, false) if err != nil { i.logger.Warn().Str("userid", memberID).Msg("Class does not exist") - return err + return } currentSchools := class.GetEqualFoldAttributeValues(i.educationConfig.memberOfSchoolAttribute) for _, currentSchool := range currentSchools { if currentSchool == schoolID { mr := ldap.ModifyRequest{DN: class.DN} mr.Delete(i.educationConfig.memberOfSchoolAttribute, []string{schoolID}) - if err := i.conn.Modify(&mr); err != nil { - return err + if err = i.client.Modify(&mr); err != nil { + return } break } } - return nil + return } func (i *LDAP) getSchoolByDN(dn string) (*ldap.Entry, error) { @@ -706,26 +885,33 @@ func (i *LDAP) getSchoolByFilter(filter string) (*ldap.Entry, error) { i.getEducationSchoolAttrTypes(), nil, ) - i.logger.Debug().Str("backend", "ldap"). + i.logger.Debug(). Str("base", searchRequest.BaseDN). Str("filter", searchRequest.Filter). Int("scope", searchRequest.Scope). Int("sizelimit", searchRequest.SizeLimit). Interface("attributes", searchRequest.Attributes). Msg("getSchoolByFilter") - res, err := i.conn.Search(searchRequest) + res, err := i.client.Search(searchRequest) if err != nil { - var errmsg string if lerr, ok := err.(*ldap.Error); ok { if lerr.ResultCode == ldap.LDAPResultSizeLimitExceeded { - errmsg = fmt.Sprintf("too many results searching for school '%s'", filter) - i.logger.Debug().Str("backend", "ldap").Err(lerr). + errmsg := fmt.Sprintf("too many results searching for school '%s'", filter) + i.logger.Debug().Err(lerr). Str("schoolfilter", filter).Msg("too many results searching for school") + return nil, errorcode.New(errorcode.ItemNotFound, errmsg) } } - return nil, errorcode.New(errorcode.ItemNotFound, errmsg) + msg := "school search failed" + errMap := ldapResultToErrMap{ + ldap.LDAPResultNoSuchObject: ErrNotFound, + ldap.LDAPResultUnwillingToPerform: errorcode.New(errorcode.NotAllowed, msg), + ldap.LDAPResultInsufficientAccessRights: errorcode.New(errorcode.NotAllowed, msg), + ldapGenericErr: errorcode.New(errorcode.GeneralException, msg), + } + return nil, i.mapLDAPError(err, errMap) } - if len(res.Entries) == 0 { + if res == nil || len(res.Entries) == 0 { return nil, ErrNotFound } diff --git a/services/graph/pkg/identity/ldap_education_school_test.go b/services/graph/pkg/identity/ldap_education_school_test.go index 0eb4fcc048..b17099a83e 100644 --- a/services/graph/pkg/identity/ldap_education_school_test.go +++ b/services/graph/pkg/identity/ldap_education_school_test.go @@ -376,8 +376,10 @@ func TestDeleteEducationSchool(t *testing.T) { b, err := getMockedBackend(lm, eduConfig, &logger) assert.Nil(t, err) - err = b.DeleteEducationSchool(context.Background(), tt.numberOrId) + var ok bool + ok, err = b.DeleteEducationSchool(context.Background(), tt.numberOrId) lm.AssertNumberOfCalls(t, "Search", 1) + assert.True(t, ok) if tt.expectedItemNotFound { lm.AssertNumberOfCalls(t, "Del", 0) @@ -572,22 +574,28 @@ func TestAddUsersToEducationSchool(t *testing.T) { lm.On("Modify", userToSchoolModRequest).Return(nil) b, err := getMockedBackend(lm, eduConfig, &logger) assert.Nil(t, err) - err = b.AddUsersToEducationSchool(context.Background(), "abcd-defg", []string{"does-not-exist"}) + var ok bool + ok, err = b.AddUsersToEducationSchool(context.Background(), "abcd-defg", []string{"does-not-exist"}) lm.AssertNumberOfCalls(t, "Search", 2) + assert.True(t, ok) assert.NotNil(t, err) - err = b.AddUsersToEducationSchool(context.Background(), "abcd-defg", []string{"abcd-defg", "does-not-exist"}) + ok, err = b.AddUsersToEducationSchool(context.Background(), "abcd-defg", []string{"abcd-defg", "does-not-exist"}) lm.AssertNumberOfCalls(t, "Search", 5) + assert.True(t, ok) assert.NotNil(t, err) - err = b.AddUsersToEducationSchool(context.Background(), "abcd-defg", []string{"abcd-defg"}) + ok, err = b.AddUsersToEducationSchool(context.Background(), "abcd-defg", []string{"abcd-defg"}) lm.AssertNumberOfCalls(t, "Search", 7) + assert.True(t, ok) assert.Nil(t, err) // try to add by school number (instead or id) - err = b.AddUsersToEducationSchool(context.Background(), "0123", []string{"abcd-defg"}) + ok, err = b.AddUsersToEducationSchool(context.Background(), "0123", []string{"abcd-defg"}) lm.AssertNumberOfCalls(t, "Search", 9) + assert.True(t, ok) assert.Nil(t, err) } func TestRemoveMemberFromEducationSchool(t *testing.T) { + var foundSchool, foundUser, foundUserInSchool bool lm := &mocks.Client{} lm.On("Search", schoolByIDSearch1).Return(&ldap.SearchResult{Entries: []*ldap.Entry{schoolEntry}}, nil) lm.On("Search", schoolByNumberSearch).Return(&ldap.SearchResult{Entries: []*ldap.Entry{schoolEntry}}, nil) @@ -596,18 +604,25 @@ func TestRemoveMemberFromEducationSchool(t *testing.T) { lm.On("Modify", userFromSchoolModRequest).Return(nil) b, err := getMockedBackend(lm, eduConfig, &logger) assert.Nil(t, err) - err = b.RemoveUserFromEducationSchool(context.Background(), "abcd-defg", "does-not-exist") + foundSchool, _, _, err = b.RemoveUserFromEducationSchool(context.Background(), "abcd-defg", "does-not-exist") lm.AssertNumberOfCalls(t, "Search", 2) assert.NotNil(t, err) + assert.False(t, foundSchool) assert.Equal(t, "itemNotFound: not found", err.Error()) - err = b.RemoveUserFromEducationSchool(context.Background(), "abcd-defg", "abcd-defg") + foundSchool, foundUser, foundUserInSchool, err = b.RemoveUserFromEducationSchool(context.Background(), "abcd-defg", "abcd-defg") lm.AssertNumberOfCalls(t, "Search", 4) lm.AssertNumberOfCalls(t, "Modify", 1) + assert.True(t, foundSchool) + assert.True(t, foundUser) + assert.True(t, foundUserInSchool) // try to remove by school number (instead or id) - err = b.RemoveUserFromEducationSchool(context.Background(), "0123", "abcd-defg") + foundSchool, foundUser, foundUserInSchool, err = b.RemoveUserFromEducationSchool(context.Background(), "0123", "abcd-defg") lm.AssertNumberOfCalls(t, "Search", 6) lm.AssertNumberOfCalls(t, "Modify", 2) assert.Nil(t, err) + assert.True(t, foundSchool) + assert.True(t, foundUser) + assert.True(t, foundUserInSchool) } var usersBySchoolIDSearch *ldap.SearchRequest = &ldap.SearchRequest{ diff --git a/services/graph/pkg/identity/ldap_education_user.go b/services/graph/pkg/identity/ldap_education_user.go index 88855d52e0..dd7a50e0a3 100644 --- a/services/graph/pkg/identity/ldap_education_user.go +++ b/services/graph/pkg/identity/ldap_education_user.go @@ -35,7 +35,7 @@ func (i *LDAP) CreateEducationUser(ctx context.Context, user libregraph.Educatio return nil, err } - if err = i.conn.Add(ar); err != nil { + if err = i.client.Add(ar); err != nil { var lerr *ldap.Error logger.Debug().Err(err).Msg("error adding user") if errors.As(err, &lerr) { @@ -51,7 +51,7 @@ func (i *LDAP) CreateEducationUser(ctx context.Context, user libregraph.Educatio if err != nil { return nil, err } - return i.createEducationUserModelFromLDAP(e), nil + return i.createEducationUserModelFromLDAP(e) } // DeleteEducationUser deletes a given education user, identified by username or id, from the backend @@ -68,7 +68,7 @@ func (i *LDAP) DeleteEducationUser(ctx context.Context, nameOrID string) error { } dr := ldap.DelRequest{DN: e.DN} - if err = i.conn.Del(&dr); err != nil { + if err = i.client.Del(&dr); err != nil { return err } return nil @@ -169,7 +169,7 @@ func (i *LDAP) UpdateEducationUser(ctx context.Context, nameOrID string, user li } if updateNeeded { - if err := i.conn.Modify(&mr); err != nil { + if err := i.client.Modify(&mr); err != nil { return nil, err } } @@ -180,7 +180,10 @@ func (i *LDAP) UpdateEducationUser(ctx context.Context, nameOrID string, user li return nil, err } - returnUser := i.createEducationUserModelFromLDAP(e) + returnUser, err := i.createEducationUserModelFromLDAP(e) + if err != nil { + return nil, err + } // To avoid a ldap lookup for group membership, set the enabled flag to same as input value // since this would have been updated with group membership from the input anyway. @@ -199,7 +202,10 @@ func (i *LDAP) GetEducationUser(ctx context.Context, nameOrID string) (*libregra if err != nil { return nil, err } - u := i.createEducationUserModelFromLDAP(e) + u, err := i.createEducationUserModelFromLDAP(e) + if err != nil { + return nil, err + } if u == nil { return nil, ErrNotFound } @@ -259,19 +265,18 @@ func (i *LDAP) searchEducationUsers(ctx context.Context, filter string) ([]*libr Interface("attributes", searchRequest.Attributes). Msg("searchEducationUsers") - res, err := i.conn.Search(searchRequest) + res, err := i.client.Search(searchRequest) if err != nil { return nil, errorcode.New(errorcode.ItemNotFound, err.Error()) } users := make([]*libregraph.EducationUser, 0, len(res.Entries)) for _, e := range res.Entries { - u := i.createEducationUserModelFromLDAP(e) - // Skip invalid LDAP users - if u == nil { - continue + if u, err := i.createEducationUserModelFromLDAP(e); u != nil && err == nil { + users = append(users, u) + } else { + // Skip invalid LDAP users } - users = append(users, u) } return users, nil } @@ -344,9 +349,12 @@ func (i *LDAP) educationUserToAddRequest(user libregraph.EducationUser) (*ldap.A return ar, nil } -func (i *LDAP) createEducationUserModelFromLDAP(e *ldap.Entry) *libregraph.EducationUser { - user := i.createUserModelFromLDAP(e) - return i.userToEducationUser(*user, e) +func (i *LDAP) createEducationUserModelFromLDAP(e *ldap.Entry) (*libregraph.EducationUser, error) { + user, err := i.createUserModelFromLDAP(e) + if err != nil { + return nil, err + } + return i.userToEducationUser(*user, e), nil } func (i *LDAP) getEducationUserAttrTypes() []string { diff --git a/services/graph/pkg/identity/ldap_group.go b/services/graph/pkg/identity/ldap_group.go index abcfa7abb6..c838d69b05 100644 --- a/services/graph/pkg/identity/ldap_group.go +++ b/services/graph/pkg/identity/ldap_group.go @@ -14,6 +14,8 @@ import ( "github.com/libregraph/idm/pkg/ldapdn" libregraph "github.com/opencloud-eu/libre-graph-api-go" + "github.com/prometheus/client_golang/prometheus" + "github.com/opencloud-eu/opencloud/services/graph/pkg/errorcode" "github.com/opencloud-eu/opencloud/services/graph/pkg/odata" ) @@ -25,54 +27,80 @@ type groupAttributeMap struct { } // GetGroup implements the Backend Interface for the LDAP Backend -func (i *LDAP) GetGroup(ctx context.Context, nameOrID string, queryParam url.Values) (*libregraph.Group, error) { +func (i *LDAP) GetGroup(ctx context.Context, nameOrID string, queryParam url.Values) (g *libregraph.Group, err error) { logger := i.logger.SubloggerWithRequestID(ctx) - logger.Debug().Str("backend", "ldap").Msg("GetGroup") + logger.Debug().Msg("GetGroup") + + timer := prometheus.NewTimer(prometheus.ObserverFunc(func(seconds float64) { + result := MetricResultSuccess + if err != nil { + result = MetricResultFailure + } else if g == nil { + result = MetricResultNotFound + } + i.metrics.ApiOperationDuration.WithLabelValues(MetricOpGetGroup, result).Observe(seconds) + })) + defer timer.ObserveDuration() + e, err := i.getLDAPGroupByNameOrID(nameOrID, true) if err != nil { - return nil, err + return + } + if e == nil { + err = errorcode.New(errorcode.ItemNotFound, "not found") + return } sel := strings.Split(queryParam.Get("$select"), ",") exp := strings.Split(queryParam.Get("$expand"), ",") - var g *libregraph.Group if g = i.createGroupModelFromLDAP(e); g == nil { - return nil, errorcode.New(errorcode.ItemNotFound, "not found") + err = errorcode.New(errorcode.ItemNotFound, "not found") + return } if slices.Contains(sel, "members") || slices.Contains(exp, "members") { - members, err := i.expandLDAPAttributeEntries(ctx, e, i.groupAttributeMap.member, "") - if err != nil { - return nil, err + members, expanderr := i.expandLDAPAttributeEntries(ctx, e, i.groupAttributeMap.member, "") + if expanderr != nil { + err = expanderr + return } g.Members = make([]libregraph.User, 0, len(members)) if len(members) > 0 { for _, ue := range members { - if u := i.createUserModelFromLDAP(ue); u != nil { + if u, uerr := i.createUserModelFromLDAP(ue); u != nil && uerr == nil { g.Members = append(g.Members, *u) } } } } - return g, nil + return } // GetGroups implements the Backend Interface for the LDAP Backend -func (i *LDAP) GetGroups(ctx context.Context, oreq *godata.GoDataRequest) ([]*libregraph.Group, error) { +func (i *LDAP) GetGroups(ctx context.Context, oreq *godata.GoDataRequest) (groups []*libregraph.Group, err error) { logger := i.logger.SubloggerWithRequestID(ctx) - logger.Debug().Str("backend", "ldap").Msg("GetGroups") + logger.Debug().Msg("GetGroups") + + timer := prometheus.NewTimer(prometheus.ObserverFunc(func(seconds float64) { + result := MetricResultSuccess + if err != nil { + result = MetricResultFailure + } + i.metrics.ApiOperationDuration.WithLabelValues(MetricOpGetGroups, result).Observe(seconds) + })) + defer timer.ObserveDuration() search, err := odata.GetSearchValues(oreq.Query) if err != nil { - return nil, err + return } var expandMembers bool exp, err := odata.GetExpandValues(oreq.Query) if err != nil { - return nil, err + return } sel, err := odata.GetSelectValues(oreq.Query) if err != nil { - return nil, err + return } if slices.Contains(exp, "members") || slices.Contains(sel, "members") { @@ -103,19 +131,20 @@ func (i *LDAP) GetGroups(ctx context.Context, oreq *godata.GoDataRequest) ([]*li groupAttrs, nil, ) - logger.Debug().Str("backend", "ldap"). + logger.Debug(). Str("base", searchRequest.BaseDN). Str("filter", searchRequest.Filter). Int("scope", searchRequest.Scope). Int("sizelimit", searchRequest.SizeLimit). Interface("attributes", searchRequest.Attributes). Msg("GetGroups") - res, err := i.conn.Search(searchRequest) + res, err := i.client.Search(searchRequest) if err != nil { - return nil, errorcode.New(errorcode.ItemNotFound, err.Error()) + err = errorcode.New(errorcode.ItemNotFound, err.Error()) + return } - groups := make([]*libregraph.Group, 0, len(res.Entries)) + groups = make([]*libregraph.Group, 0, len(res.Entries)) var g *libregraph.Group for _, e := range res.Entries { @@ -123,14 +152,15 @@ func (i *LDAP) GetGroups(ctx context.Context, oreq *godata.GoDataRequest) ([]*li continue } if expandMembers { - members, err := i.expandLDAPAttributeEntries(ctx, e, i.groupAttributeMap.member, "") - if err != nil { - return nil, err + members, expanderr := i.expandLDAPAttributeEntries(ctx, e, i.groupAttributeMap.member, "") + if expanderr != nil { + err = expanderr + return } g.Members = make([]libregraph.User, 0, len(members)) if len(members) > 0 { for _, ue := range members { - if u := i.createUserModelFromLDAP(ue); u != nil { + if u, uerr := i.createUserModelFromLDAP(ue); u != nil && uerr == nil { g.Members = append(g.Members, *u) } } @@ -138,36 +168,47 @@ func (i *LDAP) GetGroups(ctx context.Context, oreq *godata.GoDataRequest) ([]*li } groups = append(groups, g) } - return groups, nil + return } // GetGroupMembers implements the Backend Interface for the LDAP Backend -func (i *LDAP) GetGroupMembers(ctx context.Context, groupID string, req *godata.GoDataRequest) ([]*libregraph.User, error) { +func (i *LDAP) GetGroupMembers(ctx context.Context, groupID string, req *godata.GoDataRequest) (result []*libregraph.User, err error) { logger := i.logger.SubloggerWithRequestID(ctx) - logger.Debug().Str("backend", "ldap").Msg("GetGroupMembers") + logger.Debug().Msg("GetGroupMembers") + + timer := prometheus.NewTimer(prometheus.ObserverFunc(func(seconds float64) { + r := MetricResultSuccess + if err != nil { + r = MetricResultFailure + } else if result == nil { + r = MetricResultNotFound + } + i.metrics.ApiOperationDuration.WithLabelValues(MetricOpGetGroups, r).Observe(seconds) + })) + defer timer.ObserveDuration() exp, err := odata.GetExpandValues(req.Query) if err != nil { - return nil, err + return } e, err := i.getLDAPGroupByNameOrID(groupID, true) if err != nil { - return nil, err + return } searchTerm, err := odata.GetSearchValues(req.Query) if err != nil { - return nil, err + return } memberEntries, err := i.expandLDAPAttributeEntries(ctx, e, i.groupAttributeMap.member, searchTerm) - result := make([]*libregraph.User, 0, len(memberEntries)) if err != nil { - return nil, err + return } + result = make([]*libregraph.User, 0, len(memberEntries)) for _, member := range memberEntries { - if u := i.createUserModelFromLDAP(member); u != nil { + if u, uerr := i.createUserModelFromLDAP(member); u != nil && uerr == nil { if slices.Contains(exp, "memberOf") { userGroups, err := i.getGroupsForUser(member.DN) if err != nil { @@ -179,25 +220,37 @@ func (i *LDAP) GetGroupMembers(ctx context.Context, groupID string, req *godata. } } - return result, nil + return } // CreateGroup implements the Backend Interface for the LDAP Backend // It is currently restricted to managing groups based on the "groupOfNames" ObjectClass. // As "groupOfNames" requires a "member" Attribute to be present. Empty Groups (groups // without a member) a represented by adding an empty DN as the single member. -func (i *LDAP) CreateGroup(ctx context.Context, group libregraph.Group) (*libregraph.Group, error) { +func (i *LDAP) CreateGroup(ctx context.Context, group libregraph.Group) (g *libregraph.Group, err error) { logger := i.logger.SubloggerWithRequestID(ctx) - logger.Debug().Str("backend", "ldap").Msg("create group") + logger.Debug().Msg("create group") if !i.writeEnabled && i.groupCreateBaseDN == i.groupBaseDN { - return nil, errorcode.New(errorcode.NotAllowed, "server is configured read-only") + err = errorcode.New(errorcode.NotAllowed, "server is configured read-only") + return } + + timer := prometheus.NewTimer(prometheus.ObserverFunc(func(seconds float64) { + r := MetricResultSuccess + if err != nil { + r = MetricResultFailure + } + i.metrics.ApiOperationDuration.WithLabelValues(MetricOpCreateGroup, r).Observe(seconds) + })) + defer timer.ObserveDuration() + ar, err := i.groupToAddRequest(group) if err != nil { - return nil, err + return } - if err := i.conn.Add(ar); err != nil { + if adderr := i.client.Add(ar); adderr != nil { + err = adderr var lerr *ldap.Error logger.Debug().Str("backend", "ldap").Str("dn", group.GetDisplayName()).Err(err).Msg("Failed to create group") if errors.As(err, &lerr) { @@ -205,60 +258,102 @@ func (i *LDAP) CreateGroup(ctx context.Context, group libregraph.Group) (*libreg err = errorcode.New(errorcode.NameAlreadyExists, "group already exists") } } - return nil, err + return } // Read back group from LDAP to get the generated UUID e, err := i.getGroupByDN(ar.DN) if err != nil { - return nil, err + return } - return i.createGroupModelFromLDAP(e), nil + + g = i.createGroupModelFromLDAP(e) + return } // DeleteGroup implements the Backend Interface. -func (i *LDAP) DeleteGroup(ctx context.Context, id string) error { +func (i *LDAP) DeleteGroup(ctx context.Context, id string) (found bool, err error) { logger := i.logger.SubloggerWithRequestID(ctx) - logger.Debug().Str("backend", "ldap").Msg("DeleteGroup") + logger.Debug().Msg("DeleteGroup") if !i.writeEnabled && i.groupCreateBaseDN == i.groupBaseDN { - return errorcode.New(errorcode.NotAllowed, "server is configured read-only") + err = errorcode.New(errorcode.NotAllowed, "server is configured read-only") + return } + timer := prometheus.NewTimer(prometheus.ObserverFunc(func(seconds float64) { + r := MetricResultSuccess + if err != nil { + r = MetricResultFailure + } else if !found { + r = MetricResultNotFound + } + i.metrics.ApiOperationDuration.WithLabelValues(MetricOpDeleteGroup, r).Observe(seconds) + })) + defer timer.ObserveDuration() + e, err := i.getLDAPGroupByID(id, false) if err != nil { - return err + return + } + if e == nil { + // group does not exist in LDAP: debatable whether that should be an error, or whether + // it should be silently treated as successful, which is something only the caller can + // decide + return } + found = true if i.isLDAPGroupReadOnly(e) { - return errorcode.New(errorcode.NotAllowed, "group is read-only") + err = errorcode.New(errorcode.NotAllowed, "group is read-only") + return } dr := ldap.DelRequest{DN: e.DN} - if err = i.conn.Del(&dr); err != nil { - return err - } - return nil + err = i.client.Del(&dr) + return } // UpdateGroupName implements the Backend Interface. -func (i *LDAP) UpdateGroupName(ctx context.Context, groupID string, groupName string) error { +func (i *LDAP) UpdateGroupName(ctx context.Context, groupID string, groupName string) (found bool, err error) { logger := i.logger.SubloggerWithRequestID(ctx) - logger.Debug().Str("backend", "ldap").Msg("AddMembersToGroup") + logger.Debug().Msg("AddMembersToGroup") if !i.writeEnabled && i.groupCreateBaseDN == i.groupBaseDN { - return errorcode.New(errorcode.NotAllowed, "server is configured read-only") + err = errorcode.New(errorcode.NotAllowed, "server is configured read-only") + return } + timer := prometheus.NewTimer(prometheus.ObserverFunc(func(seconds float64) { + r := MetricResultSuccess + if err != nil { + r = MetricResultFailure + } else if !found { + r = MetricResultNotFound + } + i.metrics.ApiOperationDuration.WithLabelValues(MetricOpUpdateGroupName, r).Observe(seconds) + })) + defer timer.ObserveDuration() + ge, err := i.getLDAPGroupByID(groupID, true) if err != nil { - return err + return + } + if ge == nil { + // group does not exist in LDAP: debatable whether that should be an error, or whether + // it should be silently treated as successful, which is something only the caller can + // decide + return } + found = true + if i.isLDAPGroupReadOnly(ge) { - return errorcode.New(errorcode.NotAllowed, "group is read-only") + err = errorcode.New(errorcode.NotAllowed, "group is read-only") + return } if ge.GetEqualFoldAttributeValue(i.groupAttributeMap.name) == groupName { - return nil + // no need to do anything + return } attributeTypeAndValue := ldap.AttributeTypeAndValue{ @@ -270,7 +365,8 @@ func (i *LDAP) UpdateGroupName(ctx context.Context, groupID string, groupName st logger.Debug().Str("originalDN", ge.DN).Str("newDN", newDNString).Msg("Modifying DN") mrdn := ldap.NewModifyDNRequest(ge.DN, newDNString, true, "") - if err := i.conn.ModifyDN(mrdn); err != nil { + if moderr := i.client.ModifyDN(mrdn); moderr != nil { + err = moderr var lerr *ldap.Error logger.Debug().Str("originalDN", ge.DN).Str("newDN", newDNString).Err(err).Msg("Failed to modify DN") if errors.As(err, &lerr) { @@ -278,28 +374,44 @@ func (i *LDAP) UpdateGroupName(ctx context.Context, groupID string, groupName st err = errorcode.New(errorcode.NameAlreadyExists, "Group name already in use") } } - return err + return } - return nil + return } // AddMembersToGroup implements the Backend Interface for the LDAP backend. // Currently, it is limited to adding Users as Group members. Adding other groups // as members is not yet implemented -func (i *LDAP) AddMembersToGroup(ctx context.Context, groupID string, memberIDs []string) error { +func (i *LDAP) AddMembersToGroup(ctx context.Context, groupID string, memberIDs []string) (err error) { logger := i.logger.SubloggerWithRequestID(ctx) - logger.Debug().Str("backend", "ldap").Msg("AddMembersToGroup") + logger.Debug().Msg("AddMembersToGroup") if !i.writeEnabled && i.groupCreateBaseDN == i.groupBaseDN { - return errorcode.New(errorcode.NotAllowed, "server is configured read-only") + err = errorcode.New(errorcode.NotAllowed, "server is configured read-only") + return } + + timer := prometheus.NewTimer(prometheus.ObserverFunc(func(seconds float64) { + r := MetricResultSuccess + if err != nil { + r = MetricResultFailure + } + i.metrics.ApiOperationDuration.WithLabelValues(MetricOpAddMembersToGroup, r).Observe(seconds) + })) + defer timer.ObserveDuration() + ge, err := i.getLDAPGroupByNameOrID(groupID, true) if err != nil { - return err + return + } + if ge == nil { + err = errorcode.New(errorcode.ItemNotFound, "failed to find group") + return } if i.isLDAPGroupReadOnly(ge) { - return errorcode.New(errorcode.NotAllowed, "group is read-only") + err = errorcode.New(errorcode.NotAllowed, "group is read-only") + return } mr := ldap.ModifyRequest{DN: ge.DN} @@ -326,15 +438,22 @@ func (i *LDAP) AddMembersToGroup(ctx context.Context, groupID string, memberIDs } var newMemberDN []string + var me *ldap.Entry + var nDN string for _, memberID := range memberIDs { - me, err := i.getLDAPUserByID(memberID) + me, err = i.getLDAPUserByID(memberID) if err != nil { - return err + return } - nDN, err := ldapdn.ParseNormalize(me.DN) + if me == nil { + err = errorcode.New(errorcode.ItemNotFound, fmt.Sprintf("failed to find group member %q", memberID)) + logger.Error().Err(err).Str("memberID", memberID).Msg("Failed to find member by ID") + return + } + nDN, err = ldapdn.ParseNormalize(me.DN) if err != nil { - logger.Error().Str("new member", me.DN).Err(err).Msg("Couldn't parse DN") - return err + logger.Error().Err(err).Str("memberId", memberID).Str("new-member", me.DN).Msg("Couldn't parse DN") + return } if _, present := currentSet[nDN]; !present { newMemberDN = append(newMemberDN, me.DN) @@ -351,7 +470,8 @@ func (i *LDAP) AddMembersToGroup(ctx context.Context, groupID string, memberIDs // without to delete. for j := 0; j < 2; j++ { mr.Add(i.groupAttributeMap.member, newMemberDN) - if err := i.conn.Modify(&mr); err != nil { + if moderr := i.client.Modify(&mr); moderr != nil { + err = moderr if lerr, ok := err.(*ldap.Error); ok { switch lerr.ResultCode { case ldap.LDAPResultAttributeOrValueExists: @@ -371,45 +491,77 @@ func (i *LDAP) AddMembersToGroup(ctx context.Context, groupID string, memberIDs err = fmt.Errorf("unknown error when trying to modify group member entries") } } - return err + return } // succeeded break } } - return nil + return } // RemoveMemberFromGroup implements the Backend Interface. -func (i *LDAP) RemoveMemberFromGroup(ctx context.Context, groupID string, memberID string) error { +func (i *LDAP) RemoveMemberFromGroup(ctx context.Context, groupID string, memberID string) (foundGroup bool, foundMember bool, foundMemberInGroup bool, err error) { logger := i.logger.SubloggerWithRequestID(ctx) - logger.Debug().Str("backend", "ldap").Msg("RemoveMemberFromGroup") + logger.Debug().Msg("RemoveMemberFromGroup") + if !i.writeEnabled && i.groupCreateBaseDN == i.groupBaseDN { - return errorcode.New(errorcode.NotAllowed, "server is configured read-only") + err = errorcode.New(errorcode.NotAllowed, "server is configured read-only") + return } + timer := prometheus.NewTimer(prometheus.ObserverFunc(func(seconds float64) { + r := MetricResultSuccess + if err != nil { + r = MetricResultFailure + } else if !foundGroup { + r = MetricResultNotFound + } + i.metrics.ApiOperationDuration.WithLabelValues(MetricOpRemoveMemberFromGroup, r).Observe(seconds) + })) + defer timer.ObserveDuration() + ge, err := i.getLDAPGroupByID(groupID, true) if err != nil { - logger.Debug().Str("backend", "ldap").Str("groupID", groupID).Msg("Error looking up group") - return err + logger.Debug().Str("groupID", groupID).Msg("Error looking up group") + return + } + if ge == nil { + // group does not exist in LDAP: debatable whether that should be an error, or whether + // it should be silently treated as successful, which is something only the caller can + // decide + return } + foundGroup = true if i.isLDAPGroupReadOnly(ge) { - return errorcode.New(errorcode.NotAllowed, "group is read-only") + err = errorcode.New(errorcode.NotAllowed, "group is read-only") + return } me, err := i.getLDAPUserByID(memberID) if err != nil { - logger.Debug().Str("backend", "ldap").Str("memberID", memberID).Msg("Error looking up group member") - return err + logger.Debug().Str("memberID", memberID).Msg("Error looking up group member") + return } + if me == nil { + logger.Debug().Str("memberID", memberID).Msg("Failed to find group member") + err = errorcode.New(errorcode.ItemNotFound, fmt.Sprintf("failed to find group member %q", memberID)) + return + } + foundMember = true - logger.Debug().Str("backend", "ldap").Str("groupdn", ge.DN).Str("member", me.DN).Msg("remove member") + logger.Debug().Str("groupdn", ge.DN).Str("member", me.DN).Msg("removing member") if err = i.removeEntryByDNAndAttributeFromEntry(ge, me.DN, i.groupAttributeMap.member); err != nil { - logger.Error().Err(err).Str("backend", "ldap").Str("group", groupID).Str("member", memberID).Msg("Failed to remove member from group.") + if err == ErrNotFound { + logger.Error().Err(err).Str("group", groupID).Str("member", memberID).Msg("Failed to find member in group.") + } else { + foundMemberInGroup = true + logger.Error().Err(err).Str("group", groupID).Str("member", memberID).Msg("Failed to remove member from group.") + } } - return err + return } func (i *LDAP) groupToAddRequest(group libregraph.Group) (*ldap.AddRequest, error) { @@ -518,7 +670,7 @@ func (i *LDAP) getLDAPGroupsByFilter(filter string, requestMembers, single bool) Int("sizelimit", searchRequest.SizeLimit). Interface("attributes", searchRequest.Attributes). Msg("getLDAPGroupsByFilter") - res, err := i.conn.Search(searchRequest) + res, err := i.client.Search(searchRequest) if err != nil { var errmsg string if lerr, ok := err.(*ldap.Error); ok { diff --git a/services/graph/pkg/identity/ldap_group_test.go b/services/graph/pkg/identity/ldap_group_test.go index 26ab8fdb89..541349c714 100644 --- a/services/graph/pkg/identity/ldap_group_test.go +++ b/services/graph/pkg/identity/ldap_group_test.go @@ -334,6 +334,7 @@ func TestUpdateGroupName(t *testing.T) { tests := []struct { name string args args + found bool assertion assert.ErrorAssertionFunc ldapMocks []mockInputs }{ @@ -343,6 +344,7 @@ func TestUpdateGroupName(t *testing.T) { groupId: "some-uuid-string", newName: "TheGroup", }, + found: true, assertion: func(t assert.TestingT, err error, args ...any) bool { return assert.Nil(t, err, args...) }, @@ -384,6 +386,7 @@ func TestUpdateGroupName(t *testing.T) { groupId: "some-uuid-string", newName: "TheGroupWithShinyNewName", }, + found: true, assertion: func(t assert.TestingT, err error, args ...any) bool { return assert.Nil(t, err, args...) }, @@ -446,7 +449,8 @@ func TestUpdateGroupName(t *testing.T) { ldapConfig := lconfig i, _ := getMockedBackend(lm, ldapConfig, &logger) - err := i.UpdateGroupName(context.Background(), tt.args.groupId, tt.args.newName) + ok, err := i.UpdateGroupName(context.Background(), tt.args.groupId, tt.args.newName) + assert.Equal(t, tt.found, ok) tt.assertion(t, err) }) } diff --git a/services/graph/pkg/identity/ldap_test.go b/services/graph/pkg/identity/ldap_test.go index 0f3caf03e4..d58e11d764 100644 --- a/services/graph/pkg/identity/ldap_test.go +++ b/services/graph/pkg/identity/ldap_test.go @@ -14,12 +14,13 @@ import ( "github.com/opencloud-eu/opencloud/pkg/log" "github.com/opencloud-eu/opencloud/services/graph/pkg/config" "github.com/opencloud-eu/opencloud/services/graph/pkg/identity/mocks" + "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" ) func getMockedBackend(l ldap.Client, lc config.LDAP, logger *log.Logger) (*LDAP, error) { - return NewLDAPBackend(l, lc, logger) + return NewLDAPBackend(l, lc, logger, "opencloud", "test", prometheus.NewRegistry()) } const ( @@ -107,29 +108,29 @@ func TestNewLDAPBackend(t *testing.T) { tc := lconfig tc.UserDisplayNameAttribute = "" - if _, err := NewLDAPBackend(l, tc, &logger); err == nil { + if _, err := NewLDAPBackend(l, tc, &logger, "opencloud", "test", prometheus.NewRegistry()); err == nil { t.Error("Should fail with incomplete user attr config") } tc = lconfig tc.GroupIDAttribute = "" - if _, err := NewLDAPBackend(l, tc, &logger); err == nil { + if _, err := NewLDAPBackend(l, tc, &logger, "opencloud", "test", prometheus.NewRegistry()); err == nil { t.Errorf("Should fail with incomplete group config") } tc = lconfig tc.UserSearchScope = "" - if _, err := NewLDAPBackend(l, tc, &logger); err == nil { + if _, err := NewLDAPBackend(l, tc, &logger, "opencloud", "test", prometheus.NewRegistry()); err == nil { t.Errorf("Should fail with invalid user search scope") } tc = lconfig tc.GroupSearchScope = "" - if _, err := NewLDAPBackend(l, tc, &logger); err == nil { + if _, err := NewLDAPBackend(l, tc, &logger, "opencloud", "test", prometheus.NewRegistry()); err == nil { t.Errorf("Should fail with invalid group search scope") } - if _, err := NewLDAPBackend(l, lconfig, &logger); err != nil { + if _, err := NewLDAPBackend(l, lconfig, &logger, "opencloud", "test", prometheus.NewRegistry()); err != nil { t.Errorf("Should fail with invalid group search scope") } } @@ -172,7 +173,7 @@ func TestCreateUser(t *testing.T) { c := lconfig c.UseServerUUID = true - b, _ := NewLDAPBackend(l, c, &logger) + b, _ := NewLDAPBackend(l, c, &logger, "opencloud", "test", prometheus.NewRegistry()) newUser, err := b.CreateUser(context.Background(), *user) assert.Nil(t, err) @@ -189,12 +190,14 @@ func TestCreateUserModelFromLDAP(t *testing.T) { l := &mocks.Client{} logger := log.NewLogger(log.Level("debug")) - b, _ := NewLDAPBackend(l, lconfig, &logger) - if user := b.createUserModelFromLDAP(nil); user != nil { - t.Errorf("createUserModelFromLDAP should return on nil Entry") + b, _ := NewLDAPBackend(l, lconfig, &logger, "opencloud", "test", prometheus.NewRegistry()) + if _, err := b.createUserModelFromLDAP(nil); err == nil { + t.Errorf("createUserModelFromLDAP should return an error on nil Entry") } - user := b.createUserModelFromLDAP(userEntry) - if user == nil { + user, err := b.createUserModelFromLDAP(userEntry) + if err != nil { + t.Error("Converting a valid LDAP Entry should succeed and not return an error") + } else if user == nil { t.Error("Converting a valid LDAP Entry should succeed") } else { if user.OnPremisesSamAccountName != userEntry.GetEqualFoldAttributeValue(b.userAttributeMap.userName) { diff --git a/services/graph/pkg/identity/mocks/backend.go b/services/graph/pkg/identity/mocks/backend.go index ec056e5e80..60f45803f4 100644 --- a/services/graph/pkg/identity/mocks/backend.go +++ b/services/graph/pkg/identity/mocks/backend.go @@ -241,20 +241,29 @@ func (_c *Backend_CreateUser_Call) RunAndReturn(run func(ctx context.Context, us } // DeleteGroup provides a mock function for the type Backend -func (_mock *Backend) DeleteGroup(ctx context.Context, id string) error { +func (_mock *Backend) DeleteGroup(ctx context.Context, id string) (bool, error) { ret := _mock.Called(ctx, id) if len(ret) == 0 { panic("no return value specified for DeleteGroup") } - var r0 error - if returnFunc, ok := ret.Get(0).(func(context.Context, string) error); ok { + var r0 bool + var r1 error + if returnFunc, ok := ret.Get(0).(func(context.Context, string) (bool, error)); ok { + return returnFunc(ctx, id) + } + if returnFunc, ok := ret.Get(0).(func(context.Context, string) bool); ok { r0 = returnFunc(ctx, id) } else { - r0 = ret.Error(0) + r0 = ret.Get(0).(bool) } - return r0 + if returnFunc, ok := ret.Get(1).(func(context.Context, string) error); ok { + r1 = returnFunc(ctx, id) + } else { + r1 = ret.Error(1) + } + return r0, r1 } // Backend_DeleteGroup_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'DeleteGroup' @@ -287,31 +296,40 @@ func (_c *Backend_DeleteGroup_Call) Run(run func(ctx context.Context, id string) return _c } -func (_c *Backend_DeleteGroup_Call) Return(err error) *Backend_DeleteGroup_Call { - _c.Call.Return(err) +func (_c *Backend_DeleteGroup_Call) Return(foundGroup bool, err error) *Backend_DeleteGroup_Call { + _c.Call.Return(foundGroup, err) return _c } -func (_c *Backend_DeleteGroup_Call) RunAndReturn(run func(ctx context.Context, id string) error) *Backend_DeleteGroup_Call { +func (_c *Backend_DeleteGroup_Call) RunAndReturn(run func(ctx context.Context, id string) (bool, error)) *Backend_DeleteGroup_Call { _c.Call.Return(run) return _c } // DeleteUser provides a mock function for the type Backend -func (_mock *Backend) DeleteUser(ctx context.Context, nameOrID string) error { +func (_mock *Backend) DeleteUser(ctx context.Context, nameOrID string) (bool, error) { ret := _mock.Called(ctx, nameOrID) if len(ret) == 0 { panic("no return value specified for DeleteUser") } - var r0 error - if returnFunc, ok := ret.Get(0).(func(context.Context, string) error); ok { + var r0 bool + var r1 error + if returnFunc, ok := ret.Get(0).(func(context.Context, string) (bool, error)); ok { + return returnFunc(ctx, nameOrID) + } + if returnFunc, ok := ret.Get(0).(func(context.Context, string) bool); ok { r0 = returnFunc(ctx, nameOrID) } else { - r0 = ret.Error(0) + r0 = ret.Get(0).(bool) } - return r0 + if returnFunc, ok := ret.Get(1).(func(context.Context, string) error); ok { + r1 = returnFunc(ctx, nameOrID) + } else { + r1 = ret.Error(1) + } + return r0, r1 } // Backend_DeleteUser_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'DeleteUser' @@ -344,12 +362,12 @@ func (_c *Backend_DeleteUser_Call) Run(run func(ctx context.Context, nameOrID st return _c } -func (_c *Backend_DeleteUser_Call) Return(err error) *Backend_DeleteUser_Call { - _c.Call.Return(err) +func (_c *Backend_DeleteUser_Call) Return(b bool, err error) *Backend_DeleteUser_Call { + _c.Call.Return(b, err) return _c } -func (_c *Backend_DeleteUser_Call) RunAndReturn(run func(ctx context.Context, nameOrID string) error) *Backend_DeleteUser_Call { +func (_c *Backend_DeleteUser_Call) RunAndReturn(run func(ctx context.Context, nameOrID string) (bool, error)) *Backend_DeleteUser_Call { _c.Call.Return(run) return _c } @@ -787,20 +805,41 @@ func (_c *Backend_GetUsers_Call) RunAndReturn(run func(ctx context.Context, oreq } // RemoveMemberFromGroup provides a mock function for the type Backend -func (_mock *Backend) RemoveMemberFromGroup(ctx context.Context, groupID string, memberID string) error { +func (_mock *Backend) RemoveMemberFromGroup(ctx context.Context, groupID string, memberID string) (bool, bool, bool, error) { ret := _mock.Called(ctx, groupID, memberID) if len(ret) == 0 { panic("no return value specified for RemoveMemberFromGroup") } - var r0 error - if returnFunc, ok := ret.Get(0).(func(context.Context, string, string) error); ok { + var r0 bool + var r1 bool + var r2 bool + var r3 error + if returnFunc, ok := ret.Get(0).(func(context.Context, string, string) (bool, bool, bool, error)); ok { + return returnFunc(ctx, groupID, memberID) + } + if returnFunc, ok := ret.Get(0).(func(context.Context, string, string) bool); ok { r0 = returnFunc(ctx, groupID, memberID) } else { - r0 = ret.Error(0) + r0 = ret.Get(0).(bool) } - return r0 + if returnFunc, ok := ret.Get(1).(func(context.Context, string, string) bool); ok { + r1 = returnFunc(ctx, groupID, memberID) + } else { + r1 = ret.Get(1).(bool) + } + if returnFunc, ok := ret.Get(2).(func(context.Context, string, string) bool); ok { + r2 = returnFunc(ctx, groupID, memberID) + } else { + r2 = ret.Get(2).(bool) + } + if returnFunc, ok := ret.Get(3).(func(context.Context, string, string) error); ok { + r3 = returnFunc(ctx, groupID, memberID) + } else { + r3 = ret.Error(3) + } + return r0, r1, r2, r3 } // Backend_RemoveMemberFromGroup_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'RemoveMemberFromGroup' @@ -839,31 +878,40 @@ func (_c *Backend_RemoveMemberFromGroup_Call) Run(run func(ctx context.Context, return _c } -func (_c *Backend_RemoveMemberFromGroup_Call) Return(err error) *Backend_RemoveMemberFromGroup_Call { - _c.Call.Return(err) +func (_c *Backend_RemoveMemberFromGroup_Call) Return(foundGroup bool, foundMember bool, foundMemberInGroup bool, err error) *Backend_RemoveMemberFromGroup_Call { + _c.Call.Return(foundGroup, foundMember, foundMemberInGroup, err) return _c } -func (_c *Backend_RemoveMemberFromGroup_Call) RunAndReturn(run func(ctx context.Context, groupID string, memberID string) error) *Backend_RemoveMemberFromGroup_Call { +func (_c *Backend_RemoveMemberFromGroup_Call) RunAndReturn(run func(ctx context.Context, groupID string, memberID string) (bool, bool, bool, error)) *Backend_RemoveMemberFromGroup_Call { _c.Call.Return(run) return _c } // UpdateGroupName provides a mock function for the type Backend -func (_mock *Backend) UpdateGroupName(ctx context.Context, groupID string, groupName string) error { +func (_mock *Backend) UpdateGroupName(ctx context.Context, groupID string, groupName string) (bool, error) { ret := _mock.Called(ctx, groupID, groupName) if len(ret) == 0 { panic("no return value specified for UpdateGroupName") } - var r0 error - if returnFunc, ok := ret.Get(0).(func(context.Context, string, string) error); ok { + var r0 bool + var r1 error + if returnFunc, ok := ret.Get(0).(func(context.Context, string, string) (bool, error)); ok { + return returnFunc(ctx, groupID, groupName) + } + if returnFunc, ok := ret.Get(0).(func(context.Context, string, string) bool); ok { r0 = returnFunc(ctx, groupID, groupName) } else { - r0 = ret.Error(0) + r0 = ret.Get(0).(bool) } - return r0 + if returnFunc, ok := ret.Get(1).(func(context.Context, string, string) error); ok { + r1 = returnFunc(ctx, groupID, groupName) + } else { + r1 = ret.Error(1) + } + return r0, r1 } // Backend_UpdateGroupName_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'UpdateGroupName' @@ -902,31 +950,40 @@ func (_c *Backend_UpdateGroupName_Call) Run(run func(ctx context.Context, groupI return _c } -func (_c *Backend_UpdateGroupName_Call) Return(err error) *Backend_UpdateGroupName_Call { - _c.Call.Return(err) +func (_c *Backend_UpdateGroupName_Call) Return(foundGroup bool, err error) *Backend_UpdateGroupName_Call { + _c.Call.Return(foundGroup, err) return _c } -func (_c *Backend_UpdateGroupName_Call) RunAndReturn(run func(ctx context.Context, groupID string, groupName string) error) *Backend_UpdateGroupName_Call { +func (_c *Backend_UpdateGroupName_Call) RunAndReturn(run func(ctx context.Context, groupID string, groupName string) (bool, error)) *Backend_UpdateGroupName_Call { _c.Call.Return(run) return _c } // UpdateLastSignInDate provides a mock function for the type Backend -func (_mock *Backend) UpdateLastSignInDate(ctx context.Context, userID string, timestamp time.Time) error { +func (_mock *Backend) UpdateLastSignInDate(ctx context.Context, userID string, timestamp time.Time) (bool, error) { ret := _mock.Called(ctx, userID, timestamp) if len(ret) == 0 { panic("no return value specified for UpdateLastSignInDate") } - var r0 error - if returnFunc, ok := ret.Get(0).(func(context.Context, string, time.Time) error); ok { + var r0 bool + var r1 error + if returnFunc, ok := ret.Get(0).(func(context.Context, string, time.Time) (bool, error)); ok { + return returnFunc(ctx, userID, timestamp) + } + if returnFunc, ok := ret.Get(0).(func(context.Context, string, time.Time) bool); ok { r0 = returnFunc(ctx, userID, timestamp) } else { - r0 = ret.Error(0) + r0 = ret.Get(0).(bool) } - return r0 + if returnFunc, ok := ret.Get(1).(func(context.Context, string, time.Time) error); ok { + r1 = returnFunc(ctx, userID, timestamp) + } else { + r1 = ret.Error(1) + } + return r0, r1 } // Backend_UpdateLastSignInDate_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'UpdateLastSignInDate' @@ -965,12 +1022,12 @@ func (_c *Backend_UpdateLastSignInDate_Call) Run(run func(ctx context.Context, u return _c } -func (_c *Backend_UpdateLastSignInDate_Call) Return(err error) *Backend_UpdateLastSignInDate_Call { - _c.Call.Return(err) +func (_c *Backend_UpdateLastSignInDate_Call) Return(b bool, err error) *Backend_UpdateLastSignInDate_Call { + _c.Call.Return(b, err) return _c } -func (_c *Backend_UpdateLastSignInDate_Call) RunAndReturn(run func(ctx context.Context, userID string, timestamp time.Time) error) *Backend_UpdateLastSignInDate_Call { +func (_c *Backend_UpdateLastSignInDate_Call) RunAndReturn(run func(ctx context.Context, userID string, timestamp time.Time) (bool, error)) *Backend_UpdateLastSignInDate_Call { _c.Call.Return(run) return _c } diff --git a/services/graph/pkg/identity/mocks/education_backend.go b/services/graph/pkg/identity/mocks/education_backend.go index f710fa9467..462cbefa1d 100644 --- a/services/graph/pkg/identity/mocks/education_backend.go +++ b/services/graph/pkg/identity/mocks/education_backend.go @@ -165,20 +165,29 @@ func (_c *EducationBackend_AddTeacherToEducationClass_Call) RunAndReturn(run fun } // AddUsersToEducationSchool provides a mock function for the type EducationBackend -func (_mock *EducationBackend) AddUsersToEducationSchool(ctx context.Context, schoolID string, memberID []string) error { +func (_mock *EducationBackend) AddUsersToEducationSchool(ctx context.Context, schoolID string, memberID []string) (bool, error) { ret := _mock.Called(ctx, schoolID, memberID) if len(ret) == 0 { panic("no return value specified for AddUsersToEducationSchool") } - var r0 error - if returnFunc, ok := ret.Get(0).(func(context.Context, string, []string) error); ok { + var r0 bool + var r1 error + if returnFunc, ok := ret.Get(0).(func(context.Context, string, []string) (bool, error)); ok { + return returnFunc(ctx, schoolID, memberID) + } + if returnFunc, ok := ret.Get(0).(func(context.Context, string, []string) bool); ok { r0 = returnFunc(ctx, schoolID, memberID) } else { - r0 = ret.Error(0) + r0 = ret.Get(0).(bool) } - return r0 + if returnFunc, ok := ret.Get(1).(func(context.Context, string, []string) error); ok { + r1 = returnFunc(ctx, schoolID, memberID) + } else { + r1 = ret.Error(1) + } + return r0, r1 } // EducationBackend_AddUsersToEducationSchool_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'AddUsersToEducationSchool' @@ -217,12 +226,12 @@ func (_c *EducationBackend_AddUsersToEducationSchool_Call) Run(run func(ctx cont return _c } -func (_c *EducationBackend_AddUsersToEducationSchool_Call) Return(err error) *EducationBackend_AddUsersToEducationSchool_Call { - _c.Call.Return(err) +func (_c *EducationBackend_AddUsersToEducationSchool_Call) Return(found bool, err error) *EducationBackend_AddUsersToEducationSchool_Call { + _c.Call.Return(found, err) return _c } -func (_c *EducationBackend_AddUsersToEducationSchool_Call) RunAndReturn(run func(ctx context.Context, schoolID string, memberID []string) error) *EducationBackend_AddUsersToEducationSchool_Call { +func (_c *EducationBackend_AddUsersToEducationSchool_Call) RunAndReturn(run func(ctx context.Context, schoolID string, memberID []string) (bool, error)) *EducationBackend_AddUsersToEducationSchool_Call { _c.Call.Return(run) return _c } @@ -489,20 +498,29 @@ func (_c *EducationBackend_DeleteEducationClass_Call) RunAndReturn(run func(ctx } // DeleteEducationSchool provides a mock function for the type EducationBackend -func (_mock *EducationBackend) DeleteEducationSchool(ctx context.Context, id string) error { +func (_mock *EducationBackend) DeleteEducationSchool(ctx context.Context, id string) (bool, error) { ret := _mock.Called(ctx, id) if len(ret) == 0 { panic("no return value specified for DeleteEducationSchool") } - var r0 error - if returnFunc, ok := ret.Get(0).(func(context.Context, string) error); ok { + var r0 bool + var r1 error + if returnFunc, ok := ret.Get(0).(func(context.Context, string) (bool, error)); ok { + return returnFunc(ctx, id) + } + if returnFunc, ok := ret.Get(0).(func(context.Context, string) bool); ok { r0 = returnFunc(ctx, id) } else { - r0 = ret.Error(0) + r0 = ret.Get(0).(bool) } - return r0 + if returnFunc, ok := ret.Get(1).(func(context.Context, string) error); ok { + r1 = returnFunc(ctx, id) + } else { + r1 = ret.Error(1) + } + return r0, r1 } // EducationBackend_DeleteEducationSchool_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'DeleteEducationSchool' @@ -535,12 +553,12 @@ func (_c *EducationBackend_DeleteEducationSchool_Call) Run(run func(ctx context. return _c } -func (_c *EducationBackend_DeleteEducationSchool_Call) Return(err error) *EducationBackend_DeleteEducationSchool_Call { - _c.Call.Return(err) +func (_c *EducationBackend_DeleteEducationSchool_Call) Return(found bool, err error) *EducationBackend_DeleteEducationSchool_Call { + _c.Call.Return(found, err) return _c } -func (_c *EducationBackend_DeleteEducationSchool_Call) RunAndReturn(run func(ctx context.Context, id string) error) *EducationBackend_DeleteEducationSchool_Call { +func (_c *EducationBackend_DeleteEducationSchool_Call) RunAndReturn(run func(ctx context.Context, id string) (bool, error)) *EducationBackend_DeleteEducationSchool_Call { _c.Call.Return(run) return _c } @@ -1539,20 +1557,41 @@ func (_c *EducationBackend_RemoveTeacherFromEducationClass_Call) RunAndReturn(ru } // RemoveUserFromEducationSchool provides a mock function for the type EducationBackend -func (_mock *EducationBackend) RemoveUserFromEducationSchool(ctx context.Context, schoolID string, memberID string) error { +func (_mock *EducationBackend) RemoveUserFromEducationSchool(ctx context.Context, schoolID string, memberID string) (bool, bool, bool, error) { ret := _mock.Called(ctx, schoolID, memberID) if len(ret) == 0 { panic("no return value specified for RemoveUserFromEducationSchool") } - var r0 error - if returnFunc, ok := ret.Get(0).(func(context.Context, string, string) error); ok { + var r0 bool + var r1 bool + var r2 bool + var r3 error + if returnFunc, ok := ret.Get(0).(func(context.Context, string, string) (bool, bool, bool, error)); ok { + return returnFunc(ctx, schoolID, memberID) + } + if returnFunc, ok := ret.Get(0).(func(context.Context, string, string) bool); ok { r0 = returnFunc(ctx, schoolID, memberID) } else { - r0 = ret.Error(0) + r0 = ret.Get(0).(bool) } - return r0 + if returnFunc, ok := ret.Get(1).(func(context.Context, string, string) bool); ok { + r1 = returnFunc(ctx, schoolID, memberID) + } else { + r1 = ret.Get(1).(bool) + } + if returnFunc, ok := ret.Get(2).(func(context.Context, string, string) bool); ok { + r2 = returnFunc(ctx, schoolID, memberID) + } else { + r2 = ret.Get(2).(bool) + } + if returnFunc, ok := ret.Get(3).(func(context.Context, string, string) error); ok { + r3 = returnFunc(ctx, schoolID, memberID) + } else { + r3 = ret.Error(3) + } + return r0, r1, r2, r3 } // EducationBackend_RemoveUserFromEducationSchool_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'RemoveUserFromEducationSchool' @@ -1591,12 +1630,12 @@ func (_c *EducationBackend_RemoveUserFromEducationSchool_Call) Run(run func(ctx return _c } -func (_c *EducationBackend_RemoveUserFromEducationSchool_Call) Return(err error) *EducationBackend_RemoveUserFromEducationSchool_Call { - _c.Call.Return(err) +func (_c *EducationBackend_RemoveUserFromEducationSchool_Call) Return(foundSchool bool, foundUser bool, foundUserInSchool bool, err error) *EducationBackend_RemoveUserFromEducationSchool_Call { + _c.Call.Return(foundSchool, foundUser, foundUserInSchool, err) return _c } -func (_c *EducationBackend_RemoveUserFromEducationSchool_Call) RunAndReturn(run func(ctx context.Context, schoolID string, memberID string) error) *EducationBackend_RemoveUserFromEducationSchool_Call { +func (_c *EducationBackend_RemoveUserFromEducationSchool_Call) RunAndReturn(run func(ctx context.Context, schoolID string, memberID string) (bool, bool, bool, error)) *EducationBackend_RemoveUserFromEducationSchool_Call { _c.Call.Return(run) return _c } diff --git a/services/graph/pkg/metrics/metrics.go b/services/graph/pkg/metrics/metrics.go index 7e597327a2..778538e650 100644 --- a/services/graph/pkg/metrics/metrics.go +++ b/services/graph/pkg/metrics/metrics.go @@ -1,6 +1,12 @@ package metrics -import "github.com/prometheus/client_golang/prometheus" +import ( + "strconv" + "strings" + "time" + + "github.com/prometheus/client_golang/prometheus" +) var ( // Namespace defines the namespace for the defines metrics. @@ -12,12 +18,41 @@ var ( // Metrics defines the available metrics of this service. type Metrics struct { - // Counter *prometheus.CounterVec - BuildInfo *prometheus.GaugeVec + BuildInfo *prometheus.GaugeVec + EventsEnabled prometheus.Gauge + HttpEnabled prometheus.Gauge + EventsProcessed *prometheus.CounterVec + InvalidEvents prometheus.Counter + UnsupportedEvents prometheus.Counter + UserPasswordChanges prometheus.Counter + httpRequestDuration *prometheus.HistogramVec + httpPathSplitter func(pieces []string) (string, string) } +const ( + ResultSuccess = "success" + ResultFailure = "failure" + + ResultClientError = "client-error" + ResultServerError = "server-error" +) + +const ( + LabelMethod = "method" + LabelPath = "path" + LabelVersion = "version" + LabelResource = "resource" + LabelCode = "code" + LabelResult = "result" + LabelEvent = "event" +) + +const ( + UnmatchedRoutePattern = "unknown" +) + // New initializes the available metrics. -func New() *Metrics { +func New(registerer prometheus.Registerer, httpPathSplitter func(pieces []string) (string, string)) *Metrics { m := &Metrics{ BuildInfo: prometheus.NewGaugeVec(prometheus.GaugeOpts{ Namespace: Namespace, @@ -25,9 +60,83 @@ func New() *Metrics { Name: "build_info", Help: "Build information", }, []string{"version"}), + EventsEnabled: prometheus.NewGauge(prometheus.GaugeOpts{ + Namespace: Namespace, + Subsystem: Subsystem, + Name: "events_enabled", + Help: "Whether this instance consumes events (1) or not (0)", + }), + HttpEnabled: prometheus.NewGauge(prometheus.GaugeOpts{ + Namespace: Namespace, + Subsystem: Subsystem, + Name: "http_enabled", + Help: "Whether this instance processes HTTP API calls (1) or not (0)", + }), + EventsProcessed: prometheus.NewCounterVec(prometheus.CounterOpts{ + Namespace: Namespace, + Subsystem: Subsystem, + Name: "events", + Help: "Number of consumed events", + }, []string{LabelEvent, LabelResult}), + InvalidEvents: prometheus.NewCounter(prometheus.CounterOpts{ + Namespace: Namespace, + Subsystem: Subsystem, + Name: "events_invalid", + Help: "Number of supported events with invalid data", + }), + UnsupportedEvents: prometheus.NewCounter(prometheus.CounterOpts{ + Namespace: Namespace, + Subsystem: Subsystem, + Name: "events_unsupported", + Help: "Number of unsupported events that were consumed and ignored", + }), + UserPasswordChanges: prometheus.NewCounter(prometheus.CounterOpts{ + Namespace: Namespace, + Subsystem: Subsystem, + Name: "user_password_changes", + Help: "Counts occurences of users changing their password", + }), + // keeping this one private as it should only be used via the RecordHTTPDuration() method below, + // as its number of labels is too fragile to keep in check if they ever change + httpRequestDuration: prometheus.NewHistogramVec(prometheus.HistogramOpts{ + Namespace: Namespace, + Subsystem: Subsystem, + Name: "http_request_duration_seconds", + Help: "Duration of HTTP operations in seconds.", + Buckets: prometheus.DefBuckets, + }, []string{LabelMethod, LabelPath, LabelVersion, LabelResource, LabelCode, LabelResult}), // when changing these, make sure to also modify the methods below accordingly + httpPathSplitter: httpPathSplitter, } _ = prometheus.Register(m.BuildInfo) - // TODO: implement metrics + _ = prometheus.Register(m.EventsEnabled) + _ = prometheus.Register(m.HttpEnabled) + _ = prometheus.Register(m.EventsProcessed) + _ = prometheus.Register(m.InvalidEvents) + _ = prometheus.Register(m.UnsupportedEvents) + _ = prometheus.Register(m.UserPasswordChanges) + _ = prometheus.Register(m.httpRequestDuration) + + // TODO: implement more metrics + return m } + +func (m Metrics) RecordHTTPDuration(method string, pattern string, statusCode int, duration time.Duration) { + result := "" + if statusCode < 400 { + result = ResultSuccess + } else if statusCode < 500 { + result = ResultClientError + } else { + result = ResultServerError + } + // all the HTTP routes for the Graph API start with a version (v1.0 or v1beta1), followed by a top level + // resource "module", which might be useful to extract and include as a label, to aggregate metrics and + // statistics before drilling down further + pieces := strings.FieldsFunc(pattern, func(r rune) bool { + return r == '/' + }) + version, resource := m.httpPathSplitter(pieces) + m.httpRequestDuration.WithLabelValues(method, pattern, version, resource, strconv.Itoa(statusCode), result).Observe(duration.Seconds()) +} diff --git a/services/graph/pkg/metrics/middleware.go b/services/graph/pkg/metrics/middleware.go new file mode 100644 index 0000000000..555aa3cd03 --- /dev/null +++ b/services/graph/pkg/metrics/middleware.go @@ -0,0 +1,50 @@ +package metrics + +import ( + "net/http" + "time" + + "github.com/go-chi/chi/v5" +) + +type statusResponseWriter struct { + http.ResponseWriter + statusCode int +} + +func (rw *statusResponseWriter) WriteHeader(code int) { + rw.statusCode = code + rw.ResponseWriter.WriteHeader(code) +} + +// A middleware that tracks the duration of every inbound Graph API HTTP call +// and calls a function to delegate the storage of that duration into a +// histogram metric, analyzing the incoming query and deconstructing it into +// method, path pattern, as well as the resulting status code. +// +// Note that to avoid a high cardinality on the path label, the URL is matched +// against the chi routing rules, passing the path pattern to the function +// instead of the actual URI. +func DurationMetric(observe func(method, pattern string, statusCode int, duration time.Duration)) func(next http.Handler) http.Handler { + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + start := time.Now() + responseWrapper := &statusResponseWriter{ResponseWriter: w, statusCode: 200} // 200 OK is the default when it's not set + next.ServeHTTP(responseWrapper, r) + duration := time.Since(start) + + method := r.Method + routePattern := UnmatchedRoutePattern + if rctx := chi.RouteContext(r.Context()); rctx != nil { + if pattern := rctx.RoutePattern(); pattern != "" { + routePattern = pattern + if method == "" { + method = rctx.RouteMethod + } + } + } + + observe(r.Method, routePattern, responseWrapper.statusCode, duration) + }) + } +} diff --git a/services/graph/pkg/middleware/requireadmin.go b/services/graph/pkg/middleware/requireadmin.go index 81fc3374c4..515b309c2d 100644 --- a/services/graph/pkg/middleware/requireadmin.go +++ b/services/graph/pkg/middleware/requireadmin.go @@ -12,15 +12,21 @@ import ( // RequireAdmin middleware is used to require the user in context to be an admin / have account management permissions func RequireAdmin(rm *roles.Manager, logger log.Logger) func(next http.Handler) http.Handler { + l := log.Logger{Logger: logger.With().Str("middleware", "requireAdmin").Logger()} return func(next http.Handler) http.Handler { - l := logger.With().Str("middleware", "requireAdmin").Logger() return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { u, ok := revactx.ContextGetUser(r.Context()) if !ok { errorcode.AccessDenied.Render(w, r, http.StatusUnauthorized, "Unauthorized") return } + if u == nil { + l.Debug().Msg("Bad request: user is missing") + errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "user is missing an id") + return + } if u.Id == nil || u.Id.OpaqueId == "" { + l.Debug().Msg("Bad request: user does not have an id") errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "user is missing an id") return } @@ -48,6 +54,7 @@ func RequireAdmin(rm *roles.Manager, logger log.Logger) func(next http.Handler) return } + l.Debug().Str("userid", u.Id.OpaqueId).Str("permission", settings.AccountManagementPermissionID).Msg("Access denied: necessary permission %q not present in user's roles") errorcode.AccessDenied.Render(w, r, http.StatusForbidden, "Forbidden") }) } diff --git a/services/graph/pkg/server/http/server.go b/services/graph/pkg/server/http/server.go index c5830949b0..1f978f4423 100644 --- a/services/graph/pkg/server/http/server.go +++ b/services/graph/pkg/server/http/server.go @@ -8,7 +8,6 @@ import ( gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" chimiddleware "github.com/go-chi/chi/v5/middleware" - "github.com/opencloud-eu/reva/v2/pkg/events/stream" "github.com/opencloud-eu/reva/v2/pkg/rgrpc/todo/pool" revaMetadata "github.com/opencloud-eu/reva/v2/pkg/storage/utils/metadata" "go-micro.dev/v4" @@ -16,7 +15,6 @@ import ( "github.com/opencloud-eu/opencloud/pkg/account" "github.com/opencloud-eu/opencloud/pkg/cors" - "github.com/opencloud-eu/opencloud/pkg/generators" "github.com/opencloud-eu/opencloud/pkg/keycloak" "github.com/opencloud-eu/opencloud/pkg/middleware" "github.com/opencloud-eu/opencloud/pkg/registry" @@ -27,12 +25,14 @@ import ( ehsvc "github.com/opencloud-eu/opencloud/protogen/gen/opencloud/services/eventhistory/v0" searchsvc "github.com/opencloud-eu/opencloud/protogen/gen/opencloud/services/search/v0" settingssvc "github.com/opencloud-eu/opencloud/protogen/gen/opencloud/services/settings/v0" + "github.com/opencloud-eu/opencloud/services/graph/pkg/identity" + "github.com/opencloud-eu/opencloud/services/graph/pkg/metrics" graphMiddleware "github.com/opencloud-eu/opencloud/services/graph/pkg/middleware" svc "github.com/opencloud-eu/opencloud/services/graph/pkg/service/v0" ) // Server initializes the http service and server. -func Server(opts ...Option) (http.Service, error) { +func Server(identityBackend identity.Backend, eduBackend identity.EducationBackend, eventsStream events.Stream, opts ...Option) (http.Service, error) { options := newOptions(opts...) service, err := http.NewService( @@ -53,20 +53,6 @@ func Server(opts ...Option) (http.Service, error) { return http.Service{}, fmt.Errorf("could not initialize http service: %w", err) } - var eventsStream events.Stream - - if options.Config.Events.Endpoint != "" { - var err error - connName := generators.GenerateConnectionName(options.Config.Service.Name, generators.NTypeBus) - eventsStream, err = stream.NatsFromConfig(connName, false, stream.NatsConfig(options.Config.Events)) - if err != nil { - options.Logger.Error(). - Err(err). - Msg("Error initializing events publisher") - return http.Service{}, fmt.Errorf("could not initialize events publisher: %w", err) - } - } - middlewares := []func(stdhttp.Handler) stdhttp.Handler{ middleware.TraceContext, chimiddleware.RequestID, @@ -77,6 +63,13 @@ func Server(opts ...Option) (http.Service, error) { middleware.Logger( options.Logger, ), + } + + if !options.Config.HTTP.Metrics.Disabled { + middlewares = append(middlewares, metrics.DurationMetric(options.Metrics.RecordHTTPDuration)) + } + + middlewares = append(middlewares, middleware.Cors( cors.Logger(options.Logger), cors.AllowedOrigins(options.Config.HTTP.CORS.AllowedOrigins), @@ -84,7 +77,8 @@ func Server(opts ...Option) (http.Service, error) { cors.AllowedHeaders(options.Config.HTTP.CORS.AllowedHeaders), cors.AllowCredentials(options.Config.HTTP.CORS.AllowCredentials), ), - } + ) + // how do we secure the api? var requireAdminMiddleware func(stdhttp.Handler) stdhttp.Handler var roleService svc.RoleService @@ -167,9 +161,9 @@ func Server(opts ...Option) (http.Service, error) { svc.UserProfilePhotoService(userProfilePhotoService), svc.Logger(options.Logger), svc.Config(options.Config), + svc.Metrics(options.Metrics), svc.Middleware(middlewares...), - svc.EventsPublisher(eventsStream), - svc.EventsConsumer(eventsStream), + svc.EventsPublisher(eventsStream), // is required even when event consumption is disabled svc.WithRoleService(roleService), svc.WithValueService(valueService), svc.WithRequireAdminMiddleware(requireAdminMiddleware), @@ -179,6 +173,8 @@ func Server(opts ...Option) (http.Service, error) { svc.EventHistoryClient(hClient), svc.TraceProvider(options.TraceProvider), svc.WithNatsKeyValue(options.NatsKeyValue), + svc.WithIdentityBackend(identityBackend), + svc.WithIdentityEducationBackend(eduBackend), ) if err != nil { @@ -188,6 +184,5 @@ func Server(opts ...Option) (http.Service, error) { if err := micro.RegisterHandler(service.Server(), handle); err != nil { return http.Service{}, fmt.Errorf("could not register graph service handler: %w", err) } - return service, nil } diff --git a/services/graph/pkg/service/events/service.go b/services/graph/pkg/service/events/service.go new file mode 100644 index 0000000000..8e70c1f909 --- /dev/null +++ b/services/graph/pkg/service/events/service.go @@ -0,0 +1,123 @@ +package events + +import ( + "context" + "io" + "sync/atomic" + + "github.com/opencloud-eu/reva/v2/pkg/events" + "github.com/opencloud-eu/reva/v2/pkg/utils" + + "github.com/opencloud-eu/opencloud/pkg/log" + "github.com/opencloud-eu/opencloud/services/graph/pkg/identity" + "github.com/opencloud-eu/opencloud/services/graph/pkg/metrics" +) + +func processEvents(ctx context.Context, consumer events.Consumer, stop *atomic.Bool, stopCh chan struct{}, + backend identity.Backend, m *metrics.Metrics, logger *log.Logger) error { + var _registeredEvents = []events.Unmarshaller{ + events.UserSignedIn{}, + } + evChannel, err := events.Consume(consumer, "graph", _registeredEvents...) + if err != nil { + logger.Error().Err(err).Msg("cannot consume from nats") + return err + } + logger.Debug().Msg("listening for events") + for loop := true; loop; { + select { + case e := <-evChannel: + switch ev := e.Event.(type) { + default: + // this branch is currently impossible to test and run into because we pick which events we're interested in + // through the _registeredEvents above, and the stream won't hand us events we didn't register for + m.UnsupportedEvents.Inc() + logger.Error().Interface("event", e).Msg("unhandled event") + case events.UserSignedIn: + name := "UserSignedIn" + userId := "" + if ev.Executant != nil && ev.Executant.OpaqueId != "" { + userId = ev.Executant.OpaqueId + } else { + m.InvalidEvents.Inc() + logger.Error().Err(err).Interface("event", ev).Msg("Received invalid event: executant.opaqueId not set") + continue + } + if ok, err := backend.UpdateLastSignInDate(ctx, userId, utils.TSToTime(ev.Timestamp)); err != nil { + m.EventsProcessed.WithLabelValues(name, metrics.ResultFailure).Inc() + logger.Error().Err(err).Str("userid", userId).Str("event", name).Msg("Error updating last sign in date") + } else if ok { + m.EventsProcessed.WithLabelValues(name, metrics.ResultSuccess).Inc() + logger.Debug().Str("userid", userId).Str("event", name).Msg("Successfully updated last sign in date") + } + } + if stop.Load() { + loop = false + } + case <-stopCh: + logger.Info().Msg("instructed to stop") + loop = false + case <-ctx.Done(): + logger.Info().Msg("context cancelled") + loop = false + } + } + return nil +} + +type GraphEventConsumer interface { + Start() error + io.Closer +} + +type GraphEventConsumerImpl struct { + ctx context.Context + consumer events.Consumer + backend identity.Backend + metrics *metrics.Metrics + logger *log.Logger + stopped atomic.Bool + stopCh chan struct{} +} + +var _ GraphEventConsumer = &GraphEventConsumerImpl{} + +func (g *GraphEventConsumerImpl) Start() error { + return processEvents(g.ctx, g.consumer, &g.stopped, g.stopCh, g.backend, g.metrics, g.logger) +} + +func (g *GraphEventConsumerImpl) Close() error { + if g.stopped.CompareAndSwap(false, true) { + close(g.stopCh) + } + return nil +} + +type NullGraphEventConsumer struct { +} + +var _ GraphEventConsumer = &NullGraphEventConsumer{} + +func (n *NullGraphEventConsumer) Start() error { + return nil +} + +func (n *NullGraphEventConsumer) Close() error { + return nil +} + +func NewService(ctx context.Context, consumer events.Consumer, backend identity.Backend, metrics *metrics.Metrics, logger *log.Logger) (GraphEventConsumer, error) { + if consumer == nil { + return &NullGraphEventConsumer{}, nil + } else { + stopCh := make(chan struct{}, 1) + return &GraphEventConsumerImpl{ + ctx: ctx, + consumer: consumer, + backend: backend, + metrics: metrics, + logger: logger, + stopCh: stopCh, + }, nil + } +} diff --git a/services/graph/pkg/service/events/service_test.go b/services/graph/pkg/service/events/service_test.go new file mode 100644 index 0000000000..c687e9fece --- /dev/null +++ b/services/graph/pkg/service/events/service_test.go @@ -0,0 +1,124 @@ +package events_test + +import ( + "context" + "errors" + "fmt" + "math/rand/v2" + "sync" + "testing" + "time" + + userv1beta1 "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" + "github.com/prometheus/client_golang/prometheus" + "github.com/test-go/testify/mock" + + "github.com/opencloud-eu/opencloud/internal/eventstest" + "github.com/opencloud-eu/opencloud/internal/metricstest" + "github.com/opencloud-eu/opencloud/pkg/log" + "github.com/opencloud-eu/opencloud/services/graph/pkg/identity/mocks" + "github.com/opencloud-eu/opencloud/services/graph/pkg/metrics" + g "github.com/opencloud-eu/opencloud/services/graph/pkg/service/events" + "github.com/opencloud-eu/reva/v2/pkg/events" + "github.com/stretchr/testify/require" +) + +func TestSuccessfulCall(t *testing.T) { + require := require.New(t) + + ctx, cancel := context.WithCancel(t.Context()) + + bus := eventstest.NewTestBus() + + var wg sync.WaitGroup + wg.Add(1) + + userId := fmt.Sprintf("user%d", 1000+rand.IntN(10000)) + + backend := mocks.NewBackend(t) + backend.EXPECT().UpdateLastSignInDate(mock.Anything, mock.Anything, mock.Anything).RunAndReturn(func(_ context.Context, _ string, _ time.Time) (bool, error) { + defer wg.Done() + return true, nil + }) + + reg := prometheus.NewRegistry() + m := metrics.New(reg, func(_ []string) (string, string) { return "", "" }) + + logger := log.NewLogger() + + svc, err := g.NewService(ctx, bus, backend, m, &logger) + require.NoError(err) + t.Cleanup(func() { svc.Close() }) + t.Cleanup(cancel) + go func() { + require.NoError(svc.Start()) + }() + + metricstest.RequireEqual(t, 0, m.UnsupportedEvents) + metricstest.RequireIsNotSet(t, m.EventsProcessed) + + _ = bus.Publish(events.UserSignedIn{ + Timestamp: nil, + Executant: &userv1beta1.UserId{ + OpaqueId: userId, + }, + }) + + wg.Wait() + require.Len(backend.Mock.Calls, 1) + require.Len(backend.Mock.Calls[0].Arguments, 3) + require.Equal(userId, backend.Mock.Calls[0].Arguments[1]) + + metricstest.RequireEqual(t, 0, m.UnsupportedEvents) + metricstest.RequireEqualWithLabels(t, 1, map[string]string{"event": "UserSignedIn", "result": "success"}, m.EventsProcessed) +} + +func TestBackendReturningAnError(t *testing.T) { + require := require.New(t) + + ctx, cancel := context.WithCancel(t.Context()) + + bus := eventstest.NewTestBus() + + var wg sync.WaitGroup + wg.Add(1) + + userId := fmt.Sprintf("user%d", 1000+rand.IntN(10000)) + + backend := mocks.NewBackend(t) + backend.EXPECT().UpdateLastSignInDate(mock.Anything, mock.Anything, mock.Anything).RunAndReturn(func(_ context.Context, _ string, _ time.Time) (bool, error) { + defer wg.Done() + return true, errors.New("test") + }) + + reg := prometheus.NewRegistry() + m := metrics.New(reg, func(_ []string) (string, string) { return "", "" }) + + logger := log.NewLogger() + + svc, err := g.NewService(ctx, bus, backend, m, &logger) + require.NoError(err) + t.Cleanup(func() { svc.Close() }) + t.Cleanup(cancel) + go func() { + require.NoError(svc.Start()) + }() + + metricstest.RequireEqual(t, 0, m.UnsupportedEvents) + metricstest.RequireIsNotSet(t, m.EventsProcessed) + + _ = bus.Publish(events.UserSignedIn{ + Timestamp: nil, + Executant: &userv1beta1.UserId{ + OpaqueId: userId, + }, + }) + + wg.Wait() + require.Len(backend.Mock.Calls, 1) + require.Len(backend.Mock.Calls[0].Arguments, 3) + require.Equal(userId, backend.Mock.Calls[0].Arguments[1]) + + metricstest.RequireEqual(t, 0, m.UnsupportedEvents) + metricstest.RequireEqualWithLabels(t, 1, map[string]string{"event": "UserSignedIn", "result": "failure"}, m.EventsProcessed) +} diff --git a/services/graph/pkg/service/v0/application_test.go b/services/graph/pkg/service/v0/application_test.go index b61714967a..60355cfbfd 100644 --- a/services/graph/pkg/service/v0/application_test.go +++ b/services/graph/pkg/service/v0/application_test.go @@ -14,6 +14,7 @@ import ( libregraph "github.com/opencloud-eu/libre-graph-api-go" "github.com/opencloud-eu/reva/v2/pkg/rgrpc/todo/pool" cs3mocks "github.com/opencloud-eu/reva/v2/tests/cs3mocks/mocks" + "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/mock" "google.golang.org/grpc" @@ -24,6 +25,7 @@ import ( "github.com/opencloud-eu/opencloud/services/graph/pkg/config" "github.com/opencloud-eu/opencloud/services/graph/pkg/config/defaults" 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" ) @@ -50,6 +52,7 @@ var _ = Describe("Applications", func() { identityBackend = &identitymocks.Backend{} roleService = &mocks.RoleService{} + metrics := metrics.New(prometheus.NewRegistry(), func([]string) (string, string) { return "", "" }) pool.RemoveSelector("GatewaySelector" + "eu.opencloud.api.gateway") gatewayClient = &cs3mocks.GatewayAPIClient{} @@ -74,6 +77,7 @@ var _ = Describe("Applications", func() { var err error svc, err = service.NewService( service.Config(cfg), + service.Metrics(metrics), service.WithGatewaySelector(gatewaySelector), service.EventsPublisher(&eventsPublisher), service.WithIdentityBackend(identityBackend), diff --git a/services/graph/pkg/service/v0/approleassignments_test.go b/services/graph/pkg/service/v0/approleassignments_test.go index 9cdf1e9925..8590acbcdd 100644 --- a/services/graph/pkg/service/v0/approleassignments_test.go +++ b/services/graph/pkg/service/v0/approleassignments_test.go @@ -18,6 +18,7 @@ import ( revactx "github.com/opencloud-eu/reva/v2/pkg/ctx" "github.com/opencloud-eu/reva/v2/pkg/rgrpc/todo/pool" cs3mocks "github.com/opencloud-eu/reva/v2/tests/cs3mocks/mocks" + "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/mock" "google.golang.org/grpc" @@ -28,6 +29,7 @@ import ( "github.com/opencloud-eu/opencloud/services/graph/pkg/config" "github.com/opencloud-eu/opencloud/services/graph/pkg/config/defaults" 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" ) @@ -60,6 +62,7 @@ var _ = Describe("AppRoleAssignments", func() { identityBackend = &identitymocks.Backend{} roleService = &mocks.RoleService{} + metrics := metrics.New(prometheus.NewRegistry(), func([]string) (string, string) { return "", "" }) pool.RemoveSelector("GatewaySelector" + "eu.opencloud.api.gateway") gatewayClient = &cs3mocks.GatewayAPIClient{} @@ -84,6 +87,7 @@ var _ = Describe("AppRoleAssignments", func() { var err error svc, err = service.NewService( service.Config(cfg), + service.Metrics(metrics), service.WithGatewaySelector(gatewaySelector), service.EventsPublisher(&eventsPublisher), service.WithIdentityBackend(identityBackend), diff --git a/services/graph/pkg/service/v0/driveitems_test.go b/services/graph/pkg/service/v0/driveitems_test.go index f36f6c06fd..4d2b90f56f 100644 --- a/services/graph/pkg/service/v0/driveitems_test.go +++ b/services/graph/pkg/service/v0/driveitems_test.go @@ -16,6 +16,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" libregraph "github.com/opencloud-eu/libre-graph-api-go" + "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/mock" "google.golang.org/grpc" @@ -30,6 +31,7 @@ import ( "github.com/opencloud-eu/opencloud/services/graph/pkg/config" "github.com/opencloud-eu/opencloud/services/graph/pkg/config/defaults" 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" ) @@ -72,6 +74,7 @@ var _ = Describe("Driveitems", func() { ) identityBackend = &identitymocks.Backend{} + metrics := metrics.New(prometheus.NewRegistry(), func([]string) (string, string) { return "", "" }) newGroup = libregraph.NewGroup() newGroup.SetMembersodataBind([]string{"/users/user1"}) newGroup.SetId("group1") @@ -88,6 +91,7 @@ var _ = Describe("Driveitems", func() { var err error svc, err = service.NewService( service.Config(cfg), + service.Metrics(metrics), service.WithGatewaySelector(gatewaySelector), service.EventsPublisher(&eventsPublisher), service.WithIdentityBackend(identityBackend), diff --git a/services/graph/pkg/service/v0/educationclasses.go b/services/graph/pkg/service/v0/educationclasses.go index cdb315a43b..dc81821453 100644 --- a/services/graph/pkg/service/v0/educationclasses.go +++ b/services/graph/pkg/service/v0/educationclasses.go @@ -8,10 +8,10 @@ import ( "strings" "github.com/CiscoM31/godata" + libregraph "github.com/opencloud-eu/libre-graph-api-go" "github.com/opencloud-eu/opencloud/services/graph/pkg/errorcode" revactx "github.com/opencloud-eu/reva/v2/pkg/ctx" "github.com/opencloud-eu/reva/v2/pkg/events" - libregraph "github.com/opencloud-eu/libre-graph-api-go" "github.com/go-chi/chi/v5" "github.com/go-chi/render" @@ -392,12 +392,21 @@ func (g Graph) DeleteEducationClassMember(w http.ResponseWriter, r *http.Request return } logger.Debug().Str("classID", classID).Str("memberID", memberID).Msg("calling delete member on backend") - err = g.identityBackend.RemoveMemberFromGroup(r.Context(), classID, memberID) - + var foundGroup, foundMember, foundMemberInGroup bool + foundGroup, foundMember, foundMemberInGroup, err = g.identityBackend.RemoveMemberFromGroup(r.Context(), classID, memberID) if err != nil { logger.Debug().Err(err).Msg("could not delete class member: backend error") errorcode.RenderError(w, r, err) return + } else if !foundGroup { + errorcode.ItemNotFound.Render(w, r, http.StatusNotFound, "failed to find class") + return + } else if !foundMember { + errorcode.ItemNotFound.Render(w, r, http.StatusNotFound, "failed to find member") + return + } else if !foundMemberInGroup { + errorcode.ItemNotFound.Render(w, r, http.StatusNotFound, "failed to find member in class") + return } /* TODO requires reva changes currentUser := revactx.ContextMustGetUser(r.Context()) diff --git a/services/graph/pkg/service/v0/educationclasses_test.go b/services/graph/pkg/service/v0/educationclasses_test.go index 5e17e62435..2a86cc3fdf 100644 --- a/services/graph/pkg/service/v0/educationclasses_test.go +++ b/services/graph/pkg/service/v0/educationclasses_test.go @@ -18,6 +18,7 @@ import ( revactx "github.com/opencloud-eu/reva/v2/pkg/ctx" "github.com/opencloud-eu/reva/v2/pkg/rgrpc/todo/pool" cs3mocks "github.com/opencloud-eu/reva/v2/tests/cs3mocks/mocks" + "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/mock" "google.golang.org/grpc" @@ -27,6 +28,7 @@ import ( "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/metrics" service "github.com/opencloud-eu/opencloud/services/graph/pkg/service/v0" ) @@ -66,6 +68,7 @@ var _ = Describe("EducationClass", func() { identityEducationBackend = &identitymocks.EducationBackend{} identityBackend = &identitymocks.Backend{} + metrics := metrics.New(prometheus.NewRegistry(), func([]string) (string, string) { return "", "" }) newClass = libregraph.NewEducationClass("math", "course") newClass.SetMembersodataBind([]string{"/users/user1"}) newClass.SetId("math") @@ -82,6 +85,7 @@ var _ = Describe("EducationClass", func() { var err error svc, err = service.NewService( service.Config(cfg), + service.Metrics(metrics), service.WithGatewaySelector(gatewaySelector), service.EventsPublisher(&eventsPublisher), service.WithIdentityBackend(identityBackend), @@ -328,10 +332,13 @@ var _ = Describe("EducationClass", func() { updatedClassJson, err := json.Marshal(updatedClass) Expect(err).ToNot(HaveOccurred()) + metrics := metrics.New(prometheus.NewRegistry(), func([]string) (string, string) { return "", "" }) + cfg.API.GroupMembersPatchLimit = 21 svc, err = service.NewService( service.Config(cfg), + service.Metrics(metrics), service.WithGatewaySelector(gatewaySelector), service.EventsPublisher(&eventsPublisher), service.WithIdentityBackend(identityBackend), @@ -527,7 +534,7 @@ var _ = Describe("EducationClass", func() { }) It("deletes members", func() { - identityBackend.On("RemoveMemberFromGroup", mock.Anything, mock.Anything, mock.Anything).Return(nil) + identityBackend.On("RemoveMemberFromGroup", mock.Anything, mock.Anything, mock.Anything).Return(true, true, true, nil) r := httptest.NewRequest(http.MethodDelete, "/graph/v1.0/education/classes/{classID}/members/{memberID}/$ref", nil) rctx := chi.NewRouteContext() diff --git a/services/graph/pkg/service/v0/educationschools.go b/services/graph/pkg/service/v0/educationschools.go index d363a43494..bf1492ecea 100644 --- a/services/graph/pkg/service/v0/educationschools.go +++ b/services/graph/pkg/service/v0/educationschools.go @@ -216,6 +216,11 @@ func (g Graph) DeleteEducationSchool(w http.ResponseWriter, r *http.Request) { errorcode.RenderError(w, r, err) return } + if school == nil { + logger.Debug().Str("school-id", schoolID).Msg("failed to find school") + errorcode.ItemNotFound.Render(w, r, http.StatusNotFound, "failed to find school") + return + } termination, ok := school.GetTerminationDateOk() if !ok { logger.Debug().Msg("cannot delete school: not termination date set") @@ -239,25 +244,39 @@ func (g Graph) DeleteEducationSchool(w http.ResponseWriter, r *http.Request) { for _, user := range users { logger.Debug().Str("schoolID", schoolID).Str("userID", *user.Id).Msg("calling delete member on backend") - if err := g.identityEducationBackend.RemoveUserFromEducationSchool(r.Context(), schoolID, *user.Id); err != nil { + if foundSchool, foundUser, foundUserInSchool, err := g.identityEducationBackend.RemoveUserFromEducationSchool(r.Context(), schoolID, *user.Id); err != nil { if errors.Is(err, identity.ErrNotFound) { logger.Debug().Str("schoolID", schoolID).Str("userID", *user.Id).Msg("user not found") continue } logger.Debug().Err(err).Msg("could not delete school member: backend error") errorcode.RenderError(w, r, err) - // TODO Do we need return right hear? + return + } else if !foundSchool { + logger.Debug().Str("schoolID", schoolID).Str("userID", *user.Id).Msg("school not found") + errorcode.ItemNotFound.Render(w, r, http.StatusNotFound, "failed to find school") + return + } else if !foundUser { + logger.Debug().Str("schoolID", schoolID).Str("userID", *user.Id).Msg("failed to find user") + continue + } else if !foundUserInSchool { + logger.Debug().Str("schoolID", schoolID).Str("userID", *user.Id).Msg("failed to find user in school") + continue } } logger.Debug().Str("id", schoolID).Msg("calling delete school on backend") - err = g.identityEducationBackend.DeleteEducationSchool(r.Context(), schoolID) - + found, err := g.identityEducationBackend.DeleteEducationSchool(r.Context(), schoolID) if err != nil { - logger.Debug().Err(err).Msg("could not delete school: backend error") + logger.Debug().Err(err).Str("school-id", schoolID).Msg("could not delete school: backend error") errorcode.RenderError(w, r, err) return } + if !found { + logger.Debug().Str("school-id", schoolID).Msg("could not delete school: failed to find school") + errorcode.ItemNotFound.Render(w, r, http.StatusNotFound, "failed to find school when attempting to delete") + return + } /* TODO requires reva changes e := events.SchoolDeleted{SchoolID: schoolID} @@ -353,13 +372,17 @@ func (g Graph) PostEducationSchoolUser(w http.ResponseWriter, r *http.Request) { } logger.Debug().Str("memberType", memberType).Str("id", id).Msg("calling add user on backend") - err = g.identityEducationBackend.AddUsersToEducationSchool(r.Context(), schoolID, []string{id}) - + ok, err = g.identityEducationBackend.AddUsersToEducationSchool(r.Context(), schoolID, []string{id}) if err != nil { - logger.Debug().Err(err).Msg("could not add school user: backend error") + logger.Debug().Err(err).Str("school-id", schoolID).Msg("could not add school user: backend error") errorcode.RenderError(w, r, err) return } + if !ok { + logger.Debug().Str("school-id", schoolID).Msg("could not add school user: failed to find school") + errorcode.ItemNotFound.Render(w, r, http.StatusNotFound, "failed to find school") + return + } /* TODO requires reva changes e := events.SchoolMemberAdded{SchoolID: schoolID, UserID: id} @@ -406,13 +429,27 @@ func (g Graph) DeleteEducationSchoolUser(w http.ResponseWriter, r *http.Request) return } logger.Debug().Str("schoolID", schoolID).Str("userID", userID).Msg("calling delete member on backend") - err = g.identityEducationBackend.RemoveUserFromEducationSchool(r.Context(), schoolID, userID) - + foundSchool, foundUser, foundUserInSchool, err := g.identityEducationBackend.RemoveUserFromEducationSchool(r.Context(), schoolID, userID) if err != nil { logger.Debug().Err(err).Msg("could not delete school member: backend error") errorcode.RenderError(w, r, err) return } + if !foundSchool { + logger.Debug().Str("school-id", schoolID).Msg("could not delete school member: failed to find school") + errorcode.ItemNotFound.Render(w, r, http.StatusNotFound, "failed to find school") + return + } + if !foundUser { + logger.Debug().Str("school-id", schoolID).Str("user-id", userID).Msg("could not delete school member: failed to find user") + errorcode.ItemNotFound.Render(w, r, http.StatusNotFound, "failed to find user") + return + } + if !foundUserInSchool { + logger.Debug().Str("school-id", schoolID).Str("user-id", userID).Msg("could not delete school member: failed to find user in school") + errorcode.ItemNotFound.Render(w, r, http.StatusNotFound, "failed to find user in school") + return + } /* TODO requires reva changes e := events.SchoolMemberRemoved{SchoolID: schoolID, UserID: userID} diff --git a/services/graph/pkg/service/v0/educationschools_test.go b/services/graph/pkg/service/v0/educationschools_test.go index c2886bc732..325a7add4e 100644 --- a/services/graph/pkg/service/v0/educationschools_test.go +++ b/services/graph/pkg/service/v0/educationschools_test.go @@ -19,6 +19,7 @@ import ( ctxpkg "github.com/opencloud-eu/reva/v2/pkg/ctx" "github.com/opencloud-eu/reva/v2/pkg/rgrpc/todo/pool" cs3mocks "github.com/opencloud-eu/reva/v2/tests/cs3mocks/mocks" + "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/mock" "google.golang.org/grpc" @@ -29,6 +30,7 @@ import ( "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/metrics" service "github.com/opencloud-eu/opencloud/services/graph/pkg/service/v0" ) @@ -67,6 +69,7 @@ var _ = Describe("Schools", func() { ) identityEducationBackend = &identitymocks.EducationBackend{} + metrics := metrics.New(prometheus.NewRegistry(), func([]string) (string, string) { return "", "" }) newSchool = libregraph.NewEducationSchool() newSchool.SetId("school1") @@ -83,6 +86,7 @@ var _ = Describe("Schools", func() { var err error svc, err = service.NewService( service.Config(cfg), + service.Metrics(metrics), service.WithGatewaySelector(gatewaySelector), service.WithIdentityEducationBackend(identityEducationBackend), ) @@ -351,7 +355,7 @@ var _ = Describe("Schools", func() { DescribeTable("checks terminnation date", func(schoolId string, statusCode int) { - identityEducationBackend.On("DeleteEducationSchool", mock.Anything, mock.Anything, mock.Anything).Return(nil) + identityEducationBackend.On("DeleteEducationSchool", mock.Anything, mock.Anything, mock.Anything).Return(true, nil) identityEducationBackend.On("GetEducationSchoolUsers", mock.Anything, mock.Anything, mock.Anything).Return([]*libregraph.EducationUser{}, nil) r := httptest.NewRequest(http.MethodDelete, "/graph/v1.0/education/schools", nil) rctx := chi.NewRouteContext() @@ -377,9 +381,9 @@ var _ = Describe("Schools", func() { user2 := libregraph.NewEducationUser() user2.SetId("user2") identityEducationBackend.On("GetEducationSchoolUsers", mock.Anything, mock.Anything, mock.Anything).Return([]*libregraph.EducationUser{user1, user2}, nil) - identityEducationBackend.On("DeleteEducationSchool", mock.Anything, mock.Anything, mock.Anything).Return(nil) - identityEducationBackend.On("RemoveUserFromEducationSchool", mock.Anything, mock.Anything, *user1.Id).Return(nil) - identityEducationBackend.On("RemoveUserFromEducationSchool", mock.Anything, mock.Anything, *user2.Id).Return(nil) + identityEducationBackend.On("DeleteEducationSchool", mock.Anything, mock.Anything, mock.Anything).Return(true, nil) + identityEducationBackend.On("RemoveUserFromEducationSchool", mock.Anything, mock.Anything, *user1.Id).Return(true, true, true, nil) + identityEducationBackend.On("RemoveUserFromEducationSchool", mock.Anything, mock.Anything, *user2.Id).Return(true, true, true, nil) r := httptest.NewRequest(http.MethodDelete, "/graph/v1.0/education/schools", nil) rctx := chi.NewRouteContext() @@ -465,7 +469,7 @@ var _ = Describe("Schools", func() { member.SetOdataId("/users/user") data, err := json.Marshal(member) Expect(err).ToNot(HaveOccurred()) - identityEducationBackend.On("AddUsersToEducationSchool", mock.Anything, mock.Anything, mock.Anything).Return(nil) + identityEducationBackend.On("AddUsersToEducationSchool", mock.Anything, mock.Anything, mock.Anything).Return(true, nil) r := httptest.NewRequest(http.MethodPost, "/graph/v1.0/education/schools/{schoolID}/members", bytes.NewBuffer(data)) rctx := chi.NewRouteContext() @@ -497,7 +501,7 @@ var _ = Describe("Schools", func() { }) It("deletes members", func() { - identityEducationBackend.On("RemoveUserFromEducationSchool", mock.Anything, mock.Anything, mock.Anything).Return(nil) + identityEducationBackend.On("RemoveUserFromEducationSchool", mock.Anything, mock.Anything, mock.Anything).Return(true, true, true, nil) r := httptest.NewRequest(http.MethodDelete, "/graph/v1.0/education/schools/{schoolID}/members/{userID}/$ref", nil) rctx := chi.NewRouteContext() diff --git a/services/graph/pkg/service/v0/educationuser_test.go b/services/graph/pkg/service/v0/educationuser_test.go index 077f270bb1..bf8aecfbca 100644 --- a/services/graph/pkg/service/v0/educationuser_test.go +++ b/services/graph/pkg/service/v0/educationuser_test.go @@ -21,6 +21,7 @@ import ( "github.com/opencloud-eu/reva/v2/pkg/rgrpc/status" "github.com/opencloud-eu/reva/v2/pkg/rgrpc/todo/pool" cs3mocks "github.com/opencloud-eu/reva/v2/tests/cs3mocks/mocks" + "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/mock" "google.golang.org/grpc" @@ -30,6 +31,7 @@ import ( "github.com/opencloud-eu/opencloud/services/graph/pkg/config" "github.com/opencloud-eu/opencloud/services/graph/pkg/config/defaults" 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" ) @@ -71,6 +73,7 @@ var _ = Describe("EducationUsers", func() { ) identityEducationBackend = &identitymocks.EducationBackend{} + metrics := metrics.New(prometheus.NewRegistry(), func([]string) (string, string) { return "", "" }) roleService = &mocks.RoleService{} rr = httptest.NewRecorder() @@ -85,6 +88,7 @@ var _ = Describe("EducationUsers", func() { var err error svc, err = service.NewService( service.Config(cfg), + service.Metrics(metrics), service.WithGatewaySelector(gatewaySelector), service.EventsPublisher(&eventsPublisher), service.WithIdentityEducationBackend(identityEducationBackend), diff --git a/services/graph/pkg/service/v0/graph.go b/services/graph/pkg/service/v0/graph.go index c6ef4fa74c..913d2ab087 100644 --- a/services/graph/pkg/service/v0/graph.go +++ b/services/graph/pkg/service/v0/graph.go @@ -26,6 +26,7 @@ import ( settingssvc "github.com/opencloud-eu/opencloud/protogen/gen/opencloud/services/settings/v0" "github.com/opencloud-eu/opencloud/services/graph/pkg/errorcode" "github.com/opencloud-eu/opencloud/services/graph/pkg/identity" + "github.com/opencloud-eu/opencloud/services/graph/pkg/metrics" ) // Permissions is the interface used to access the permissions service @@ -63,14 +64,16 @@ type Graph struct { valueService settingssvc.ValueService specialDriveItemsCache *ttlcache.Cache[string, any] eventsPublisher events.Publisher - eventsConsumer events.Consumer searchService searchsvc.SearchProviderService keycloakClient keycloak.Client historyClient ehsvc.EventHistoryService + metrics *metrics.Metrics traceProvider trace.TracerProvider natskv jetstream.KeyValue } +var _ Service = Graph{} // ensure that the Graph struct implements all of the Service interface + // ServeHTTP implements the Service interface. func (g Graph) ServeHTTP(w http.ResponseWriter, r *http.Request) { // There was a number of issues with the chi router and parameters with diff --git a/services/graph/pkg/service/v0/graph_test.go b/services/graph/pkg/service/v0/graph_test.go index eb09d89d1e..a6da83d21e 100644 --- a/services/graph/pkg/service/v0/graph_test.go +++ b/services/graph/pkg/service/v0/graph_test.go @@ -25,6 +25,7 @@ import ( "github.com/opencloud-eu/reva/v2/pkg/utils" cs3mocks "github.com/opencloud-eu/reva/v2/tests/cs3mocks/mocks" "github.com/pkg/errors" + "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/mock" "github.com/tidwall/gjson" "google.golang.org/grpc" @@ -36,6 +37,7 @@ 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/metrics" service "github.com/opencloud-eu/opencloud/services/graph/pkg/service/v0" "github.com/opencloud-eu/opencloud/services/graph/pkg/unifiedrole" ) @@ -61,6 +63,8 @@ var _ = Describe("Graph", func() { BeforeEach(func() { rr = httptest.NewRecorder() + metrics := metrics.New(prometheus.NewRegistry(), func([]string) (string, string) { return "", "" }) + ctx = revactx.ContextSetUser(context.Background(), &userprovider.User{Id: &userprovider.UserId{Type: userprovider.UserType_USER_TYPE_PRIMARY, OpaqueId: "testuser"}, Username: "testuser"}) cfg = defaults.FullDefaultConfig() cfg.Identity.LDAP.CACert = "" // skip the startup checks, we don't use LDAP at all in this tests @@ -84,6 +88,7 @@ var _ = Describe("Graph", func() { var err error svc, err = service.NewService( service.Config(cfg), + service.Metrics(metrics), service.WithGatewaySelector(gatewaySelector), service.EventsPublisher(&eventsPublisher), service.PermissionService(&permissionService), diff --git a/services/graph/pkg/service/v0/groups.go b/services/graph/pkg/service/v0/groups.go index d56b1b4590..3b41051e02 100644 --- a/services/graph/pkg/service/v0/groups.go +++ b/services/graph/pkg/service/v0/groups.go @@ -163,7 +163,7 @@ func (g Graph) PatchGroup(w http.ResponseWriter, r *http.Request) { } if reflect.ValueOf(*changes).IsZero() { - logger.Debug().Interface("body", r.Body).Msg("ignoring empyt request body") + logger.Debug().Interface("body", r.Body).Msg("ignoring empty request body") render.Status(r, http.StatusNoContent) render.NoContent(w, r) return @@ -176,10 +176,15 @@ func (g Graph) PatchGroup(w http.ResponseWriter, r *http.Request) { errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "Invalid displayName") return } - if err = g.identityBackend.UpdateGroupName(r.Context(), groupID, displayName); err != nil { + var ok bool + if ok, err = g.identityBackend.UpdateGroupName(r.Context(), groupID, displayName); err != nil { logger.Debug().Err(err).Msg("could not update group displayName") errorcode.RenderError(w, r, err) return + } else if !ok { + // failed to find the group to update, for backwards compatibility, this is treated as an error + errorcode.ItemNotFound.Render(w, r, http.StatusNotFound, "failed to find group") + return } } @@ -278,12 +283,16 @@ func (g Graph) DeleteGroup(w http.ResponseWriter, r *http.Request) { } logger.Debug().Str("id", groupID).Msg("calling delete group on backend") - err = g.identityBackend.DeleteGroup(r.Context(), groupID) - + var ok bool + ok, err = g.identityBackend.DeleteGroup(r.Context(), groupID) if err != nil { logger.Debug().Err(err).Msg("could not delete group: backend error") errorcode.RenderError(w, r, err) return + } else if !ok { + // failed to find the group to delete: we treat this as an error + errorcode.ItemNotFound.Render(w, r, http.StatusNotFound, "failed to find group") + return } e := events.GroupDeleted{ @@ -439,14 +448,25 @@ func (g Graph) DeleteGroupMember(w http.ResponseWriter, r *http.Request) { errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "missing member id") return } - logger.Debug().Str("groupID", groupID).Str("memberID", memberID).Msg("calling delete member on backend") - err = g.identityBackend.RemoveMemberFromGroup(r.Context(), groupID, memberID) + logger.Debug().Str("groupID", groupID).Str("memberID", memberID).Msg("calling delete member on backend") + var foundGroup, foundMember, foundMemberInGroup bool + foundGroup, foundMember, foundMemberInGroup, err = g.identityBackend.RemoveMemberFromGroup(r.Context(), groupID, memberID) if err != nil { logger.Debug().Err(err).Msg("could not delete group member: backend error") errorcode.RenderError(w, r, err) return + } else if !foundGroup { + errorcode.ItemNotFound.Render(w, r, http.StatusNotFound, "failed to find group") + return + } else if !foundMember { + errorcode.ItemNotFound.Render(w, r, http.StatusNotFound, "failed to find member") + return + } else if !foundMemberInGroup { + errorcode.ItemNotFound.Render(w, r, http.StatusNotFound, "failed to find member in group") + return } + e := events.GroupMemberRemoved{ GroupID: groupID, UserID: memberID, diff --git a/services/graph/pkg/service/v0/groups_test.go b/services/graph/pkg/service/v0/groups_test.go index 4b61e5e8c2..a8b2c901e0 100644 --- a/services/graph/pkg/service/v0/groups_test.go +++ b/services/graph/pkg/service/v0/groups_test.go @@ -18,6 +18,7 @@ import ( revactx "github.com/opencloud-eu/reva/v2/pkg/ctx" "github.com/opencloud-eu/reva/v2/pkg/rgrpc/todo/pool" cs3mocks "github.com/opencloud-eu/reva/v2/tests/cs3mocks/mocks" + "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/mock" "google.golang.org/grpc" @@ -29,6 +30,7 @@ import ( "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/metrics" service "github.com/opencloud-eu/opencloud/services/graph/pkg/service/v0" ) @@ -72,6 +74,7 @@ var _ = Describe("Groups", func() { permissionService = &mocks.Permissions{} identityBackend = &identitymocks.Backend{} + metrics := metrics.New(prometheus.NewRegistry(), func([]string) (string, string) { return "", "" }) newGroup = libregraph.NewGroup() newGroup.SetMembersodataBind([]string{"/users/user1"}) newGroup.SetId("group1") @@ -88,6 +91,7 @@ var _ = Describe("Groups", func() { var err error svc, err = service.NewService( service.Config(cfg), + service.Metrics(metrics), service.WithGatewaySelector(gatewaySelector), service.EventsPublisher(&eventsPublisher), service.WithIdentityBackend(identityBackend), @@ -413,9 +417,12 @@ var _ = Describe("Groups", func() { updatedGroupJson, err := json.Marshal(updatedGroup) Expect(err).ToNot(HaveOccurred()) + metrics := metrics.New(prometheus.NewRegistry(), func([]string) (string, string) { return "", "" }) + cfg.API.GroupMembersPatchLimit = 21 svc, err = service.NewService( service.Config(cfg), + service.Metrics(metrics), service.WithGatewaySelector(gatewaySelector), service.EventsPublisher(&eventsPublisher), service.WithIdentityBackend(identityBackend), @@ -484,7 +491,7 @@ var _ = Describe("Groups", func() { }) It("updates the group name", func() { - identityBackend.On("UpdateGroupName", mock.Anything, mock.Anything, mock.Anything).Return(nil) + identityBackend.On("UpdateGroupName", mock.Anything, mock.Anything, mock.Anything).Return(true, nil) updatedGroup := libregraph.NewGroup() updatedGroup.SetDisplayName("group1 updated") @@ -512,7 +519,7 @@ var _ = Describe("Groups", func() { }) It("deletes the group", func() { - identityBackend.On("DeleteGroup", mock.Anything, mock.Anything, mock.Anything).Return(nil) + identityBackend.On("DeleteGroup", mock.Anything, mock.Anything, mock.Anything).Return(true, nil) r := httptest.NewRequest(http.MethodPatch, "/graph/v1.0/groups", nil) rctx := chi.NewRouteContext() rctx.URLParams.Add("groupID", *newGroup.Id) @@ -624,7 +631,7 @@ var _ = Describe("Groups", func() { }) It("deletes members", func() { - identityBackend.On("RemoveMemberFromGroup", mock.Anything, mock.Anything, mock.Anything).Return(nil) + identityBackend.On("RemoveMemberFromGroup", mock.Anything, mock.Anything, mock.Anything).Return(true, true, true, nil) r := httptest.NewRequest(http.MethodDelete, "/graph/v1.0/groups/{groupID}/members/{memberID}/$ref", nil) rctx := chi.NewRouteContext() diff --git a/services/graph/pkg/service/v0/option.go b/services/graph/pkg/service/v0/option.go index 5330fd5783..774fb176d7 100644 --- a/services/graph/pkg/service/v0/option.go +++ b/services/graph/pkg/service/v0/option.go @@ -18,6 +18,7 @@ import ( settingssvc "github.com/opencloud-eu/opencloud/protogen/gen/opencloud/services/settings/v0" "github.com/opencloud-eu/opencloud/services/graph/pkg/config" "github.com/opencloud-eu/opencloud/services/graph/pkg/identity" + "github.com/opencloud-eu/opencloud/services/graph/pkg/metrics" ) // Option defines a single option function. @@ -28,6 +29,7 @@ type Options struct { Context context.Context Logger log.Logger Config *config.Config + Metrics *metrics.Metrics Middleware []func(http.Handler) http.Handler RequireAdminMiddleware func(http.Handler) http.Handler GatewaySelector pool.Selectable[gateway.GatewayAPIClient] @@ -39,7 +41,6 @@ type Options struct { ValueService settingssvc.ValueService RoleManager *roles.Manager EventsPublisher events.Publisher - EventsConsumer events.Consumer SearchService searchsvc.SearchProviderService KeycloakClient keycloak.Client EventHistoryClient ehsvc.EventHistoryService @@ -79,6 +80,13 @@ func Config(val *config.Config) Option { } } +// Context provides a function to set the context option. +func Metrics(m *metrics.Metrics) Option { + return func(o *Options) { + o.Metrics = m + } +} + // Middleware provides a function to set the middleware option. func Middleware(val ...func(http.Handler) http.Handler) Option { return func(o *Options) { @@ -163,13 +171,6 @@ func EventsPublisher(val events.Publisher) Option { } } -// EventsConsumer provides a function to set the EventsConsumer option. -func EventsConsumer(val events.Consumer) Option { - return func(o *Options) { - o.EventsConsumer = val - } -} - // KeycloakClient provides a function to set the KeycloakCient option. func KeycloakClient(val keycloak.Client) Option { return func(o *Options) { diff --git a/services/graph/pkg/service/v0/password.go b/services/graph/pkg/service/v0/password.go index 96b98721a9..2cf8b96a60 100644 --- a/services/graph/pkg/service/v0/password.go +++ b/services/graph/pkg/service/v0/password.go @@ -88,12 +88,17 @@ func (g Graph) ChangeOwnPassword(w http.ResponseWriter, r *http.Request) { newPwProfile.SetPassword(newPw) changes := libregraph.NewUserUpdate() changes.SetPasswordProfile(*newPwProfile) - _, err = g.identityBackend.UpdateUser(ctx, u.Id.OpaqueId, *changes) + found, err := g.identityBackend.UpdateUser(ctx, u.Id.OpaqueId, *changes) if err != nil { errorcode.InvalidRequest.Render(w, r, http.StatusInternalServerError, "password change failed") g.logger.Debug().Err(err).Str("userid", u.Id.OpaqueId).Msg("failed to update user password") return } + if found == nil { + errorcode.ItemNotFound.Render(w, r, http.StatusNotFound, "password change failed") + g.logger.Debug().Err(err).Str("userid", u.Id.OpaqueId).Msg("failed to update user password: user not found in backend") + return + } currentUser := revactx.ContextMustGetUser(r.Context()) g.publishEvent( @@ -107,6 +112,8 @@ func (g Graph) ChangeOwnPassword(w http.ResponseWriter, r *http.Request) { }, ) + g.metrics.UserPasswordChanges.Inc() + render.Status(r, http.StatusNoContent) render.NoContent(w, r) } diff --git a/services/graph/pkg/service/v0/password_test.go b/services/graph/pkg/service/v0/password_test.go index f8464cabb7..8505630bb4 100644 --- a/services/graph/pkg/service/v0/password_test.go +++ b/services/graph/pkg/service/v0/password_test.go @@ -18,6 +18,7 @@ import ( "github.com/opencloud-eu/reva/v2/pkg/rgrpc/status" "github.com/opencloud-eu/reva/v2/pkg/rgrpc/todo/pool" cs3mocks "github.com/opencloud-eu/reva/v2/tests/cs3mocks/mocks" + "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/mock" "google.golang.org/grpc" @@ -28,6 +29,7 @@ import ( "github.com/opencloud-eu/opencloud/services/graph/pkg/config/defaults" "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" ) @@ -76,14 +78,17 @@ var _ = Describe("Users changing their own password", func() { GroupSearchScope: "sub", } logger := log.NewLogger() - identityBackend, err = identity.NewLDAPBackend(ldapClient, ldapConfig, &logger) + reg := prometheus.NewRegistry() + identityBackend, err = identity.NewLDAPBackend(ldapClient, ldapConfig, &logger, "opencloud", "test", reg) Expect(err).To(BeNil()) + metrics := metrics.New(reg, func([]string) (string, string) { return "", "" }) eventsPublisher = mocks.Publisher{} var err error svc, err = service.NewService( service.Config(cfg), + service.Metrics(metrics), service.WithGatewaySelector(gatewaySelector), service.WithIdentityBackend(identityBackend), service.EventsPublisher(&eventsPublisher), @@ -145,9 +150,10 @@ func mockedLDAPClient() *identitymocks.Client { lm := &identitymocks.Client{} userEntry := ldap.NewEntry("uid=test", map[string][]string{ - "uid": {"test"}, - "displayName": {"test"}, - "mail": {"test@example.org"}, + "openCloudUUID": {"test"}, + "uid": {"test"}, + "displayName": {"test"}, + "mail": {"test@example.org"}, }) lm.On("Search", mock.Anything, mock.Anything, mock.Anything, mock.Anything, diff --git a/services/graph/pkg/service/v0/rolemanagement_test.go b/services/graph/pkg/service/v0/rolemanagement_test.go index fcb77f7c6b..a75a63b79e 100644 --- a/services/graph/pkg/service/v0/rolemanagement_test.go +++ b/services/graph/pkg/service/v0/rolemanagement_test.go @@ -12,12 +12,14 @@ import ( libregraph "github.com/opencloud-eu/libre-graph-api-go" "github.com/opencloud-eu/reva/v2/pkg/rgrpc/todo/pool" cs3mocks "github.com/opencloud-eu/reva/v2/tests/cs3mocks/mocks" + "github.com/prometheus/client_golang/prometheus" "google.golang.org/grpc" "github.com/opencloud-eu/opencloud/pkg/shared" "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/metrics" service "github.com/opencloud-eu/opencloud/services/graph/pkg/service/v0" "github.com/opencloud-eu/opencloud/services/graph/pkg/unifiedrole" ) @@ -55,10 +57,12 @@ var _ = Describe("RoleManagement", func() { ) eventsPublisher = mocks.Publisher{} permSvc = mocks.Permissions{} + metrics := metrics.New(prometheus.NewRegistry(), func([]string) (string, string) { return "", "" }) var err error svc, err = service.NewService( service.Config(cfg), + service.Metrics(metrics), service.WithGatewaySelector(gatewaySelector), service.EventsPublisher(&eventsPublisher), service.PermissionService(&permSvc), diff --git a/services/graph/pkg/service/v0/service.go b/services/graph/pkg/service/v0/service.go index fb5e490851..1442c084fe 100644 --- a/services/graph/pkg/service/v0/service.go +++ b/services/graph/pkg/service/v0/service.go @@ -1,39 +1,25 @@ package svc import ( - "context" - "crypto/tls" - "crypto/x509" - "errors" "fmt" "net/http" "net/url" - "os" "strconv" "time" "github.com/go-chi/chi/v5" "github.com/go-chi/chi/v5/middleware" - ldapv3 "github.com/go-ldap/ldap/v3" "github.com/jellydator/ttlcache/v3" "github.com/opencloud-eu/opencloud/services/graph/pkg/identity/cache" "github.com/riandyrn/otelchi" microstore "go-micro.dev/v4/store" - "github.com/opencloud-eu/reva/v2/pkg/events" - "github.com/opencloud-eu/reva/v2/pkg/rgrpc/todo/pool" "github.com/opencloud-eu/reva/v2/pkg/store" - "github.com/opencloud-eu/reva/v2/pkg/utils" - "github.com/opencloud-eu/reva/v2/pkg/utils/ldap" - ocldap "github.com/opencloud-eu/opencloud/pkg/ldap" - "github.com/opencloud-eu/opencloud/pkg/log" - "github.com/opencloud-eu/opencloud/pkg/registry" "github.com/opencloud-eu/opencloud/pkg/roles" "github.com/opencloud-eu/opencloud/pkg/service/grpc" "github.com/opencloud-eu/opencloud/pkg/tracing" settingssvc "github.com/opencloud-eu/opencloud/protogen/gen/opencloud/services/settings/v0" - "github.com/opencloud-eu/opencloud/services/graph/pkg/identity" graphm "github.com/opencloud-eu/opencloud/services/graph/pkg/middleware" "github.com/opencloud-eu/opencloud/services/graph/pkg/unifiedrole" ) @@ -199,20 +185,17 @@ func NewService(opts ...Option) (Graph, error) { //nolint:maintidx mux: m, specialDriveItemsCache: spacePropertiesCache, eventsPublisher: options.EventsPublisher, - eventsConsumer: options.EventsConsumer, searchService: options.SearchService, + identityBackend: options.IdentityBackend, identityEducationBackend: options.IdentityEducationBackend, keycloakClient: options.KeycloakClient, historyClient: options.EventHistoryClient, + metrics: options.Metrics, traceProvider: options.TraceProvider, valueService: options.ValueService, natskv: options.NatsKeyValue, } - if err := setIdentityBackends(options, &svc); err != nil { - return svc, err - } - if options.PermissionService == nil { grpcClient, err := grpc.NewClient(append(grpc.GetClientOptions(options.Config.GRPCClientTLS), grpc.WithTraceProvider(options.TraceProvider))...) if err != nil { @@ -449,162 +432,31 @@ func NewService(opts ...Option) (Graph, error) { //nolint:maintidx return svc, nil } -func setIdentityBackends(options Options, svc *Graph) error { - if options.IdentityBackend == nil { - switch options.Config.Identity.Backend { - case "cs3": - gatewaySelector, err := pool.GatewaySelector( - options.Config.Reva.Address, - append( - options.Config.Reva.GetRevaOptions(), - pool.WithRegistry(registry.GetRegistry()), - pool.WithTracerProvider(options.TraceProvider), - )..., - ) - if err != nil { - return err - } - - svc.identityBackend = &identity.CS3{ - Config: options.Config.Reva, - Logger: &options.Logger, - GatewaySelector: gatewaySelector, - } - case "ldap": - var err error - - var tlsConf *tls.Config - if options.Config.Identity.LDAP.Insecure { - - // When insecure is set to true then we don't need a certificate. - options.Config.Identity.LDAP.CACert = "" - tlsConf = &tls.Config{ - MinVersion: tls.VersionTLS12, - - //nolint:gosec // We need the ability to run with "insecure" (dev/testing) - InsecureSkipVerify: options.Config.Identity.LDAP.Insecure, - } - } - - if options.Config.Identity.LDAP.CACert != "" { - if err := ocldap.WaitForCA(options.Logger, - options.Config.Identity.LDAP.Insecure, - options.Config.Identity.LDAP.CACert); err != nil { - options.Logger.Fatal().Err(err).Msg("The configured LDAP CA cert does not exist") - } - if tlsConf == nil { - tlsConf = &tls.Config{ - MinVersion: tls.VersionTLS12, - } - } - certs := x509.NewCertPool() - pemData, err := os.ReadFile(options.Config.Identity.LDAP.CACert) - if err != nil { - options.Logger.Error().Err(err).Msg("Error initializing LDAP Backend") - return err - } - if !certs.AppendCertsFromPEM(pemData) { - options.Logger.Error().Msg("Error initializing LDAP Backend. Adding CA cert failed") - return err - } - tlsConf.RootCAs = certs - } - - conn := ldap.NewLDAPWithReconnect( - ldap.Config{ - URI: options.Config.Identity.LDAP.URI, - BindDN: options.Config.Identity.LDAP.BindDN, - BindPassword: options.Config.Identity.LDAP.BindPassword, - TLSConfig: tlsConf, - }, - ) - conn.SetLogger(&options.Logger.Logger) - lb, err := identity.NewLDAPBackend(conn, options.Config.Identity.LDAP, &options.Logger) - if err != nil { - options.Logger.Error().Err(err).Msg("Error initializing LDAP Backend") - return err - } - svc.identityBackend = lb - if options.IdentityEducationBackend == nil { - if options.Config.Identity.LDAP.EducationResourcesEnabled { - svc.identityEducationBackend = lb - } else { - errEduBackend := &identity.ErrEducationBackend{} - svc.identityEducationBackend = errEduBackend - } - } - - disableMechanismType, err := identity.ParseDisableMechanismType(options.Config.Identity.LDAP.DisableUserMechanism) - if err != nil { - options.Logger.Error().Err(err).Msg("Error initializing LDAP Backend") - return err - } - - if disableMechanismType == identity.DisableMechanismGroup { - options.Logger.Info().Msg("LocalUserDisable is true, will create group if not exists") - err := lb.CreateLDAPGroupByDN(options.Config.Identity.LDAP.LdapDisabledUsersGroupDN) - if err != nil { - isAnError := false - var lerr *ldapv3.Error - if errors.As(err, &lerr) { - if lerr.ResultCode != ldapv3.LDAPResultEntryAlreadyExists { - isAnError = true - } - } else { - isAnError = true - } - - if isAnError { - msg := "error adding group for disabling users" - options.Logger.Error().Err(err).Str("local_user_disable", options.Config.Identity.LDAP.LdapDisabledUsersGroupDN).Msg(msg) - return err - } - } - } - - default: - err := fmt.Errorf("unknown identity backend: '%s'", options.Config.Identity.Backend) - options.Logger.Err(err) - return err +// this function receives the request URI path chi pattern, split cleanly on '/' +// and is tasked with returning a value for the Graph API version, +// as well as a value for the Graph API resource +// +// e.g. for +// +// '/graph/v1.0/users/{userid}' +// -> receive ['graph', 'v1.0', 'users', '{userid}'] +// <- return ('v1.0', 'users') +func DecomposeGraphApiRequestPattern(pieces []string) (string, string) { + // we keep this function close to the chi routes to improve our changes of + // changing this implementation whenever we change the routes + version := "" + resource := "" + if len(pieces) >= 2 { + // first path element is the /graph prefix, ignore that + // followed by the version (v1.0) + version = pieces[1] + if len(pieces) >= 3 { + // and the resource + resource = pieces[2] } - } else { - svc.identityBackend = options.IdentityBackend } + return version, resource - return svc.StartListenForLogonEvents(options.Context, options.Logger) -} - -func (g *Graph) StartListenForLogonEvents(ctx context.Context, l log.Logger) error { - if g.eventsConsumer == nil { - return nil - } - var _registeredEvents = []events.Unmarshaller{ - events.UserSignedIn{}, - } - evChannel, err := events.Consume(g.eventsConsumer, "graph", _registeredEvents...) - if err != nil { - l.Error().Err(err).Msg("cannot consume from nats") - return err - } - go func() { - for loop := true; loop; { - select { - case e := <-evChannel: - switch ev := e.Event.(type) { - default: - l.Error().Interface("event", e).Msg("unhandled event") - case events.UserSignedIn: - if err := g.identityBackend.UpdateLastSignInDate(ctx, ev.Executant.OpaqueId, utils.TSToTime(ev.Timestamp)); err != nil { - l.Error().Err(err).Str("userid", ev.Executant.OpaqueId).Msg("Error updating last sign in date") - } - } - case <-ctx.Done(): - l.Info().Msg("context cancelled") - loop = false - } - } - }() - return nil } // parseHeaderPurge parses the 'Purge' header. diff --git a/services/graph/pkg/service/v0/sharedbyme_test.go b/services/graph/pkg/service/v0/sharedbyme_test.go index 80ef743538..5496675337 100644 --- a/services/graph/pkg/service/v0/sharedbyme_test.go +++ b/services/graph/pkg/service/v0/sharedbyme_test.go @@ -24,6 +24,7 @@ import ( "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/stretchr/testify/mock" "google.golang.org/grpc" @@ -33,6 +34,7 @@ import ( "github.com/opencloud-eu/opencloud/services/graph/pkg/config/defaults" 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" service "github.com/opencloud-eu/opencloud/services/graph/pkg/service/v0" "github.com/opencloud-eu/opencloud/services/graph/pkg/unifiedrole" ) @@ -236,6 +238,7 @@ var _ = Describe("sharedbyme", func() { ) identityBackend = &identitymocks.Backend{} + metrics := metrics.New(prometheus.NewRegistry(), func([]string) (string, string) { return "", "" }) rr = httptest.NewRecorder() ctx = context.Background() @@ -248,6 +251,7 @@ var _ = Describe("sharedbyme", func() { svc, err = service.NewService( service.Config(cfg), + service.Metrics(metrics), service.WithGatewaySelector(gatewaySelector), service.EventsPublisher(&eventsPublisher), service.WithIdentityBackend(identityBackend), diff --git a/services/graph/pkg/service/v0/sharedwithme_test.go b/services/graph/pkg/service/v0/sharedwithme_test.go index 7d0200e776..07153a2962 100644 --- a/services/graph/pkg/service/v0/sharedwithme_test.go +++ b/services/graph/pkg/service/v0/sharedwithme_test.go @@ -21,6 +21,7 @@ import ( "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/stretchr/testify/mock" "github.com/tidwall/gjson" "google.golang.org/grpc" @@ -33,6 +34,7 @@ import ( "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/metrics" service "github.com/opencloud-eu/opencloud/services/graph/pkg/service/v0" // "github.com/opencloud-eu/opencloud/services/graph/pkg/unifiedrole" ) @@ -60,6 +62,7 @@ var _ = Describe("SharedWithMe", func() { ) identityBackend = &identitymocks.Backend{} + metrics := metrics.New(prometheus.NewRegistry(), func([]string) (string, string) { return "", "" }) tape = httptest.NewRecorder() ctx = context.Background() @@ -73,6 +76,7 @@ var _ = Describe("SharedWithMe", func() { var err error svc, err = service.NewService( service.Config(cfg), + service.Metrics(metrics), service.WithGatewaySelector(gatewaySelector), service.WithIdentityBackend(identityBackend), ) diff --git a/services/graph/pkg/service/v0/users.go b/services/graph/pkg/service/v0/users.go index 18b154546b..a840a18c62 100644 --- a/services/graph/pkg/service/v0/users.go +++ b/services/graph/pkg/service/v0/users.go @@ -79,6 +79,12 @@ func (g Graph) GetMe(w http.ResponseWriter, r *http.Request) { errorcode.RenderError(w, r, err) return } + if me == nil { + // this is an error + logger.Debug().Err(err).Interface("query", r.URL.Query()).Msg("could not get users: user not found in backend") + errorcode.ItemNotFound.Render(w, r, http.StatusNotFound, "user not found") + return + } if me.MemberOf == nil { me.MemberOf = []libregraph.Group{} } @@ -481,6 +487,12 @@ func (g Graph) GetUser(w http.ResponseWriter, r *http.Request) { errorcode.RenderError(w, r, err) return } + if user == nil { + // this is an error + logger.Debug().Err(err).Interface("query", r.URL.Query()).Msg("could not get user: user not found in backend") + errorcode.ItemNotFound.Render(w, r, http.StatusNotFound, "user not found") + return + } listDrives := slices.Contains(exp, "drives") listDrive := slices.Contains(exp, "drive") @@ -645,6 +657,12 @@ func (g Graph) DeleteUser(w http.ResponseWriter, r *http.Request) { errorcode.RenderError(w, r, err) return } + if user == nil { + // this is an error + logger.Debug().Err(err).Interface("query", r.URL.Query()).Msg("could not delete user: user not found in backend") + errorcode.ItemNotFound.Render(w, r, http.StatusNotFound, "user not found") + return + } us, err := g.getUserStateFromNatsKeyValue(r.Context(), userID) if err != nil { @@ -757,9 +775,19 @@ func (g Graph) DeleteUser(w http.ResponseWriter, r *http.Request) { if (g.config.UserSoftDeleteRetentionTime > 0 && us.State == userstate.UserStateSoftDeleted && purgeUser) || (g.config.UserSoftDeleteRetentionTime == 0) { logger.Debug().Str("id", user.GetId()).Msg("calling delete user on backend") - err = g.identityBackend.DeleteUser(r.Context(), user.GetId()) + ok, err := g.identityBackend.DeleteUser(r.Context(), user.GetId()) if err != nil { - logger.Debug().Err(err).Msg("could not delete user: backend error") + // since cases where the user cannot be found in the backend don't return an error, + // we can safely log this as an error: + logger.Error().Err(err).Msg("could not delete user: backend error") + errorcode.RenderError(w, r, err) + return + } + if !ok { + // we could not find the user to delete, we can treat that as an error, or a noop situation; + // for backwards compatibility, this is treated as an error + logger.Debug().Msg("could not delete user: user not found") + err = identity.ErrNotFound errorcode.RenderError(w, r, err) return } @@ -784,7 +812,15 @@ func (g Graph) DeleteUser(w http.ResponseWriter, r *http.Request) { errorcode.RenderError(w, r, err) return } - g.identityBackend.UpdateUser(r.Context(), user.GetId(), userUpdate) + // note: logging these as WARN for backwards compatibility reason since they previously were not logged at all + if found, err := g.identityBackend.UpdateUser(r.Context(), user.GetId(), userUpdate); err != nil { + // TODO: any reason this shouldn't be an error? (if so, please document) + logger.Warn().Err(err).Str("id", userID).Msg("failed to update user") + } else if found == nil { + // no error, but the user to update wasn't found in the backend + // TODO: any reason this shouldn't be an error? (if so, please document) + logger.Warn().Str("id", userID).Msg("failed to update user: user not found in backend") + } } if g.config.UserSoftDeleteRetentionTime == 0 || @@ -887,6 +923,12 @@ func (g Graph) patchUser(w http.ResponseWriter, r *http.Request, nameOrID string errorcode.RenderError(w, r, err) return } + if oldUserValues == nil { + // this is an error + logger.Debug().Err(err).Interface("query", r.URL.Query()).Msg("could not get user: user not found in backend") + errorcode.ItemNotFound.Render(w, r, http.StatusNotFound, "user not found") + return + } if nameOrID == "" { logger.Debug().Msg("could not update user: missing user id") @@ -1016,6 +1058,14 @@ func (g Graph) patchUser(w http.ResponseWriter, r *http.Request, nameOrID string errorcode.RenderError(w, r, err) return } + if u == nil { + // no error, but the user could not be found in the backend + // but for our use-case, this must be treated as an error + err = identity.ErrNotFound + logger.Debug().Err(err).Str("id", nameOrID).Msg("could not update user: failed to find user in backend") + errorcode.RenderError(w, r, err) + return + } u.PreferredLanguage = preferredLanguage g.patchUserResponse(w, r, u, features) diff --git a/services/graph/pkg/service/v0/users_test.go b/services/graph/pkg/service/v0/users_test.go index 5db85f90f8..e1b6564d9d 100644 --- a/services/graph/pkg/service/v0/users_test.go +++ b/services/graph/pkg/service/v0/users_test.go @@ -24,10 +24,12 @@ import ( "github.com/opencloud-eu/reva/v2/pkg/rgrpc/status" "github.com/opencloud-eu/reva/v2/pkg/rgrpc/todo/pool" cs3mocks "github.com/opencloud-eu/reva/v2/tests/cs3mocks/mocks" + "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/mock" "go-micro.dev/v4/client" "google.golang.org/grpc" + "github.com/opencloud-eu/opencloud/services/graph/pkg/metrics" "github.com/opencloud-eu/opencloud/services/graph/pkg/userstate" "github.com/opencloud-eu/opencloud/pkg/shared" @@ -57,6 +59,7 @@ var _ = Describe("Users", func() { valueService *settingsmocks.ValueService permissionService *mocks.Permissions identityBackend *identitymocks.Backend + mtrics *metrics.Metrics natsKeyValueMock *mocks.KeyValue rr *httptest.ResponseRecorder @@ -86,6 +89,7 @@ var _ = Describe("Users", func() { natsKeyValueMock = &mocks.KeyValue{} valueService = &settingsmocks.ValueService{} permissionService = &mocks.Permissions{} + mtrics = metrics.New(prometheus.NewRegistry(), func([]string) (string, string) { return "", "" }) rr = httptest.NewRecorder() ctx = context.Background() @@ -104,6 +108,7 @@ var _ = Describe("Users", func() { var err error svc, err = service.NewService( service.Config(cfg), + service.Metrics(mtrics), service.WithGatewaySelector(gatewaySelector), service.EventsPublisher(&eventsPublisher), service.WithIdentityBackend(identityBackend), @@ -916,6 +921,7 @@ var _ = Describe("Users", func() { localSvc, err := service.NewService( service.Config(localCfg), + service.Metrics(mtrics), service.WithGatewaySelector(gatewaySelector), service.EventsPublisher(&eventsPublisher), service.WithIdentityBackend(identityBackend), @@ -1012,7 +1018,7 @@ var _ = Describe("Users", func() { lu := libregraph.User{} lu.SetId(otheruser.Id.OpaqueId) identityBackend.On("GetUser", mock.Anything, mock.Anything, mock.Anything).Return(&lu, nil) - identityBackend.On("DeleteUser", mock.Anything, mock.Anything).Return(nil) + identityBackend.On("DeleteUser", mock.Anything, mock.Anything).Return(true, nil) gatewayClient.On("DeleteStorageSpace", mock.Anything, mock.Anything).Return(&provider.DeleteStorageSpaceResponse{ Status: status.NewOK(ctx), }, nil) @@ -1091,7 +1097,7 @@ var _ = Describe("Users", func() { lu := libregraph.User{} lu.SetId(otheruser.Id.OpaqueId) identityBackend.On("GetUser", mock.Anything, mock.Anything, mock.Anything).Return(&lu, nil) - //identityBackend.On("DeleteUser", mock.Anything, mock.Anything).Return(nil) + //identityBackend.On("DeleteUser", mock.Anything, mock.Anything).Return(true, nil) identityBackend.On("UpdateUser", mock.Anything, mock.Anything, mock.Anything).Return(&lu, nil) gatewayClient.On("DeleteStorageSpace", mock.Anything, mock.Anything).Return(&provider.DeleteStorageSpaceResponse{ Status: status.NewOK(ctx), @@ -1148,7 +1154,7 @@ var _ = Describe("Users", func() { lu := libregraph.User{} lu.SetId(otheruser.Id.OpaqueId) identityBackend.On("GetUser", mock.Anything, mock.Anything, mock.Anything).Return(&lu, nil) - identityBackend.On("DeleteUser", mock.Anything, mock.Anything).Return(nil) + identityBackend.On("DeleteUser", mock.Anything, mock.Anything).Return(true, nil) identityBackend.On("UpdateUser", mock.Anything, mock.Anything, mock.Anything).Return(&lu, nil) gatewayClient.On("DeleteStorageSpace", mock.Anything, mock.Anything).Return(&provider.DeleteStorageSpaceResponse{ Status: status.NewOK(ctx), @@ -1315,6 +1321,7 @@ var _ = Describe("Users", func() { var err error svc, err = service.NewService( service.Config(cfg), + service.Metrics(mtrics), service.WithGatewaySelector(gatewaySelector), service.EventsPublisher(&eventsPublisher), service.WithIdentityBackend(identityBackend),