MM-58525 Fix upload file permissions #27298

sbishel · 2024-06-06T22:26:17Z

Summary

Both create_post and upload_file are default permissions on a ChannelUser and ChannelGuest role. The create_post permission can be removed from either a ChannelUser or a ChannelGuest in the System Console. However, there is not a way to remove the upload_file permission.

The upload_file is checked when a user attempts to upload a file via API. Therefore, a user with create_post removed can still upload a file via API.

This PR ties the two permission together for the ChannelUser and ChannelGuest roles. It also createa a database migration to remove upload_file where the create_post permission does not exist.

Ticket Link

Fixes https://mattermost.atlassian.net/browse/MM-58525

Screenshots

Release Note

NONE

mm-cloud-bot · 2024-06-06T22:26:20Z

@sbishel: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

I understand the commands that are listed here

sbishel · 2024-06-11T21:07:14Z

/update-branch

sbishel

Batching the data migrations is probably overkill. There would only be 2 rows per Team Schema Override, so it would take 50 schema overrides (very doubtful) and all would have to have "create_post" removed for the loop to occur.

But...the documentation provided the code sample, so it was easy enough to add.

unified-ci-app · 2024-06-12T22:15:02Z

Not triggering E2E tests: this PR is a draft. Mark this PR as ready for review, before triggering the E2E tests.

unified-ci-app · 2024-06-12T22:15:03Z

Not triggering E2E tests: this PR is a draft. Mark this PR as ready for review, before triggering the E2E tests.

sbishel · 2024-06-12T22:20:56Z

server/channels/db/migrations/mysql/000122_remove_upload_file_permission.up.sql

+        -- update affected rows
+        UPDATE Roles 
+            SET Permissions = REGEXP_REPLACE(Permissions, 'upload_file[[:space:]|?]', '')
+            WHERE Permissions like '%upload_file%' and Permissions not REGEXP 'create_post[[:space:]|?]'


There is a permission create_post_public. Need to use regex, to ensure this one is not selected.

server/channels/db/migrations/postgres/000122_remove_upload_file_permission.up.sql

lindy65

Thanks @sbishel - looks good to me 👍

mm-cloud-bot · 2024-06-21T17:40:37Z

Test server destroyed

sbishel · 2024-06-24T14:49:30Z

/update-branch

amyblais · 2024-06-24T17:18:25Z

/cherry-pick release-9.10

mattermost-build · 2024-06-24T17:18:27Z

Cherry pick is scheduled.

mattermost-build · 2024-06-24T17:18:34Z

Error trying doing the automated Cherry picking. Please do this manually

+++ Updating remotes...
Fetching upstream
hostfile_replace_entries: mkstemp: Read-only file system
update_known_hosts: hostfile_replace_entries failed for /app/.ssh/known_hosts: Read-only file system
From github.com:mattermost/mattermost
 * [new branch]            MM-56073              -> upstream/MM-56073
 * [new branch]            MM-56339-add-to-audit-logs -> upstream/MM-56339-add-to-audit-logs
   97435bb285..9712a77251  MM-59047-keyboard-shortcut-modal-improvements -> upstream/MM-59047-keyboard-shortcut-modal-improvements
   56e952f681..bff64b6681  del-markdown-no-rende -> upstream/del-markdown-no-rende
   6a2078ecf0..71c25fb316  master                -> upstream/master
   72b91915a7..4952d11103  release-9.10          -> upstream/release-9.10
Fetching upstream
hostfile_replace_entries: mkstemp: Read-only file system
update_known_hosts: hostfile_replace_entries failed for /app/.ssh/known_hosts: Read-only file system
+++ Updating remotes done...
+++ Creating local branch automated-cherry-pick-of-mattermost-#27298-upstream-release-9.10-1719249508
Switched to a new branch 'automated-cherry-pick-of-mattermost-#27298-upstream-release-9.10-1719249508'
Branch 'automated-cherry-pick-of-mattermost-#27298-upstream-release-9.10-1719249508' set up to track remote branch 'release-9.10' from 'upstream'.

+++ About to attempt cherry pick of PR #27298 with merge commit 71c25fb316cfcda9b9af09df9318e153d5e7e66a.

