From 279f8a2ab9854db3c2765a98ff417354846b2131 Mon Sep 17 00:00:00 2001 From: Pascal Bleser Date: Thu, 9 Jul 2026 14:12:00 +0200 Subject: [PATCH] groupware: upgrade Stalwart to 0.16.12 + fix modifying the VacationResponse * fix jmap.SetVacationResponse: was using JMAP create instead of update with a patch; did so by using the modify template function as with most other JMAP APIs * better HTTP status code handling for the responses received from Stalwart * add support for not caching JMAP session retrieval failures in the Groupware backend, by setting the error TTL to 0 --- pkg/jmap/api_vacation.go | 69 ++++--------------- pkg/jmap/error.go | 10 +++ pkg/jmap/http.go | 15 ++-- pkg/jmap/model.go | 2 +- pkg/jmap/model_examples.go | 2 +- pkg/jmaptest/jmap_stalwart.go | 2 +- .../groupware/pkg/groupware/api_index_test.go | 3 +- .../pkg/groupware/api_vacation_test.go | 45 ++++++++++-- .../groupware/groupware_integration_test.go | 45 +++++++++--- services/groupware/pkg/groupware/session.go | 6 +- 10 files changed, 111 insertions(+), 88 deletions(-) diff --git a/pkg/jmap/api_vacation.go b/pkg/jmap/api_vacation.go index f80a4ec41a..fe0513e4c4 100644 --- a/pkg/jmap/api_vacation.go +++ b/pkg/jmap/api_vacation.go @@ -1,7 +1,6 @@ package jmap import ( - "fmt" "time" ) @@ -26,7 +25,7 @@ func (j *Client) GetVacationResponse(accountId AccountId, ctx Context) (Result[V // Same as VacationResponse but without the id. type VacationResponseChange struct { // Should a vacation response be sent if a message arrives between the "fromDate" and "toDate"? - IsEnabled bool `json:"isEnabled"` + IsEnabled *bool `json:"isEnabled,omitempty"` // If "isEnabled" is true, messages that arrive on or after this date-time (but before the "toDate" if defined) should receive the // user's vacation response. If null, the vacation response is effective immediately. FromDate *time.Time `json:"fromDate,omitzero"` @@ -68,62 +67,18 @@ func (c VacationResponseChanges) GetCreated() []VacationResponse { return c.Crea func (c VacationResponseChanges) GetUpdated() []VacationResponse { return c.Updated } func (c VacationResponseChanges) GetDestroyed() []string { return c.Destroyed } -func (j *Client) SetVacationResponse(accountId AccountId, vacation VacationResponseChange, +func (j *Client) SetVacationResponse(accountId AccountId, change VacationResponseChange, ctx Context) (Result[VacationResponse], error) { - logger := j.logger("SetVacationResponse", ctx) - ctx = ctx.WithLogger(logger) - - set := VacationResponseSetCommand{ - AccountId: accountId, - Create: map[string]VacationResponse{ - vacationResponseId: { - IsEnabled: vacation.IsEnabled, - FromDate: vacation.FromDate, - ToDate: vacation.ToDate, - Subject: vacation.Subject, - TextBody: vacation.TextBody, - HtmlBody: vacation.HtmlBody, - }, + return update(j, "SetVacationResponse", VacationResponseType, + func(update map[string]PatchObject) VacationResponseSetCommand { + return VacationResponseSetCommand{AccountId: accountId, Update: update} }, - } - - get := VacationResponseGetCommand{AccountId: accountId} - - cmd, err := j.request(ctx, NS_VACATION, - invocation(set, "0"), - // chain a second request to get the current complete VacationResponse object - // after performing the changes, as that makes for a better API - invocation(get, "1"), + func(_ string) VacationResponseGetCommand { + return VacationResponseGetCommand{AccountId: accountId} + }, + func(resp VacationResponseSetResponse) map[string]SetError { return resp.NotUpdated }, + func(resp VacationResponseGetResponse) VacationResponse { return resp.List[0] }, + vacationResponseId, change, + ctx, ) - if err != nil { - return ZeroResultV[VacationResponse](), err - } - return command(j, Operation("SetVacationResponse"), ctx, cmd, func(body *Response) (VacationResponse, State, Error) { - var setResponse VacationResponseSetResponse - err = retrieveSet(ctx, body, set, "0", &setResponse) - if err != nil { - return VacationResponse{}, "", err - } - - setErr, notok := setResponse.NotCreated[vacationResponseId] - if notok { - // this means that the VacationResponse was not updated - logger.Error().Msgf("%T.NotCreated contains an error: %v", setResponse, setErr) - return VacationResponse{}, "", setErrorError(setErr, VacationResponseType) - } - - var getResponse VacationResponseGetResponse - err = retrieveGet(ctx, body, get, "1", &getResponse) - if err != nil { - return VacationResponse{}, "", err - } - - if len(getResponse.List) != 1 { - berr := fmt.Errorf("failed to find %s in %s response", VacationResponseType, string(CommandVacationResponseGet)) - logger.Error().Msg(berr.Error()) - return VacationResponse{}, "", jmapError(berr, JmapErrorInvalidJmapResponsePayload) - } - - return getResponse.List[0], setResponse.NewState, nil - }) } diff --git a/pkg/jmap/error.go b/pkg/jmap/error.go index 1274256249..073268728f 100644 --- a/pkg/jmap/error.go +++ b/pkg/jmap/error.go @@ -3,6 +3,7 @@ package jmap import ( "errors" "fmt" + "net/http" "strings" ) @@ -84,6 +85,15 @@ func (e JmapError) Description() string { return e.description } +func jmapErrorCode(statusCode int) int { + code := JmapErrorServerResponse + switch statusCode { + case http.StatusUnauthorized: + code = JmapErrorAuthenticationFailed + } + return code +} + func jmapError(err error, code int) Error { if err != nil { return JmapError{code: code, err: err} diff --git a/pkg/jmap/http.go b/pkg/jmap/http.go index 8910f5e676..edbeb65b81 100644 --- a/pkg/jmap/http.go +++ b/pkg/jmap/http.go @@ -298,7 +298,7 @@ func (h *HttpJmapClient) GetSession(ctx context.Context, sessionUrl *url.URL, us if res.StatusCode < 200 || res.StatusCode > 299 { h.listener.OnFailedRequestWithStatus(endpoint, Operation("GetSession"), res.StatusCode) logger.Error().Str(logHttpStatus, log.SafeString(res.Status)).Int(logHttpStatusCode, res.StatusCode).Msg("HTTP response status code is not 200") - return SessionResponse{}, jmapError(fmt.Errorf("JMAP API response status is %v", res.Status), JmapErrorServerResponse) + return SessionResponse{}, jmapError(fmt.Errorf("JMAP API response status is %v", res.Status), jmapErrorCode(res.StatusCode)) } h.listener.OnSuccessfulRequest(endpoint, Operation("GetSession"), res.StatusCode) @@ -383,12 +383,7 @@ func (h *HttpJmapClient) Command(operation Operation, request Request, ctx Conte if res.StatusCode < 200 || res.StatusCode > 299 { h.listener.OnFailedRequestWithStatus(endpoint, operation, res.StatusCode) logger.Error().Str(logEndpoint, endpoint).Str(logHttpStatus, log.SafeString(res.Status)).Msg("HTTP response status code is not 2xx") //NOSONAR - switch res.StatusCode { - case http.StatusUnauthorized: - return nil, language, jmapError(fmt.Errorf("JMAP server responsed with '%s'", res.Status), JmapErrorAuthenticationFailed) - default: - return nil, language, jmapError(fmt.Errorf("JMAP server responsed with '%s'", res.Status), JmapErrorServerResponse) - } + return nil, language, jmapError(fmt.Errorf("JMAP server responsed with '%s'", res.Status), jmapErrorCode(res.StatusCode)) } h.listener.OnSuccessfulRequest(endpoint, operation, res.StatusCode) @@ -450,7 +445,7 @@ func (h *HttpJmapClient) UploadBinary(uploadUrl string, operation Operation, end if res.StatusCode < 200 || res.StatusCode > 299 { h.listener.OnFailedRequestWithStatus(endpoint, operation, res.StatusCode) logger.Error().Str(logHttpStatus, log.SafeString(res.Status)).Int(logHttpStatusCode, res.StatusCode).Msg("HTTP response status code is not 2xx") - return UploadedBlob{}, language, jmapError(err, JmapErrorServerResponse) + return UploadedBlob{}, language, jmapError(err, jmapErrorCode(res.StatusCode)) } h.listener.OnSuccessfulRequest(endpoint, operation, res.StatusCode) @@ -518,7 +513,7 @@ func (h *HttpJmapClient) DownloadBinary(downloadUrl string, operation Operation, if res.StatusCode < 200 || res.StatusCode > 299 { h.listener.OnFailedRequestWithStatus(endpoint, operation, res.StatusCode) logger.Error().Str(logHttpStatus, log.SafeString(res.Status)).Int(logHttpStatusCode, res.StatusCode).Msg("HTTP response status code is not 2xx") - return nil, language, jmapError(err, JmapErrorServerResponse) + return nil, language, jmapError(err, jmapErrorCode(res.StatusCode)) } h.listener.OnSuccessfulRequest(endpoint, operation, res.StatusCode) @@ -662,7 +657,7 @@ func (w *HttpWsClientFactory) connect(operation Operation, ctx context.Context, if res.StatusCode != 101 { w.eventListener.OnFailedRequestWithStatus(endpoint, operation, res.StatusCode) logger.Error().Str(logHttpStatus, log.SafeString(res.Status)).Int(logHttpStatusCode, res.StatusCode).Msg("HTTP response status code is not 101") - return nil, "", endpoint, jmapError(fmt.Errorf("JMAP WS API response status is %v", res.Status), JmapErrorServerResponse) + return nil, "", endpoint, jmapError(fmt.Errorf("JMAP WS API response status is %v", res.Status), jmapErrorCode(res.StatusCode)) } else { w.eventListener.OnSuccessfulWsRequest(endpoint, operation, res.StatusCode) } diff --git a/pkg/jmap/model.go b/pkg/jmap/model.go index 9f28950841..60f341ebce 100644 --- a/pkg/jmap/model.go +++ b/pkg/jmap/model.go @@ -4159,7 +4159,7 @@ type VacationResponse struct { Id string `json:"id,omitempty"` // Should a vacation response be sent if a message arrives between the "fromDate" and "toDate"? - IsEnabled bool `json:"isEnabled"` + IsEnabled *bool `json:"isEnabled,omitempty"` // If "isEnabled" is true, messages that arrive on or after this date-time (but before the "toDate" if defined) should receive the // user's vacation response. If null, the vacation response is effective immediately. diff --git a/pkg/jmap/model_examples.go b/pkg/jmap/model_examples.go index 4065ccad7d..db1afc923c 100644 --- a/pkg/jmap/model_examples.go +++ b/pkg/jmap/model_examples.go @@ -882,7 +882,7 @@ func (e Exemplar) VacationResponse() VacationResponse { to, _ := time.Parse(time.RFC3339, "20260114T23:59:59.999Z") return VacationResponse{ Id: "aefee7ae", - IsEnabled: true, + IsEnabled: ptr(true), FromDate: &from, ToDate: &to, Subject: "On PTO", diff --git a/pkg/jmaptest/jmap_stalwart.go b/pkg/jmaptest/jmap_stalwart.go index ddf78a9050..52fdb3fe87 100644 --- a/pkg/jmaptest/jmap_stalwart.go +++ b/pkg/jmaptest/jmap_stalwart.go @@ -114,7 +114,7 @@ var ( ) const ( - StalwartVersion = "0.16.9" + StalwartVersion = "0.16.12" StalwartCliVersion = "1.0.8" stalwartImageTemplate = "ghcr.io/stalwartlabs/stalwart:v%s-alpine" diff --git a/services/groupware/pkg/groupware/api_index_test.go b/services/groupware/pkg/groupware/api_index_test.go index f5c4529a23..a1d662ee99 100644 --- a/services/groupware/pkg/groupware/api_index_test.go +++ b/services/groupware/pkg/groupware/api_index_test.go @@ -12,9 +12,10 @@ func TestGroupwareIndex(t *testing.T) { require := require.New(t) g, err := newGroupwareTest(t) require.NoError(err) + u := g.user() index := IndexResponse{} - u := gget("get-index", g, "/", &index) + gget(g, "get-index", u, "/", &index) require.Len(index.Accounts, 1) require.Equal(u.Email, index.Accounts[0].Name) require.Len(index.Accounts[0].Identities, 2) // email + alias diff --git a/services/groupware/pkg/groupware/api_vacation_test.go b/services/groupware/pkg/groupware/api_vacation_test.go index 96ec2fe019..70c7f8e85e 100644 --- a/services/groupware/pkg/groupware/api_vacation_test.go +++ b/services/groupware/pkg/groupware/api_vacation_test.go @@ -2,6 +2,7 @@ package groupware import ( "testing" + "time" "github.com/opencloud-eu/opencloud/pkg/jmap" "github.com/stretchr/testify/require" @@ -11,12 +12,13 @@ func TestGroupwareVacation(t *testing.T) { require := require.New(t) g, err := newGroupwareTest(t) require.NoError(err) + u := g.user() { resp := jmap.VacationResponse{} - gget("get-initial-vacation", g, "/accounts/_/vacation", &resp) + gget(g, "get-initial-vacation", u, "/accounts/_/vacation", &resp) require.Equal("singleton", resp.Id) - require.False(resp.IsEnabled) + require.False(*resp.IsEnabled) require.Empty(resp.Subject) require.Empty(resp.TextBody) require.Empty(resp.HtmlBody) @@ -27,18 +29,51 @@ func TestGroupwareVacation(t *testing.T) { { resp := jmap.VacationResponse{} req := jmap.VacationResponseChange{ - IsEnabled: true, + IsEnabled: ptr(true), Subject: "testing", TextBody: "text", HtmlBody: "html", } - gput("set-vacation", g, "/accounts/_/vacation", req, &resp) + gput(g, "set-vacation", u, "/accounts/_/vacation", req, &resp) require.Equal("singleton", resp.Id) - require.True(resp.IsEnabled) + require.True(*resp.IsEnabled) require.Equal("testing", resp.Subject) require.Equal("text", resp.TextBody) require.Equal("html", resp.HtmlBody) require.Nil(resp.FromDate) require.Nil(resp.ToDate) } + + { + resp := jmap.VacationResponse{} + gget(g, "get-initial-vacation-after-change", u, "/accounts/_/vacation", &resp) + require.Equal("singleton", resp.Id) + require.True(*resp.IsEnabled) + require.Equal("testing", resp.Subject) + require.Equal("text", resp.TextBody) + require.Equal("html", resp.HtmlBody) + require.Nil(resp.FromDate) + require.Nil(resp.ToDate) + } + + { + resp := jmap.VacationResponse{} + from, err := time.Parse(time.RFC3339, "2026-07-08T00:00:00Z") + require.NoError(err) + to, err := time.Parse(time.RFC3339, "2026-07-18T23:59:59.999Z") + require.NoError(err) + req := jmap.VacationResponseChange{ + IsEnabled: nil, + FromDate: &from, + ToDate: &to, + } + gput(g, "set-vacation", u, "/accounts/_/vacation", req, &resp) + require.Equal("singleton", resp.Id) + require.False(*resp.IsEnabled) // TODO should be true: this is a bug in Stalwart up to 0.16.12, see https://support.stalw.art/t/vacationresponse-jmap-api-isenabled-reset-to-false-whenever-properties-are-changed/992 + require.Equal("testing", resp.Subject) + require.Equal("text", resp.TextBody) + require.Equal("html", resp.HtmlBody) + require.Equal("2026-07-08T00:00:00Z", resp.FromDate.Format(time.RFC3339)) + require.Equal("2026-07-18T23:59:59Z", resp.ToDate.Format(time.RFC3339)) // for some reason the .999 gets lost in Stalwart (?), but nothing critical + } } diff --git a/services/groupware/pkg/groupware/groupware_integration_test.go b/services/groupware/pkg/groupware/groupware_integration_test.go index 037a004825..d7be6b3b5c 100644 --- a/services/groupware/pkg/groupware/groupware_integration_test.go +++ b/services/groupware/pkg/groupware/groupware_integration_test.go @@ -11,19 +11,20 @@ import ( "net/url" "strings" "testing" + "time" userv1beta1 "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" "github.com/go-chi/chi/v5" "github.com/go-chi/chi/v5/middleware" "github.com/google/uuid" "github.com/opencloud-eu/opencloud/pkg/cors" + ochttp "github.com/opencloud-eu/opencloud/pkg/http" "github.com/opencloud-eu/opencloud/pkg/jmaptest" opencloudmiddleware "github.com/opencloud-eu/opencloud/pkg/middleware" "github.com/opencloud-eu/opencloud/pkg/shared" "github.com/opencloud-eu/opencloud/pkg/structs" "github.com/opencloud-eu/opencloud/services/groupware/pkg/config" "github.com/opencloud-eu/opencloud/services/groupware/pkg/logging" - groupwaremiddleware "github.com/opencloud-eu/opencloud/services/groupware/pkg/middleware" revactx "github.com/opencloud-eu/reva/v2/pkg/ctx" "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/require" @@ -39,29 +40,40 @@ type GroupwareTest struct { Users []jmaptest.User } -func gget[T any](id string, g GroupwareTest, path string, result *T) jmaptest.User { +// pick a user at random +func (g GroupwareTest) user() jmaptest.User { + return g.Users[rand.Intn(len(g.Users))] +} + +func gget[T any](g GroupwareTest, id string, user jmaptest.User, path string, result *T) { id = g.t.Name() + "/" + id u, err := url.JoinPath(g.BaseURL, path) require.NoError(g.t, err) req, err := http.NewRequest(http.MethodGet, u, nil) require.NoError(g.t, err) - user := g.Users[rand.Intn(len(g.Users))] // pick a user at random req.SetBasicAuth(user.Name, user.Password) rid := uuid.New().String() req.Header.Add("X-Request-Id", rid) req.Header.Add("Trace-Id", rid) client := http.Client{} - resp, err := client.Do(req) - require.NoError(g.t, err) + var resp *http.Response = nil + require.Eventually(g.t, func() bool { + r, err := client.Do(req) + resp = r + if err == nil && r.StatusCode == 200 { + resp = r + return true + } + return false + }, time.Duration(5*time.Second), time.Duration(1*time.Second)) require.Equal(g.t, 200, resp.StatusCode) defer resp.Body.Close() err = json.NewDecoder(resp.Body).Decode(result) require.NoError(g.t, err) - return user } -func gput[B any, T any](id string, g GroupwareTest, path string, body B, result *T) jmaptest.User { +func gput[B any, T any](g GroupwareTest, id string, user jmaptest.User, path string, body B, result *T) { id = g.t.Name() + "/" + id u, err := url.JoinPath(g.BaseURL, path) @@ -72,7 +84,6 @@ func gput[B any, T any](id string, g GroupwareTest, path string, body B, result req, err := http.NewRequest(http.MethodPut, u, bytes.NewBuffer(jsonBody)) require.NoError(g.t, err) - user := g.Users[rand.Intn(len(g.Users))] // pick a user at random req.SetBasicAuth(user.Name, user.Password) rid := uuid.New().String() req.Header.Add("X-Request-Id", id) @@ -84,7 +95,6 @@ func gput[B any, T any](id string, g GroupwareTest, path string, body B, result defer resp.Body.Close() err = json.NewDecoder(resp.Body).Decode(result) require.NoError(g.t, err) - return user } func newGroupwareTest(t *testing.T) (GroupwareTest, error) { @@ -125,7 +135,9 @@ func newGroupwareTest(t *testing.T) (GroupwareTest, error) { }, BaseUrl: s.JmapBaseUrl.String(), SessionCache: config.MailSessionCache{ - MaxCapacity: 1, + MaxCapacity: 10, + Ttl: time.Duration(30 * time.Minute), + FailureTtl: time.Duration(0), }, }, HTTP: config.HTTP{ @@ -188,7 +200,18 @@ func newGroupwareTest(t *testing.T) (GroupwareTest, error) { }) } + deeplog := func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + ochttp.DumpHttpRequest(r, 4*1024, func(method string, uri string, content string, truncated bool) { + logger.Trace().Str("method", method).Str("uri", uri).Msg(content) + t.Logf("--> Groupware Request: %s %s:\n%s", method, uri, content) + }) + next.ServeHTTP(w, r) + }) + } + m.Use( + deeplog, middleware.RequestID, opencloudmiddleware.Cors( cors.Logger(logger), @@ -197,7 +220,7 @@ func newGroupwareTest(t *testing.T) (GroupwareTest, error) { cors.AllowedHeaders(config.HTTP.CORS.AllowedHeaders), cors.AllowCredentials(config.HTTP.CORS.AllowCredentials), ), - groupwaremiddleware.GroupwareLogger(logger), + // groupwaremiddleware.GroupwareLogger(logger), auth, ) m.Route(config.HTTP.Root, gw.Route) diff --git a/services/groupware/pkg/groupware/session.go b/services/groupware/pkg/groupware/session.go index f29ed7b7f9..98c1754b63 100644 --- a/services/groupware/pkg/groupware/session.go +++ b/services/groupware/pkg/groupware/session.go @@ -156,7 +156,11 @@ func (c *ttlcacheSessionCache) Get(ctx context.Context, username string) cachedS // not great, but will do for now: // - when the result is successful, the default TTL is used // - when the result is a failure to retrieve the session, a (most probably shorter) TTL must be used instead - c.sessionCache.Set(key, value, c.errorTtl) + if c.errorTtl == 0 { + c.sessionCache.Delete(key) + } else { + c.sessionCache.Set(key, value, c.errorTtl) + } } } return value