[MI-3629] Added cluster mutexes and solved the problem of race conditions for whitelist #351

manojmalik20 · 2023-10-16T16:40:39Z

Summary

Created store functions for locking and unlocking the whitelist
Modified the store fuctions to use transactions for accessing the whitelist
Created new functions for processing and validating the configuration and used them everywhere
Fixed an existing bug in the needsConnect API
Added the logic of locking and accessing the whitelist in the complete OAuth API
Added the logic of reverting the OAuth if there's an error in accessing the whitelist

Related PRs

…lowed through a whitelist Created a new config setting called connectedUsersAllowed Added validation on this config setting that its value should be greater than or equal to the no. of connected users already present in the DB Added the logic to validate this in the OnActivate function Created several store functions for different usecases Modified the connect command handler to check for whitelist Added the logic to store the user in whitelist in the oAuth redirect API handler

Fixed a bug in the OAuth flow regarding storing the user in whitelist Added the whitelist logic in the connect bot command Added unit tests for connect and connect bot slash command Added unit tests for the store functions

…s-sync into MI-3606

…itelist size when updating the config value Changed the help text and name of the config variable Removed the function of getting count of connected users from the store

…s-sync into MI-3606

…itions for whitelist Created store functions for locking and unlocking the whitelist Modified the store fuctions to use transactions for accessing the whitelist Created new functions for processing and validating the configuration and used them everywhere Fixed an existing bug in the needsConnect API Added the logic of locking and accessing the whitelist in the complete OAuth API Added the logic of reverting the OAuth if there's an error in accessing the whitelist

… Postgres

Changed error message in the OAuth API handler Used constants in the store file for table names

…functions

…eams-sync into MI-3629

…s-sync into MI-3629

ayusht2810

Fix ci

server/api.go

server/configuration.go

server/store/store.go

server/api.go

server/plugin.go

Changed definition of a function to take the transaction as the first parameter Modified the validation of the configuration variables Fixed the data race in the store test functions

server/store/store_test.go

jespino

It looks good to me.

server/api.go

…ed the commit transaction and unlock whitelist flow in OAuth redirect handler

lieut-data

I'm hesitant to see something like LOCK TABLE used outside of exceptional scenarios. Is there a problem description of what race conditions we're trying to solve?

manojmalik20 · 2023-10-24T15:28:52Z

Is there a problem description of what race conditions we're trying to solve?

@lieut-data Consider this scenario: There is only one slot left in the whitelist. Two users run the /msteams connect command simultaneously. The code checks the size of the whitelist and sees that one slot is empty, so it responds with the link to connect their accounts. Now, both users click on the link to connect their accounts and are going through the OAuth process, the user who completes the OAuth first should be connected and acquire the spot on the whitelist. The other should see the respective error message that the max limit of connected users has been reached and he can't connect even though his OAuth was successful. But before this PR, both users were able to connect and the whitelist size was increasing more than what is allowed.

lieut-data · 2023-10-24T15:34:02Z

Thanks for clarifying @manojmalik20! Might we consider using our cluster.Mutex for this scenario instead?

See https://github.com/mattermost/mattermost/blob/4a0cf786bece1209a918ef5c19c99b69349e88e2/server/public/pluginapi/cluster/mutex.go#L32-L41

…mutex Removed the store function Lock and Unlock Whitelist along with the unit tests Added a new mutex in the plugin struct for whitelist Added the logic to lock and unlock mutex in connect slash command handlers Added the logic to lock and unlock mutex in OAuth redirect handler Fixed the failing unit tests

…x while prefilling the whitelist

…s-sync into MI-3629

server/plugin_test.go

server/store/sqlstore/store.go

server/api.go

…serInWhitelist function

lieut-data

Thanks, @manojmalik20! Awesome to see this simplify things. A few follow up comments.

server/command.go

server/store/sqlstore/store.go

…s-sync into MI-3629

Removed the use of transaction from the parameters of whitelist functions Fixed the failing unit tests Removed the logic of locking/unlocking whitelist mutex when running connect slash command Added better error handling for duplicate entry errors of Postgres and MySQL

lieut-data

Thanks, @manojmalik20!

manojmalik20 added 8 commits October 10, 2023 16:42

[MI-3606] Fixed failing lint errors

736bf30

[MI-3606] Added unit tests and fixed a bug

85bdbd3

Fixed a bug in the OAuth flow regarding storing the user in whitelist Added the whitelist logic in the connect bot command Added unit tests for connect and connect bot slash command Added unit tests for the store functions

[MI-3606] Fixed the failing unit tests in CI

9dc6c06

Merge branch 'main' of github.com:mattermost/mattermost-plugin-msteam…

fe0d6c1

…s-sync into MI-3606

[MI-3606] Fixed the bug of using no. of connected users instead of wh…

a5c0dc8

…itelist size when updating the config value Changed the help text and name of the config variable Removed the function of getting count of connected users from the store

Merge branch 'main' of github.com:mattermost/mattermost-plugin-msteam…

8e52c0c

…s-sync into MI-3606

manojmalik20 requested a review from ayusht2810 October 16, 2023 16:40

