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-223] Fixing Post Permissions Access #233

Merged
merged 8 commits into from
Nov 11, 2021
Merged

Conversation

jupriano
Copy link
Contributor

Summary

Prevent user post zoom call if channel permissions make member with read-only permissions

Ticket Link

#223

@jupriano jupriano requested a review from larkox as a code owner October 15, 2021 06:18
@mattermod
Copy link
Contributor

Hello @vicky-demansol,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

@larkox larkox requested a review from mickmister October 15, 2021 08:15
@larkox larkox added the 2: Dev Review Requires review by a core committer label Oct 15, 2021
Copy link
Contributor

@larkox larkox left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@mickmister mickmister left a comment

Choose a reason for hiding this comment

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

@vicky-demansol Thanks for implementing this. Could you please update the unit tests to take this check into account? Thank you!

@mickmister
Copy link
Contributor

Please note that there are CI failures occurring in this PR, including tests failing:

image

@hanzei
Copy link
Collaborator

hanzei commented Oct 26, 2021

/update-branch

Copy link
Collaborator

@hanzei hanzei left a comment

Choose a reason for hiding this comment

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

Per https://community-daily.mattermost.com/core/pl/os7iatp5jifhzbgssqw8gxu3xw the CI issues are unrelated. Approving as it is.

@hanzei hanzei added 3: QA Review Requires review by a QA tester and removed 2: Dev Review Requires review by a core committer labels Oct 26, 2021
@hanzei hanzei requested a review from dipak-demansol October 26, 2021 14:32
@hanzei hanzei added this to the v2.0.0 milestone Oct 26, 2021
@@ -303,6 +303,10 @@ func (p *Plugin) postMeeting(creator *model.User, meetingID int, channelID strin
topic = defaultMeetingTopic
}

if !p.API.HasPermissionToChannel(creator.Id, channelID, model.PERMISSION_CREATE_POST) {
return errors.New("this channel is not accessible, you might not have permissions to write in this channel. Contact the administrator of this channel to find out if you have access permissions")
Copy link
Contributor

Choose a reason for hiding this comment

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

@vicky-demansol We still need a unit test that covers the inside of this if statement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was add the unit test but the server response will return http code = 500, this response could be it returned as "forbidden" or "unauthorized"

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @vicky-demansol, please make sure you mention the person you're replying to on GitHub issues/PRs, so we can stay up-to-date on discussions like this.

I was add the unit test but the server response will return http code = 500, this response could be it returned as "forbidden" or "unauthorized"

Can we create a unit test just to cover that this line will run? If it take a lot of restructure to accomplish changing the response code, let's just make sure this error occurs in general.

Copy link
Contributor Author

@jupriano jupriano Nov 9, 2021

Choose a reason for hiding this comment

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

@mickmister i was trying, if we want to get http code error need to use appError but i don't think this function can use in general, but this code on code coverage was covered

@emilyacook
Copy link

Thanks for participating in Hacktoberfest! You can claim your sticker set here: https://get.printfection.com/hacktober21/4144583267 @vicky-demansol

@hanzei hanzei requested a review from mickmister November 8, 2021 11:22
@dipak-demansol
Copy link

@hanzei i tested this and its working but i have some question so after a call with @DHaussermann i will add the final review at the end of the day.

Copy link

@dipak-demansol dipak-demansol left a comment

Choose a reason for hiding this comment

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

LGTM

@dipak-demansol dipak-demansol removed the 3: QA Review Requires review by a QA tester label Nov 10, 2021
@dipak-demansol dipak-demansol added the QA Review Done PR has been approved by QA label Nov 10, 2021
@hanzei hanzei added 4: Reviews Complete All reviewers have approved the pull request and removed QA Review Done PR has been approved by QA labels Nov 11, 2021
@hanzei hanzei merged commit a13ca5c into mattermost:master Nov 11, 2021
This was referenced Aug 5, 2022
mickmister added a commit that referenced this pull request Aug 16, 2022
* Cross-plugin task: Enable the CircleCI "test" job in each plugin repo that has a webapp plugin (#237)

Co-authored-by: maisnamrajusingh <[email protected]>

* [MM-223] Fixing Post Permissions Access (#233)

* For non-cloud plugin settings, set hosting property to `on-prem` (#269)

* set hosting to on-prem for `Enable OAuth`, `APIKey`, and `APISecret` plugin settings

* use actual server commit

* force oauth to be used when using cloud version of the plugin

* bump go mod version

* test circleci config

* remove references to plugin-ci

* bump Go version in circleci

* use cimg

* add other cimg changes

* go mod tidy

* revert circleci changes

* revert build/go.mod changes so we are using the old server version

* extend manifest struct to support hosting field

* Revert "extend manifest struct to support hosting field"

This reverts commit b862b40.

* revert all go mod changes

* remove manifest field name restriction

* add comment explaining commented out code

* reorder conditions for readability

* Fix OAuth token refresh (#253)

* avoid refreshing token if an error occurs while fetching current token

* pass in firstConnect bool

* lint

* explicitly assign firstConnect boolean to local variable

* ensure oauth token is decrypted when fetched from the kv store

* rename functions

* wrap account level token refresh in firstConnect block as well

* Cherrypick "Correct docs and cleanup screenshots"

* Revert "Cherrypick "Correct docs and cleanup screenshots""

This reverts commit 7164075.

* preserve access token when saving encrypted version

* use refresh token for check

Co-authored-by: Mattermod <[email protected]>
Co-authored-by: Ben Schumacher <[email protected]>
Co-authored-by: Daniel Espino García <[email protected]>

* bump to 1.6.1

* add GetLicense call in test

* check nil license features (#277)

Co-authored-by: sibasankarnayak <[email protected]>
Co-authored-by: maisnamrajusingh <[email protected]>
Co-authored-by: Jupri Abel <[email protected]>
Co-authored-by: Mattermod <[email protected]>
Co-authored-by: Ben Schumacher <[email protected]>
Co-authored-by: Daniel Espino García <[email protected]>
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 Hacktoberfest hacktoberfest-accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

User can start meeting in a channel that they do not have appropriate permissions to post in
8 participants