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

update to use AppError.Where() to differentiate errors #24379

Merged
merged 2 commits into from
Sep 8, 2023

Conversation

sbishel
Copy link
Member

@sbishel sbishel commented Aug 24, 2023

Summary

Updates MakeBotNotFoundError to accept a where parameter for the AppError object. This allows us in code to distinguish between a bot not found in the database vs a bot was found, but user doesn't have permissions to the bot.

In the former case, we return as if the bot does not exist. The where value of the object is NOT in the return JSON for the API.

Ticket Link

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

Release Note

NONE

@mm-cloud-bot
Copy link

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

sbishel commented Aug 24, 2023

/update-branch

@mm-cloud-bot 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 Aug 25, 2023
@sbishel sbishel added 2: Dev Review Requires review by a developer 3: Security Review Review requested from Security Team labels Aug 25, 2023
@sbishel sbishel marked this pull request as ready for review August 25, 2023 23:32
@amyblais amyblais removed the 3: Security Review Review requested from Security Team label Aug 29, 2023
@amyblais amyblais added this to the v9.1.0 milestone Aug 29, 2023
@amyblais amyblais added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a developer labels Sep 5, 2023
@sbishel sbishel requested a review from lindy65 September 5, 2023 14:46
@amyblais amyblais added 3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review and removed 4: Reviews Complete All reviewers have approved the pull request labels Sep 5, 2023
@mattermost-build
Copy link
Contributor

You don't have permissions to trigger this command.
It's only available for organization members.

@lindy65 lindy65 added the Setup Cloud Test Server Setup an on-prem test server label Sep 6, 2023
@mm-cloud-bot mm-cloud-bot removed the Setup Cloud Test Server Setup an on-prem test server label Sep 6, 2023
@lindy65 lindy65 added the Setup Cloud Test Server Setup an on-prem test server label Sep 6, 2023
@sbishel
Copy link
Member Author

sbishel commented Sep 7, 2023

@lindy65 - Here are the QA Test steps, let me know if they don't make sense.

Using the test server, run a GET request to retrieve avaliable Bots. (Using admin token)

curl --location --request GET "https://mattermost-pr-24379.test.mattermost.cloud/api/v4/bots" \
--header "Authorization: Bearer 1eckxc9kkjybj8tfhq1dphy1ar"

Get one of the available token Ids and run a GET request to retrieve that specific Bot. (Using Admin Token)

curl --location --request GET "https://mattermost-pr-24379.test.mattermost.cloud/api/v4/bots/n8wqaycnntndbfaqyhauz8zyur" \
--header "Authorization: Bearer 1eckxc9kkjybj8tfhq1dphy1ar"

You should successfully retrieve as a sysadmin.
Rerun the GET request with a User's Token. You should retrieve the following exception:
{"id": "store.sql_bot.get.missing.app_error", "message": "Bot does not exist."}

Change the last character of the valid id, to make an invalid id. You should receive the same exception when using a normal User's token.

Copy link
Contributor

@lindy65 lindy65 left a comment

Choose a reason for hiding this comment

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

Thanks so much for the detailed test steps @sbishel. I had to revive my Postman skills but thanks to your help with the test steps, it wasn't too much of an ordeal 😅

Looks good to me as per your test steps 👍

@lindy65 lindy65 added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review Setup Cloud Test Server Setup an on-prem test server labels Sep 8, 2023
@lindy65 lindy65 added the QA/Review Done QA review is completed but other reviews are outstanding (exception to usual process) label Sep 8, 2023
@mm-cloud-bot
Copy link

Test server destroyed

@sbishel sbishel merged commit 15faf4a into mattermost:master Sep 8, 2023
26 checks passed
@sbishel sbishel deleted the MM-53098-update branch September 8, 2023 14:18
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Sep 8, 2023
ctlaltdieliet pushed a commit to ctlaltdieliet/mattermost-server that referenced this pull request Sep 19, 2023
)

Co-authored-by: Mattermost Build <build@mattermost.com>
ctlaltdieliet pushed a commit to ctlaltdieliet/mattermost-server that referenced this pull request Sep 19, 2023
)

Co-authored-by: Mattermost Build <build@mattermost.com>
ctlaltdieliet pushed a commit to ctlaltdieliet/mattermost-server that referenced this pull request Sep 19, 2023
)

Co-authored-by: Mattermost Build <build@mattermost.com>
@jwilander jwilander added the kind/refactor Categorizes issue or PR as related to refactor of production code. label Nov 21, 2023
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 Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation kind/refactor Categorizes issue or PR as related to refactor of production code. QA/Review Done QA review is completed but other reviews are outstanding (exception to usual process) release-note-none Denotes a PR that doesn't merit a release note.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants