[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? |
@mickmister @DHaussermann @hanzei should Brightscout handle the QA part for this PR? |
|
@Kshitij-Katiyar Yes that sounds good 👍 |
hanzei
requested review from
AayushChaudhary0001
and removed request for
DHaussermann
|
@mickmister This PR contains changes that QA cannot test directly. We will need to make changes in the code to test it (Like manually decreasing the expiry time or deleting the token from the KV store), which has already been done while development. What are your opinions on skipping the QA review on this PR? |
|
@raghavaggarwal2308 Are we able to write a test that ensures this works correctly? |
…esh token expires'
@mickmister Writing the test cases for this will be a little difficult as first of all there are no existing test cases present for the ms graph package and secondly, we are using an SDK package for making API calls to the external portal and we have to mock it to return a particular refresh token expired error. To do that we have to create a wrapper function above all the API calls and include those in an interface and only then we can create mocks for it secondly we can use monkey patching but that is not that stable. |
| @@ -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
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