From cf824b5447fa1b20792a0cbceaed03dcde395a2a Mon Sep 17 00:00:00 2001 From: Pascal Bleser Date: Wed, 4 Mar 2026 17:05:38 +0100 Subject: [PATCH] groupware: fix typos and minor issues - fix a bunch of minor issues and typos that were found using GoLand and gosec - add a gosec Makefile target for Groupware related files, in services/groupware/Makefile - enable checking JMAP session capabilities for events and contacts, and only enable skipping that check for tasks until those are implemented in Stalwart as well - fix a CWE-190 (integer overflow or wraparound) found by gosec - consistently use struct references for methods of Groupware and Request, instead of mixing up references and copies - always log errors when unable to register a Prometheus metric --- pkg/jmap/http.go | 7 +- pkg/jmap/model.go | 2 +- services/groupware/Makefile | 4 + services/groupware/apidoc-postprocess-html.ts | 3 +- .../groupware/pkg/groupware/api_emails.go | 33 +++-- services/groupware/pkg/groupware/dns.go | 4 +- services/groupware/pkg/groupware/error.go | 10 +- services/groupware/pkg/groupware/framework.go | 65 ++++++--- services/groupware/pkg/groupware/request.go | 138 ++++++++---------- services/groupware/pkg/groupware/reva.go | 16 +- services/groupware/pkg/groupware/route.go | 4 +- services/groupware/pkg/groupware/session.go | 26 ++-- services/groupware/pkg/metrics/metrics.go | 4 +- .../groupware/pkg/metrics/startup_metrics.go | 9 +- .../groupware/pkg/service/http/v0/service.go | 2 +- 15 files changed, 184 insertions(+), 143 deletions(-) diff --git a/pkg/jmap/http.go b/pkg/jmap/http.go index 21c62c992f..3561cd3bbc 100644 --- a/pkg/jmap/http.go +++ b/pkg/jmap/http.go @@ -555,15 +555,16 @@ type HttpWsClient struct { } func (w *HttpWsClient) readPump() { + logger := log.From(w.logger.With().Str("username", w.username)) defer func() { - w.c.Close() + if err := w.c.Close(); err != nil { + logger.Warn().Err(err).Msg("failed to close websocket connection") + } }() //w.c.SetReadLimit(maxMessageSize) //c.conn.SetReadDeadline(time.Now().Add(pongWait)) //c.conn.SetPongHandler(func(string) error { c.conn.SetReadDeadline(time.Now().Add(pongWait)); return nil }) - logger := log.From(w.logger.With().Str("username", w.username)) - for { if _, message, err := w.c.ReadMessage(); err != nil { if websocket.IsUnexpectedCloseError(err, websocket.CloseGoingAway, websocket.CloseAbnormalClosure) { diff --git a/pkg/jmap/model.go b/pkg/jmap/model.go index a70694c1a2..54712fa86d 100644 --- a/pkg/jmap/model.go +++ b/pkg/jmap/model.go @@ -100,7 +100,7 @@ const ( JmapSieve = "urn:ietf:params:jmap:sieve" JmapBlob = "urn:ietf:params:jmap:blob" JmapQuota = "urn:ietf:params:jmap:quota" - JmapWebsocket = "urn:ietf:params:jmap:websocket" + JmapWebsocket = "urn:ietf:params:jmap:websocket" // #nosec G101 false positive: these are not credentials JmapPrincipals = "urn:ietf:params:jmap:principals" JmapPrincipalsOwner = "urn:ietf:params:jmap:principals:owner" JmapTasks = "urn:ietf:params:jmap:tasks" diff --git a/services/groupware/Makefile b/services/groupware/Makefile index 017bbb32b0..bd8066817d 100644 --- a/services/groupware/Makefile +++ b/services/groupware/Makefile @@ -44,3 +44,7 @@ examples: cd ../../pkg/jscontact/ && go test -tags=groupware_examples . -v -count=1 -run '^.*Example$'' cd ../../pkg/jscalendar/ && go test -tags=groupware_examples . -v -count=1 -run '^.*Example$'' cd ./pkg/groupware/ && go test -tags=groupware_examples . -v -count=1 -run '^.*Example$'' + +.PHONY: gosec +gosec: + cd ../../ && gosec ./pkg/jmap/... ./pkg/jscalendar/... ./pkg/jscontact/... ./services/groupware/pkg/... diff --git a/services/groupware/apidoc-postprocess-html.ts b/services/groupware/apidoc-postprocess-html.ts index ec9dc38c3a..6a10baf00c 100644 --- a/services/groupware/apidoc-postprocess-html.ts +++ b/services/groupware/apidoc-postprocess-html.ts @@ -16,10 +16,9 @@ process.stdin.on('end', () => { process.stdout.write("\n") } catch (error) { if (error instanceof Error) { - console.error(`Error occured while post-processing HTML: ${error.message}`) + console.error(`Error occurred while post-processing HTML: ${error.message}`) } else { console.error("Unknown error occurred") } } }); - diff --git a/services/groupware/pkg/groupware/api_emails.go b/services/groupware/pkg/groupware/api_emails.go index 2dc81cd57a..2c754f5e64 100644 --- a/services/groupware/pkg/groupware/api_emails.go +++ b/services/groupware/pkg/groupware/api_emails.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "io" + "math" "mime" "net/http" "slices" @@ -1293,9 +1294,17 @@ func relatedEmailsFilter(email jmap.Email, beacon time.Time, days uint) jmap.Ema } } - timeFilter := jmap.EmailFilterCondition{ - Before: beacon.Add(time.Duration(days) * time.Hour * 24), - After: beacon.Add(time.Duration(-days) * time.Hour * 24), + var timeFilter jmap.EmailFilterCondition + { + if days > math.MaxInt64 { + days = math.MaxInt64 // avoid gosec G115 (CWE-190) + } + hours := int64(days) * 24 + delta := time.Duration(hours) * time.Hour + timeFilter = jmap.EmailFilterCondition{ + Before: beacon.Add(delta), + After: beacon.Add(-delta), + } } var filter jmap.EmailFilterElement @@ -1382,7 +1391,7 @@ func (g *Groupware) RelatedToEmail(w http.ResponseWriter, r *http.Request) { if results, ok := resultsByAccountId[accountId]; ok { duration := time.Since(before) if jerr != nil { - req.observeJmapError(jerr) + _ = req.observeJmapError(jerr) l.Error().Err(jerr).Msgf("failed to query %v emails", RelationTypeSameSender) } else { req.observe(g.metrics.EmailSameSenderDuration.WithLabelValues(req.session.JmapEndpoint), duration.Seconds()) @@ -1402,7 +1411,7 @@ func (g *Groupware) RelatedToEmail(w http.ResponseWriter, r *http.Request) { emails, _, _, _, jerr := g.jmap.EmailsInThread(accountId, email.ThreadId, req.session, bgctx, l, req.language(), false, g.config.maxBodyValueBytes) duration := time.Since(before) if jerr != nil { - req.observeJmapError(jerr) + _ = req.observeJmapError(jerr) l.Error().Err(jerr).Msgf("failed to list %v emails", RelationTypeSameThread) } else { req.observe(g.metrics.EmailSameThreadDuration.WithLabelValues(req.session.JmapEndpoint), duration.Seconds()) @@ -1735,8 +1744,8 @@ var sanitizableMediaTypes = []string{ "text/xhtml", } -func (req *Request) sanitizeEmail(source jmap.Email) (jmap.Email, *Error) { - if !req.g.config.sanitize { +func (r *Request) sanitizeEmail(source jmap.Email) (jmap.Email, *Error) { + if !r.g.config.sanitize { return source, nil } memory := map[string]int{} @@ -1746,8 +1755,8 @@ func (req *Request) sanitizeEmail(source jmap.Email) (jmap.Email, *Error) { t, _, err := mime.ParseMediaType(p.Type) if err != nil { msg := fmt.Sprintf("failed to parse the mime type '%s'", p.Type) - req.logger.Error().Str("type", log.SafeString(p.Type)).Msg(msg) - return source, req.apiError(&ErrorFailedToSanitizeEmail, withDetail(msg)) + r.logger.Error().Str("type", log.SafeString(p.Type)).Msg(msg) + return source, r.apiError(&ErrorFailedToSanitizeEmail, withDetail(msg)) } if slices.Contains(sanitizableMediaTypes, t) { if already, done := memory[p.PartId]; !done { @@ -1783,13 +1792,13 @@ func (req *Request) sanitizeEmail(source jmap.Email) (jmap.Email, *Error) { return source, nil } -func (req *Request) sanitizeEmails(source []jmap.Email) ([]jmap.Email, *Error) { - if !req.g.config.sanitize { +func (r *Request) sanitizeEmails(source []jmap.Email) ([]jmap.Email, *Error) { + if !r.g.config.sanitize { return source, nil } result := make([]jmap.Email, len(source)) for i, email := range source { - sanitized, gwerr := req.sanitizeEmail(email) + sanitized, gwerr := r.sanitizeEmail(email) if gwerr != nil { return nil, gwerr } diff --git a/services/groupware/pkg/groupware/dns.go b/services/groupware/pkg/groupware/dns.go index 3b726d5594..2ca67df8e4 100644 --- a/services/groupware/pkg/groupware/dns.go +++ b/services/groupware/pkg/groupware/dns.go @@ -35,7 +35,7 @@ func NewDnsSessionUrlResolver( dialTimeout time.Duration, readTimeout time.Duration, ) (DnsSessionUrlResolver, error) { - // TODO the whole udp or tcp dialier configuration, see https://github.com/miekg/exdns/blob/master/q/q.go + // TODO the whole udp or tcp dialer configuration, see https://github.com/miekg/exdns/blob/master/q/q.go c := &dns.Client{ DialTimeout: dialTimeout, @@ -45,6 +45,8 @@ func NewDnsSessionUrlResolver( return DnsSessionUrlResolver{ defaultSessionUrlSupplier: defaultSessionUrlSupplier, defaultDomain: defaultDomain, + domainGreenList: domainGreenList, + domainRedList: domainRedList, config: config, client: c, }, nil diff --git a/services/groupware/pkg/groupware/error.go b/services/groupware/pkg/groupware/error.go index b97a5a051a..f5b3666ec9 100644 --- a/services/groupware/pkg/groupware/error.go +++ b/services/groupware/pkg/groupware/error.go @@ -587,7 +587,7 @@ func errorId(r *http.Request, ctx context.Context) string { } } -func (r Request) errorId() string { +func (r *Request) errorId() string { return errorId(r.r, r.ctx) } @@ -608,11 +608,11 @@ func apiError(id string, gwerr GroupwareError, options ...ErrorOpt) *Error { return err } -func (r Request) observedParameterError(gwerr GroupwareError, options ...ErrorOpt) *Error { +func (r *Request) observedParameterError(gwerr GroupwareError, options ...ErrorOpt) *Error { return r.observeParameterError(apiError(r.errorId(), gwerr, options...)) } -func (r Request) apiError(err *GroupwareError, options ...ErrorOpt) *Error { +func (r *Request) apiError(err *GroupwareError, options ...ErrorOpt) *Error { if err == nil { return nil } @@ -620,7 +620,7 @@ func (r Request) apiError(err *GroupwareError, options ...ErrorOpt) *Error { return apiError(errorId, *err, options...) } -func (r Request) apiErrorFromJmap(err jmap.Error) *Error { +func (r *Request) apiErrorFromJmap(err jmap.Error) *Error { if err == nil { return nil } @@ -637,6 +637,6 @@ func errorResponses(errors ...Error) ErrorResponse { return ErrorResponse{Errors: errors} } -func (r Request) errorResponseFromJmap(accountIds []string, err jmap.Error) Response { +func (r *Request) errorResponseFromJmap(accountIds []string, err jmap.Error) Response { return errorResponse(accountIds, r.apiErrorFromJmap(r.observeJmapError(err))) } diff --git a/services/groupware/pkg/groupware/framework.go b/services/groupware/pkg/groupware/framework.go index d728c1761a..9b384955a4 100644 --- a/services/groupware/pkg/groupware/framework.go +++ b/services/groupware/pkg/groupware/framework.go @@ -230,7 +230,7 @@ func NewGroupware(config *config.Config, logger *log.Logger, mux *chi.Mux, prome httpTransport := http.DefaultTransport.(*http.Transport).Clone() httpTransport.ResponseHeaderTimeout = responseHeaderTimeout if insecureTls { - tlsConfig := &tls.Config{InsecureSkipVerify: true} + tlsConfig := &tls.Config{InsecureSkipVerify: true} // #nosec G402 insecure TLS is a configuration option for development httpTransport.TLSClientConfig = tlsConfig } httpClient = *http.DefaultClient @@ -242,6 +242,11 @@ func NewGroupware(config *config.Config, logger *log.Logger, mux *chi.Mux, prome auth, jmapMetricsAdapter, ) + defer func() { + if err := api.Close(); err != nil { + logger.Error().Err(err).Msgf("failed to close HTTP JMAP API client") + } + }() } var wsf *jmap.HttpWsClientFactory @@ -250,7 +255,7 @@ func NewGroupware(config *config.Config, logger *log.Logger, mux *chi.Mux, prome HandshakeTimeout: wsHandshakeTimeout, } if insecureTls { - wsDialer.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} + wsDialer.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} // #nosec G402 insecure TLS is a configuration option for development } wsf, err = jmap.NewHttpWsClientFactory(wsDialer, auth, logger, jmapMetricsAdapter) @@ -262,6 +267,11 @@ func NewGroupware(config *config.Config, logger *log.Logger, mux *chi.Mux, prome // api implements all three interfaces: jmapClient = jmap.NewClient(api, api, api, wsf) + defer func() { + if err := jmapClient.Close(); err != nil { + logger.Error().Err(err).Msgf("failed to close JMAP client") + } + }() } sessionCacheBuilder := newSessionCacheBuilder( @@ -297,7 +307,6 @@ func NewGroupware(config *config.Config, logger *log.Logger, mux *chi.Mux, prome } sessionCache, err := sessionCacheBuilder.build() - if err != nil { // assuming that the error was logged in great detail upstream return nil, GroupwareInitializationError{Message: "failed to initialize the session cache", Err: err} @@ -311,11 +320,15 @@ func NewGroupware(config *config.Config, logger *log.Logger, mux *chi.Mux, prome if err != nil { logger.Warn().Err(err).Msgf("failed to create metric %v", m.EventBufferSizeDesc.String()) } else { - prometheusRegistry.Register(metrics.ConstMetricCollector{Metric: eventBufferSizeMetric}) + if err := prometheusRegistry.Register(metrics.ConstMetricCollector{Metric: eventBufferSizeMetric}); err != nil { + logger.Error().Err(err).Msg("failed to register event buffer size metric collector") + } } - prometheusRegistry.Register(prometheus.NewGaugeFunc(m.EventBufferQueuedOpts, func() float64 { + if err := prometheusRegistry.Register(prometheus.NewGaugeFunc(m.EventBufferQueuedOpts, func() float64 { return float64(len(eventChannel)) - })) + })); err != nil { + logger.Error().Err(err).Msg("failed to reigster event buffer queue metric") + } } sseServer := sse.New() @@ -328,9 +341,11 @@ func NewGroupware(config *config.Config, logger *log.Logger, mux *chi.Mux, prome sseServer.OnUnsubscribe = func(streamID string, sub *sse.Subscriber) { sseSubscribers.Add(-1) } - prometheusRegistry.Register(prometheus.NewGaugeFunc(m.SSESubscribersOpts, func() float64 { + if err := prometheusRegistry.Register(prometheus.NewGaugeFunc(m.SSESubscribersOpts, func() float64 { return float64(sseSubscribers.Load()) - })) + })); err != nil { + logger.Error().Err(err).Msg("failed to register SSE subscribers metric") + } } jobsChannel := make(chan Job, workerQueueSize) @@ -339,12 +354,16 @@ func NewGroupware(config *config.Config, logger *log.Logger, mux *chi.Mux, prome if err != nil { logger.Warn().Err(err).Msgf("failed to create metric %v", m.WorkersBufferSizeDesc.String()) } else { - prometheusRegistry.Register(metrics.ConstMetricCollector{Metric: totalWorkerBufferMetric}) + if err := prometheusRegistry.Register(metrics.ConstMetricCollector{Metric: totalWorkerBufferMetric}); err != nil { + logger.Error().Err(err).Msg("failed to register total worker buffer metric") + } } - prometheusRegistry.Register(prometheus.NewGaugeFunc(m.WorkersBufferQueuedOpts, func() float64 { + if err := prometheusRegistry.Register(prometheus.NewGaugeFunc(m.WorkersBufferQueuedOpts, func() float64 { return float64(len(jobsChannel)) - })) + })); err != nil { + logger.Error().Err(err).Msg("failed to register jobs channel size metric") + } } var busyWorkers atomic.Int32 @@ -353,12 +372,16 @@ func NewGroupware(config *config.Config, logger *log.Logger, mux *chi.Mux, prome if err != nil { logger.Warn().Err(err).Msgf("failed to create metric %v", m.TotalWorkersDesc.String()) } else { - prometheusRegistry.Register(metrics.ConstMetricCollector{Metric: totalWorkersMetric}) + if err := prometheusRegistry.Register(metrics.ConstMetricCollector{Metric: totalWorkersMetric}); err != nil { + logger.Error().Err(err).Msg("failed to register worker pool size metric") + } } - prometheusRegistry.Register(prometheus.NewGaugeFunc(m.BusyWorkersOpts, func() float64 { + if err := prometheusRegistry.Register(prometheus.NewGaugeFunc(m.BusyWorkersOpts, func() float64 { return float64(busyWorkers.Load()) - })) + })); err != nil { + logger.Error().Err(err).Msg("failed to register busy workers metric") + } } g := &Groupware{ @@ -501,7 +524,7 @@ func (g *Groupware) session(ctx context.Context, user user, logger *log.Logger) return jmap.Session{}, false, s.Error(), s.Until() } } - // not sure this should/could happen: + // not sure whether this should/could happen: logger.Warn().Msg("session cache returned nil") return jmap.Session{}, false, nil, time.Time{} } @@ -554,7 +577,9 @@ func (g *Groupware) serveError(w http.ResponseWriter, r *http.Request, error *Er } render.Status(r, error.NumStatus) w.WriteHeader(error.NumStatus) - render.Render(w, r, errorResponses(*error)) + if err := render.Render(w, r, errorResponses(*error)); err != nil { + g.logger.Error().Err(err).Msgf("failed to render error response") + } } // Execute a closure with a JMAP Session. @@ -644,7 +669,9 @@ func (g *Groupware) sendResponse(w http.ResponseWriter, r *http.Request, respons g.log(response.err) w.Header().Add("Content-Type", ContentTypeJsonApi) render.Status(r, response.err.NumStatus) - render.Render(w, r, errorResponses(*response.err)) + if err := render.Render(w, r, errorResponses(*response.err)); err != nil { + g.logger.Error().Err(err).Msgf("failed to render error response") + } return } @@ -757,7 +784,9 @@ func (g *Groupware) stream(w http.ResponseWriter, r *http.Request, handler func( w.Header().Add("Content-Type", ContentTypeJsonApi) render.Status(r, apierr.NumStatus) w.WriteHeader(apierr.NumStatus) - render.Render(w, r, errorResponses(*apierr)) + if err := render.Render(w, r, errorResponses(*apierr)); err != nil { + logger.Error().Err(err).Msgf("failed to render error response") + } } } diff --git a/services/groupware/pkg/groupware/request.go b/services/groupware/pkg/groupware/request.go index 29dff5ff84..b14df9e38a 100644 --- a/services/groupware/pkg/groupware/request.go +++ b/services/groupware/pkg/groupware/request.go @@ -26,8 +26,8 @@ import ( ) const ( - // TODO remove this once Stalwart has actual support for Tasks, Calendars, Contacts and we don't need to mock it any more - IgnoreSessionCapabilityChecks = true + // TODO remove this once Stalwart has actual support for Tasks and we don't need to mock it any more + IgnoreSessionCapabilityChecksForTasks = true ) // using a wrapper class for requests, to group multiple parameters, really to avoid crowding the @@ -42,23 +42,23 @@ type Request struct { session *jmap.Session } -func isDefaultAccountid(accountId string) bool { +func isDefaultAccountId(accountId string) bool { return slices.Contains(defaultAccountIds, accountId) } -func (r Request) push(typ string, event any) { +func (r *Request) push(typ string, event any) { r.g.push(r.user, typ, event) } -func (r Request) GetUser() user { +func (r *Request) GetUser() user { return r.user } -func (r Request) GetRequestId() string { +func (r *Request) GetRequestId() string { return chimiddleware.GetReqID(r.ctx) } -func (r Request) GetTraceId() string { +func (r *Request) GetTraceId() string { return groupwaremiddleware.GetTraceID(r.ctx) } @@ -76,7 +76,7 @@ var ( // errNoPrimaryAccountForWebsocket = errors.New("no primary account for websocket") ) -func (r Request) HeaderParam(name string) (string, *Error) { +func (r *Request) HeaderParam(name string) (string, *Error) { value := r.r.Header.Get(name) if value == "" { msg := fmt.Sprintf("Missing mandatory request header '%s'", name) @@ -89,19 +89,19 @@ func (r Request) HeaderParam(name string) (string, *Error) { } } -func (r Request) HeaderParamDoc(name string, _ string) (string, *Error) { +func (r *Request) HeaderParamDoc(name string, _ string) (string, *Error) { return r.HeaderParam(name) } -func (r Request) OptHeaderParam(name string) string { +func (r *Request) OptHeaderParam(name string) string { return r.r.Header.Get(name) } -func (r Request) OptHeaderParamDoc(name string, _ string) string { +func (r *Request) OptHeaderParamDoc(name string, _ string) string { return r.OptHeaderParam(name) } -func (r Request) PathParam(name string) (string, *Error) { +func (r *Request) PathParam(name string) (string, *Error) { value := chi.URLParam(r.r, name) if value == "" { msg := fmt.Sprintf("Missing mandatory path parameter '%s'", name) @@ -114,11 +114,11 @@ func (r Request) PathParam(name string) (string, *Error) { } } -func (r Request) PathParamDoc(name string, _ string) (string, *Error) { +func (r *Request) PathParamDoc(name string, _ string) (string, *Error) { return r.PathParam(name) } -func (r Request) PathListParamDoc(name string, _ string) ([]string, *Error) { +func (r *Request) PathListParamDoc(name string, _ string) ([]string, *Error) { value, err := r.PathParam(name) if err != nil { return nil, err @@ -126,14 +126,14 @@ func (r Request) PathListParamDoc(name string, _ string) ([]string, *Error) { return strings.Split(value, ","), nil } -func (r Request) AllAccountIds() []string { +func (r *Request) AllAccountIds() []string { // TODO potentially filter on "subscribed" accounts? return structs.Uniq(structs.Keys(r.session.Accounts)) } -func (r Request) GetAccountIdWithoutFallback() (string, *Error) { +func (r *Request) GetAccountIdWithoutFallback() (string, *Error) { accountId := chi.URLParam(r.r, UriParamAccountId) - if accountId == "" || isDefaultAccountid(accountId) { + if accountId == "" || isDefaultAccountId(accountId) { r.logger.Error().Err(errNoPrimaryAccountFallback).Msg("failed to determine the accountId") return "", apiError(r.errorId(), ErrorNonExistingAccount, withDetail("Failed to determine the account to use"), @@ -143,9 +143,9 @@ func (r Request) GetAccountIdWithoutFallback() (string, *Error) { return accountId, nil } -func (r Request) getAccountId(fallback string, err error) (string, *Error) { +func (r *Request) getAccountId(fallback string, err error) (string, *Error) { accountId := chi.URLParam(r.r, UriParamAccountId) - if accountId == "" || isDefaultAccountid(accountId) { + if accountId == "" || isDefaultAccountId(accountId) { accountId = fallback } if accountId == "" { @@ -158,45 +158,45 @@ func (r Request) getAccountId(fallback string, err error) (string, *Error) { return accountId, nil } -func (r Request) GetAccountIdForMail() (string, *Error) { +func (r *Request) GetAccountIdForMail() (string, *Error) { return r.getAccountId(r.session.PrimaryAccounts.Mail, errNoPrimaryAccountForMail) } -func (r Request) GetAccountIdForBlob() (string, *Error) { +func (r *Request) GetAccountIdForBlob() (string, *Error) { return r.getAccountId(r.session.PrimaryAccounts.Blob, errNoPrimaryAccountForBlob) } -func (r Request) GetAccountIdForVacationResponse() (string, *Error) { +func (r *Request) GetAccountIdForVacationResponse() (string, *Error) { return r.getAccountId(r.session.PrimaryAccounts.VacationResponse, errNoPrimaryAccountForVacationResponse) } -func (r Request) GetAccountIdForQuota() (string, *Error) { +func (r *Request) GetAccountIdForQuota() (string, *Error) { return r.getAccountId(r.session.PrimaryAccounts.Quota, errNoPrimaryAccountForQuota) } -func (r Request) GetAccountIdForSubmission() (string, *Error) { +func (r *Request) GetAccountIdForSubmission() (string, *Error) { return r.getAccountId(r.session.PrimaryAccounts.Blob, errNoPrimaryAccountForSubmission) } -func (r Request) GetAccountIdForTask() (string, *Error) { +func (r *Request) GetAccountIdForTask() (string, *Error) { // TODO we don't have these yet, not implemented in Stalwart // return r.getAccountId(r.session.PrimaryAccounts.Task, errNoPrimaryAccountForTask) return r.GetAccountIdForMail() } -func (r Request) GetAccountIdForCalendar() (string, *Error) { +func (r *Request) GetAccountIdForCalendar() (string, *Error) { // TODO we don't have these yet, not implemented in Stalwart // return r.getAccountId(r.session.PrimaryAccounts.Calendar, errNoPrimaryAccountForCalendar) return r.GetAccountIdForMail() } -func (r Request) GetAccountIdForContact() (string, *Error) { +func (r *Request) GetAccountIdForContact() (string, *Error) { // TODO we don't have these yet, not implemented in Stalwart // return r.getAccountId(r.session.PrimaryAccounts.Contact, errNoPrimaryAccountForContact) return r.GetAccountIdForMail() } -func (r Request) GetAccountForMail() (string, jmap.Account, *Error) { +func (r *Request) GetAccountForMail() (string, jmap.Account, *Error) { accountId, err := r.GetAccountIdForMail() if err != nil { return "", jmap.Account{}, err @@ -214,17 +214,17 @@ func (r Request) GetAccountForMail() (string, jmap.Account, *Error) { return accountId, account, nil } -func (r Request) parameterError(param string, detail string) *Error { +func (r *Request) parameterError(param string, detail string) *Error { return r.observedParameterError(ErrorInvalidRequestParameter, withDetail(detail), withSource(&ErrorSource{Parameter: param})) } -func (r Request) parameterErrorResponse(accountIds []string, param string, detail string) Response { +func (r *Request) parameterErrorResponse(accountIds []string, param string, detail string) Response { return errorResponse(accountIds, r.parameterError(param, detail)) } -func (r Request) getStringParam(param string, defaultValue string) (string, bool) { +func (r *Request) getStringParam(param string, defaultValue string) (string, bool) { q := r.r.URL.Query() if !q.Has(param) { return defaultValue, false @@ -236,7 +236,7 @@ func (r Request) getStringParam(param string, defaultValue string) (string, bool return str, true } -func (r Request) getMandatoryStringParam(param string) (string, *Error) { +func (r *Request) getMandatoryStringParam(param string) (string, *Error) { str := "" q := r.r.URL.Query() if q.Has(param) { @@ -252,7 +252,7 @@ func (r Request) getMandatoryStringParam(param string) (string, *Error) { return str, nil } -func (r Request) parseIntParam(param string, defaultValue int) (int, bool, *Error) { +func (r *Request) parseIntParam(param string, defaultValue int) (int, bool, *Error) { q := r.r.URL.Query() if !q.Has(param) { return defaultValue, false, nil @@ -276,7 +276,7 @@ func (r Request) parseIntParam(param string, defaultValue int) (int, bool, *Erro return int(value), true, nil } -func (r Request) parseUIntParam(param string, defaultValue uint) (uint, bool, *Error) { +func (r *Request) parseUIntParam(param string, defaultValue uint) (uint, bool, *Error) { q := r.r.URL.Query() if !q.Has(param) { return defaultValue, false, nil @@ -300,7 +300,7 @@ func (r Request) parseUIntParam(param string, defaultValue uint) (uint, bool, *E return uint(value), true, nil } -func (r Request) parseDateParam(param string) (time.Time, bool, *Error) { +func (r *Request) parseDateParam(param string) (time.Time, bool, *Error) { q := r.r.URL.Query() if !q.Has(param) { return time.Time{}, false, nil @@ -322,7 +322,7 @@ func (r Request) parseDateParam(param string) (time.Time, bool, *Error) { return t, true, nil } -func (r Request) parseBoolParam(param string, defaultValue bool) (bool, bool, *Error) { +func (r *Request) parseBoolParam(param string, defaultValue bool) (bool, bool, *Error) { q := r.r.URL.Query() if !q.Has(param) { return defaultValue, false, nil @@ -344,7 +344,7 @@ func (r Request) parseBoolParam(param string, defaultValue bool) (bool, bool, *E return b, true, nil } -func (r Request) parseMapParam(param string) (map[string]string, bool, *Error) { +func (r *Request) parseMapParam(param string) (map[string]string, bool, *Error) { q := r.r.URL.Query() if !q.Has(param) { return map[string]string{}, false, nil @@ -363,7 +363,7 @@ func (r Request) parseMapParam(param string) (map[string]string, bool, *Error) { return result, true, nil } -func (r Request) parseOptStringListParam(param string) ([]string, bool, *Error) { +func (r *Request) parseOptStringListParam(param string) ([]string, bool, *Error) { result := []string{} q := r.r.URL.Query() if !q.Has(param) { @@ -379,11 +379,11 @@ func (r Request) parseOptStringListParam(param string) ([]string, bool, *Error) return result, true, nil } -func (r Request) bodydoc(target any, _ string) *Error { +func (r *Request) bodydoc(target any, _ string) *Error { return r.body(target) } -func (r Request) body(target any) *Error { +func (r *Request) body(target any) *Error { body := r.r.Body defer func(b io.ReadCloser) { err := b.Close() @@ -400,30 +400,30 @@ func (r Request) body(target any) *Error { return nil } -func (r Request) language() string { +func (r *Request) language() string { return r.r.Header.Get("Accept-Language") } -func (r Request) observe(obs prometheus.Observer, value float64) { +func (r *Request) observe(obs prometheus.Observer, value float64) { metrics.WithExemplar(obs, value, r.GetRequestId(), r.GetTraceId()) } -func (r Request) observeParameterError(err *Error) *Error { +func (r *Request) observeParameterError(err *Error) *Error { if err != nil { r.g.metrics.ParameterErrorCounter.WithLabelValues(err.Code).Inc() } return err } -func (r Request) observeJmapError(jerr jmap.Error) jmap.Error { +func (r *Request) observeJmapError(jerr jmap.Error) jmap.Error { if jerr != nil { r.g.metrics.JmapErrorCounter.WithLabelValues(r.session.JmapEndpoint, strconv.Itoa(jerr.Code())).Inc() } return jerr } -func (r Request) needTask(accountId string) (bool, Response) { - if !IgnoreSessionCapabilityChecks { +func (r *Request) needTask(accountId string) (bool, Response) { + if !IgnoreSessionCapabilityChecksForTasks { if r.session.Capabilities.Tasks == nil { return false, errorResponseWithSessionState(single(accountId), r.apiError(&ErrorMissingTasksSessionCapability), r.session.State) } @@ -431,7 +431,7 @@ func (r Request) needTask(accountId string) (bool, Response) { return true, Response{} } -func (r Request) needTaskForAccount(accountId string) (bool, Response) { +func (r *Request) needTaskForAccount(accountId string) (bool, Response) { if ok, resp := r.needTask(accountId); !ok { return ok, resp } @@ -439,37 +439,31 @@ func (r Request) needTaskForAccount(accountId string) (bool, Response) { if !ok { return false, errorResponseWithSessionState(single(accountId), r.apiError(&ErrorAccountNotFound), r.session.State) } - if !IgnoreSessionCapabilityChecks { - if account.AccountCapabilities.Tasks == nil { - return false, errorResponseWithSessionState(single(accountId), r.apiError(&ErrorMissingTasksAccountCapability), r.session.State) - } + if account.AccountCapabilities.Tasks == nil { + return false, errorResponseWithSessionState(single(accountId), r.apiError(&ErrorMissingTasksAccountCapability), r.session.State) } return true, Response{} } -func (r Request) needTaskWithAccount() (bool, string, Response) { +func (r *Request) needTaskWithAccount() (bool, string, Response) { accountId, err := r.GetAccountIdForTask() if err != nil { return false, "", errorResponse(single(accountId), err) } - if !IgnoreSessionCapabilityChecks { - if ok, resp := r.needTaskForAccount(accountId); !ok { - return false, accountId, resp - } + if ok, resp := r.needTaskForAccount(accountId); !ok { + return false, accountId, resp } return true, accountId, Response{} } -func (r Request) needCalendar(accountId string) (bool, Response) { - if !IgnoreSessionCapabilityChecks { - if r.session.Capabilities.Calendars == nil { - return false, errorResponseWithSessionState(single(accountId), r.apiError(&ErrorMissingCalendarsSessionCapability), r.session.State) - } +func (r *Request) needCalendar(accountId string) (bool, Response) { + if r.session.Capabilities.Calendars == nil { + return false, errorResponseWithSessionState(single(accountId), r.apiError(&ErrorMissingCalendarsSessionCapability), r.session.State) } return true, Response{} } -func (r Request) needCalendarForAccount(accountId string) (bool, Response) { +func (r *Request) needCalendarForAccount(accountId string) (bool, Response) { if ok, resp := r.needCalendar(accountId); !ok { return ok, resp } @@ -477,35 +471,31 @@ func (r Request) needCalendarForAccount(accountId string) (bool, Response) { if !ok { return false, errorResponseWithSessionState(single(accountId), r.apiError(&ErrorAccountNotFound), r.session.State) } - if !IgnoreSessionCapabilityChecks { - if account.AccountCapabilities.Calendars == nil { - return false, errorResponseWithSessionState(single(accountId), r.apiError(&ErrorMissingCalendarsAccountCapability), r.session.State) - } + if account.AccountCapabilities.Calendars == nil { + return false, errorResponseWithSessionState(single(accountId), r.apiError(&ErrorMissingCalendarsAccountCapability), r.session.State) } return true, Response{} } -func (r Request) needCalendarWithAccount() (bool, string, Response) { +func (r *Request) needCalendarWithAccount() (bool, string, Response) { accountId, err := r.GetAccountIdForCalendar() if err != nil { return false, "", errorResponse(single(accountId), err) } - if !IgnoreSessionCapabilityChecks { - if ok, resp := r.needCalendarForAccount(accountId); !ok { - return false, accountId, resp - } + if ok, resp := r.needCalendarForAccount(accountId); !ok { + return false, accountId, resp } return true, accountId, Response{} } -func (r Request) needContact(accountId string) (bool, Response) { +func (r *Request) needContact(accountId string) (bool, Response) { if r.session.Capabilities.Contacts == nil { return false, errorResponseWithSessionState(single(accountId), r.apiError(&ErrorMissingContactsSessionCapability), r.session.State) } return true, Response{} } -func (r Request) needContactForAccount(accountId string) (bool, Response) { +func (r *Request) needContactForAccount(accountId string) (bool, Response) { if ok, resp := r.needContact(accountId); !ok { return ok, resp } @@ -519,7 +509,7 @@ func (r Request) needContactForAccount(accountId string) (bool, Response) { return true, Response{} } -func (r Request) needContactWithAccount() (bool, string, Response) { +func (r *Request) needContactWithAccount() (bool, string, Response) { accountId, err := r.GetAccountIdForContact() if err != nil { return false, "", errorResponse(single(accountId), err) diff --git a/services/groupware/pkg/groupware/reva.go b/services/groupware/pkg/groupware/reva.go index 41335c66b3..d7f3ef7f3c 100644 --- a/services/groupware/pkg/groupware/reva.go +++ b/services/groupware/pkg/groupware/reva.go @@ -23,14 +23,14 @@ func newRevaContextUsernameProvider() userProvider { } var ( - errUserNotInRevaContext = errors.New("failed to find user in reva context") + errUserNotInRevaContext = errors.New("failed to find user in Reva context") ) -func (r revaContextUsernameProvider) GetUser(req *http.Request, ctx context.Context, logger *log.Logger) (user, error) { +func (r revaContextUsernameProvider) GetUser(_ *http.Request, ctx context.Context, logger *log.Logger) (user, error) { u, ok := revactx.ContextGetUser(ctx) if !ok { err := errUserNotInRevaContext - logger.Error().Err(err).Ctx(ctx).Msgf("could not get user: user not in reva context: %v", ctx) + logger.Error().Err(err).Ctx(ctx).Msgf("could not get user: user not in Reva context: %v", ctx) return nil, err } return revaUser{user: u}, nil @@ -86,14 +86,14 @@ const ( var tokenMissingInRevaContext = RevaError{ code: revaErrorTokenMissingInRevaContext, - err: errors.New("Token is missing from Reva context"), + err: errors.New("token is missing from Reva context"), } -func (h *RevaBearerHttpJmapClientAuthenticator) Authenticate(ctx context.Context, username string, logger *log.Logger, req *http.Request) jmap.Error { +func (h *RevaBearerHttpJmapClientAuthenticator) Authenticate(ctx context.Context, _ string, logger *log.Logger, req *http.Request) jmap.Error { token, ok := revactx.ContextGetToken(ctx) if !ok { err := tokenMissingInRevaContext - logger.Error().Err(err).Ctx(ctx).Msgf("could not get token: token not in reva context: %v", ctx) + logger.Error().Err(err).Ctx(ctx).Msgf("could not get token: token not in Reva context: %v", ctx) return err } else { req.Header.Add("Authorization", "Bearer "+token) @@ -101,11 +101,11 @@ func (h *RevaBearerHttpJmapClientAuthenticator) Authenticate(ctx context.Context } } -func (h *RevaBearerHttpJmapClientAuthenticator) AuthenticateWS(ctx context.Context, username string, logger *log.Logger, headers http.Header) jmap.Error { +func (h *RevaBearerHttpJmapClientAuthenticator) AuthenticateWS(ctx context.Context, _ string, logger *log.Logger, headers http.Header) jmap.Error { token, ok := revactx.ContextGetToken(ctx) if !ok { err := tokenMissingInRevaContext - logger.Error().Err(err).Ctx(ctx).Msgf("could not get token: token not in reva context: %v", ctx) + logger.Error().Err(err).Ctx(ctx).Msgf("could not get token: token not in Reva context: %v", ctx) return err } else { headers.Add("Authorization", "Bearer "+token) diff --git a/services/groupware/pkg/groupware/route.go b/services/groupware/pkg/groupware/route.go index 08f21bc0d1..0184c5741a 100644 --- a/services/groupware/pkg/groupware/route.go +++ b/services/groupware/pkg/groupware/route.go @@ -117,7 +117,7 @@ func (g *Groupware) Route(r chi.Router) { r.Post("/", g.SendEmail) r.Patch("/", g.UpdateEmail) r.Delete("/", g.DeleteEmail) - Report(r, "/", g.RelatedToEmail) + report(r, "/", g.RelatedToEmail) r.Route("/related", func(r chi.Router) { r.Get("/", g.RelatedToEmail) }) @@ -178,6 +178,6 @@ func (g *Groupware) Route(r chi.Router) { r.MethodNotAllowed(g.MethodNotAllowed) } -func Report(r chi.Router, pattern string, h http.HandlerFunc) { +func report(r chi.Router, pattern string, h http.HandlerFunc) { r.MethodFunc("REPORT", pattern, h) } diff --git a/services/groupware/pkg/groupware/session.go b/services/groupware/pkg/groupware/session.go index 5e1918ac8a..2ef0aaa86b 100644 --- a/services/groupware/pkg/groupware/session.go +++ b/services/groupware/pkg/groupware/session.go @@ -116,25 +116,25 @@ type ttlcacheSessionCache struct { var _ sessionCache = &ttlcacheSessionCache{} var _ jmap.SessionEventListener = &ttlcacheSessionCache{} -func (l *ttlcacheSessionCache) load(key sessionCacheKey, ctx context.Context) cachedSession { +func (c *ttlcacheSessionCache) load(key sessionCacheKey, ctx context.Context) cachedSession { username := key.username() - sessionUrl, gwerr := l.sessionUrlProvider(ctx, username) + sessionUrl, gwerr := c.sessionUrlProvider(ctx, username) if gwerr != nil { - l.logger.Warn().Str(logUsername, username).Str(logErrorCode, gwerr.Code).Msgf("failed to determine session URL for '%v'", key) + c.logger.Warn().Str(logUsername, username).Str(logErrorCode, gwerr.Code).Msgf("failed to determine session URL for '%v'", key) now := time.Now() - until := now.Add(l.errorTtl) + until := now.Add(c.errorTtl) return failedSession{since: now, until: until, err: gwerr} } - session, jerr := l.sessionSupplier(ctx, sessionUrl, username, l.logger) + session, jerr := c.sessionSupplier(ctx, sessionUrl, username, c.logger) if jerr != nil { - l.logger.Warn().Str(logUsername, username).Err(jerr).Msgf("failed to create session for '%v'", key) + c.logger.Warn().Str(logUsername, username).Err(jerr).Msgf("failed to create session for '%v'", key) now := time.Now() - until := now.Add(l.errorTtl) + until := now.Add(c.errorTtl) return failedSession{since: now, until: until, err: groupwareErrorFromJmap(jerr)} } else { - l.logger.Debug().Str(logUsername, username).Msgf("successfully created session for '%v'", key) + c.logger.Debug().Str(logUsername, username).Msgf("successfully created session for '%v'", key) now := time.Now() - until := now.Add(l.successTtl) + until := now.Add(c.successTtl) return succeededSession{since: now, until: until, session: session} } } @@ -229,7 +229,7 @@ func (b *sessionCacheBuilder) withDnsAutoDiscovery( return b } -func (b sessionCacheBuilder) build() (sessionCache, error) { +func (b *sessionCacheBuilder) build() (sessionCache, error) { var cache *ttlcache.Cache[sessionCacheKey, cachedSession] sessionUrlResolver, err := b.sessionUrlResolverFactory() @@ -243,7 +243,9 @@ func (b sessionCacheBuilder) build() (sessionCache, error) { ttlcache.WithDisableTouchOnHit[sessionCacheKey, cachedSession](), ) - b.prometheusRegistry.Register(sessionCacheMetricsCollector{desc: b.m.SessionCacheDesc, supply: cache.Metrics}) + if err := b.prometheusRegistry.Register(sessionCacheMetricsCollector{desc: b.m.SessionCacheDesc, supply: cache.Metrics}); err != nil { + b.logger.Error().Err(err).Msg("failed to register session cache metrics") + } cache.OnEviction(func(c context.Context, r ttlcache.EvictionReason, item *ttlcache.Item[sessionCacheKey, cachedSession]) { if b.logger.Trace().Enabled() { @@ -291,7 +293,7 @@ func (b sessionCacheBuilder) build() (sessionCache, error) { return s, nil } -func (c ttlcacheSessionCache) OnSessionOutdated(session *jmap.Session, newSessionState jmap.SessionState) { +func (c *ttlcacheSessionCache) OnSessionOutdated(session *jmap.Session, newSessionState jmap.SessionState) { // it's enough to remove the session from the cache, as it will be fetched on-demand // the next time an operation is performed on behalf of the user c.sessionCache.Delete(toSessionCacheKey(session.Username)) diff --git a/services/groupware/pkg/metrics/metrics.go b/services/groupware/pkg/metrics/metrics.go index 02ac85f4fd..08828efdb7 100644 --- a/services/groupware/pkg/metrics/metrics.go +++ b/services/groupware/pkg/metrics/metrics.go @@ -324,7 +324,9 @@ func (r *LoggingPrometheusRegisterer) Register(c prometheus.Collector) error { func (r *LoggingPrometheusRegisterer) MustRegister(collectors ...prometheus.Collector) { for _, c := range collectors { - r.Register(c) + if err := r.Register(c); err != nil { + r.logger.Error().Err(err).Msg("failed to register metrics collector") + } } } diff --git a/services/groupware/pkg/metrics/startup_metrics.go b/services/groupware/pkg/metrics/startup_metrics.go index f762286243..e67d8ba67d 100644 --- a/services/groupware/pkg/metrics/startup_metrics.go +++ b/services/groupware/pkg/metrics/startup_metrics.go @@ -3,19 +3,20 @@ package metrics import ( "sync/atomic" + "github.com/opencloud-eu/opencloud/pkg/log" "github.com/opencloud-eu/opencloud/pkg/version" "github.com/prometheus/client_golang/prometheus" ) var registered atomic.Bool -func StartupMetrics(registerer prometheus.Registerer) { +func StartupMetrics(registerer prometheus.Registerer, logger *log.Logger) { // use an atomic boolean to make the operation idempotent, // instead of causing a panic in case this function is // called twice if registered.CompareAndSwap(false, true) { // https://github.com/prometheus/common/blob/8558a5b7db3c84fa38b4766966059a7bd5bfa2ee/version/info.go#L36-L56 - registerer.Register(prometheus.NewGaugeFunc(prometheus.GaugeOpts{ + if err := registerer.Register(prometheus.NewGaugeFunc(prometheus.GaugeOpts{ Namespace: Namespace, Subsystem: Subsystem, Name: "build_info", @@ -23,6 +24,8 @@ func StartupMetrics(registerer prometheus.Registerer) { ConstLabels: prometheus.Labels{ "version": version.GetString(), }, - }, func() float64 { return 1 })) + }, func() float64 { return 1 })); err != nil { + logger.Error().Err(err).Msg("failed to register startup metrics") + } } } diff --git a/services/groupware/pkg/service/http/v0/service.go b/services/groupware/pkg/service/http/v0/service.go index 432c7cece7..76c6d9a5d8 100644 --- a/services/groupware/pkg/service/http/v0/service.go +++ b/services/groupware/pkg/service/http/v0/service.go @@ -58,7 +58,7 @@ func NewService(opts ...Option) (Service, error) { } } - metrics.StartupMetrics(registerer) + metrics.StartupMetrics(registerer, logger) return gw, nil }