Auto-merging server/channels/db/migrations/migrations.list
CONFLICT (content): Merge conflict in server/channels/db/migrations/migrations.list
error: could not apply 71c25fb316... MM-58525 Fix upload file permissions (#27298)
hint: After resolving the conflicts, mark them with
hint: "git add/rm <pathspec>", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

+++ Conflicts detected:

UU server/channels/db/migrations/migrations.list
Aborting.

+++ Aborting in-progress git cherry-pick.

+++ Returning you to the master branch and cleaning up.

amyblais · 2024-06-24T18:44:11Z

@sbishel Can you help submit a manual cherry-pick PR for release-9.10 branch?

amyblais · 2024-07-02T16:15:58Z

/cherry-pick release-9.10

mattermost-build · 2024-07-02T16:16:00Z

Cherry pick is scheduled.

mattermost-build · 2024-07-02T16:16:17Z

Error trying doing the automated Cherry picking. Please do this manually

+++ Updating remotes...
Fetching upstream
hostfile_replace_entries: mkstemp: Read-only file system
update_known_hosts: hostfile_replace_entries failed for /app/.ssh/known_hosts: Read-only file system
From github.com:mattermost/mattermost
 * [new branch]            CLD-7958-plugin-store-migration -> upstream/CLD-7958-plugin-store-migration
   db936e5b53..af35818df9  MM-56073             -> upstream/MM-56073
 * [new branch]            MM-56774             -> upstream/MM-56774
   e03d32f63c..0b93f2aff3  MM-56904_getGMsForLoading -> upstream/MM-56904_getGMsForLoading
   1123a07a98..4b208a86f6  MM-58378             -> upstream/MM-58378
   6bab2667d1..19002704a9  MM-58535_attribution -> upstream/MM-58535_attribution
   4c276e3965..49f4290326  es8                  -> upstream/es8
   70b218839f..1f9c9486b8  master               -> upstream/master
 * [new branch]            metadata-spec_v2     -> upstream/metadata-spec_v2
   cc5f6d46b0..d9bd84cea8  release-9.10         -> upstream/release-9.10
   55b24d27c8..98e51c9d45  release-9.5          -> upstream/release-9.5
   d45148a458..05b7845bbc  release-9.7          -> upstream/release-9.7
   33db68643e..5de01512cc  release-9.8          -> upstream/release-9.8
 * [new branch]            releasetesting-without-new-permissions -> upstream/releasetesting-without-new-permissions
   834da302f6..0f9c893ec0  remotecluster-endpoints -> upstream/remotecluster-endpoints
 * [new branch]            sysadmin_manage_user_settings -> upstream/sysadmin_manage_user_settings
 * [new branch]            user_report_permission_fix -> upstream/user_report_permission_fix
 * [new tag]               v9.5.7               -> v9.5.7
 * [new tag]               v9.7.6               -> v9.7.6
 * [new tag]               v9.8.2               -> v9.8.2
 * [new tag]               v9.10.0              -> v9.10.0
 * [new tag]               v9.10.0-rc3          -> v9.10.0-rc3
Fetching upstream
hostfile_replace_entries: mkstemp: Read-only file system
update_known_hosts: hostfile_replace_entries failed for /app/.ssh/known_hosts: Read-only file system
+++ Updating remotes done...
+++ Creating local branch automated-cherry-pick-of-mattermost-#27298-upstream-release-9.10-1719936962
Switched to a new branch 'automated-cherry-pick-of-mattermost-#27298-upstream-release-9.10-1719936962'
Branch 'automated-cherry-pick-of-mattermost-#27298-upstream-release-9.10-1719936962' set up to track remote branch 'release-9.10' from 'upstream'.

+++ About to attempt cherry pick of PR #27298 with merge commit 71c25fb316cfcda9b9af09df9318e153d5e7e66a.

Auto-merging server/channels/db/migrations/migrations.list
CONFLICT (content): Merge conflict in server/channels/db/migrations/migrations.list
error: could not apply 71c25fb316... MM-58525 Fix upload file permissions (#27298)
hint: After resolving the conflicts, mark them with
hint: "git add/rm <pathspec>", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

+++ Conflicts detected:

UU server/channels/db/migrations/migrations.list
Aborting.

+++ Aborting in-progress git cherry-pick.

+++ Returning you to the master branch and cleaning up.

amyblais · 2024-07-02T16:16:50Z

@sbishel Can you help submit manual cherry-picks PR for release-9.10, release-9.9, release-9.8, and release-9.5 branches?

* tie create_post and upload_file permissions together * update tests * update file name * update migration to do in batches * Update 000122_remove_upload_file_permission.up.sql * fix formatting * simplify migrations, fix regex issue * update file names for recent merge --------- Co-authored-by: Mattermost Build <build@mattermost.com> (cherry picked from commit 71c25fb)

Automatic Merge

tie create_post and upload_file permissions together

68af626

mm-cloud-bot added the do-not-merge/release-note-label-needed label Jun 6, 2024

sbishel added 2 commits June 7, 2024 11:11

update tests

4e32849

update file name

e73e05f

mm-cloud-bot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed labels Jun 11, 2024

mattermost-build and others added 4 commits June 12, 2024 00:07

Merge branch 'master' into MM-58525-ReadOnly-Upload

9691eb7

update migration to do in batches

d721c93

Update 000122_remove_upload_file_permission.up.sql

c304885

fix formatting

12a649d

sbishel commented Jun 12, 2024

View reviewed changes

sbishel added 2: Dev Review Requires review by a developer 3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review 3: Security Review Review requested from Security Team labels Jun 12, 2024

sbishel commented Jun 12, 2024

View reviewed changes

sbishel marked this pull request as ready for review June 12, 2024 22:32

sbishel requested a review from a team as a code owner June 12, 2024 22:32

sbishel requested review from esarafianou and mgdelacroix June 12, 2024 22:33

agnivade reviewed Jun 13, 2024

View reviewed changes

server/channels/db/migrations/postgres/000122_remove_upload_file_permission.up.sql Outdated Show resolved Hide resolved

simplify migrations, fix regex issue

f502354

agnivade approved these changes Jun 14, 2024

View reviewed changes

Merge branch 'master' into MM-58525-ReadOnly-Upload

e9cf785

lindy65 approved these changes Jun 21, 2024

View reviewed changes

Merge branch 'master' into MM-58525-ReadOnly-Upload

436765b

sbishel merged commit 71c25fb into mattermost:master Jun 24, 2024
33 checks passed

sbishel deleted the MM-58525-ReadOnly-Upload branch June 24, 2024 16:47

amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation and removed Changelog/Not Needed Does not require a changelog entry labels Jun 26, 2024

mattermost-build pushed a commit that referenced this pull request Jul 3, 2024

MM-58525 Fix upload file permissions (#27298) (#27530)

6147c51

Automatic Merge

mattermost-build pushed a commit that referenced this pull request Jul 3, 2024

MM-58525 Fix upload file permissions (#27298) (#27531)

ea78382

Automatic Merge

mattermost-build pushed a commit that referenced this pull request Jul 3, 2024

MM-58525 Fix upload file permissions (#27298) (#27532)

06249d9

Automatic Merge

mattermost-build pushed a commit that referenced this pull request Jul 5, 2024

MM-58525 Fix upload file permissions (#27298) (#27533)

b9c444c

Automatic Merge

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MM-58525 Fix upload file permissions #27298

MM-58525 Fix upload file permissions #27298

sbishel commented Jun 6, 2024 •

edited

Loading

mm-cloud-bot commented Jun 6, 2024

sbishel commented Jun 11, 2024

sbishel left a comment •

edited

Loading

unified-ci-app bot commented Jun 12, 2024

unified-ci-app bot commented Jun 12, 2024

sbishel Jun 12, 2024

lindy65 left a comment

mm-cloud-bot commented Jun 21, 2024

sbishel commented Jun 24, 2024

amyblais commented Jun 24, 2024

mattermost-build commented Jun 24, 2024

mattermost-build commented Jun 24, 2024

amyblais commented Jun 24, 2024

amyblais commented Jul 2, 2024

mattermost-build commented Jul 2, 2024

mattermost-build commented Jul 2, 2024

amyblais commented Jul 2, 2024

MM-58525 Fix upload file permissions #27298

MM-58525 Fix upload file permissions #27298

Conversation

sbishel commented Jun 6, 2024 • edited Loading

Summary

Ticket Link

Screenshots

Release Note

mm-cloud-bot commented Jun 6, 2024

sbishel commented Jun 11, 2024

sbishel left a comment • edited Loading

Choose a reason for hiding this comment

unified-ci-app bot commented Jun 12, 2024

unified-ci-app bot commented Jun 12, 2024

sbishel Jun 12, 2024

Choose a reason for hiding this comment

lindy65 left a comment

Choose a reason for hiding this comment

mm-cloud-bot commented Jun 21, 2024

sbishel commented Jun 24, 2024

amyblais commented Jun 24, 2024

mattermost-build commented Jun 24, 2024

mattermost-build commented Jun 24, 2024

amyblais commented Jun 24, 2024

amyblais commented Jul 2, 2024

mattermost-build commented Jul 2, 2024

mattermost-build commented Jul 2, 2024

amyblais commented Jul 2, 2024

sbishel commented Jun 6, 2024 •

edited

Loading

sbishel left a comment •

edited

Loading