[GH-198]:Fixed issue #198 'Handle case where a user's refresh token expires' #256
Conversation
|
Hello @Kshitij-Katiyar, 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. |
|
@m1lt0n Not able to add you as a reviewer here. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #256 +/- ##
==========================================
+ Coverage 23.71% 27.31% +3.59%
==========================================
Files 62 44 -18
Lines 3087 2541 -546
==========================================
- Hits 732 694 -38
+ Misses 2274 1771 -503
+ Partials 81 76 -5 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Great work on this @Kshitij-Katiyar 👍
I have a few requests mainly pertaining to avoiding DM'ing a user multiple times, and putting in some defensive code to avoid token expiry issues.
|
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
|
@Kshitij-Katiyar This issue has become a more pressing issue, so I would prioritize this PR if possible. There some remaining change requests, though the main one is here #256 (comment) |
|
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
mickmister
added
3: QA Review
|
@Kshitij-Katiyar Can you please provide testing steps for this PR for @DHaussermann |
@DHaussermann Testing steps:-
|
|
Hi @Kshitij-Katiyar Also I'm a bit stuck here because the token could take possibly weeks to expire and simply by invalidating it in the data store, am I thang just hitting a different code path? If so, I'm not sure if there is functional testing I can cover here. Maybe some type of test build that will mimic the refresh process is an option? |
| @@ -295,6 +305,79 @@ func (s *pluginStore) StoreUserActiveEvents(mattermostUserID string, events []st | |||
| return kvstore.StoreJSON(s.userKV, mattermostUserID, u) | |||
| } | |||
|
|
|||
| // RefreshAndStoreToken checks whether the current access token is expired or not. If it is, | |||
| // then it refreshes the token and stores the new pair of access and refresh tokens in kv store. | |||
| func (s *pluginStore) RefreshAndStoreToken(token *oauth2.Token, oconf *oauth2.Config, mattermostUserID string) (*oauth2.Token, error) { | |||
There was a problem hiding this comment.
@Kshitij-Katiyar This is where much of the functionality was added in this PR. I'm thinking we can test this function and CheckUserConnected directly. What do you think?
We basically want to emulate this in a test since this is the code that is applied throughout the PR:
if !c.tokenHelpers.CheckUserConnected(c.mattermostUserID) {
c.Logger.Warnf(LogUserInactive, c.mattermostUserID)
return nil, errors.New(ErrorUserInactive)
}
err := someCallToAPI // this can be whatever we want in the test
if err != nil {
c.tokenHelpers.DisconnectUserFromStoreIfNecessary(err, c.mattermostUserID)We can use a "real" KV store in the test with one of the following strategies:
There was a problem hiding this comment.
@wiggin77 Writing test cases for these functions without refactoring is challenging because many of the functions used within them are standalone and not implemented by any interface, making it difficult to mock them.
One workaround is to use monkeyPatch. However, this approach isn't currently used in the plugin, and introducing it just for this test case might not be ideal. Additionally, no test cases have been added for any store or msgraph functions so far, possibly because adding them is not straightforward.
There was a problem hiding this comment.
This method can be unit tested with a memory store.
There was a problem hiding this comment.
Hi @wiggin77,
I looked into the memoryStore and its implementation in Mattermost. It seems useful for mocking the config store in Mattermost and other similar stores within Mattermost plugins. However, I’m unsure how it applies to our current use case. Could you provide some guidance on this?
There was a problem hiding this comment.
A solution needs to be found here. If the implementation does not lend itself to writing clean unit tests, then the implementation needs to be refactored.
…lendar into MM-198
…lendar into MM-198
…lendar into MM-198
|
There was an issue with the execution slash commands involving any external API call if executed with an expired or invalid refresh token. Fixed it in this commit by checking for the expired token error in execution of related slash commands |
|
@wiggin77 This PR has been dev tested properly. I do not think we need to get it QA tested as it won't be readily tested because we have to manually go into the DB and change the refresh token multiple times. Please let me know your thoughts on merging this? |
|
/update-branch |
|
We don't have permissions to update this PR, please contact the submitter to apply the update. |
Ok |
raghavaggarwal2308
added
4: Reviews Complete
Summary
Added the logic that if an API request fails, with the error refresh token expired then marking the user inactive in the KV store and also sending a DM to the user to reconnect their account.
If the user is marked as inactive, aborting any future API requests done for this user, and logging via the WARN log level.
Issue #198