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

Adding the remoteID migration, and making the synthetic user identification more robust #539

Merged
merged 13 commits into from
Mar 12, 2024
10 changes: 5 additions & 5 deletions server/command_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1368,7 +1368,7 @@ func TestExecutePromoteCommand(t *testing.T) {
params: []string{"existing-user", "existing-user"},
setupAPI: func(api *plugintest.API) {
api.On("HasPermissionTo", testutils.GetUserID(), model.PermissionManageSystem).Return(true).Times(1)
api.On("GetUserByUsername", "existing-user").Return(&model.User{Id: "test", Username: "existing-user", RemoteId: model.NewString("test")}, nil).Once()
api.On("GetUserByUsername", "existing-user").Return(&model.User{Id: "test", Username: "existing-user", RemoteId: model.NewString("remote-id")}, nil).Once()
api.On("SendEphemeralPost", testutils.GetUserID(), testutils.GetEphemeralPost(p.userID, testutils.GetChannelID(), "Error: Unable to promote account existing-user, it is not a known msteams user account")).Return(testutils.GetPost(testutils.GetChannelID(), testutils.GetUserID(), time.Now().UnixMicro())).Once()
},
setupStore: func(s *mockStore.Store) {
Expand All @@ -1392,7 +1392,7 @@ func TestExecutePromoteCommand(t *testing.T) {
params: []string{"valid-user", "new-user"},
setupAPI: func(api *plugintest.API) {
api.On("HasPermissionTo", testutils.GetUserID(), model.PermissionManageSystem).Return(true).Times(1)
api.On("GetUserByUsername", "valid-user").Return(&model.User{Id: "test", Username: "valid-user", RemoteId: model.NewString("test")}, nil).Once()
api.On("GetUserByUsername", "valid-user").Return(&model.User{Id: "test", Username: "valid-user", RemoteId: model.NewString("remote-id")}, nil).Once()
api.On("GetUserByUsername", "new-user").Return(&model.User{Id: "test2", Username: "new-user", RemoteId: nil}, nil).Once()
api.On("SendEphemeralPost", testutils.GetUserID(), testutils.GetEphemeralPost(p.userID, testutils.GetChannelID(), "Error: the promoted username already exists, please use a different username.")).Return(testutils.GetPost(testutils.GetChannelID(), testutils.GetUserID(), time.Now().UnixMicro())).Once()
},
Expand All @@ -1405,7 +1405,7 @@ func TestExecutePromoteCommand(t *testing.T) {
params: []string{"valid-user", "new-user"},
setupAPI: func(api *plugintest.API) {
api.On("HasPermissionTo", testutils.GetUserID(), model.PermissionManageSystem).Return(true).Times(1)
api.On("GetUserByUsername", "valid-user").Return(&model.User{Id: "test", Username: "valid-user", RemoteId: model.NewString("test")}, nil).Once()
api.On("GetUserByUsername", "valid-user").Return(&model.User{Id: "test", Username: "valid-user", RemoteId: model.NewString("remote-id")}, nil).Once()
api.On("GetUserByUsername", "new-user").Return(nil, &model.AppError{}).Once()
api.On("UpdateUser", mock.Anything).Return(nil, &model.AppError{}).Once()
api.On("SendEphemeralPost", testutils.GetUserID(), testutils.GetEphemeralPost(p.userID, testutils.GetChannelID(), "Error: Unable to promote account valid-user")).Return(testutils.GetPost(testutils.GetChannelID(), testutils.GetUserID(), time.Now().UnixMicro())).Once()
Expand All @@ -1419,7 +1419,7 @@ func TestExecutePromoteCommand(t *testing.T) {
params: []string{"valid-user", "new-user"},
setupAPI: func(api *plugintest.API) {
api.On("HasPermissionTo", testutils.GetUserID(), model.PermissionManageSystem).Return(true).Times(1)
api.On("GetUserByUsername", "valid-user").Return(&model.User{Id: "test", Username: "valid-user", RemoteId: model.NewString("test")}, nil).Once()
api.On("GetUserByUsername", "valid-user").Return(&model.User{Id: "test", Username: "valid-user", RemoteId: model.NewString("remote-id")}, nil).Once()
api.On("GetUserByUsername", "new-user").Return(nil, &model.AppError{}).Once()
api.On("UpdateUser", &model.User{Id: "test", Username: "new-user", RemoteId: nil}).Return(&model.User{Id: "test", Username: "new-user", RemoteId: nil}, nil).Once()
api.On("SendEphemeralPost", testutils.GetUserID(), testutils.GetEphemeralPost(p.userID, testutils.GetChannelID(), "Account valid-user has been promoted and updated the username to new-user")).Return(testutils.GetPost(testutils.GetChannelID(), testutils.GetUserID(), time.Now().UnixMicro())).Once()
Expand All @@ -1433,7 +1433,7 @@ func TestExecutePromoteCommand(t *testing.T) {
params: []string{"valid-user", "valid-user"},
setupAPI: func(api *plugintest.API) {
api.On("HasPermissionTo", testutils.GetUserID(), model.PermissionManageSystem).Return(true).Times(1)
api.On("GetUserByUsername", "valid-user").Return(&model.User{Id: "test", Username: "valid-user", RemoteId: model.NewString("test")}, nil).Times(2)
api.On("GetUserByUsername", "valid-user").Return(&model.User{Id: "test", Username: "valid-user", RemoteId: model.NewString("remote-id")}, nil).Times(2)
api.On("UpdateUser", &model.User{Id: "test", Username: "valid-user", RemoteId: nil}).Return(&model.User{Id: "test", Username: "valid-user", RemoteId: nil}, nil).Times(1)
api.On("SendEphemeralPost", testutils.GetUserID(), testutils.GetEphemeralPost(p.userID, testutils.GetChannelID(), "Account valid-user has been promoted and updated the username to valid-user")).Return(testutils.GetPost(testutils.GetChannelID(), testutils.GetUserID(), time.Now().UnixMicro())).Once()
},
Expand Down
3 changes: 2 additions & 1 deletion server/handlers/handlers.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ type PluginIface interface {
GenerateRandomPassword() string
ChatSpansPlatforms(channelID string) (bool, *model.AppError)
GetSelectiveSync() bool
IsRemoteUser(user *model.User) bool
}

type ActivityHandler struct {
Expand Down Expand Up @@ -596,7 +597,7 @@ func (ah *ActivityHandler) isRemoteUser(userID string) bool {
return false
}

return user.RemoteId != nil && *user.RemoteId != "" && strings.HasPrefix(user.Username, "msteams_")
return ah.plugin.IsRemoteUser(user)
}

func IsDirectMessage(chatID string) bool {
Expand Down
2 changes: 1 addition & 1 deletion server/message_hooks.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import (
)

func (p *Plugin) UserWillLogIn(_ *plugin.Context, user *model.User) string {
if user.RemoteId != nil && *user.RemoteId != "" && p.getConfiguration().AutomaticallyPromoteSyntheticUsers {
if p.IsRemoteUser(user) && p.getConfiguration().AutomaticallyPromoteSyntheticUsers {
*user.RemoteId = ""
if _, appErr := p.API.UpdateUser(user); appErr != nil {
p.API.LogWarn("Unable to promote synthetic user", "user_id", user.Id, "error", appErr.Error())
Expand Down
52 changes: 24 additions & 28 deletions server/plugin.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"crypto/rand"
"crypto/rsa"
"crypto/x509"
"encoding/base32"
"encoding/base64"
"encoding/pem"
"fmt"
Expand All @@ -19,7 +18,6 @@ import (
"time"

"github.com/gosimple/slug"
"github.com/pborman/uuid"
"github.com/pkg/errors"
"golang.org/x/oauth2"

Expand Down Expand Up @@ -484,6 +482,22 @@ func (p *Plugin) onActivate() error {
return err
}

p.remoteID = "msteams"
if !p.getConfiguration().DisableSyncMsg {
Copy link
Member Author

Choose a reason for hiding this comment

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

Can you confirm if this check is needed? I would like to always register the SharedChannels for the plugin.

Copy link
Member Author

Choose a reason for hiding this comment

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

@wiggin77 ☝️

Copy link
Member

Choose a reason for hiding this comment

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

I think we want an easy way to to turn this off in case of a problem in production. However we don't want to use a fake remoteID either since that may be hard to clean up later. I would suggest calling RegisterPluginForSharedChannels always, but then calling UnregisterPluginForSharedChannels if p.getConfiguration().DisableSyncMsg == true. That way you will have the real remoteID.

Copy link
Member Author

Choose a reason for hiding this comment

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

My concern is the remoteID changing over disabling/enabling. Is the remote id going to be stable or it is going to change every time that I unregister the plugin?

Copy link
Member Author

Choose a reason for hiding this comment

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

Anyway, I applied the changes that you proposed.

Copy link
Member

Choose a reason for hiding this comment

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

Either way we want the same remoteID for every unique plugin ID.

This is probably fine for our current purposes, but thinking (very) long term, I could also see us supporting multiple Microsoft tenants concurrently which might necessitate a distinct remoteID for each (if we wanted to, for example, distinguish each in the UI?)

Stepping back, I'm also just wanting to confirm that /we/ (the plugin) don't really care about the value here at all, and that it's just an artifact of the way the shared channels subsystem works with users that happen to have a RemoteID set. In other words, we want all DMs and all GMs for all users, regardless of whether anyone has a RemoteID set. Does that remain accurate @wiggin77?

Copy link
Member

Choose a reason for hiding this comment

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

If you set the appropriate flag when registering then yes, the MS Teams plugin will get sync for all DM/GMs regardless of remoteID value. Other plugins using the API will likely not set that flag and the remoteID on posts/users/etc will matter.

Should we modify the register API to accept a remoteID, thus allowing a plugin to call register for each tenant? That would also leave it up to the plugin to ensure idempotency. cc @jespino @lieut-data

Copy link
Member

Choose a reason for hiding this comment

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

Should we modify the register API to accept a remoteID, thus allowing a plugin to call register for each tenant? That would also leave it up to the plugin to ensure idempotency. cc @jespino @lieut-data

On the one hand, I don't want to preemptively build for the future. But I do like the idea of giving the plugin control over the "primary key" backing the cursor based model. At the moment, it feels just a bit too decoupled. So I'm in favour of the idea. Is the RemoteId constrained by some length we should remain aware of? (i.e. is it long enough for a UUID?)

Copy link
Member

Choose a reason for hiding this comment

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

It's the same length as most IDs in MM.

Copy link
Member

Choose a reason for hiding this comment

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

Circling back on this from a discussion with @jespino: I'm agreeing with his proposal to just use the RemoteID as-is and deal with the multi-tenant situation in the future (if needed). This unblocks things and gets us closer to being able to rely on shared channels for message delivery. So essentially no changes required on this thread.

remoteID, err := p.API.RegisterPluginForSharedChannels(model.RegisterPluginOpts{
Displayname: pluginID,
PluginID: pluginID,
CreatorID: p.userID,
AutoShareDMs: true,
AutoInvited: true,
})
if err != nil {
return err
}
p.remoteID = remoteID
p.API.LogInfo("Registered plugin for shared channels", "remote_id", p.remoteID)
}

if p.store == nil {
if p.apiClient.Store.DriverName() != model.DatabaseDriverPostgres {
return fmt.Errorf("unsupported database driver: %s", p.apiClient.Store.DriverName())
Expand All @@ -502,26 +516,12 @@ func (p *Plugin) onActivate() error {
)
p.store = timerlayer.New(store, p.GetMetrics())

if err = p.store.Init(); err != nil {
if err = p.store.Init(p.remoteID); err != nil {
return err
}
}

if !p.getConfiguration().DisableSyncMsg {
remoteID, err := p.API.RegisterPluginForSharedChannels(model.RegisterPluginOpts{
Displayname: pluginID,
PluginID: pluginID,
CreatorID: p.userID,
AutoShareDMs: true,
AutoInvited: true,
})
if err != nil {
return err
}
p.remoteID = remoteID

p.API.LogInfo("Registered plugin for shared channels", "remote_id", p.remoteID)

linkedChannels, err := p.store.ListChannelLinks()
if err != nil {
p.API.LogError("Failed to list channel links for shared channels", "error", err.Error())
Expand Down Expand Up @@ -662,7 +662,7 @@ func (p *Plugin) syncUsers() {
}
}

if isRemoteUser(mmUser) {
if p.IsRemoteUser(mmUser) {
if msUser.IsAccountEnabled {
// Activate the deactivated Mattermost user corresponding to the MS Teams user.
if mmUser.DeleteAt != 0 {
Expand Down Expand Up @@ -704,7 +704,7 @@ func (p *Plugin) syncUsers() {
if msUser.Type == msteamsUserTypeGuest {
// Check if syncing of MS Teams guest users is disabled.
if !syncGuestUsers {
if isUserPresent && isRemoteUser(mmUser) && mmUser.DeleteAt == 0 {
if isUserPresent && p.IsRemoteUser(mmUser) && mmUser.DeleteAt == 0 {
// Deactivate the Mattermost user corresponding to the MS Teams guest user.
p.API.LogInfo("Deactivating the guest user account", "user_id", mmUser.Id, "teams_user_id", msUser.ID)
if err := p.API.UpdateUserActive(mmUser.Id, false); err != nil {
Expand All @@ -718,13 +718,9 @@ func (p *Plugin) syncUsers() {

username := "msteams_" + slug.Make(msUser.DisplayName)
if !isUserPresent {
userUUID := uuid.Parse(msUser.ID)
encoding := base32.NewEncoding("ybndrfg8ejkmcpqxot1uwisza345h769").WithPadding(base32.NoPadding)
shortUserID := encoding.EncodeToString(userUUID)

newMMUser := &model.User{
Email: msUser.Mail,
RemoteId: &shortUserID,
RemoteId: &p.remoteID,
FirstName: msUser.DisplayName,
Username: username,
}
Expand Down Expand Up @@ -777,7 +773,7 @@ func (p *Plugin) syncUsers() {
if err = p.store.SetUserInfo(newUser.Id, msUser.ID, nil); err != nil {
p.API.LogWarn("Unable to set user info during sync user job", "user_id", newUser.Id, "teams_user_id", msUser.ID, "error", err.Error())
}
} else if mmUser.RemoteId != nil {
} else if p.IsRemoteUser(mmUser) {
shouldUpdate := false
if !strings.HasPrefix(mmUser.Username, "msteams_") && username != mmUser.Username {
mmUser.Username = username
Expand Down Expand Up @@ -848,9 +844,9 @@ func getRandomString(characterSet string, length int) string {
return randomString.String()
}

// isRemoteUser returns true if the given user is a remote user managed by this plugin.
func isRemoteUser(user *model.User) bool {
return user.RemoteId != nil && *user.RemoteId != "" && strings.HasPrefix(user.Username, "msteams_")
// IsRemoteUser returns true if the given user is a remote user managed by this plugin.
func (p *Plugin) IsRemoteUser(user *model.User) bool {
return user.RemoteId != nil && *user.RemoteId == p.remoteID
}

func (p *Plugin) updateMetrics() {
Expand Down
1 change: 1 addition & 0 deletions server/plugin_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ func newTestPlugin(t *testing.T) *Plugin {
clientBuilderWithToken: func(redirectURL, tenantID, clientId, clientSecret string, token *oauth2.Token, apiClient *pluginapi.LogService) msteams.Client {
return clientMock
},
remoteID: "remote-id",
}
plugin.store.(*storemocks.Store).On("Shutdown").Return(nil)
plugin.store.(*storemocks.Store).Test(t)
Expand Down
2 changes: 1 addition & 1 deletion server/selective_sync.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ func (p *Plugin) ChatMembersSpanPlatforms(members model.ChannelMembers) (bool, *
return false, appErr
}

if isRemoteUser(user) {
if p.IsRemoteUser(user) {
// Synthetic users are always remote.
atLeastOneRemoteUser = true
} else if p.getAutomuteIsEnabledForUser(user.Id) {
Expand Down
33 changes: 17 additions & 16 deletions server/selective_sync_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ func setupPlugin(t *testing.T) *Plugin {
t.Helper()

p := &Plugin{}
p.remoteID = "remote-id"

ctx, cancel := context.WithCancel(context.Background())
t.Cleanup(cancel)
Expand Down Expand Up @@ -122,11 +123,11 @@ func TestChatSpansPlatforms(t *testing.T) {

var appErr *model.AppError

user1.RemoteId = model.NewString(model.NewId())
user1.RemoteId = model.NewString("remote-id")
user1, appErr = p.API.UpdateUser(user1)
require.Nil(t, appErr)

user2.RemoteId = model.NewString(model.NewId())
user2.RemoteId = model.NewString("remote-id")
user2, appErr = p.API.UpdateUser(user2)
require.Nil(t, appErr)

Expand All @@ -145,7 +146,7 @@ func TestChatSpansPlatforms(t *testing.T) {

var appErr *model.AppError

user1.RemoteId = model.NewString(model.NewId())
user1.RemoteId = model.NewString("remote-id")
user1, appErr = p.API.UpdateUser(user1)
require.Nil(t, appErr)

Expand Down Expand Up @@ -196,15 +197,15 @@ func TestChatSpansPlatforms(t *testing.T) {

var appErr *model.AppError

user1.RemoteId = model.NewString(model.NewId())
user1.RemoteId = model.NewString("remote-id")
user1, appErr = p.API.UpdateUser(user1)
require.Nil(t, appErr)

user2.RemoteId = model.NewString(model.NewId())
user2.RemoteId = model.NewString("remote-id")
user2, appErr = p.API.UpdateUser(user2)
require.Nil(t, appErr)

user3.RemoteId = model.NewString(model.NewId())
user3.RemoteId = model.NewString("remote-id")
user3, appErr = p.API.UpdateUser(user3)
require.Nil(t, appErr)

Expand All @@ -224,11 +225,11 @@ func TestChatSpansPlatforms(t *testing.T) {

var appErr *model.AppError

user1.RemoteId = model.NewString(model.NewId())
user1.RemoteId = model.NewString("remote-id")
user1, appErr = p.API.UpdateUser(user1)
require.Nil(t, appErr)

user3.RemoteId = model.NewString(model.NewId())
user3.RemoteId = model.NewString("remote-id")
user3, appErr = p.API.UpdateUser(user3)
require.Nil(t, appErr)

Expand Down Expand Up @@ -296,11 +297,11 @@ func TestChatMembersSpanPlatforms(t *testing.T) {

var appErr *model.AppError

user1.RemoteId = model.NewString(model.NewId())
user1.RemoteId = model.NewString("remote-id")
user1, appErr = p.API.UpdateUser(user1)
require.Nil(t, appErr)

user2.RemoteId = model.NewString(model.NewId())
user2.RemoteId = model.NewString("remote-id")
user2, appErr = p.API.UpdateUser(user2)
require.Nil(t, appErr)

Expand All @@ -319,7 +320,7 @@ func TestChatMembersSpanPlatforms(t *testing.T) {

var appErr *model.AppError

user1.RemoteId = model.NewString(model.NewId())
user1.RemoteId = model.NewString("remote-id")
user1, appErr = p.API.UpdateUser(user1)
require.Nil(t, appErr)

Expand Down Expand Up @@ -374,15 +375,15 @@ func TestChatMembersSpanPlatforms(t *testing.T) {

var appErr *model.AppError

user1.RemoteId = model.NewString(model.NewId())
user1.RemoteId = model.NewString("remote-id")
user1, appErr = p.API.UpdateUser(user1)
require.Nil(t, appErr)

user2.RemoteId = model.NewString(model.NewId())
user2.RemoteId = model.NewString("remote-id")
user2, appErr = p.API.UpdateUser(user2)
require.Nil(t, appErr)

user3.RemoteId = model.NewString(model.NewId())
user3.RemoteId = model.NewString("remote-id")
user3, appErr = p.API.UpdateUser(user3)
require.Nil(t, appErr)

Expand All @@ -403,11 +404,11 @@ func TestChatMembersSpanPlatforms(t *testing.T) {

var appErr *model.AppError

user1.RemoteId = model.NewString(model.NewId())
user1.RemoteId = model.NewString("remote-id")
user1, appErr = p.API.UpdateUser(user1)
require.Nil(t, appErr)

user3.RemoteId = model.NewString(model.NewId())
user3.RemoteId = model.NewString("remote-id")
user3, appErr = p.API.UpdateUser(user3)
require.Nil(t, appErr)

Expand Down
10 changes: 5 additions & 5 deletions server/store/mocks/Store.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 6 additions & 2 deletions server/store/sqlstore/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@
return nil
}

func (s *SQLStore) Init() error {
func (s *SQLStore) Init(remoteID string) error {
if err := s.createTable(subscriptionsTableName, "subscriptionID VARCHAR(255) PRIMARY KEY, type VARCHAR(255), msTeamsTeamID VARCHAR(255), msTeamsChannelID VARCHAR(255), msTeamsUserID VARCHAR(255), secret VARCHAR(255), expiresOn BIGINT"); err != nil {
return err
}
Expand Down Expand Up @@ -144,7 +144,11 @@
return err
}

return s.createTable(whitelistedUsersTableName, "mmUserID VARCHAR(255) PRIMARY KEY")
if err := s.createTable(whitelistedUsersTableName, "mmUserID VARCHAR(255) PRIMARY KEY"); err != nil {
return err
}

return s.runMigrationRemoteID(remoteID)

Check failure on line 151 in server/store/sqlstore/store.go

View workflow job for this annotation

GitHub Actions / plugin-ci / lint

s.runMigrationRemoteID undefined (type *SQLStore has no field or method runMigrationRemoteID)

Check failure on line 151 in server/store/sqlstore/store.go

View workflow job for this annotation

GitHub Actions / plugin-ci / test

s.runMigrationRemoteID undefined (type *SQLStore has no field or method runMigrationRemoteID)
}

func (s *SQLStore) ListChannelLinksWithNames() ([]*storemodels.ChannelLink, error) {
Expand Down
2 changes: 1 addition & 1 deletion server/store/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import (
)

type Store interface {
Init() error
Init(remoteID string) error
GetLinkByChannelID(channelID string) (*storemodels.ChannelLink, error)
ListChannelLinks() ([]storemodels.ChannelLink, error)
ListChannelLinksWithNames() ([]*storemodels.ChannelLink, error)
Expand Down
4 changes: 2 additions & 2 deletions server/store/timerlayer/timerlayer.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading