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

MM-61822 Fix the boards LHS and channel RHS boards channel memeber permission issue. #37

Merged
merged 3 commits into from
Nov 20, 2024

Conversation

Rajat-Dabade
Copy link
Contributor

Summary

This PR addresses a regression where channel members could not see boards linked to the channel in the channel's board RHS or the boards LHS. The issue occurred because boards only checked for board membership, which previously relied on Mattermost's auth layer. When the auth layer was removed during the plugin refactoring, the related code was lost.

This fix ensures channel members can now see linked boards in both the channel RHS and the boards LHS. Additionally, I reviewed the removed Mattermost auth layer code and restored all the missing functionality in this PR.

Ticket Link

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

@Rajat-Dabade Rajat-Dabade added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Nov 18, 2024
@Rajat-Dabade Rajat-Dabade self-assigned this Nov 18, 2024
if err != nil {
return nil, err
}
if user.IsGuest {
Copy link
Member

Choose a reason for hiding this comment

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

This is incorrect, we need to return explicitMembers not members if user is guest.

Also, lets move this check to before we fetch the synthetic board memberships.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At this point, member contains only explicitMembers, but yes, you are correct that it should be moved before fetching the synthetic board members.

Done ✅

@harshilsharma63
Copy link
Member

Other than the one comment, it works perfectly locally.

Copy link
Member

@harshilsharma63 harshilsharma63 left a comment

Choose a reason for hiding this comment

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

Can you also add tests for this on store and app level?

@Rajat-Dabade
Copy link
Contributor Author

Can you also add tests for this on store and app level?

@harshilsharma63 Server tests are not configured yet in this repo because it was causing some issues (related to the plugin which needs MM table migration), Maybe this open source Friday I will look into this and then I can add a test for these file changes as well.

@hmhealey
Copy link
Member

Can you also add tests for this on store and app level?

@harshilsharma63 Server tests are not configured yet in this repo because it was causing some issues (related to the plugin which needs MM table migration), Maybe this open source Friday I will look into this and then I can add a test for these file changes as well.

Would those tests possibly have caught this issue before? If we aren't able to run existing tests, that seems like something we should see about getting you some non-OSF time to work on

Copy link
Member

@hmhealey hmhealey left a comment

Choose a reason for hiding this comment

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

It's pretty weird to me that the old code handled explicit and implicit members in separate packages there. I would've expected that to be done in one place, but I guess there were good reasons to do that. I notice that we seem to also be mixing the terms "implicit" and "synthetic" for those as well, but those both sound nicer than the "fake" channels term that we use in the web app 😛

This looks good to me, although the lack of tests for this code is concerning

server/services/store/sqlstore/board.go Outdated Show resolved Hide resolved
server/services/store/sqlstore/board.go Outdated Show resolved Hide resolved
@Rajat-Dabade
Copy link
Contributor Author

Rajat-Dabade commented Nov 20, 2024

@hmhealey I will try to fix the server test issue in this OSF or maybe will create a ticket to get it done.

@Rajat-Dabade Rajat-Dabade added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Nov 20, 2024
@Rajat-Dabade Rajat-Dabade merged commit ba6b2a5 into main Nov 20, 2024
6 checks passed
@hanzei hanzei deleted the permission-issue branch December 10, 2024 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants