Skip to content

Commit 1984f40

Browse files
nononono
authored
Fix revoke member from a shared drive (#4574)
2 parents 78d9d22 + 7255830 commit 1984f40

8 files changed

Lines changed: 159 additions & 28 deletions

File tree

‎model/sharing/invitation.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,10 +50,11 @@ func (s *Sharing) SendInvitations(inst *instance.Instance, perms *permission.Per
5050

5151
link := m.InvitationLink(inst, s, state, perms)
5252
if m.Instance != "" && canSendShortcut {
53+
m.Status = MemberStatusPendingInvitation
5354
if err := m.SendShortcut(inst, s, link); err == nil {
54-
m.Status = MemberStatusPendingInvitation
5555
return nil
5656
}
57+
m.Status = MemberStatusMailNotSent
5758
}
5859
if m.Email == "" {
5960
if len(m.Groups) > 0 {

‎model/sharing/member.go‎

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -961,17 +961,23 @@ func (s *Sharing) RevokeMember(inst *instance.Instance, index int) error {
961961
c := &s.Credentials[index-1]
962962

963963
// No need to contact the revoked member if the sharing is not ready
964-
if m.Status == MemberStatusReady {
964+
if m.Status == MemberStatusReady || s.Drive {
965965
if err := s.NotifyMemberRevocation(inst, m, c); err != nil {
966966
inst.Logger().WithNamespace("sharing").
967967
Warnf("Error on revocation notification: %s", err)
968968
}
969+
}
969970

971+
if m.Status == MemberStatusReady {
970972
if err := DeleteOAuthClient(inst, m, c); err != nil {
971973
return err
972974
}
973975
}
974976

977+
if err := s.RemoveInteractPermissionsForAMember(inst, m, index); err != nil {
978+
return err
979+
}
980+
975981
// We may have concurrency issues where RevokeMember is called several
976982
// times on different goroutines/processes. So, we may need to retry the
977983
// operation several times.
@@ -1025,8 +1031,22 @@ func (s *Sharing) NotifyMemberRevocation(inst *instance.Instance, m *Member, c *
10251031
if m.Instance == "" || err != nil {
10261032
return ErrInvalidSharing
10271033
}
1028-
if c.Client == nil || c.AccessToken == nil {
1029-
return ErrNoOAuthClient
1034+
var token string
1035+
if s.Drive {
1036+
interact, err := permission.GetForShareInteract(inst, s.ID())
1037+
if err != nil {
1038+
return err
1039+
}
1040+
if code, ok := interact.Codes[m.Instance]; ok {
1041+
token = code
1042+
} else if code, ok := interact.Codes[m.Email]; ok {
1043+
token = code
1044+
}
1045+
} else {
1046+
if c.Client == nil || c.AccessToken == nil {
1047+
return ErrNoOAuthClient
1048+
}
1049+
token = c.AccessToken.AccessToken
10301050
}
10311051

10321052
var path string
@@ -1042,7 +1062,7 @@ func (s *Sharing) NotifyMemberRevocation(inst *instance.Instance, m *Member, c *
10421062
Domain: u.Host,
10431063
Path: path,
10441064
Headers: request.Headers{
1045-
echo.HeaderAuthorization: "Bearer " + c.AccessToken.AccessToken,
1065+
echo.HeaderAuthorization: "Bearer " + token,
10461066
},
10471067
ParseError: ParseRequestError,
10481068
}

‎model/sharing/sharing.go‎

Lines changed: 68 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -314,6 +314,9 @@ func (s *Sharing) GetInteractCode(inst *instance.Instance, member *Member, membe
314314
if err != nil {
315315
return "", err
316316
}
317+
if interact.Codes == nil {
318+
interact.Codes = make(map[string]string)
319+
}
317320
interact.Codes[key] = code
318321
if err := couchdb.UpdateDoc(inst, interact); err != nil {
319322
return "", err
@@ -462,6 +465,9 @@ func (s *Sharing) Revoke(inst *instance.Instance) error {
462465
return err
463466
}
464467
}
468+
if err := s.RevokeInteractPermissions(inst); err != nil {
469+
return err
470+
}
465471
if rule := s.FirstBitwardenOrganizationRule(); rule != nil && len(rule.Values) > 0 {
466472
if err := s.RemoveAllBitwardenMembers(inst, rule.Values[0]); err != nil {
467473
return err
@@ -486,6 +492,43 @@ func (s *Sharing) RevokePreviewPermissions(inst *instance.Instance) error {
486492
return couchdb.UpdateDoc(inst, perms)
487493
}
488494

495+
// RevokeInteractPermissions ensure that the permissions for interact tokens
496+
// are no longer valid.
497+
func (s *Sharing) RevokeInteractPermissions(inst *instance.Instance) error {
498+
perms, err := permission.GetForShareInteract(inst, s.SID)
499+
if err != nil {
500+
if couchdb.IsNotFoundError(err) {
501+
return nil
502+
}
503+
return err
504+
}
505+
now := time.Now()
506+
perms.ExpiresAt = &now
507+
return couchdb.UpdateDoc(inst, perms)
508+
}
509+
510+
func (s *Sharing) RemoveInteractPermissionsForAMember(inst *instance.Instance, m *Member, memberIndex int) error {
511+
interact, err := permission.GetForShareInteract(inst, s.SID)
512+
if err != nil {
513+
if couchdb.IsNotFoundError(err) {
514+
return nil
515+
}
516+
return err
517+
}
518+
519+
indexKey := keyFromMemberIndex(memberIndex)
520+
for key := range interact.Codes {
521+
if key == "" {
522+
continue
523+
}
524+
if key == m.Instance || key == m.Email || key == indexKey {
525+
delete(interact.Codes, key)
526+
return couchdb.UpdateDoc(inst, interact)
527+
}
528+
}
529+
return nil
530+
}
531+
489532
// RevokeRecipient revoke only one recipient on the sharer. After that, if the
490533
// sharing has still at least one active member, we keep it as is. Else, we
491534
// disable the sharing.
@@ -606,31 +649,35 @@ func (s *Sharing) RevokeByNotification(inst *instance.Instance) error {
606649
if s.Owner {
607650
return ErrInvalidSharing
608651
}
609-
if err := DeleteOAuthClient(inst, &s.Members[0], &s.Credentials[0]); err != nil {
610-
return err
611-
}
612-
if err := s.RemoveTriggers(inst); err != nil {
613-
return err
614-
}
615-
if err := s.ClearLastSequenceNumbers(inst, &s.Members[0]); err != nil {
616-
return err
617-
}
618-
if err := RemoveSharedRefs(inst, s.SID); err != nil {
619-
return err
620-
}
621-
if err := s.FixRevokedNotes(inst); err != nil {
622-
inst.Logger().WithNamespace("sharing").
623-
Warnf("RevokeByNotification failed to fix notes for revoked sharing %s: %s", s.ID(), err)
624-
}
625-
if rule := s.FirstFilesRule(); rule != nil && rule.Mime == "" {
626-
if err := s.RemoveSharingDir(inst); err != nil {
652+
if s.Drive {
653+
s.cleanShortcutID(inst)
654+
} else {
655+
if err := DeleteOAuthClient(inst, &s.Members[0], &s.Credentials[0]); err != nil {
627656
return err
628657
}
629-
}
630-
if rule := s.FirstBitwardenOrganizationRule(); rule != nil && len(rule.Values) > 0 {
631-
if err := s.RemoveBitwardenOrganization(inst, rule.Values[0]); err != nil {
658+
if err := s.RemoveTriggers(inst); err != nil {
659+
return err
660+
}
661+
if err := s.ClearLastSequenceNumbers(inst, &s.Members[0]); err != nil {
662+
return err
663+
}
664+
if err := s.FixRevokedNotes(inst); err != nil {
665+
inst.Logger().WithNamespace("sharing").
666+
Warnf("RevokeByNotification failed to fix notes for revoked sharing %s: %s", s.ID(), err)
667+
}
668+
if rule := s.FirstFilesRule(); rule != nil && rule.Mime == "" {
669+
if err := s.RemoveSharingDir(inst); err != nil {
670+
return err
671+
}
672+
}
673+
if err := RemoveSharedRefs(inst, s.SID); err != nil {
632674
return err
633675
}
676+
if rule := s.FirstBitwardenOrganizationRule(); rule != nil && len(rule.Values) > 0 {
677+
if err := s.RemoveBitwardenOrganization(inst, rule.Values[0]); err != nil {
678+
return err
679+
}
680+
}
634681
}
635682

636683
var err error

‎web/files/files.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1764,10 +1764,13 @@ var allowedChangesParams = map[string]bool{
17641764
// deletions with the document ID as only information)
17651765
func ChangesFeed(c echo.Context, inst *instance.Instance, sharedDir *vfs.DirDoc) error {
17661766
if sharedDir == nil {
1767-
//TODO: WARNING: check security here
17681767
if err := middlewares.AllowWholeType(c, permission.GET, consts.Files); err != nil {
17691768
return err
17701769
}
1770+
} else {
1771+
if err := middlewares.AllowVFS(c, permission.GET, sharedDir); err != nil {
1772+
return err
1773+
}
17711774
}
17721775

17731776
// Drop a clear error for parameters not supported by stack

‎web/sharings/drives.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -472,6 +472,9 @@ func proxy(fn func(c echo.Context, inst *instance.Instance, s *sharing.Sharing)
472472
if !s.Drive {
473473
return jsonapi.NotFound(errors.New("not a drive"))
474474
}
475+
if !s.Active {
476+
return jsonapi.Forbidden(middlewares.ErrForbidden)
477+
}
475478

476479
if s.Owner {
477480
return fn(c, inst, s)

‎web/sharings/drives_test.go‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1068,4 +1068,49 @@ func TestSharedDrives(t *testing.T) {
10681068
Expect().Status(401)
10691069
})
10701070
})
1071+
1072+
t.Run("RevokeRecipientAccess", func(t *testing.T) {
1073+
eA := httpexpect.Default(t, tsA.URL)
1074+
eB := httpexpect.Default(t, tsB.URL)
1075+
1076+
t.Run("RecipientCanInitiallyAccessSharedDrive", func(t *testing.T) {
1077+
eB.GET("/sharings/drives/"+sharingID+"/"+checklistID).
1078+
WithHeader("Authorization", "Bearer "+bettyAppToken).
1079+
Expect().Status(200).
1080+
JSON(httpexpect.ContentOpts{MediaType: "application/vnd.api+json"}).
1081+
Object().
1082+
Path("$.data.attributes.name").String().IsEqual("Checklist.txt")
1083+
obj := eB.GET("/sharings/drives").
1084+
WithHeader("Authorization", "Bearer "+bettyAppToken).
1085+
Expect().Status(200).
1086+
JSON(httpexpect.ContentOpts{MediaType: "application/vnd.api+json"}).
1087+
Object()
1088+
obj.Value("data").Array().Length().IsEqual(1)
1089+
})
1090+
1091+
t.Run("RemoveRecipientFromSharing", func(t *testing.T) {
1092+
eA.DELETE("/sharings/"+sharingID+"/recipients/1").
1093+
WithHeader("Authorization", "Bearer "+acmeAppToken).
1094+
Expect().Status(204)
1095+
})
1096+
1097+
t.Run("RevokedRecipientCannotAccessSharedDrive", func(t *testing.T) {
1098+
eB.GET("/sharings/drives/"+sharingID+"/"+checklistID).
1099+
WithHeader("Authorization", "Bearer "+bettyAppToken).
1100+
Expect().Status(403)
1101+
eB.GET("/sharings/drives/"+sharingID+"/metadata").
1102+
WithQuery("Path", "/Product team/Meetings/Checklist.txt").
1103+
WithHeader("Authorization", "Bearer "+bettyAppToken).
1104+
Expect().Status(403)
1105+
eB.GET("/sharings/drives/"+sharingID+"/_changes").
1106+
WithHeader("Authorization", "Bearer "+bettyAppToken).
1107+
Expect().Status(403)
1108+
obj := eB.GET("/sharings/drives").
1109+
WithHeader("Authorization", "Bearer "+bettyAppToken).
1110+
Expect().Status(200).
1111+
JSON(httpexpect.ContentOpts{MediaType: "application/vnd.api+json"}).
1112+
Object()
1113+
obj.Value("data").Array().Length().IsEqual(0)
1114+
})
1115+
})
10711116
}

‎web/sharings/revoke.go‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"errors"
55
"net/http"
66
"strconv"
7+
"strings"
78

89
"github.com/cozy/cozy-stack/model/sharing"
910
"github.com/cozy/cozy-stack/pkg/jsonapi"
@@ -89,6 +90,17 @@ func RevocationRecipientNotif(c echo.Context) error {
8990
if err != nil {
9091
return wrapErrors(err)
9192
}
93+
if s.Drive {
94+
token := c.Request().Header.Get(echo.HeaderAuthorization)
95+
token = strings.TrimPrefix(token, "Bearer ")
96+
if token == "" || token != s.Credentials[0].DriveToken {
97+
return echo.NewHTTPError(http.StatusForbidden)
98+
}
99+
} else {
100+
if err := hasSharingWritePermissions(c); err != nil {
101+
return err
102+
}
103+
}
92104
if err = s.RevokeByNotification(inst); err != nil {
93105
return wrapErrors(err)
94106
}

‎web/sharings/sharings.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -941,7 +941,7 @@ func Routes(router *echo.Group) {
941941
router.POST("/:sharing-id/recipients/self/readonly", DowngradeToReadOnly, checkSharingWritePermissions) // On the recipient
942942
router.DELETE("/:sharing-id/recipients/:index/readonly", RemoveReadOnly) // On the sharer
943943
router.DELETE("/:sharing-id/recipients/self/readonly", UpgradeToReadWrite, checkSharingWritePermissions) // On the recipient
944-
router.DELETE("/:sharing-id", RevocationRecipientNotif, checkSharingWritePermissions) // On the recipient
944+
router.DELETE("/:sharing-id", RevocationRecipientNotif) // On the recipient
945945
router.DELETE("/:sharing-id/recipients/self", RevokeRecipientBySelf) // On the recipient
946946
router.DELETE("/:sharing-id/answer", RevocationOwnerNotif, checkSharingWritePermissions) // On the sharer
947947
router.POST("/:sharing-id/public-key", ReceivePublicKey)

0 commit comments

Comments
 (0)