Skip to content

Commit

Permalink
graph/sharing: Add check for role conditions
Browse files Browse the repository at this point in the history
Use the condition from the unifiedrole to check if the requested role
is actually applicable to the selected resource.

Fixes: owncloud#8131
  • Loading branch information
rhafer committed Jan 24, 2024
1 parent fa70d58 commit 94eaa02
Show file tree
Hide file tree
Showing 3 changed files with 48 additions and 4 deletions.
6 changes: 6 additions & 0 deletions services/graph/pkg/service/v0/driveitems.go
Original file line number Diff line number Diff line change
Expand Up @@ -460,6 +460,12 @@ func (g Graph) Invite(w http.ResponseWriter, r *http.Request) {
errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, http.StatusText(http.StatusInternalServerError))
return
}
// FIXME: When setting permissions on a space, we need to use UnifiedRoleConditionOwner here
allowedResourceActions := unifiedrole.GetAllowedResourceActions(role, unifiedrole.UnifiedRoleConditionGrantee)
if len(allowedResourceActions) == 0 {
errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "role not applicable to this resource")
return
}

unifiedRolePermissions = append(unifiedRolePermissions, conversions.ToPointerSlice(role.GetRolePermissions())...)
}
Expand Down
30 changes: 30 additions & 0 deletions services/graph/pkg/service/v0/driveitems_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -685,6 +685,25 @@ var _ = Describe("Driveitems", func() {
_, ok := res.GetRolesOk()
Expect(ok).To(BeTrue())
})
It("fails to update the share permissions for a file share when setting a space specific role", func() {
updateShareMock := gatewayClient.On("UpdateShare",
mock.Anything,
mock.MatchedBy(func(req *collaboration.UpdateShareRequest) bool {
return req.GetShare().GetId().GetOpaqueId() == "permissionid"
}),
)
updateShareMock.Return(updateShareMockResponse, nil)

driveItemPermission.SetRoles([]string{unifiedrole.NewSpaceViewerUnifiedRole().GetId()})
body, err := driveItemPermission.MarshalJSON()
Expect(err).To(BeNil())
svc.UpdatePermission(
rr,
httptest.NewRequest(http.MethodPatch, "/", strings.NewReader(string(body))).
WithContext(ctx),
)
Expect(rr.Code).To(Equal(http.StatusBadRequest))
})
It("updates the share permissions when changing the resource permission actions", func() {
updateShareMock := gatewayClient.On("UpdateShare",
mock.Anything,
Expand Down Expand Up @@ -1007,6 +1026,17 @@ var _ = Describe("Driveitems", func() {
Expect(jsonData.Get("0.roles.0").String()).To(Equal(unifiedrole.NewViewerUnifiedRole(true).GetId()))
})

It("fails with wrong role", func() {
driveItemInvite.Roles = []string{unifiedrole.NewCoownerUnifiedRole().GetId()}
svc.Invite(
rr,
httptest.NewRequest(http.MethodPost, "/", toJSONReader(driveItemInvite)).
WithContext(ctx),
)

Expect(rr.Code).To(Equal(http.StatusBadRequest))
})

It("with actions (happy path)", func() {
driveItemInvite.Roles = nil
driveItemInvite.LibreGraphPermissionsActions = []string{unifiedrole.DriveItemContentRead}
Expand Down
16 changes: 12 additions & 4 deletions services/graph/pkg/unifiedrole/unifiedrole_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,7 @@ var _ = Describe("unifiedroles", func() {
Entry(rConversions.RoleViewer, rConversions.NewViewerRole(true), unifiedrole.NewViewerUnifiedRole(true), unifiedrole.UnifiedRoleConditionGrantee),
Entry(rConversions.RoleEditor, rConversions.NewEditorRole(true), unifiedrole.NewEditorUnifiedRole(true), unifiedrole.UnifiedRoleConditionGrantee),
Entry(rConversions.RoleFileEditor, rConversions.NewFileEditorRole(true), unifiedrole.NewFileEditorUnifiedRole(true), unifiedrole.UnifiedRoleConditionGrantee),
Entry(rConversions.RoleCoowner, rConversions.NewCoownerRole(), unifiedrole.NewCoownerUnifiedRole(), unifiedrole.UnifiedRoleConditionGrantee),
Entry(rConversions.RoleManager, rConversions.NewManagerRole(), unifiedrole.NewManagerUnifiedRole(), unifiedrole.UnifiedRoleConditionGrantee),
Entry(rConversions.RoleCoowner, rConversions.NewCoownerRole(), unifiedrole.NewCoownerUnifiedRole(), unifiedrole.UnifiedRoleConditionOwner),
Entry(rConversions.RoleManager, rConversions.NewManagerRole(), unifiedrole.NewManagerUnifiedRole(), unifiedrole.UnifiedRoleConditionOwner),
Entry(rConversions.RoleSpaceViewer, rConversions.NewSpaceViewerRole(), unifiedrole.NewSpaceViewerUnifiedRole(), unifiedrole.UnifiedRoleConditionOwner),
Entry(rConversions.RoleSpaceEditor, rConversions.NewSpaceEditorRole(), unifiedrole.NewSpaceEditorUnifiedRole(), unifiedrole.UnifiedRoleConditionOwner),
Expand Down Expand Up @@ -208,6 +207,17 @@ var _ = Describe("unifiedroles", func() {
unifiedrole.NewViewerUnifiedRole(false),
unifiedrole.NewFileEditorUnifiedRole(false),
unifiedrole.NewEditorUnifiedRole(false),
},
),

Entry(
"GetBuiltinRoleDefinitionList",
rolesToAction(unifiedrole.GetBuiltinRoleDefinitionList(false)...),
unifiedrole.UnifiedRoleConditionOwner,
false,
[]*libregraph.UnifiedRoleDefinition{
unifiedrole.NewSpaceViewerUnifiedRole(),
unifiedrole.NewSpaceEditorUnifiedRole(),
unifiedrole.NewCoownerUnifiedRole(),
unifiedrole.NewManagerUnifiedRole(),
},
Expand All @@ -223,8 +233,6 @@ var _ = Describe("unifiedroles", func() {
unifiedrole.NewViewerUnifiedRole(true),
unifiedrole.NewFileEditorUnifiedRole(true),
unifiedrole.NewEditorUnifiedRole(true),
unifiedrole.NewCoownerUnifiedRole(),
unifiedrole.NewManagerUnifiedRole(),
},
),

Expand Down

0 comments on commit 94eaa02

Please sign in to comment.