Skip to content

Commit

Permalink
Merge pull request #13652 from opf/bug/49956-accidentaly-granting-acc…
Browse files Browse the repository at this point in the history 
…ess-to-nextcloud-project-folders-that-are-no-members-of-the-project

[#49956] Accidentaly granting access to Nextcloud project folders that are no members of the project
  • Loading branch information
@wielinde
wielinde committed Sep 7, 2023
2 parents a231e88 + 2bde008 commit ce8a477
Show file tree
Hide file tree
Showing 2 changed files with 130 additions and 44 deletions.
48 changes: 27 additions & 21 deletions modules/storages/app/services/storages/group_folder_properties_sync_service.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -201,35 +201,41 @@ def group_users
end

def project_folder_permissions(project:)
{
users: admins_project_folder_permissions.merge!(members_project_folder_permissions(project:)),
groups: { "#{@group}": NO_PERMISSIONS }
}
end

def admins_project_folder_permissions
@admin_nextcloud_usernames.each_with_object(
"#{@nextcloud_system_user}": ALL_PERMISSIONS
) do |admin_nextcloud_username, hash_map|
hash_map[admin_nextcloud_username.to_sym] = ALL_PERMISSIONS
end
end

def members_project_folder_permissions(project:)
tokens_query(project:).each_with_object({}) do |token, permissions|
nextcloud_username = token.origin_user_id
permissions[nextcloud_username.to_sym] = calculate_permissions(user: token.user, project:)
@nextcloud_usernames_used_in_openproject << nextcloud_username
end
end

def tokens_query(project:)
tokens_query = OAuthClientToken
.where(oauth_client: @storage.oauth_client)
.where.not(id: @admin_tokens_query)
.includes(:user)
.where(oauth_client: @storage.oauth_client)
.where.not(id: @admin_tokens_query)
.includes(:user)
# The user scope is required in all cases except one:
# when the project is public and non member has at least one storage permission
# then all non memebers should have access to the project folder
if !(project.public? && Role.non_member.permissions.intersect?(PERMISSIONS_KEYS))
tokens_query = tokens_query.where(users: project.users)
end
tokens_query.each_with_object({
users: admins_project_folder_permissions,
groups: { "#{@group}": NO_PERMISSIONS }
}) do |token, permissions|
nextcloud_username = token.origin_user_id
permissions[:users][nextcloud_username.to_sym] = calculate_permissions(user: token.user, project:)
@nextcloud_usernames_used_in_openproject << nextcloud_username
end
end

def admins_project_folder_permissions
@admins_project_folder_permissions ||=
{
"#{@nextcloud_system_user}": ALL_PERMISSIONS
}.tap do |map|
@admin_nextcloud_usernames.each do |admin_nextcloud_username|
map[admin_nextcloud_username.to_sym] = ALL_PERMISSIONS
end
end
tokens_query
end

def add_active_users_to_group
Expand Down
126 changes: 103 additions & 23 deletions modules/storages/spec/services/storages/group_folder_properties_sync_service_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,12 @@
<nc:acl-mask>31</nc:acl-mask>
<nc:acl-permissions>3</nc:acl-permissions>
</nc:acl>
<nc:acl>
<nc:acl-mapping-type>user</nc:acl-mapping-type>
<nc:acl-mapping-id>Yoda</nc:acl-mapping-id>
<nc:acl-mask>31</nc:acl-mask>
<nc:acl-permissions>3</nc:acl-permissions>
</nc:acl>
</nc:acl-list>
</d:prop>
</d:set>
Expand Down Expand Up @@ -314,6 +320,43 @@
</d:multistatus>
XML
end
let(:set_permissions_request_body4) do
<<~XML
<?xml version="1.0"?>
<d:propertyupdate xmlns:d="DAV:" xmlns:nc="http://nextcloud.org/ns">
<d:set>
<d:prop>
<nc:acl-list>
<nc:acl>
<nc:acl-mapping-type>group</nc:acl-mapping-type>
<nc:acl-mapping-id>OpenProject</nc:acl-mapping-id>
<nc:acl-mask>31</nc:acl-mask>
<nc:acl-permissions>0</nc:acl-permissions>
</nc:acl>
<nc:acl>
<nc:acl-mapping-type>user</nc:acl-mapping-type>
<nc:acl-mapping-id>OpenProject</nc:acl-mapping-id>
<nc:acl-mask>31</nc:acl-mask>
<nc:acl-permissions>31</nc:acl-permissions>
</nc:acl>
<nc:acl>
<nc:acl-mapping-type>user</nc:acl-mapping-type>
<nc:acl-mapping-id>Darth Vader</nc:acl-mapping-id>
<nc:acl-mask>31</nc:acl-mask>
<nc:acl-permissions>31</nc:acl-permissions>
</nc:acl>
<nc:acl>
<nc:acl-mapping-type>user</nc:acl-mapping-type>
<nc:acl-mapping-id>Obi-Wan</nc:acl-mapping-id>
<nc:acl-mask>31</nc:acl-mask>
<nc:acl-permissions>3</nc:acl-permissions>
</nc:acl>
</nc:acl-list>
</d:prop>
</d:set>
</d:propertyupdate>
XML
end
let(:set_permissions_response_body4) do
<<~XML
<?xml version="1.0"?>
Expand Down Expand Up @@ -410,6 +453,12 @@
<nc:acl-mask>31</nc:acl-mask>
<nc:acl-permissions>1</nc:acl-permissions>
</nc:acl>
<nc:acl>
<nc:acl-mapping-type>user</nc:acl-mapping-type>
<nc:acl-mapping-id>Yoda</nc:acl-mapping-id>
<nc:acl-mask>31</nc:acl-mask>
<nc:acl-permissions>1</nc:acl-permissions>
</nc:acl>
</nc:acl-list>
</d:prop>
</d:set>
Expand Down Expand Up @@ -438,11 +487,29 @@
end
let(:request_stubs) { [] }

let(:project1) { create(:project, name: '[Sample] Project Name / Ehuu', members: { user => ordinary_role }) }
let(:project2) { create(:project, name: 'Jedi Project Folder ///', members: { user => ordinary_role }) }
let(:project3) { create(:project, name: 'NOT ACTIVE PROJECT', active: false, members: { user => ordinary_role }) }
let(:public_project) { create(:public_project, name: 'PUBLIC PROJECT', active: true) }
let(:user) { create(:user) }
let(:project1) do
create(:project,
name: '[Sample] Project Name / Ehuu',
members: { multiple_projects_user => ordinary_role, single_project_user => ordinary_role })
end
let(:project2) do
create(:project,
name: 'Jedi Project Folder ///',
members: { multiple_projects_user => ordinary_role })
end
let(:project3) do
create(:project,
name: 'NOT ACTIVE PROJECT',
active: false,
members: { multiple_projects_user => ordinary_role })
end
let(:public_project) do
create(:public_project,
name: 'PUBLIC PROJECT',
active: true)
end
let(:single_project_user) { create(:user) }
let(:multiple_projects_user) { create(:user) }
let!(:admin) { create(:admin) }
let(:ordinary_role) { create(:role, permissions: %w[read_files write_files]) }
let!(:non_member_role) { create(:non_member, permissions: %w[read_files]) }
Expand Down Expand Up @@ -486,7 +553,11 @@
before do
create(:oauth_client_token,
origin_user_id: 'Obi-Wan',
user:,
user: multiple_projects_user,
oauth_client:)
create(:oauth_client_token,
origin_user_id: 'Yoda',
user: single_project_user,
oauth_client:)
create(:oauth_client_token,
origin_user_id: 'Darth Vader',
Expand Down Expand Up @@ -516,7 +587,8 @@
).to_return(status: 207, body: propfind_response_body1, headers: {})
request_stubs << stub_request(
:mkcol,
"#{storage.host}/remote.php/dav/files/OpenProject/OpenProject/%5BSample%5D%20Project%20Name%20%7C%20Ehuu%20(#{project1.id})"
"#{storage.host}/remote.php/dav/files/OpenProject/OpenProject/" \
"%5BSample%5D%20Project%20Name%20%7C%20Ehuu%20(#{project1.id})"
).with(
headers: {
'Authorization' => 'Basic T3BlblByb2plY3Q6MTIzNDU2Nzg='
Expand All @@ -541,6 +613,14 @@
'Ocs-Apirequest' => 'true'
}
).to_return(status: 200, body: add_user_to_group_response_body, headers: {})
request_stubs << stub_request(:post, "#{storage.host}/ocs/v1.php/cloud/users/Yoda/groups")
.with(
body: "groupid=OpenProject",
headers: {
'Authorization' => 'Basic T3BlblByb2plY3Q6MTIzNDU2Nzg=',
'Ocs-Apirequest' => 'true'
}
).to_return(status: 200, body: add_user_to_group_response_body, headers: {})
request_stubs << stub_request(:post, "#{storage.host}/ocs/v1.php/cloud/users/Darth%20Vader/groups")
.with(
body: "groupid=OpenProject",
Expand Down Expand Up @@ -573,23 +653,23 @@
request_stubs << stub_request(
:proppatch,
"#{storage.host}/remote.php/dav/files/OpenProject/OpenProject/" \
"Jedi%20Project%20Folder%20%7C%7C%7C%20%28#{project2.id}%29"
"Lost%20Jedi%20Project%20Folder%20%232"
).with(
body: set_permissions_request_body2,
body: set_permissions_request_body3,
headers: {
'Authorization' => 'Basic T3BlblByb2plY3Q6MTIzNDU2Nzg='
}
).to_return(status: 207, body: set_permissions_response_body4, headers: {})
).to_return(status: 207, body: set_permissions_response_body3, headers: {})
request_stubs << stub_request(
:proppatch,
"#{storage.host}/remote.php/dav/files/OpenProject/OpenProject/" \
"PUBLIC%20PROJECT%20%28#{public_project.id}%29"
"Jedi%20Project%20Folder%20%7C%7C%7C%20%28#{project2.id}%29"
).with(
body: set_permissions_request_body6,
body: set_permissions_request_body4,
headers: {
'Authorization' => 'Basic T3BlblByb2plY3Q6MTIzNDU2Nzg='
}
).to_return(status: 207, body: set_permissions_response_body6, headers: {})
).to_return(status: 207, body: set_permissions_response_body4, headers: {})
request_stubs << stub_request(
:proppatch,
"#{storage.host}/remote.php/dav/files/OpenProject/OpenProject/" \
Expand All @@ -600,6 +680,16 @@
'Authorization' => 'Basic T3BlblByb2plY3Q6MTIzNDU2Nzg='
}
).to_return(status: 207, body: set_permissions_response_body5, headers: {})
request_stubs << stub_request(
:proppatch,
"#{storage.host}/remote.php/dav/files/OpenProject/OpenProject/" \
"PUBLIC%20PROJECT%20%28#{public_project.id}%29"
).with(
body: set_permissions_request_body6,
headers: {
'Authorization' => 'Basic T3BlblByb2plY3Q6MTIzNDU2Nzg='
}
).to_return(status: 207, body: set_permissions_response_body6, headers: {})
request_stubs << stub_request(
:delete,
"#{storage.host}/ocs/v1.php/cloud/users/Darth%20Maul/groups?groupid=OpenProject"
Expand All @@ -609,16 +699,6 @@
'Ocs-Apirequest' => 'true'
}
).to_return(status: 200, body: remove_user_from_group_response, headers: {})
request_stubs << stub_request(
:proppatch,
"#{storage.host}/remote.php/dav/files/OpenProject/OpenProject/" \
"Lost%20Jedi%20Project%20Folder%20%232"
).with(
body: set_permissions_request_body3,
headers: {
'Authorization' => 'Basic T3BlblByb2plY3Q6MTIzNDU2Nzg='
}
).to_return(status: 207, body: set_permissions_response_body3, headers: {})
end

it 'sets project folders properties' do
Expand Down

0 comments on commit ce8a477

Please sign in to comment.