mattermost-plugin-gitlab
mattermost-plugin-gitlab copied to clipboard
mattermost
GitLab users are not disconnected when their Mattermost account is deactivated
"I noticed that the gitlab plugin record is not always expiring or deleting when the user's mattermost account is made inactive by an LDAP sync."
Issue created from a Mattermost message by @sjenkins.
Note that in order to properly support this, we will need to have a new plugin hook UserHasBeenDeactivated
Why should the gitlab-mattermost connection be deleted when the Mattermost account is disabled? If the account get's enabled again, the existing connection be in place, right?
@hanzei To comply with GDPR, we should clean up this information when the user disconnects their account. Note that I'm basing this off of Atlassian's policy https://developer.atlassian.com/cloud/jira/platform/user-privacy-developer-guide/#storing-user-personal-data-for-your-apps
@dimashasbi You will need to add a new plugin hook to the plugin framework as the first part of this task. You can see in https://github.com/mattermost/mattermost-server how this is done, in the plugin package.
The function signature should look like:
func UserHasBeenDeactivated(userID string, permanentDelete bool)
Then this plugin will implement this hook, and will delete the user's tokens when this function is called.
Hi @MatthewDorner Sorry for the late response here. Would you still like to take this ticket?
The function signature should look like:
func UserHasBeenDeactivated(userID string, permanentDelete bool)
@mickmister It would be easier on the server side if we didn't need the permanentDelete argument. Since I only want to call the plugin hook from one place, I think that would be inside App.UpdateActive (https://github.com/mattermost/mattermost-server/blob/7643a28263f3afd9b87c721daf100beb070516c3/app/user.go#L929), which itself is called from several different places.
So I'd have to add a parameter permanentDelete to UpdateActive and decide whether it should be true or false for each existing place where it's called, which would seem to overload the responsibility of UpdateActive as it now needs to know whether it's being called as part of a delete operation or not.
If a plugin really does need to know the difference between a user being deactivated and deleted, imo these could be two separate hooks instead.
For the purposes here, it sounds like we only need to know when they're deactivated.
@MatthewDorner The idea behind the permanentDelete is to allow the plugin to use its own discretion to determine the level of data deletion it should perform itself. Maybe this isn't actually necessary. If the user is deactivated, they are deactivated, and should have their OAuth tokens etc. removed from the database. Let's get rid of the permanentDelete boolean.
@mickmister Since this issue was created due to the case where user is made inactive via LDAP sync, I want to make sure my solution works for that. But I think the LDAP sync code might be in a different, closed repository because it's an Enterprise feature? Is that correct? I see only the interface at einterfaces.LdapInterface but not the implementation of StartSynchronizeJob, for instance.
If it uses the API or the App methods to update the user, which I imagine it does one of those, it should be fine.
@MatthewDorner Thanks for the thorough investigation :+1: Looks like the enterprise code is calling this:
app.UpdateActive(c, existingUser, false)
So as long as it's handled in that case we should be good. In the case of deactivating from the UI, it looks like UpdateUserActive calls this function, so UpdateActive is the one we want to have this change.