mattermost-community / focalboard

Focalboard is an open source, self-hosted alternative to Trello, Notion, and Asana.
https://www.focalboard.com
Other
22.06k stars 1.99k forks source link

Sanitize user following config for ShowFullName and ShowEmailAddress #4820

Closed enahum closed 1 year ago

enahum commented 1 year ago

Ticket Link

https://mattermost.atlassian.net/browse/MM-53190

enahum commented 1 year ago

@mgdelacroix any idea why the tests are failing?

esarafianou commented 1 year ago

Is there a way to check from the plugin if the requesting user is the System Admin? Because currently the fix is indeed correct but it also applies to System Admins whereas it shouldn't. These two settings don't apply to system admins.

mgdelacroix commented 1 year ago

@enahum tests are failing because we're calling GetConfig on a mock without setting the expectation first:

2023-07-24T14:17:06.8596890Z     mattermostauthlayer_test.go:22: Unexpected call to *mocks.MockServicesAPI.GetConfig([]) at /home/runner/work/focalboard/focalboard/focalboard/server/services/store/mattermostauthlayer/mattermostauthlayer_test.go:22 because: there are no expected calls of the method "GetConfig" for that receiver
2023-07-24T14:17:06.8597086Z --- FAIL: TestGetBoardsBotID (0.00s)

I'd suggest to create the config directly as you're only going to use those two properties, so there is no need to fill the rest of the values

enahum commented 1 year ago

@mgdelacroix thanks.

Any idea on how to check if the user is an admin?

mgdelacroix commented 1 year ago

@esarafianou @enahum I think the best path forward is to do the same we're doing in mattermost, which is to sanitize the profile at the API level, as there's we have the permissions service available: https://github.com/mattermost/mattermost/blob/master/server/channels/api4/user.go#L281-L285

enahum commented 1 year ago

@esarafianou @enahum I think the best path forward is to do the same we're doing in mattermost, which is to sanitize the profile at the API level, as there's we have the permissions service available: https://github.com/mattermost/mattermost/blob/master/server/channels/api4/user.go#L281-L285

Sure, but how do I determine if the user is a sysadmin?

mgdelacroix commented 1 year ago

@enahum same as we do in channels, we can use the permissions service to check if the user is an admin through the HasPermissionTo method. Here is an example of use in the API layer

sbishel commented 1 year ago

/cherry-pick release-7.11

mattermost-build commented 1 year ago

Cherry pick is scheduled.

sbishel commented 1 year ago

/cherry-pick release-7.10

sbishel commented 1 year ago

/cherry-pick release-7.8

mattermost-build commented 1 year ago

Cherry pick is scheduled.

mattermost-build commented 1 year ago

Cherry pick is scheduled.

mattermost-build commented 1 year ago

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

+++ Updating remotes...
Fetching upstream
Failed to add the RSA host key for IP address '140.82.112.3' to the list of known hosts (/app/.ssh/known_hosts).
Fetching origin
Failed to add the RSA host key for IP address '140.82.112.3' to the list of known hosts (/app/.ssh/known_hosts).
+++ Updating remotes done...
+++ Creating local branch automated-cherry-pick-of-focalboard-#4820-upstream-release-7.8-1692129590
Switched to a new branch 'automated-cherry-pick-of-focalboard-#4820-upstream-release-7.8-1692129590'
Branch 'automated-cherry-pick-of-focalboard-#4820-upstream-release-7.8-1692129590' set up to track remote branch 'release-7.8' from 'upstream'.

+++ About to attempt cherry pick of PR #4820 with merge commit 3625c535275378e46b0cc4dcf19295cd6d2a275b.

error: could not apply 3625c535... Sanitize user following config for ShowFullName and ShowEmailAddress (#4820)
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add <paths>' or 'git rm <paths>'
hint: and commit the result with 'git commit'

+++ Conflicts detected:

UU server/api/users.go
Aborting.

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

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