Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sanitize user following config for ShowFullName and ShowEmailAddress #4820

Merged
merged 4 commits into from
Jul 26, 2023

Conversation

enahum
Copy link
Contributor

@enahum enahum commented Jul 24, 2023

@enahum enahum added 2: Dev Review Requires review by a core committer 3: Security Review Review requested from Security Team labels Jul 24, 2023
Copy link
Contributor

@jespino jespino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@enahum
Copy link
Contributor Author

enahum commented Jul 24, 2023

@mgdelacroix any idea why the tests are failing?

@esarafianou
Copy link

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
Copy link
Contributor

@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
Copy link
Contributor Author

enahum commented Jul 25, 2023

@mgdelacroix thanks.

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

@mgdelacroix
Copy link
Contributor

mgdelacroix commented Jul 25, 2023

@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
Copy link
Contributor Author

enahum commented Jul 25, 2023

@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
Copy link
Contributor

mgdelacroix commented Jul 25, 2023

@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

@mgdelacroix mgdelacroix removed the 2: Dev Review Requires review by a core committer label Jul 25, 2023
Copy link

@esarafianou esarafianou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@esarafianou esarafianou removed the 3: Security Review Review requested from Security Team label Jul 26, 2023
@enahum enahum added the 3: Reviews Complete All reviewers have approved the pull request label Jul 26, 2023
@enahum enahum merged commit 3625c53 into main Jul 26, 2023
16 checks passed
@enahum enahum deleted the MM-53190 branch July 26, 2023 12:42
@sbishel
Copy link
Collaborator

sbishel commented Aug 8, 2023

/cherry-pick release-7.11

@mattermost-build
Copy link
Contributor

Cherry pick is scheduled.

@mattermost-build mattermost-build added the CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone label Aug 8, 2023
mattermost-build pushed a commit to mattermost-build/focalboard that referenced this pull request Aug 8, 2023
sbishel added a commit that referenced this pull request Aug 8, 2023
…of-focalboard-#4820-upstream-release-7.11

Automated cherry pick of #4820
@sbishel
Copy link
Collaborator

sbishel commented Aug 15, 2023

/cherry-pick release-7.10

@sbishel
Copy link
Collaborator

sbishel commented Aug 15, 2023

/cherry-pick release-7.8

@mattermost-build
Copy link
Contributor

Cherry pick is scheduled.

mattermost-build pushed a commit to mattermost-build/focalboard that referenced this pull request Aug 15, 2023
@mattermost-build
Copy link
Contributor

Cherry pick is scheduled.

@mattermost-build
Copy link
Contributor

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.

sbishel pushed a commit to sbishel/focalboard that referenced this pull request Aug 17, 2023
sbishel added a commit that referenced this pull request Aug 17, 2023
…of-focalboard-#4820-upstream-release-7.10

Automated cherry pick of #4820
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3: Reviews Complete All reviewers have approved the pull request CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants