Plugin should avoid using revoked tokens

[mickmister]

If we perform an API request using a user's token, and we receive a response noting that the token is revoked, we should:

• delete the token from the KV store, declaring the user as disconnected
• and notify the user via DM that their token has been revoked, and that they will need to reconnect their account in order to use the plugin

Issue created from a Mattermost message by @thiefmaster.

[MatthewDorner]

I would like to work on this.

[mickmister]

Great! Thank you @MatthewDorner!

[MatthewDorner]

I'm adding checks for the "401 {error: invalid_token}" error in the places where code from main calls into the gitlab client package. I believe I need to handle it in main in order to Disconnect user and send the bot message.

But I'm afraid it is complicating the error handling code in main a bit and may be prone to bugs when further changes are made in main. It's unfortunate since all calls into gitlab would need the exact same check but I can't find a way to avoid duplicating it. I'd need to add like 10 instances of checking such as:

hooks, err := p.GitlabClient.GetGroupHooks(ctx, user, namespace)
	if err != nil {
		if strings.Contains(err.Error(), "401 {error: invalid_token}") {
			p.handleRevokedToken(user)
		}
		return false, errors.New("unable to connect to GitLab")
	}

where the second if statement is what's added to the existing check at: https://github.com/mattermost/mattermost-plugin-gitlab/blob/master/server/plugin.go#L570

[MatthewDorner]

Just waiting to see the resolution of https://github.com/mattermost/mattermost-plugin-gitlab/pull/298, but may have another issue.

In the case where the token is revoked AND expired, the attempt to renew the token will fail and there, I would like to Disconnect the account and DM the user. However, the call to p.disconnectGitlabAccount will call into p.getGitlabUserInfoByMattermostID again which will then call (in the PR above, at least) p.checkAndRefreshToken and attempt to refresh the token, causing an infinite loop.

I don't think this should block https://github.com/mattermost/mattermost-plugin-gitlab/pull/298 but wondering if this will require some more extensive refactoring

[MatthewDorner]

Issue created from a Mattermost message by @thiefmaster.

I was trying to see what specific need prompted this Issue, but the link doesn't work for me.

[ThiefMaster]

Ping :)

I was trying to see what specific need prompted this Issue, but the link doesn't work for me.

I'd say the "specific need" is simply being a "polite API user". Repeating requests with a token you know is no longer valid is not nice. Some APIs might even (rightfully!) block/throttle you if a client does this too often (luckily GitLab doesn't).

It also spams my log file with errors about the invalid tokens, which makes it hard to see if there's anything useful in the log file.

[mickmister]

@ThiefMaster

1. Are you running at least v1.5.0 of the plugin?
2. If so, can you ask your users to reconnect their GitLab accounts, if their token is not working?

We are in the process of a fix that involves automatically disconnecting users that try to use an expired token, and pinging them to reconnect their account.

[ThiefMaster]

Besides there not being an option to get a list of users who use the plugin / have invalid tokens, I don't think that scales well (I'd have to @all ping a major channel where hopefully most people are). So I'll rather wait a bit until it's fixed properly instead of manually pinging my users..