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-809]: Fixed the issue of getting errors when using github api with revoked/invalid token #832

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Kshitij-Katiyar
Copy link
Contributor

@Kshitij-Katiyar Kshitij-Katiyar commented Sep 26, 2024

Summary

  • Fixed the issue of getting errors when using GitHub API with revoked/invalid tokens.
  • Created a wrapper function and used it on all the githubClient calls, in case of invalid token error, disconnected the user and DMed him to reconnect

Issue Link

Fixes #809

Testing steps

  • Do a complete sanity on a high level along with the below things on the side
  • Connect your mattermost to Github plugin
  • Revoke the user access token on the Github Oauth app as shown in the screenshot
    Screenshot from 2024-09-27 13-30-52
  • Perform any action which involves API call using the Github client like LHS refresh, LHS button click

@raghavaggarwal2308 raghavaggarwal2308 added this to the v2.4.0 milestone Sep 26, 2024
@wiggin77 wiggin77 added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Oct 22, 2024
@wiggin77
Copy link
Member

/update-branch

@mattermost-build
Copy link
Contributor

Error trying to update the PR.
Please do it manually.

@Kshitij-Katiyar
Copy link
Contributor Author

@wiggin77 I have synced this branch with the master

server/plugin/plugin.go Outdated Show resolved Hide resolved
server/plugin/plugin.go Outdated Show resolved Hide resolved
server/plugin/plugin.go Outdated Show resolved Hide resolved
Copy link
Member

@wiggin77 wiggin77 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

server/plugin/api.go Show resolved Hide resolved
server/plugin/api.go Outdated Show resolved Hide resolved
server/plugin/api.go Show resolved Hide resolved
server/plugin/api.go Outdated Show resolved Hide resolved
server/plugin/api.go Outdated Show resolved Hide resolved
server/plugin/api.go Outdated Show resolved Hide resolved
return nil
})
if cErr != nil {
return nil, cErr
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to log the error here or it is handled somewhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is logged from where it is called

server/plugin/plugin.go Outdated Show resolved Hide resolved
server/plugin/plugin.go Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GitHub plugin issues on community after switching to SSO
4 participants