From 331417933e538f47c02e670f26712b40136b1128 Mon Sep 17 00:00:00 2001 From: Anton Shepilov Date: Wed, 1 Apr 2026 17:18:35 +0200 Subject: [PATCH] fix: avoid races during interactive drive acceptance --- model/sharing/oauth.go | 97 ++++- web/oidc/oidc.go | 2 +- web/sharings/drives_test.go | 694 +++++++++++++++++++++++++++--------- web/sharings/move_test.go | 108 ++---- web/sharings/sharings.go | 8 +- 5 files changed, 639 insertions(+), 270 deletions(-) diff --git a/model/sharing/oauth.go b/model/sharing/oauth.go index cc77bc7f446..5bace8292cf 100644 --- a/model/sharing/oauth.go +++ b/model/sharing/oauth.go @@ -28,6 +28,8 @@ import ( "github.com/labstack/echo/v4" ) +const InteractiveSharingQueryParam = "interactive" + // CreateSharingRequest sends information about the sharing to the recipient's cozy func (m *Member) CreateSharingRequest(inst *instance.Instance, s *Sharing, c *Credentials, u *url.URL) error { if len(c.XorKey) == 0 { @@ -233,6 +235,16 @@ func countFilesInDirectory(inst *instance.Instance, dir *vfs.DirDoc) (int, error // RegisterCozyURL saves a new Cozy URL for a member func (s *Sharing) RegisterCozyURL(inst *instance.Instance, m *Member, cozyURL string) error { + return s.registerCozyURL(inst, m, cozyURL, false) +} + +// RegisterCozyURLInteractive saves a Cozy URL discovered during an interactive +// acceptance flow and marks the recipient request so auto-accept is skipped. +func (s *Sharing) RegisterCozyURLInteractive(inst *instance.Instance, m *Member, cozyURL string) error { + return s.registerCozyURL(inst, m, cozyURL, true) +} + +func (s *Sharing) registerCozyURL(inst *instance.Instance, m *Member, cozyURL string, interactive bool) error { if !s.Owner { return ErrInvalidSharing } @@ -256,6 +268,11 @@ func (s *Sharing) RegisterCozyURL(inst *instance.Instance, m *Member, cozyURL st u.RawQuery = "" u.Fragment = "" m.Instance = u.String() + if interactive { + q := u.Query() + q.Set(InteractiveSharingQueryParam, "true") + u.RawQuery = q.Encode() + } creds := s.FindCredentials(m) if creds == nil { @@ -383,6 +400,44 @@ func CreateAccessToken(inst *instance.Instance, cli *oauth.Client, sharingID str }, nil } +// updateSharingWithConflictRetry persists the sharing and resolves stale-revision +// conflicts by reconciling the desired changes onto the latest revision. +// reconcile can mutate latest for the next retry and should return true when the +// latest revision already contains a usable final state and no write is needed. +func updateSharingWithConflictRetry( + inst *instance.Instance, + s *Sharing, + retries int, + reconcile func(latest *Sharing) bool, +) error { + log := inst.Logger().WithNamespace("sharing") + current := s + for attempt := 0; ; attempt++ { + err := couchdb.UpdateDoc(inst, current) + if err == nil { + *s = *current + return nil + } + if !couchdb.IsConflictError(err) { + return err + } + + latest, findErr := FindSharing(inst, s.SID) + if findErr != nil { + return err + } + if reconcile(latest) { + *s = *latest + return nil + } + if attempt >= retries { + return err + } + log.Debugf("Conflict while updating sharing %s, retrying with latest revision (%d/%d)", s.SID, attempt+1, retries) + current = latest + } +} + // SendAnswer says to the sharer's Cozy that the sharing has been accepted, and // materialize that by an exchange of credentials. func (s *Sharing) SendAnswer(inst *instance.Instance, state string) error { @@ -510,7 +565,19 @@ func (s *Sharing) SendAnswer(inst *instance.Instance, state string) error { } } - return couchdb.UpdateDoc(inst, s) + return updateSharingWithConflictRetry(inst, s, 1, func(latest *Sharing) bool { + if latest.Active { + return true + } + latest.Members = s.Members + latest.Credentials = s.Credentials + latest.Active = s.Active + latest.Initial = s.Initial + if s.ShortcutID != "" { + latest.ShortcutID = s.ShortcutID + } + return false + }) } // ProcessAnswer takes somes credentials and update the sharing with those. @@ -562,22 +629,20 @@ func (s *Sharing) ProcessAnswer(inst *instance.Instance, creds *APICredentials) } s.Active = true - if err := couchdb.UpdateDoc(inst, s); err != nil { - if !couchdb.IsConflictError(err) { - return nil, err - } - // A conflict can occur when several users accept a sharing at - // the same time, and we should just retry in that case - s2, err2 := FindSharing(inst, s.SID) - if err2 != nil { - return nil, err - } - s2.Members[i+1] = s.Members[i+1] - s2.Credentials[i] = s.Credentials[i] - if err2 := couchdb.UpdateDoc(inst, s2); err2 != nil { - return nil, err + if err := updateSharingWithConflictRetry(inst, s, 1, func(latest *Sharing) bool { + if latest.Members[i+1].Status == MemberStatusReady && + latest.Credentials[i].Client != nil && + latest.Credentials[i].AccessToken != nil { + ac.Credentials.Client = latest.Credentials[i].Client + ac.Credentials.AccessToken = latest.Credentials[i].AccessToken + return true } - s = s2 + latest.Members[i+1] = s.Members[i+1] + latest.Credentials[i] = s.Credentials[i] + latest.Active = s.Active + return false + }); err != nil { + return nil, err } if creds.Bitwarden != nil { if err := s.SaveBitwarden(inst, &s.Members[i+1], creds.Bitwarden); err != nil { diff --git a/web/oidc/oidc.go b/web/oidc/oidc.go index 850b04b57ca..82f952eca8f 100644 --- a/web/oidc/oidc.go +++ b/web/oidc/oidc.go @@ -489,7 +489,7 @@ func acceptSharing(c echo.Context, ownerInst *instance.Instance, conf *Config, t return renderError(c, ownerInst, http.StatusBadRequest, "Sorry, you cannot accept your own sharing invitation.") } memberURL := memberInst.PageURL("/", nil) - if err = s.RegisterCozyURL(ownerInst, member, memberURL); err != nil { + if err = s.RegisterCozyURLInteractive(ownerInst, member, memberURL); err != nil { return renderError(c, ownerInst, http.StatusInternalServerError, "Sorry, the sharing invitation could not be processed.") } sharing.PersistInstanceURL(ownerInst, member.Email, member.Instance) diff --git a/web/sharings/drives_test.go b/web/sharings/drives_test.go index 57dbf2f674a..83b7990282b 100644 --- a/web/sharings/drives_test.go +++ b/web/sharings/drives_test.go @@ -9,6 +9,7 @@ import ( "io" "net" "net/http" + "net/http/httptest" "net/url" "os" "strings" @@ -344,80 +345,474 @@ func findSharingMemberByEmail(t *testing.T, inst *instance.Instance, sharingID, return sharing.Member{} } -// acceptSharedDrive performs the acceptance flow on the recipient side. -// It extracts the discovery link for the specific recipient and completes the acceptance flow. -func acceptSharedDrive( +type driveAutoAcceptEnv struct { + ownerInstance *instance.Instance + recipientInstance *instance.Instance + ownerAppToken string + ownerURL string + recipientURL string + eOwner *httpexpect.Expect + eRecipient *httpexpect.Expect +} + +func newSharingExpect(t *testing.T, baseURL string) *httpexpect.Expect { + t.Helper() + + return httpexpect.WithConfig(httpexpect.Config{ + BaseURL: baseURL, + Reporter: httpexpect.NewRequireReporter(t), + Printers: []httpexpect.Printer{httpexpect.NewCompactPrinter(t)}, + }) +} + +func mergeRouteHandlers( + base map[string]func(*echo.Group), + extra map[string]func(*echo.Group), +) map[string]func(*echo.Group) { + if len(extra) == 0 { + return base + } + + routes := make(map[string]func(*echo.Group), len(base)+len(extra)) + for path, handler := range base { + routes[path] = handler + } + for path, handler := range extra { + routes[path] = handler + } + return routes +} + +func newDriveOwnerTestServer( + t *testing.T, + setupName string, + email string, + publicName string, + render echo.Renderer, + extraRoutes map[string]func(*echo.Group), +) (*instance.Instance, string, *httptest.Server) { + t.Helper() + + return newDriveOwnerTestServerWithOptions( + t, + setupName, + &lifecycle.Options{ + Email: email, + PublicName: publicName, + }, + render, + extraRoutes, + ) +} + +func newDriveOwnerTestServerWithOptions( + t *testing.T, + setupName string, + ownerOptions *lifecycle.Options, + render echo.Renderer, + extraRoutes map[string]func(*echo.Group), +) (*instance.Instance, string, *httptest.Server) { + t.Helper() + + setup := testutils.NewSetup(t, setupName) + options := lifecycle.Options{} + if ownerOptions != nil { + options = *ownerOptions + } + if options.Email == "" { + options.Email = "acme@example.net" + } + if options.PublicName == "" { + options.PublicName = "ACME" + } + + inst := setup.GetTestInstance(&options) + token := generateAppToken(inst, "drive", consts.Files) + + routes := mergeRouteHandlers(map[string]func(*echo.Group){ + "/files": files.Routes, + "/sharings": sharings.Routes, + }, extraRoutes) + ts := setup.GetTestServerMultipleRoutes(routes) + ts.Config.Handler.(*echo.Echo).Renderer = render + ts.Config.Handler.(*echo.Echo).HTTPErrorHandler = errors.ErrorHandler + t.Cleanup(ts.Close) + + return inst, token, ts +} + +func newDriveRecipientTestServer( + t *testing.T, + setupName string, + email string, + publicName string, + render echo.Renderer, + extraRoutes map[string]func(*echo.Group), +) (*instance.Instance, string, *httptest.Server) { + t.Helper() + + setup := testutils.NewSetup(t, setupName) + inst := setup.GetTestInstance(&lifecycle.Options{ + Email: email, + PublicName: publicName, + Passphrase: "MyPassphrase", + KdfIterations: 5000, + Key: "xxx", + }) + token := generateAppToken(inst, "drive", consts.Files) + + routes := mergeRouteHandlers(map[string]func(*echo.Group){ + "/auth": func(g *echo.Group) { + g.Use(middlewares.LoadSession) + auth.Routes(g) + }, + "/files": files.Routes, + "/sharings": sharings.Routes, + }, extraRoutes) + ts := setup.GetTestServerMultipleRoutes(routes) + ts.Config.Handler.(*echo.Echo).Renderer = render + ts.Config.Handler.(*echo.Echo).HTTPErrorHandler = errors.ErrorHandler + t.Cleanup(ts.Close) + + return inst, token, ts +} + +func setupDriveAutoAcceptEnv(t *testing.T, ownerAnswerDelay time.Duration) *driveAutoAcceptEnv { + t.Helper() + + config.UseTestFile(t) + build.BuildMode = build.ModeDev + cfg := config.GetConfig() + cfg.Assets = "../../assets" + _ = web.LoadSupportedLocales() + testutils.NeedCouchdb(t) + render, _ := statik.NewDirRenderer("../../assets") + middlewares.BuildTemplates() + + require.NoError(t, dynamic.InitDynamicAssetFS(config.FsURL().String()), "Could not init dynamic FS") + + prevContexts := cfg.Contexts + cfg.Contexts = map[string]interface{}{ + config.DefaultInstanceContext: map[string]interface{}{ + "sharing": map[string]interface{}{ + "auto_accept_trusted": true, + "trusted_domains": []interface{}{"cozy.local", "example.com"}, + }, + }, + } + t.Cleanup(func() { + cfg.Contexts = prevContexts + }) + + testName := strings.ReplaceAll(t.Name(), "/", "_") + ownerInstance, ownerAppToken, tsOwner := newDriveOwnerTestServer( + t, + testName+"_owner", + "owner@example.com", + "Owner", + render, + map[string]func(*echo.Group){ + "/sharings": func(g *echo.Group) { + if ownerAnswerDelay > 0 { + g.Use(func(next echo.HandlerFunc) echo.HandlerFunc { + return func(c echo.Context) error { + if c.Request().Method == http.MethodPost && strings.HasSuffix(c.Request().URL.Path, "/answer") { + time.Sleep(ownerAnswerDelay) + } + return next(c) + } + }) + } + sharings.Routes(g) + }, + }, + ) + + recipientInstance, _, tsRecipient := newDriveRecipientTestServer( + t, + testName+"_recipient", + "recipient@example.com", + "Recipient", + render, + nil, + ) + + return &driveAutoAcceptEnv{ + ownerInstance: ownerInstance, + recipientInstance: recipientInstance, + ownerAppToken: ownerAppToken, + ownerURL: tsOwner.URL, + recipientURL: tsRecipient.URL, + eOwner: newSharingExpect(t, tsOwner.URL), + eRecipient: newSharingExpect(t, tsRecipient.URL), + } +} + +func createDirectRecipientDriveSharing( t *testing.T, ownerInstance *instance.Instance, - recipientInstance *instance.Instance, + eOwner *httpexpect.Expect, + ownerAppToken string, recipientName string, - tsAURL string, - tsRecipientURL string, - sharingID string, -) { + recipientEmail string, + recipientURL string, + driveName string, + description string, +) string { t.Helper() - eA := httpexpect.Default(t, tsAURL) - eR := httpexpect.Default(t, tsRecipientURL) - // Extract the discovery link for this specific recipient - ownerPublicName, _ := ownerInstance.SettingsPublicName() - _, discoveryLink := extractInvitationLink(t, ownerInstance, sharingID, ownerPublicName, recipientName) + sharedDirID := createRootDirectory(t, eOwner, driveName, ownerAppToken) + recipientContact := createContact(t, ownerInstance, recipientName, recipientEmail) + recipientContact.M["cozy"] = []interface{}{ + map[string]interface{}{"url": recipientURL, "primary": true}, + } + require.NoError(t, couchdb.UpdateDoc(ownerInstance, recipientContact)) - // Recipient login - token := eR.GET("/auth/login"). - Expect().Status(200). + payload := fmt.Sprintf(`{ + "data": { + "type": "%s", + "attributes": { + "description": "%s", + "drive": true, + "rules": [{ + "title": "%s", + "doctype": "%s", + "values": ["%s"] + }] + }, + "relationships": { + "recipients": { + "data": [{ + "id": "%s", + "type": "%s" + }] + } + } + } + }`, consts.Sharings, description, driveName, consts.Files, sharedDirID, recipientContact.ID(), recipientContact.DocType()) + + return eOwner.POST("/sharings/"). + WithHeader("Authorization", "Bearer "+ownerAppToken). + WithHeader("Content-Type", "application/vnd.api+json"). + WithBytes([]byte(payload)). + Expect(). + Status(http.StatusCreated). + JSON(httpexpect.ContentOpts{MediaType: "application/vnd.api+json"}). + Object(). + Path("$.data.id").String().NotEmpty().Raw() +} + +func loginSharingRecipient(t *testing.T, eRecipient *httpexpect.Expect) string { + t.Helper() + + token := eRecipient.GET("/auth/login"). + Expect().Status(http.StatusOK). Cookie("_csrf").Value().NotEmpty().Raw() - eR.POST("/auth/login"). + eRecipient.POST("/auth/login"). WithCookie("_csrf", token). WithFormField("passphrase", "MyPassphrase"). WithFormField("csrf_token", token). WithRedirectPolicy(httpexpect.DontFollowRedirects). - Expect().Status(303). + Expect().Status(http.StatusSeeOther). Header("Location").Contains("home") + return token +} - // Recipient goes to the discovery link on owner host - u, err := url.Parse(discoveryLink) - assert.NoError(t, err) - state := u.Query()["state"][0] +func prepareSharingAuthorizeLink( + t *testing.T, + ownerInstance *instance.Instance, + recipientName string, + sharingID string, + eOwner *httpexpect.Expect, + recipientURL string, +) string { + t.Helper() + + ownerPublicName, _ := ownerInstance.SettingsPublicName() + _, discoveryLink := extractInvitationLink(t, ownerInstance, sharingID, ownerPublicName, recipientName) + return submitSharingDiscovery(t, eOwner, discoveryLink, recipientURL) +} - eA.GET(u.Path). +func submitSharingDiscovery( + t *testing.T, + eOwner *httpexpect.Expect, + discoveryLink string, + recipientURL string, +) string { + t.Helper() + + discoveryURL, err := url.Parse(discoveryLink) + require.NoError(t, err) + state := discoveryURL.Query().Get("state") + require.NotEmpty(t, state) + + eOwner.GET(discoveryURL.Path). WithQuery("state", state). - Expect().Status(200). + Expect().Status(http.StatusOK). HasContentType("text/html", "utf-8"). Body(). Contains("Connect to your Twake"). Contains(` 0 { sender := &s.Members[0] if sharing.IsTrustedMember(inst, sender) { - if len(s.Credentials) > 0 && s.Credentials[0].State != "" { + if c.QueryParam(sharing.InteractiveSharingQueryParam) == "true" { + log.Debugf("Skipping auto-accept for interactive drive sharing %s from trusted sender %s", s.SID, sender.Instance) + } else if len(s.Credentials) > 0 && s.Credentials[0].State != "" { state := s.Credentials[0].State log.Debugf("Auto-accepting drive sharing %s from trusted sender %s", s.SID, sender.Instance) if err := sharing.EnqueueAutoAccept(inst, s.SID, state); err != nil { @@ -825,7 +827,7 @@ func GetDiscovery(c echo.Context) error { if m.Instance != "" { if m.Status != sharing.MemberStatusSeen { - err = s.RegisterCozyURL(inst, m, m.Instance) + err = s.RegisterCozyURLInteractive(inst, m, m.Instance) } if err == nil { redirectURL, err := m.GenerateOAuthURL(s, shortcut) @@ -888,7 +890,7 @@ func PostDiscovery(c echo.Context) error { return renderDiscoveryForm(c, inst, http.StatusPreconditionFailed, sharingID, state, sharecode, shortcut, member) } email = member.Email - if err = s.RegisterCozyURL(inst, member, cozyURL); err != nil { + if err = s.RegisterCozyURLInteractive(inst, member, cozyURL); err != nil { if c.Request().Header.Get(echo.HeaderAccept) == echo.MIMEApplicationJSON { return c.JSON(http.StatusBadRequest, echo.Map{"error": err.Error()}) }