Fixing handling of permissions for external users (#5097)

<!-- PR description-->

---

#### Does this PR need a docs update or release note?

- [x]  Yes, it's included
- [ ] 🕐 Yes, but in a later PR
- [ ]  No

#### Type of change

<!--- Please check the type of change your PR introduces: --->
- [ ] 🌻 Feature
- [x] 🐛 Bugfix
- [ ] 🗺️ Documentation
- [ ] 🤖 Supportability/Tests
- [ ] 💻 CI/Deployment
- [ ] 🧹 Tech Debt/Cleanup

#### Issue(s)

<!-- Can reference multiple issues. Use one of the following "magic words" - "closes, fixes" to auto-close the Github issue. -->
* fixes https://github.com/alcionai/corso/issues/5046

#### Test Plan

<!-- How will this be tested prior to merging.-->
- [ ] 💪 Manual
- [x]  Unit test
- [ ] 💚 E2E
This commit is contained in:
Abin Simon 2024-01-25 01:02:03 +05:30 committed by GitHub
parent c526ced23e
commit 214d8a6030
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
9 changed files with 469 additions and 79 deletions

View File

@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Backup attachments associated with group mailbox items.
- Groups and Teams backups no longer fail when a resource has no display name.
- Contacts in-place restore failed if the restore destination was empty.
- Link shares with external users are now backed up and restored as expected
### Changed
- When running `backup details` on an empty backup returns a more helpful error message.
@ -24,6 +25,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Event description for exchange exports might look slightly different for certain events.
- Exchange in-place restore may restore items in well-known folders to different folders if the user has well-known folder names change based on locale and has updated the locale since the backup was created.
- In-place Exchange contacts restore will merge items in folders named "Contacts" or "contacts" into the default folder.
- External users with access through shared links will not receive these links as they are not sent via email during restore.
## [v0.18.0] (beta) - 2024-01-02

View File

@ -6,6 +6,11 @@ import (
type Entity struct {
ID string `json:"id,omitempty"`
// Email is necessary when the user is external to the
// organization and we do not have an ID associated with the user.
Email string `json:"email,omitempty"`
EntityType GV2Type `json:"entityType,omitempty"`
}

View File

@ -8,6 +8,7 @@ import (
"golang.org/x/exp/slices"
"github.com/alcionai/corso/src/internal/common/ptr"
"github.com/alcionai/corso/src/internal/common/str"
"github.com/alcionai/corso/src/pkg/logger"
)
@ -42,20 +43,32 @@ type Permission struct {
// Equal checks equality of two UserPermission objects
func (p Permission) Equals(other Permission) bool {
// EntityID can be empty for older backups and Email can be empty
// for newer ones. It is not possible for both to be empty. Also,
// if EntityID/Email for one is not empty then the other will also
// have EntityID/Email as we backup permissions for all the
// parents and children when we have a change in permissions.
// We cannot just compare id because of the problem described in #3117
if len(p.EntityID) > 0 && p.EntityID != other.EntityID {
if p.EntityType != other.EntityType {
return false
}
if len(p.Email) > 0 && p.Email != other.Email {
// NOTE: v1 of permissions only contain emails, v2 only contains IDs.
// The current one will contain both ID and email.
if len(p.EntityID) > 0 && len(other.EntityID) > 0 &&
p.EntityID != other.EntityID {
return false
}
// In cases where we have shared an item with an external user,
// the user will not have an id
if len(p.EntityID) == 0 && len(other.EntityID) == 0 {
if len(p.Email) > 0 && len(other.Email) > 0 &&
p.Email != other.Email {
return false
}
}
// Possible that one is empty and the other is not
if p.EntityID != other.EntityID {
return false
}
// We cannot just compare id/email because of #3117
p1r := p.Roles
p2r := other.Roles
@ -166,20 +179,20 @@ func FilterPermissions(ctx context.Context, perms []models.Permissionable) []Per
// https://devblogs.microsoft.com/microsoft365dev/controlling-app-access-on-specific-sharepoint-site-collections/
roles := p.GetRoles()
gv2t, entityID := getIdentityDetails(ctx, p.GetGrantedToV2())
// Technically GrantedToV2 can also contain devices, but the
// documentation does not mention about devices in permissions
if len(entityID) == 0 {
// This should ideally not be hit
ent, ok := getIdentityDetails(ctx, p.GetGrantedToV2())
if !ok {
// We log the inability to handle certain type of
// permissions within the getIdentityDetails function and so
// we just skip here
continue
}
up = append(up, Permission{
ID: ptr.Val(p.GetId()),
Roles: roles,
EntityID: entityID,
EntityType: gv2t,
EntityID: ent.ID,
Email: ent.Email, // not necessary if we have ID, but useful for debugging
EntityType: ent.EntityType,
Expiration: p.GetExpirationDateTime(),
})
}
@ -205,16 +218,12 @@ func FilterLinkShares(ctx context.Context, perms []models.Permissionable) []Link
idens := []Entity{}
for _, g := range gv2 {
gv2t, entityID := getIdentityDetails(ctx, g)
// Technically GrantedToV2 can also contain devices, but the
// documentation does not mention about devices in permissions
if len(entityID) == 0 {
// This should ideally not be hit
ent, ok := getIdentityDetails(ctx, g)
if !ok {
continue
}
idens = append(idens, Entity{ID: entityID, EntityType: gv2t})
idens = append(idens, ent)
}
up = append(up, LinkShare{
@ -235,34 +244,44 @@ func FilterLinkShares(ctx context.Context, perms []models.Permissionable) []Link
return up
}
func getIdentityDetails(ctx context.Context, gv2 models.SharePointIdentitySetable) (GV2Type, string) {
var (
gv2t GV2Type
entityID string
)
switch true {
func getIdentityDetails(ctx context.Context, gv2 models.SharePointIdentitySetable) (Entity, bool) {
switch {
case gv2.GetUser() != nil:
gv2t = GV2User
entityID = ptr.Val(gv2.GetUser().GetId())
add := gv2.GetUser().GetAdditionalData()
email, _ := str.AnyToString(add["email"]) // empty will be dropped automatically when writing
return Entity{
ID: ptr.Val(gv2.GetUser().GetId()),
Email: email,
EntityType: GV2User,
}, true
case gv2.GetSiteUser() != nil:
gv2t = GV2SiteUser
entityID = ptr.Val(gv2.GetSiteUser().GetId())
return Entity{
ID: ptr.Val(gv2.GetSiteUser().GetId()),
EntityType: GV2SiteUser,
}, true
case gv2.GetGroup() != nil:
gv2t = GV2Group
entityID = ptr.Val(gv2.GetGroup().GetId())
return Entity{
ID: ptr.Val(gv2.GetGroup().GetId()),
EntityType: GV2Group,
}, true
case gv2.GetSiteGroup() != nil:
gv2t = GV2SiteGroup
entityID = ptr.Val(gv2.GetSiteGroup().GetId())
return Entity{
ID: ptr.Val(gv2.GetSiteGroup().GetId()),
EntityType: GV2SiteGroup,
}, true
case gv2.GetApplication() != nil:
gv2t = GV2App
entityID = ptr.Val(gv2.GetApplication().GetId())
return Entity{
ID: ptr.Val(gv2.GetApplication().GetId()),
EntityType: GV2App,
}, true
case gv2.GetDevice() != nil:
gv2t = GV2Device
entityID = ptr.Val(gv2.GetDevice().GetId())
return Entity{
ID: ptr.Val(gv2.GetDevice().GetId()),
EntityType: GV2Device,
}, true
default:
logger.Ctx(ctx).Info("untracked permission")
return Entity{}, false
}
return gv2t, entityID
}

View File

@ -7,6 +7,7 @@ import (
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/suite"
"github.com/alcionai/corso/src/internal/common/ptr"
"github.com/alcionai/corso/src/internal/tester"
)
@ -225,10 +226,14 @@ func getPermsAndResourceOwnerPerms(
case GV2App, GV2Device, GV2Group, GV2User:
identity := models.NewIdentity()
identity.SetId(&resourceOwner)
identity.SetAdditionalData(map[string]any{"email": &resourceOwner})
switch gv2t {
case GV2User:
// user's need to handle email
identity := models.NewUser()
identity.SetId(&resourceOwner)
identity.SetAdditionalData(map[string]any{"email": &resourceOwner})
sharepointIdentitySet.SetUser(identity)
case GV2Group:
sharepointIdentitySet.SetGroup(identity)
@ -241,7 +246,6 @@ func getPermsAndResourceOwnerPerms(
case GV2SiteUser, GV2SiteGroup:
spIdentity := models.NewSharePointIdentity()
spIdentity.SetId(&resourceOwner)
spIdentity.SetAdditionalData(map[string]any{"email": &resourceOwner})
switch gv2t {
case GV2SiteUser:
@ -263,6 +267,11 @@ func getPermsAndResourceOwnerPerms(
EntityType: gv2t,
}
if gv2t == GV2User {
// we currently don't parse out emails for others
ownersPerm.Email = resourceOwner
}
return perm, ownersPerm
}
@ -397,3 +406,315 @@ func (suite *PermissionsUnitTestSuite) TestDrivePermissionsFilter() {
})
}
}
func (suite *PermissionsUnitTestSuite) TestEqual() {
table := []struct {
name string
perm1 Permission
perm2 Permission
expected bool
}{
{
name: "same id no email",
perm1: Permission{
Roles: []string{"read"},
EntityID: "user-id1",
EntityType: GV2User,
},
perm2: Permission{
Roles: []string{"read"},
EntityID: "user-id1",
EntityType: GV2User,
},
expected: true,
},
{
name: "no id same email",
perm1: Permission{
Roles: []string{"read"},
Email: "id1@provider.com",
EntityType: GV2User,
},
perm2: Permission{
Roles: []string{"read"},
Email: "id1@provider.com",
EntityType: GV2User,
},
expected: true,
},
{
// Can happen if user changes email
name: "same id different email",
perm1: Permission{
EntityID: "user-id1",
Roles: []string{"read"},
Email: "id1@provider.com",
EntityType: GV2User,
},
perm2: Permission{
EntityID: "user-id1",
Roles: []string{"read"},
Email: "id1-new@provider.com",
EntityType: GV2User,
},
expected: true,
},
{
name: "different id different email",
perm1: Permission{
EntityID: "user-id1",
Roles: []string{"read"},
Email: "id1@provider.com",
EntityType: GV2User,
},
perm2: Permission{
EntityID: "user-id2",
Roles: []string{"read"},
Email: "id2@provider.com",
EntityType: GV2User,
},
expected: false,
},
{
name: "different id same email",
perm1: Permission{
EntityID: "user-id1",
Roles: []string{"read"},
Email: "id1@provider.com",
EntityType: GV2User,
},
perm2: Permission{
EntityID: "user-id2",
Roles: []string{"read"},
Email: "id1@provider.com",
EntityType: GV2User,
},
expected: false,
},
{
name: "one with id one with email",
perm1: Permission{
EntityID: "user-id1",
Roles: []string{"read"},
EntityType: GV2User,
},
perm2: Permission{
Email: "id2@provider.com",
Roles: []string{"read"},
EntityType: GV2User,
},
expected: false,
},
{
name: "same email one with no id",
perm1: Permission{
EntityID: "user-id1",
Email: "id1@provider.com",
Roles: []string{"read"},
EntityType: GV2User,
},
perm2: Permission{
Email: "id1@provider.com",
Roles: []string{"read"},
EntityType: GV2User,
},
expected: false,
},
{
// should not ideally happen, not entirely sure if it
// should be false as we could just be missing the id
name: "same email one with no id",
perm1: Permission{
EntityID: "user-id1",
Email: "id1@provider.com",
Roles: []string{"read"},
EntityType: GV2User,
},
perm2: Permission{
Email: "id1@provider.com",
Roles: []string{"read"},
EntityType: GV2User,
},
expected: false,
},
{
name: "same id different role",
perm1: Permission{
EntityID: "user-id1",
Roles: []string{"read"},
EntityType: GV2User,
},
perm2: Permission{
EntityID: "user-id1",
Roles: []string{"write"},
EntityType: GV2User,
},
expected: false,
},
{
name: "same email different role",
perm1: Permission{
Email: "id1@provider.com",
Roles: []string{"read"},
EntityType: GV2User,
},
perm2: Permission{
Email: "id1@provider.com",
Roles: []string{"write"},
EntityType: GV2User,
},
expected: false,
},
{
name: "same id different entity type",
perm1: Permission{
EntityID: "user-id1",
Roles: []string{"read"},
EntityType: GV2User,
},
perm2: Permission{
EntityID: "user-id1",
Roles: []string{"read"},
EntityType: GV2Group,
},
expected: false,
},
}
for _, test := range table {
suite.Run(test.name, func() {
t := suite.T()
assert.Equal(t, test.expected, test.perm1.Equals(test.perm2), "permissions equality")
})
}
}
func (suite *PermissionsUnitTestSuite) TestFilterLinkShares() {
table := []struct {
name string
perms func() []models.Permissionable
expected [][]Entity
}{
{
name: "with id and email",
perms: func() []models.Permissionable {
perm1 := models.NewPermission()
perm1.SetId(ptr.To("id1"))
perm1.SetRoles([]string{"read"})
spi11 := models.NewUser()
spi11.SetId(ptr.To("user-id1"))
spi11.SetAdditionalData(map[string]any{"email": ptr.To("id1@provider")})
spi12 := models.NewUser()
spi12.SetId(ptr.To("user-id2"))
spi12.SetAdditionalData(map[string]any{"email": ptr.To("id2@provider")})
gv21 := models.NewSharePointIdentitySet()
gv21.SetUser(spi11)
gv22 := models.NewSharePointIdentitySet()
gv22.SetUser(spi12)
perm1.SetGrantedToIdentitiesV2([]models.SharePointIdentitySetable{gv21, gv22})
li1 := models.NewSharingLink()
li1.SetWebUrl(ptr.To("https://link1"))
perm1.SetLink(li1)
return []models.Permissionable{perm1}
},
expected: [][]Entity{
{
{ID: "user-id1", Email: "id1@provider", EntityType: GV2User},
{ID: "user-id2", Email: "id2@provider", EntityType: GV2User},
},
},
},
{
name: "only email",
perms: func() []models.Permissionable {
perm1 := models.NewPermission()
perm1.SetId(ptr.To("id1"))
perm1.SetRoles([]string{"read"})
spi11 := models.NewUser()
spi11.SetAdditionalData(map[string]any{"email": ptr.To("id1@provider")})
spi12 := models.NewUser()
spi12.SetAdditionalData(map[string]any{"email": ptr.To("id2@provider")})
gv21 := models.NewSharePointIdentitySet()
gv21.SetUser(spi11)
gv22 := models.NewSharePointIdentitySet()
gv22.SetUser(spi12)
perm1.SetGrantedToIdentitiesV2([]models.SharePointIdentitySetable{gv21, gv22})
li1 := models.NewSharingLink()
li1.SetWebUrl(ptr.To("https://link1"))
perm1.SetLink(li1)
return []models.Permissionable{perm1}
},
expected: [][]Entity{
{
{Email: "id1@provider", EntityType: GV2User},
{Email: "id2@provider", EntityType: GV2User},
},
},
},
{
name: "one with id one with email",
perms: func() []models.Permissionable {
perm1 := models.NewPermission()
perm1.SetId(ptr.To("id1"))
perm1.SetRoles([]string{"read"})
spi11 := models.NewUser()
spi11.SetId(ptr.To("user-id1"))
spi12 := models.NewUser()
spi12.SetAdditionalData(map[string]any{"email": ptr.To("id2@provider")})
gv21 := models.NewSharePointIdentitySet()
gv21.SetUser(spi11)
gv22 := models.NewSharePointIdentitySet()
gv22.SetUser(spi12)
perm1.SetGrantedToIdentitiesV2([]models.SharePointIdentitySetable{gv21, gv22})
li1 := models.NewSharingLink()
li1.SetWebUrl(ptr.To("https://link1"))
perm1.SetLink(li1)
return []models.Permissionable{perm1}
},
expected: [][]Entity{
{
{ID: "user-id1", EntityType: GV2User},
{Email: "id2@provider", EntityType: GV2User},
},
},
},
}
for _, test := range table {
suite.Run(test.name, func() {
t := suite.T()
ctx, flush := tester.NewContext(t)
defer flush()
actual := FilterLinkShares(ctx, test.perms())
assert.Equal(t, len(test.expected), len(actual), "number of link shares")
for i, expected := range test.expected {
assert.ElementsMatch(t, expected, actual[i].Entities, "link share entities")
}
})
}
}

View File

@ -2,7 +2,6 @@ package drive
import (
"context"
"strings"
"github.com/alcionai/clues"
"github.com/microsoftgraph/msgraph-sdk-go/drives"
@ -208,7 +207,8 @@ func UpdatePermissions(
ictx := clues.Add(
ctx,
"permission_entity_type", p.EntityType,
"permission_entity_id", clues.Hide(p.EntityID))
"permission_entity_id", clues.Hide(p.EntityID),
"permission_entity_email", clues.Hide(p.Email))
// deletes require unique http clients
// https://github.com/alcionai/corso/issues/2707
@ -245,7 +245,8 @@ func UpdatePermissions(
ictx := clues.Add(
ctx,
"permission_entity_type", p.EntityType,
"permission_entity_id", clues.Hide(p.EntityID))
"permission_entity_id", clues.Hide(p.EntityID),
"permission_entity_email", clues.Hide(p.Email))
// We are not able to restore permissions when there are no
// roles or for owner, this seems to be restriction in graph
@ -351,11 +352,16 @@ func UpdateLinkShares(
}
// Using DriveRecipient seems to error out on Graph end
if len(iden.ID) > 0 {
idens = append(idens, map[string]string{"objectId": iden.ID})
entities = append(entities, iden.ID)
} else {
idens = append(idens, map[string]string{"email": iden.Email})
entities = append(entities, iden.Email)
}
}
ictx = clues.Add(ictx, "link_share_entity_ids", strings.Join(entities, ","))
ictx = clues.Add(ictx, "link_share_entities", clues.HideAll(entities))
// https://learn.microsoft.com/en-us/graph/api/driveitem-createlink?view=graph-rest-beta&tabs=http
// v1.0 version of the graph API does not support creating a
@ -471,11 +477,14 @@ func filterUnavailableEntitiesInLinkShare(
switch e.EntityType {
case metadata.GV2User:
_, ok := availableEntities.Users.NameOf(e.ID)
available = available || ok
// Link shares with external users won't have IDs
if len(e.ID) == 0 && len(e.Email) > 0 {
available = true
} else {
_, available = availableEntities.Users.NameOf(e.ID)
}
case metadata.GV2Group:
_, ok := availableEntities.Groups.NameOf(e.ID)
available = available || ok
_, available = availableEntities.Groups.NameOf(e.ID)
default:
// We only know about users and groups
available = true

View File

@ -731,10 +731,24 @@ func linkSharesEqual(expected metadata.LinkShare, got metadata.LinkShare) bool {
return false
}
if !slices.Equal(expected.Entities, got.Entities) {
if len(expected.Entities) != len(got.Entities) {
return false
}
for i, e := range expected.Entities {
if !strings.EqualFold(e.ID, got.Entities[i].ID) {
return false
}
if !strings.EqualFold(e.Email, got.Entities[i].Email) {
return false
}
if e.EntityType != got.Entities[i].EntityType {
return false
}
}
if (expected.Expiration == nil && got.Expiration != nil) ||
(expected.Expiration != nil && got.Expiration == nil) {
return false

View File

@ -1083,8 +1083,19 @@ func testLinkSharesInheritanceRestoreAndBackup(suite oneDriveSuite, startVersion
ctx, flush := tester.NewContext(t)
defer flush()
_, secondaryUserID := suite.SecondaryUser()
_, tertiaryUserID := suite.TertiaryUser()
secondaryUserName, secondaryUserID := suite.SecondaryUser()
secondaryUser := metadata.Entity{
ID: secondaryUserID,
Email: secondaryUserName,
EntityType: metadata.GV2User,
}
tertiaryUserName, tertiaryUserID := suite.TertiaryUser()
tertiaryUser := metadata.Entity{
ID: tertiaryUserID,
Email: tertiaryUserName,
EntityType: metadata.GV2User,
}
// Get the default drive ID for the test user.
driveID := mustGetDefaultDriveID(
@ -1138,7 +1149,7 @@ func testLinkSharesInheritanceRestoreAndBackup(suite oneDriveSuite, startVersion
Meta: stub.MetaData{
LinkShares: []stub.LinkShareData{
{
EntityIDs: []string{secondaryUserID},
Entities: []metadata.Entity{secondaryUser},
Scope: "users",
Type: "edit",
},
@ -1199,7 +1210,7 @@ func testLinkSharesInheritanceRestoreAndBackup(suite oneDriveSuite, startVersion
Meta: stub.MetaData{
LinkShares: []stub.LinkShareData{
{
EntityIDs: []string{tertiaryUserID},
Entities: []metadata.Entity{tertiaryUser},
Scope: "anonymous",
Type: "edit",
},
@ -1212,7 +1223,7 @@ func testLinkSharesInheritanceRestoreAndBackup(suite oneDriveSuite, startVersion
Meta: stub.MetaData{
LinkShares: []stub.LinkShareData{
{
EntityIDs: []string{tertiaryUserID},
Entities: []metadata.Entity{tertiaryUser},
Scope: "users",
Type: "edit",
},

View File

@ -37,10 +37,9 @@ func getMetadata(fileName string, meta MetaData, permUseID bool) metadata.Metada
id := uuid.NewString()
uperm := metadata.Permission{ID: id, Roles: meta.Perms.Roles}
uperm.Email = meta.Perms.User
if permUseID {
uperm.EntityID = meta.Perms.EntityID
} else {
uperm.Email = meta.Perms.User
}
testMeta.Permissions = []metadata.Permission{uperm}
@ -48,11 +47,18 @@ func getMetadata(fileName string, meta MetaData, permUseID bool) metadata.Metada
if len(meta.LinkShares) != 0 {
for _, ls := range meta.LinkShares {
id := strings.Join(ls.EntityIDs, "-") + ls.Scope + ls.Type
eids := []string{}
for _, id := range ls.Entities {
eids = append(eids, id.ID)
}
entities := []metadata.Entity{}
for _, e := range ls.EntityIDs {
entities = append(entities, metadata.Entity{ID: e, EntityType: "user"})
id := strings.Join(eids, "-") + ls.Scope + ls.Type
roles := []string{}
if ls.Type == "edit" {
roles = []string{"write"}
} else if ls.Type == "view" {
roles = []string{"read"}
}
ls := metadata.LinkShare{
@ -62,7 +68,8 @@ func getMetadata(fileName string, meta MetaData, permUseID bool) metadata.Metada
Type: ls.Type,
WebURL: id,
},
Entities: entities,
Entities: ls.Entities,
Roles: roles,
}
testMeta.LinkShares = append(testMeta.LinkShares, ls)
@ -79,7 +86,7 @@ type PermData struct {
}
type LinkShareData struct {
EntityIDs []string
Entities []metadata.Entity
Scope string
Type string
}

View File

@ -43,3 +43,5 @@ Below is a list of known Corso issues and limitations:
updated the locale since the backup was created.
* In-place Exchange contacts restore will merge items in folders named "Contacts" or "contacts" into the default folder.
* External users with access through shared links won't receive these links as they're not sent via email during restore.