From b695bf2cc00b637bba75d3d1430722f3f5d0d8c4 Mon Sep 17 00:00:00 2001 From: Pascal Bleser Date: Tue, 11 Aug 2026 18:38:33 +0200 Subject: [PATCH 1/3] chore(graph): disable HTTP or eventhandlers by configuration In the scope of the broader issue #1312, this PR deals with performing those changes for the `graph` service, namely to add the ability to disable the HTTP API or to disable the events API handler by configuration. It also adds metrics for the events processing, and tests for the events processing. The previous implementation was combining the HTTP server service and the events consumption, which is why this PR refactors the composition of those services: * the event consumption has been moved into its own service * the identity.Backend is created beforehand, and then injected as a collaborator in both the HTTP service as well as the event consumer service It also adds metrics, mainly for the event processing. To encourage re-use in latter implementations and changes, it also introduces two top-level package changes: * internal/eventstest/events_test_helpers: contains a TestBus implementation to unit-test event consumers without NATS * internal/metricstest/metrics_test_helpers: contains assertion functions to test Prometheus metrics Additional boy-scouting: * in identity/backend.go: modify the UpdateLastSignInDate function to return a bool in addition to the error to clarify whether the operation was even attempted or not, to be able to detect when the operation is unsupported, and log errors (or not) accordingly --- internal/eventstest/events_test_helpers.go | 44 +++++ internal/metricstest/metrics_test_helpers.go | 143 ++++++++++++++ services/graph/README.md | 21 ++- services/graph/pkg/command/server.go | 69 ++++++- services/graph/pkg/config/config.go | 14 ++ .../pkg/config/defaults/defaultconfig.go | 8 +- services/graph/pkg/config/http.go | 1 + services/graph/pkg/config/parser/parse.go | 8 + services/graph/pkg/identity/backend.go | 11 +- services/graph/pkg/identity/cs3.go | 4 +- services/graph/pkg/identity/factory.go | 136 ++++++++++++++ services/graph/pkg/identity/ldap.go | 12 +- services/graph/pkg/identity/mocks/backend.go | 25 ++- services/graph/pkg/metrics/metrics.go | 50 ++++- services/graph/pkg/server/http/server.go | 25 +-- services/graph/pkg/service/events/service.go | 123 ++++++++++++ .../graph/pkg/service/events/service_test.go | 124 ++++++++++++ services/graph/pkg/service/v0/graph.go | 1 - services/graph/pkg/service/v0/option.go | 8 - services/graph/pkg/service/v0/service.go | 177 ------------------ 20 files changed, 770 insertions(+), 234 deletions(-) create mode 100644 internal/eventstest/events_test_helpers.go create mode 100644 internal/metricstest/metrics_test_helpers.go create mode 100644 services/graph/pkg/identity/factory.go create mode 100644 services/graph/pkg/service/events/service.go create mode 100644 services/graph/pkg/service/events/service_test.go 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..3a581124ee 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,14 @@ 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" + "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 +52,7 @@ func Server(cfg *config.Config) *cobra.Command { } ctx := cfg.Context - mtrcs := metrics.New() + mtrcs := metrics.New(prometheus.DefaultRegisterer) mtrcs.BuildInfo.WithLabelValues(version.GetString()).Set(1) var kv jetstream.KeyValue @@ -78,9 +84,37 @@ func Server(cfg *config.Config) *cobra.Command { } } + identityBackend, eduBackend, err := identity.CreateIdentityBackends( + cfg.Identity.Backend, + cfg, + &logger, + 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 +126,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..b8af888bdd 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. @@ -127,6 +128,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 +138,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..9e5c039842 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", @@ -118,9 +119,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..4859fa69f0 100644 --- a/services/graph/pkg/config/http.go +++ b/services/graph/pkg/config/http.go @@ -4,6 +4,7 @@ import "github.com/opencloud-eu/opencloud/pkg/shared" // 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"` 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/identity/backend.go b/services/graph/pkg/identity/backend.go index 342af7ade9..aeb7bd365e 100644 --- a/services/graph/pkg/identity/backend.go +++ b/services/graph/pkg/identity/backend.go @@ -40,8 +40,15 @@ type Backend interface { 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 diff --git a/services/graph/pkg/identity/cs3.go b/services/graph/pkg/identity/cs3.go index 93de4ecb3d..e2a34e5d94 100644 --- a/services/graph/pkg/identity/cs3.go +++ b/services/graph/pkg/identity/cs3.go @@ -147,8 +147,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. diff --git a/services/graph/pkg/identity/factory.go b/services/graph/pkg/identity/factory.go new file mode 100644 index 0000000000..52cc1f11ce --- /dev/null +++ b/services/graph/pkg/identity/factory.go @@ -0,0 +1,136 @@ +package identity + +import ( + "crypto/tls" + "crypto/x509" + "errors" + "fmt" + "os" + + 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/reva/v2/pkg/rgrpc/todo/pool" + "github.com/opencloud-eu/reva/v2/pkg/utils/ldap" + "go.opentelemetry.io/otel/trace" +) + +func CreateIdentityBackends(name string, cfg *config.Config, logger *log.Logger, traceProvider trace.TracerProvider) (Backend, EducationBackend, error) { + switch name { + case "cs3": + 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 + } + + return &CS3{ + Config: cfg.Reva, + Logger: logger, + GatewaySelector: gatewaySelector, + }, nil, nil + case "ldap": + 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 + } + + conn := ldap.NewLDAPWithReconnect( + ldap.Config{ + URI: cfg.Identity.LDAP.URI, + BindDN: cfg.Identity.LDAP.BindDN, + BindPassword: cfg.Identity.LDAP.BindPassword, + TLSConfig: tlsConf, + }, + ) + conn.SetLogger(&logger.Logger) + lb, err := NewLDAPBackend(conn, cfg.Identity.LDAP, logger) + 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: '%s'", name) + logger.Err(err) + return nil, nil, err + } +} diff --git a/services/graph/pkg/identity/ldap.go b/services/graph/pkg/identity/ldap.go index 0cd28b23f0..eb1007536d 100644 --- a/services/graph/pkg/identity/ldap.go +++ b/services/graph/pkg/identity/ldap.go @@ -700,18 +700,18 @@ 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) (bool, 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 + return false, nil } 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 + return false, nil case err != nil: - return err + return false, err } mr := ldap.ModifyRequest{DN: e.DN} @@ -725,10 +725,10 @@ 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) + return false, i.mapLDAPError(err, errMap) } - return nil + return true, nil } func (i *LDAP) changeUserName(ctx context.Context, dn, originalUserName, newUserName string) (*ldap.Entry, error) { diff --git a/services/graph/pkg/identity/mocks/backend.go b/services/graph/pkg/identity/mocks/backend.go index ec056e5e80..9ed250e50d 100644 --- a/services/graph/pkg/identity/mocks/backend.go +++ b/services/graph/pkg/identity/mocks/backend.go @@ -913,20 +913,29 @@ func (_c *Backend_UpdateGroupName_Call) RunAndReturn(run func(ctx context.Contex } // 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 +974,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/metrics/metrics.go b/services/graph/pkg/metrics/metrics.go index 7e597327a2..437822e662 100644 --- a/services/graph/pkg/metrics/metrics.go +++ b/services/graph/pkg/metrics/metrics.go @@ -12,12 +12,21 @@ 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 } +const ( + ResultSuccess = "success" + ResultFailure = "failure" +) + // New initializes the available metrics. -func New() *Metrics { +func New(registerer prometheus.Registerer) *Metrics { m := &Metrics{ BuildInfo: prometheus.NewGaugeVec(prometheus.GaugeOpts{ Namespace: Namespace, @@ -25,9 +34,44 @@ 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{"event", "result"}), + 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", + }), } _ = prometheus.Register(m.BuildInfo) + _ = prometheus.Register(m.EventsEnabled) + _ = prometheus.Register(m.HttpEnabled) + _ = prometheus.Register(m.EventsProcessed) + _ = prometheus.Register(m.UnsupportedEvents) + _ = prometheus.Register(m.InvalidEvents) // TODO: implement metrics return m } diff --git a/services/graph/pkg/server/http/server.go b/services/graph/pkg/server/http/server.go index c5830949b0..efaac0742f 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,13 @@ 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" 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 +52,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, @@ -168,8 +153,7 @@ func Server(opts ...Option) (http.Service, error) { svc.Logger(options.Logger), svc.Config(options.Config), 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 +163,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 +174,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..e5920107bd --- /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) + + 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) + + 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/graph.go b/services/graph/pkg/service/v0/graph.go index c6ef4fa74c..38246415e7 100644 --- a/services/graph/pkg/service/v0/graph.go +++ b/services/graph/pkg/service/v0/graph.go @@ -63,7 +63,6 @@ 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 diff --git a/services/graph/pkg/service/v0/option.go b/services/graph/pkg/service/v0/option.go index 5330fd5783..cc720d7b92 100644 --- a/services/graph/pkg/service/v0/option.go +++ b/services/graph/pkg/service/v0/option.go @@ -39,7 +39,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 @@ -163,13 +162,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/service.go b/services/graph/pkg/service/v0/service.go index fb5e490851..1333dc92f2 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,7 +185,6 @@ func NewService(opts ...Option) (Graph, error) { //nolint:maintidx mux: m, specialDriveItemsCache: spacePropertiesCache, eventsPublisher: options.EventsPublisher, - eventsConsumer: options.EventsConsumer, searchService: options.SearchService, identityEducationBackend: options.IdentityEducationBackend, keycloakClient: options.KeycloakClient, @@ -209,10 +194,6 @@ func NewService(opts ...Option) (Graph, error) { //nolint:maintidx 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,164 +430,6 @@ 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 - } - } else { - svc.identityBackend = options.IdentityBackend - } - - 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. // '1', 't', 'T', 'TRUE', 'true', 'True' are parsed as true // all other values are false. From ec4998963690319fc775c91b759f6148a055064c Mon Sep 17 00:00:00 2001 From: Pascal Bleser Date: Thu, 13 Aug 2026 11:56:19 +0200 Subject: [PATCH 2/3] fix missing identityBackend setting which was causing a nil pointer deref panic --- services/graph/pkg/service/v0/service.go | 1 + 1 file changed, 1 insertion(+) diff --git a/services/graph/pkg/service/v0/service.go b/services/graph/pkg/service/v0/service.go index 1333dc92f2..f956f1ffb6 100644 --- a/services/graph/pkg/service/v0/service.go +++ b/services/graph/pkg/service/v0/service.go @@ -186,6 +186,7 @@ func NewService(opts ...Option) (Graph, error) { //nolint:maintidx specialDriveItemsCache: spacePropertiesCache, eventsPublisher: options.EventsPublisher, searchService: options.SearchService, + identityBackend: options.IdentityBackend, identityEducationBackend: options.IdentityEducationBackend, keycloakClient: options.KeycloakClient, historyClient: options.EventHistoryClient, From 1f47d022aa0f45ecd6be0729d10990f3f15df9ca Mon Sep 17 00:00:00 2001 From: Pascal Bleser Date: Thu, 13 Aug 2026 21:50:40 +0200 Subject: [PATCH 3/3] chore(graph): add metrics for HTTP API and LDAP Introduce LDAP client abstraction interface to be able to wrap the go-ldap client API with metrics transparently (and possibly hooks and such in the future) Has two implementations: * a go-ldap adapter implementation that directly delegates * a time measuring and metrics collecting implementation that delegates to another LdapClient The metrics collecting one is disabled by default, can be enabled with GRAPH_LDAP_METRICS_DISABLE=false Add an HTTP middleware that measures how long Graph API requests take, storing taken time into a histogram along with labels for method, path pattern (from the chi routes), Graph API version prefix, and Graph API resource name, as well as the resulting status code. Disabled by default, can be enabled with GRAPH_HTTP_METRICS_DISABLE=false Make some internal changes to how some of the LDAP client API operations work in the LDAP backend: * check whether searches for a singular entry returns more than one result, in which case a new error TooManyResults is returned, instead of leaving that undetected, blindly taking the first result, and potentially risking data inconsistencies * DeleteUser() does not return an error any more when the user to delete cannot be found in LDAP: instead, it now also returns a bool following the 'ok' idiom, and callers can deal with whether that is supposed to end up in an error or not on their level * GetUser() and UpdateUser() also go not return an error when the user entry is not found in LDAP; instead, they returns nil for the user, which must now be checked for nilness by callers, and dealt with as they see fit depending on the context of the operation Callers have been modified accordingly, checking for nilness of the returned user in order to behave the same way as before, returning an ItemNotFound error on their level. Furthermore, due to the execution time measuring metrics that are introduced with this change, the implementation pattern of the public LDAP identity backend functions had to be changed, using named return values instead, as that is the least intrusive option to record elapsed time using defer and being able to also set a metric label value for whether the operation succeeded or not (looking at the function-global error variable that is returned). Improve the loggers in identity backends by adding attributes for their request targets (Reva gateway address or LDAP URI, respectively). Also add a "backend" attribute for all Graph API logs (set to "ldap" or "cs3"), to help debug potential issues. The LDAP identity backend logger also has two new attributes to help debugging with logs: * write (bool): whether write operations are enabled * refint (bol): whether refint is enabled or not Introduce metrics for the LDAP identity backend. Metrics measure execution time for all the higher-level LDAP identity backend operations (create-user, delete-user, update-user, get-user, filter-users, update-last-signin-date). An additional set of metrics measure the execution time for all LDAP client operations, as referred to in the LDAP Client interface introduction referred to above. Also adds a dedicated counter for user password change operations. Minor campfire improvements: * add a constructor func for the CS3 backend * add a constructor func for the LDAP backend * in the LDAP identity backend, in searchLDAPEntryByFilter (used by all search/get public functions), errors that occur when performing LDAP SEARCH operations were blindly mapped to a ItemNotFound error, instead of being analyzed as it could be caused by a technical error --- services/graph/pkg/command/server.go | 20 +- services/graph/pkg/config/config.go | 6 + .../pkg/config/defaults/defaultconfig.go | 10 + services/graph/pkg/config/http.go | 5 + services/graph/pkg/errorcode/errorcode.go | 2 + services/graph/pkg/identity/backend.go | 16 +- services/graph/pkg/identity/cs3.go | 32 +- services/graph/pkg/identity/err_education.go | 14 +- services/graph/pkg/identity/factory.go | 53 +- services/graph/pkg/identity/ldap.go | 459 +++++++++++++----- services/graph/pkg/identity/ldap_client.go | 14 + .../graph/pkg/identity/ldap_client_goldap.go | 36 ++ .../graph/pkg/identity/ldap_client_metrics.go | 100 ++++ .../pkg/identity/ldap_education_class.go | 25 +- .../pkg/identity/ldap_education_school.go | 394 +++++++++++---- .../identity/ldap_education_school_test.go | 31 +- .../graph/pkg/identity/ldap_education_user.go | 38 +- services/graph/pkg/identity/ldap_group.go | 320 ++++++++---- .../graph/pkg/identity/ldap_group_test.go | 6 +- services/graph/pkg/identity/ldap_test.go | 27 +- services/graph/pkg/identity/mocks/backend.go | 112 +++-- .../pkg/identity/mocks/education_backend.go | 87 +++- services/graph/pkg/metrics/metrics.go | 87 +++- services/graph/pkg/metrics/middleware.go | 50 ++ services/graph/pkg/middleware/requireadmin.go | 9 +- services/graph/pkg/server/http/server.go | 12 +- .../graph/pkg/service/events/service_test.go | 4 +- .../graph/pkg/service/v0/application_test.go | 4 + .../pkg/service/v0/approleassignments_test.go | 4 + .../graph/pkg/service/v0/driveitems_test.go | 4 + .../graph/pkg/service/v0/educationclasses.go | 15 +- .../pkg/service/v0/educationclasses_test.go | 9 +- .../graph/pkg/service/v0/educationschools.go | 57 ++- .../pkg/service/v0/educationschools_test.go | 16 +- .../pkg/service/v0/educationuser_test.go | 4 + services/graph/pkg/service/v0/graph.go | 4 + services/graph/pkg/service/v0/graph_test.go | 5 + services/graph/pkg/service/v0/groups.go | 32 +- services/graph/pkg/service/v0/groups_test.go | 13 +- services/graph/pkg/service/v0/option.go | 9 + services/graph/pkg/service/v0/password.go | 9 +- .../graph/pkg/service/v0/password_test.go | 14 +- .../pkg/service/v0/rolemanagement_test.go | 4 + services/graph/pkg/service/v0/service.go | 28 ++ .../graph/pkg/service/v0/sharedbyme_test.go | 4 + .../graph/pkg/service/v0/sharedwithme_test.go | 4 + services/graph/pkg/service/v0/users.go | 56 ++- services/graph/pkg/service/v0/users_test.go | 13 +- 48 files changed, 1781 insertions(+), 496 deletions(-) create mode 100644 services/graph/pkg/identity/ldap_client.go create mode 100644 services/graph/pkg/identity/ldap_client_goldap.go create mode 100644 services/graph/pkg/identity/ldap_client_metrics.go create mode 100644 services/graph/pkg/metrics/middleware.go diff --git a/services/graph/pkg/command/server.go b/services/graph/pkg/command/server.go index 3a581124ee..e6690f84b1 100644 --- a/services/graph/pkg/command/server.go +++ b/services/graph/pkg/command/server.go @@ -20,6 +20,7 @@ import ( "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" @@ -52,7 +53,14 @@ func Server(cfg *config.Config) *cobra.Command { } ctx := cfg.Context - mtrcs := metrics.New(prometheus.DefaultRegisterer) + 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 @@ -84,12 +92,20 @@ 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( - cfg.Identity.Backend, + 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) diff --git a/services/graph/pkg/config/config.go b/services/graph/pkg/config/config.go index b8af888bdd..cf244c4865 100644 --- a/services/graph/pkg/config/config.go +++ b/services/graph/pkg/config/config.go @@ -58,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"` @@ -95,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 diff --git a/services/graph/pkg/config/defaults/defaultconfig.go b/services/graph/pkg/config/defaults/defaultconfig.go index 9e5c039842..3ae60b3fad 100644 --- a/services/graph/pkg/config/defaults/defaultconfig.go +++ b/services/graph/pkg/config/defaults/defaultconfig.go @@ -53,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", @@ -110,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{ diff --git a/services/graph/pkg/config/http.go b/services/graph/pkg/config/http.go index 4859fa69f0..98f49f3f40 100644 --- a/services/graph/pkg/config/http.go +++ b/services/graph/pkg/config/http.go @@ -2,6 +2,10 @@ 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%"` @@ -11,4 +15,5 @@ type HTTP struct { 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/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 aeb7bd365e..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,7 +35,7 @@ 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) @@ -52,9 +54,9 @@ type Backend interface { // 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 @@ -62,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 @@ -70,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 @@ -82,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 e2a34e5d94..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 @@ -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 index 52cc1f11ce..fc23d9985b 100644 --- a/services/graph/pkg/identity/factory.go +++ b/services/graph/pkg/identity/factory.go @@ -6,20 +6,30 @@ import ( "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" ) -func CreateIdentityBackends(name string, cfg *config.Config, logger *log.Logger, traceProvider trace.TracerProvider) (Backend, EducationBackend, error) { +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 "cs3": + case cs3Backend: gatewaySelector, err := pool.GatewaySelector( cfg.Reva.Address, append( @@ -32,12 +42,12 @@ func CreateIdentityBackends(name string, cfg *config.Config, logger *log.Logger, return nil, nil, err } - return &CS3{ - Config: cfg.Reva, - Logger: logger, - GatewaySelector: gatewaySelector, - }, nil, nil - case "ldap": + 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 @@ -76,16 +86,21 @@ func CreateIdentityBackends(name string, cfg *config.Config, logger *log.Logger, tlsConf.RootCAs = certs } - conn := ldap.NewLDAPWithReconnect( - ldap.Config{ - URI: cfg.Identity.LDAP.URI, - BindDN: cfg.Identity.LDAP.BindDN, - BindPassword: cfg.Identity.LDAP.BindPassword, - TLSConfig: tlsConf, - }, - ) + 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) + 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 @@ -129,8 +144,8 @@ func CreateIdentityBackends(name string, cfg *config.Config, logger *log.Logger, return identityBackend, eduBackend, nil default: - err := fmt.Errorf("unknown identity backend: '%s'", name) - logger.Err(err) + 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 eb1007536d..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) (bool, 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") + 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 false, nil + err = nil + return case err != nil: - return false, 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 false, i.mapLDAPError(err, errMap) + err = i.mapLDAPError(err, errMap) + return } - return true, 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 9ed250e50d..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,12 +950,12 @@ 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 } 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 437822e662..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,21 +18,41 @@ var ( // Metrics defines the available metrics of this service. type Metrics struct { - BuildInfo *prometheus.GaugeVec - EventsEnabled prometheus.Gauge - HttpEnabled prometheus.Gauge - EventsProcessed *prometheus.CounterVec - InvalidEvents prometheus.Counter - UnsupportedEvents prometheus.Counter + 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(registerer prometheus.Registerer) *Metrics { +func New(registerer prometheus.Registerer, httpPathSplitter func(pieces []string) (string, string)) *Metrics { m := &Metrics{ BuildInfo: prometheus.NewGaugeVec(prometheus.GaugeOpts{ Namespace: Namespace, @@ -51,7 +77,7 @@ func New(registerer prometheus.Registerer) *Metrics { Subsystem: Subsystem, Name: "events", Help: "Number of consumed events", - }, []string{"event", "result"}), + }, []string{LabelEvent, LabelResult}), InvalidEvents: prometheus.NewCounter(prometheus.CounterOpts{ Namespace: Namespace, Subsystem: Subsystem, @@ -64,14 +90,53 @@ func New(registerer prometheus.Registerer) *Metrics { 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) _ = prometheus.Register(m.EventsEnabled) _ = prometheus.Register(m.HttpEnabled) _ = prometheus.Register(m.EventsProcessed) - _ = prometheus.Register(m.UnsupportedEvents) _ = prometheus.Register(m.InvalidEvents) - // TODO: implement metrics + _ = 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 efaac0742f..1f978f4423 100644 --- a/services/graph/pkg/server/http/server.go +++ b/services/graph/pkg/server/http/server.go @@ -26,6 +26,7 @@ import ( 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" ) @@ -62,6 +63,13 @@ func Server(identityBackend identity.Backend, eduBackend identity.EducationBacke 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), @@ -69,7 +77,8 @@ func Server(identityBackend identity.Backend, eduBackend identity.EducationBacke 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 @@ -152,6 +161,7 @@ func Server(identityBackend identity.Backend, eduBackend identity.EducationBacke svc.UserProfilePhotoService(userProfilePhotoService), svc.Logger(options.Logger), svc.Config(options.Config), + svc.Metrics(options.Metrics), svc.Middleware(middlewares...), svc.EventsPublisher(eventsStream), // is required even when event consumption is disabled svc.WithRoleService(roleService), diff --git a/services/graph/pkg/service/events/service_test.go b/services/graph/pkg/service/events/service_test.go index e5920107bd..c687e9fece 100644 --- a/services/graph/pkg/service/events/service_test.go +++ b/services/graph/pkg/service/events/service_test.go @@ -42,7 +42,7 @@ func TestSuccessfulCall(t *testing.T) { }) reg := prometheus.NewRegistry() - m := metrics.New(reg) + m := metrics.New(reg, func(_ []string) (string, string) { return "", "" }) logger := log.NewLogger() @@ -92,7 +92,7 @@ func TestBackendReturningAnError(t *testing.T) { }) reg := prometheus.NewRegistry() - m := metrics.New(reg) + m := metrics.New(reg, func(_ []string) (string, string) { return "", "" }) logger := log.NewLogger() 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 38246415e7..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 @@ -66,10 +67,13 @@ type Graph struct { 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 cc720d7b92..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] @@ -78,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) { 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 f956f1ffb6..1442c084fe 100644 --- a/services/graph/pkg/service/v0/service.go +++ b/services/graph/pkg/service/v0/service.go @@ -190,6 +190,7 @@ func NewService(opts ...Option) (Graph, error) { //nolint:maintidx identityEducationBackend: options.IdentityEducationBackend, keycloakClient: options.KeycloakClient, historyClient: options.EventHistoryClient, + metrics: options.Metrics, traceProvider: options.TraceProvider, valueService: options.ValueService, natskv: options.NatsKeyValue, @@ -431,6 +432,33 @@ func NewService(opts ...Option) (Graph, error) { //nolint:maintidx return svc, nil } +// 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] + } + } + return version, resource + +} + // parseHeaderPurge parses the 'Purge' header. // '1', 't', 'T', 'TRUE', 'true', 'True' are parsed as true // all other values are false. 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),