manojmalik20 added 7 commits October 17, 2023 18:30

[MI-3629] Added separate handling of locking and unlocking tables for…

9b918b0

… Postgres

[MI-3606] Review fixes

e9de039

Changed error message in the OAuth API handler Used constants in the store file for table names

[MI-3629] Fixed failing unit tests and added new tests for new store …

5aa5cc9

…functions

Merge branch 'MI-3606' of github.com:mattermost/mattermost-plugin-mst…

c8d2f36

…eams-sync into MI-3629

[MI-3606] Review fixes: Modified an error message

e554160

Merge branch 'MI-3606' of github.com:mattermost/mattermost-plugin-mst…

d069766

…eams-sync into MI-3629

[MI-3629] Fixed the race condition in unit tests

da62777

manojmalik20 added Difficulty/3:Hard Hard ticket Test Cases Includes unit test cases changes labels Oct 18, 2023

[MI-3629] Fixed the race condition in unit tests by using wait group

1c96885

Base automatically changed from MI-3606 to main October 18, 2023 13:52

manojmalik20 added 3 commits October 18, 2023 21:03

Merge branch 'main' of github.com:mattermost/mattermost-plugin-msteam…

6f2f4df

…s-sync into MI-3629

[MI-3629] Fixed the failing unit tests by using channels

720d4d4

[MI-3629] Fixed the iint errors

36f5461

ayusht2810 reviewed Oct 20, 2023

View reviewed changes

[MI-3629] Review fixes

8049793

Changed definition of a function to take the transaction as the first parameter Modified the validation of the configuration variables Fixed the data race in the store test functions

manojmalik20 requested a review from ayusht2810 October 23, 2023 10:36

ayusht2810 approved these changes Oct 23, 2023

View reviewed changes

server/store/store_test.go Outdated Show resolved Hide resolved

manojmalik20 requested review from jespino and Kshitij-Katiyar October 23, 2023 11:23

lieut-data self-requested a review October 23, 2023 13:06

jespino approved these changes Oct 24, 2023

View reviewed changes

jespino reviewed Oct 24, 2023

View reviewed changes

server/api.go Outdated Show resolved Hide resolved

ayusht2810 reviewed Oct 24, 2023

View reviewed changes

server/api.go Outdated Show resolved Hide resolved

[MI-3629] Added additional check for postgres error string and modifi…

ac2912d

…ed the commit transaction and unlock whitelist flow in OAuth redirect handler

lieut-data requested changes Oct 24, 2023

View reviewed changes

ayusht2810 assigned manojmalik20 Oct 25, 2023

manojmalik20 added 2 commits October 25, 2023 15:23

[MI-3629] Added the logic of locking and unlocking the whitelist mute…

8e0400e

…x while prefilling the whitelist

manojmalik20 changed the title ~~[MI-3629] Added table level locks and solved the problem of race conditions for whitelist~~ [MI-3629] Added cluster mutexes and solved the problem of race conditions for whitelist Oct 25, 2023

Merge branch 'main' of github.com:mattermost/mattermost-plugin-msteam…

78894e5

…s-sync into MI-3629

manojmalik20 requested review from lieut-data and jespino October 25, 2023 10:28

ayusht2810 reviewed Oct 25, 2023

View reviewed changes

[MI-3629] Review fixes: Removed unnecessary code and optimised StoreU…

ada94c6

…serInWhitelist function

ayusht2810 approved these changes Oct 25, 2023

View reviewed changes

lieut-data requested changes Oct 25, 2023

View reviewed changes

manojmalik20 added 2 commits October 25, 2023 19:36

Merge branch 'main' of github.com:mattermost/mattermost-plugin-msteam…

b37c57a

…s-sync into MI-3629

manojmalik20 requested review from lieut-data October 25, 2023 14:18

lieut-data approved these changes Oct 25, 2023

View reviewed changes

manojmalik20 merged commit c2cf8e8 into main Oct 25, 2023
1 check passed

manojmalik20 deleted the MI-3629 branch October 25, 2023 14:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MI-3629] Added cluster mutexes and solved the problem of race conditions for whitelist #351

[MI-3629] Added cluster mutexes and solved the problem of race conditions for whitelist #351

manojmalik20 commented Oct 16, 2023

ayusht2810 left a comment

jespino left a comment

lieut-data left a comment

manojmalik20 commented Oct 24, 2023

lieut-data commented Oct 24, 2023

lieut-data left a comment

lieut-data left a comment

[MI-3629] Added cluster mutexes and solved the problem of race conditions for whitelist #351

[MI-3629] Added cluster mutexes and solved the problem of race conditions for whitelist #351

Conversation

manojmalik20 commented Oct 16, 2023

Summary

Related PRs

ayusht2810 left a comment

Choose a reason for hiding this comment

jespino left a comment

Choose a reason for hiding this comment

lieut-data left a comment

Choose a reason for hiding this comment

manojmalik20 commented Oct 24, 2023

lieut-data commented Oct 24, 2023

lieut-data left a comment

Choose a reason for hiding this comment

lieut-data left a comment

Choose a reason for hiding this comment