Added a logic to disconnect users from ServiceNow on change of the encryption secret. #171

Nityanand13 · 2023-02-17T13:14:14Z

Summary

When an admin changes the encryption secret then all the users will be disconnected from ServiceNow and they have to reconnect their accounts to ServiceNow.

…e of the encryption secret. (#42) * [MI-2032]: Modified the error when a user updates his encryption secret * [MI-2032]: Review fixes done 1. Fixed linting errors * [MI-2032]: Review fixes done 1.Improved code quality * [MI-2032]: Review fixes done 1. Improved code quality * [MI-2032]: Added a logic that if we change the encryption secret then at that time only all the users will be disconnected from service now. * [MI-2032]: Modified code quality * [MI-2032]: Review fixes done 1. Improved code quality * [MI-2032]: Review fixes done 1. Implemented separate goroutine to delete users * [MI-2032]: Review fixes done 1. Used goroutines inside a loop 2. Improved code readability

ayusht2810 · 2023-07-21T11:41:58Z

@hanzei the below PR is ready for review. Can please assign the designated reviewer to it.

mickmister · 2023-08-21T14:37:45Z

server/plugin/configuration.go

+	if oldEncryptionSecret != "" && oldEncryptionSecret != p.getConfiguration().EncryptionSecret {
+		go p.store.DeleteUserTokenOnEncryptionSecretChange()
+	}


Shouldn't we delete the users even if the oldEncryptionSecret is blank? As long as the secret changed

hanzei

Thanks for the PR 👍

I'm wondering how the code behaves in a HA environment. Are there issues that can arise when multiple servers race to delete the user list?

codecov-commenter · 2023-09-14T08:29:03Z

Codecov Report

Attention: Patch coverage is 0% with 66 lines in your changes are missing coverage. Please review.

Project coverage is 63.85%. Comparing base (a5ca244) to head (30cad00).

Files	Patch %	Lines
server/plugin/kv.go	0.00%	62 Missing ⚠️
server/plugin/configuration.go	0.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #171      +/-   ##
==========================================
- Coverage   65.40%   63.85%   -1.55%     
==========================================
  Files          17       17              
  Lines        2064     2114      +50     
==========================================
  Hits         1350     1350              
- Misses        674      724      +50     
  Partials       40       40

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

AayushChaudhary0001

The PR above has been tested for the following scenario:-

On changing the encryption secret in MM, the connected users are disconnected from Servicenow.

The PR was working fine for the above condition, LGTM. Approved.

Kshitij-Katiyar · 2024-03-21T07:01:57Z

Thanks for the PR 👍

I'm wondering how the code behaves in a HA environment. Are there issues that can arise when multiple servers race to delete the user list?

@hanzei #171 (review) QA tested this PR for the HA environment and its working perfectly fine.

hanzei · 2024-03-22T10:11:16Z

I'm leaving it to @mickmister to suggest next steps on this PR

mickmister · 2024-03-22T14:43:32Z

server/plugin/kv.go

+	wg := new(sync.WaitGroup)
+	mu := new(sync.Mutex)


Right now we add to this wait group once per user. That seems a bit excessive. What if there are a very large amount of users? We should probably fan this out with at most 5-10 goroutines at a time. @hanzei Thoughts on this? Also, with the current strategy we would be slamming the database all at once, and on multiple nodes in an HA environment as @hanzei pointed out.

We definitely want to coordinate this so it only runs on one node. We can use cluster.Mutex to coordinate this. We did something similar (a "migration" of sorts) using this here mattermost/mattermost-plugin-gitlab#361.

This PR is a bit different though, since this will be run "on demand" when the secret changes. Here's how I see this working:

We check if the secret changed as usual. If not changed, do normal stuff without any of the below steps

We fetch a mutex for delete-all-users

With the mutex, we check the kv store key delete-all-users-running (@hanzei wondering about race conditions here, where the database may not be "ready" to return the freshly set key on another node's database connection. We've run into these timing things before where the kvstore had not reflected the most recently updated state in real-time)

If the key does exist

Immediately release the mutex

Do not call DeleteAllUsers

If the key does not exist

Set the delete-all-users-running value to true

In a defer block right below that, remove the delete-all-users-running key

Immediately release the mutex

Call DeleteAllUsers

This can all be done in a goroutine in OnConfigurationChange so we don't block that function from returning

We definitely want to coordinate this so it only runs on one node. We can use cluster.Mutex to coordinate this. We did something similar (a "migration" of sorts) using this here mattermost/mattermost-plugin-gitlab#361.

Strong 👍

I wonder why we are fanning out the requests at all.

Yeah I'm thinking the same now. I think we should do this synchronously here with no wait group or goroutines. @Kshitij-Katiyar What do you think?

yeah, I think we should do this synchronously as this is a very minor edge case as well. @mickmister

server/plugin/kv.go

…sers and goroutines

…icenow into fix_issue_encryption_secret_change

ayusht2810 · 2024-03-29T18:16:11Z

@hanzei @mickmister fixed the review comments. Please re-review.

ayusht2810 · 2024-05-06T17:29:48Z

@mickmister @hanzei Gentle reminder to re-review the PR

mickmister · 2024-05-13T20:42:31Z

There's no customer reported ticket associated with this, so I'm not sure if this is a priority that should require this complexity

Kshitij-Katiyar · 2024-05-14T10:47:08Z

There's no customer reported ticket associated with this, so I'm not sure if this is a priority that should require this complexity

This was a problem we faced while developing, that's why we have added this feature as an enhancement.
@mickmister

mickmister

The PR mostly LGTM. I think the goroutines used to fetch all users may be unnecessary. Otherwise looks good 👍

server/plugin/kv.go

mickmister · 2024-05-14T20:35:46Z

server/plugin/kv.go

+	wg := new(sync.WaitGroup)
+	mu := new(sync.Mutex)


Yeah I'm thinking the same now. I think we should do this synchronously here with no wait group or goroutines. @Kshitij-Katiyar What do you think?

server/plugin/kv.go

ayusht2810 · 2024-06-04T09:43:11Z

@mickmister fixed the review comments. Please re-review

mickmister

LGTM, just one suggestion to remove the lock since we're doing this synchronously now

server/plugin/kv.go

ayusht2810 · 2024-06-05T08:42:05Z

@mickmister fixed the review comments. Please re-review

OOO

fmartingr

Good work. I needed a few reads to understood what was going on, left my thought in comments but not blocking.

This would be fairly useful for other plugins as well @mickmister (mattermost/mattermost-plugin-google-calendar#61)

server/plugin/kv.go

ayusht2810 · 2024-06-20T12:45:15Z

@fmartingr Fixed the review fixes. Please re-review

fmartingr

Re-approved. Thanks for addressing my concerns!

hanzei added the Work In Progress Not yet ready for review label Mar 28, 2023

hanzei requested a review from mickmister August 8, 2023 10:38

hanzei added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester and removed Work In Progress Not yet ready for review labels Aug 8, 2023

mickmister reviewed Aug 21, 2023

View reviewed changes

mickmister requested a review from hanzei August 21, 2023 14:38

hanzei requested changes Aug 22, 2023

View reviewed changes

Merge branch 'master' into fix_issue_encryption_secret_change

27f96a4

hanzei added the Awaiting Submitter Action Blocked on the author label Oct 11, 2023

Fixed merge conflicts

bb977dc

Kshitij-Katiyar requested a review from AayushChaudhary0001 March 15, 2024 13:28

AayushChaudhary0001 approved these changes Mar 19, 2024

View reviewed changes

Kshitij-Katiyar requested review from mickmister and hanzei March 21, 2024 07:02

mickmister reviewed Mar 22, 2024

View reviewed changes

hanzei previously requested changes Mar 25, 2024

View reviewed changes

server/plugin/kv.go Outdated Show resolved Hide resolved

server/plugin/kv.go Outdated Show resolved Hide resolved

ayusht2810 added 2 commits March 29, 2024 23:32

Review fixes: Added cluster mutex and updated the logic of deleting u…

540eea1

…sers and goroutines

Merge branch 'master' of github.com:mattermost/mattermost-plugin-serv…

30cad00

…icenow into fix_issue_encryption_secret_change

hanzei requested a review from mickmister March 31, 2024 08:48

hanzei removed the Awaiting Submitter Action Blocked on the author label Mar 31, 2024

Merge branch 'master' into fix_issue_encryption_secret_change

32668c1

mickmister reviewed May 14, 2024

View reviewed changes

ayusht2810 and others added 2 commits June 4, 2024 15:11

Update the logic to work synchronously and update log statements

ce6c9ef

Merge branch 'master' into fix_issue_encryption_secret_change

7cb05d4

ayusht2810 requested a review from mickmister June 4, 2024 09:43

mickmister reviewed Jun 4, 2024

View reviewed changes

server/plugin/kv.go Outdated Show resolved Hide resolved

server/plugin/kv.go Outdated Show resolved Hide resolved

Update break with continue and remove extra lock

47e1411

mickmister approved these changes Jun 5, 2024

View reviewed changes

mickmister requested review from AayushChaudhary0001 and fmartingr June 5, 2024 16:32

fmartingr approved these changes Jun 6, 2024

View reviewed changes

server/plugin/kv.go Outdated Show resolved Hide resolved

server/plugin/kv.go Show resolved Hide resolved

ayusht2810 and others added 2 commits June 20, 2024 18:02

Add comment for function and move mutex logic to function

26105ce

Merge branch 'master' into fix_issue_encryption_secret_change

b12a746

fmartingr self-requested a review June 20, 2024 13:22

fmartingr approved these changes Jun 20, 2024

View reviewed changes

mickmister approved these changes Jul 2, 2024

View reviewed changes

mickmister removed the 2: Dev Review Requires review by a core committer label Jul 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added a logic to disconnect users from ServiceNow on change of the encryption secret. #171

Added a logic to disconnect users from ServiceNow on change of the encryption secret. #171

Nityanand13 commented Feb 17, 2023 •

edited

Loading

ayusht2810 commented Jul 21, 2023

mickmister Aug 21, 2023

hanzei left a comment •

edited

Loading

codecov-commenter commented Sep 14, 2023 •

edited

Loading

AayushChaudhary0001 left a comment

Kshitij-Katiyar commented Mar 21, 2024

hanzei commented Mar 22, 2024

mickmister Mar 22, 2024

hanzei Mar 25, 2024

hanzei Mar 25, 2024

mickmister May 14, 2024

Kshitij-Katiyar May 16, 2024

ayusht2810 commented Mar 29, 2024

ayusht2810 commented May 6, 2024

mickmister commented May 13, 2024

Kshitij-Katiyar commented May 14, 2024

mickmister left a comment

mickmister May 14, 2024

ayusht2810 commented Jun 4, 2024

mickmister left a comment

ayusht2810 commented Jun 5, 2024

fmartingr left a comment

ayusht2810 commented Jun 20, 2024

fmartingr left a comment

Added a logic to disconnect users from ServiceNow on change of the encryption secret. #171

Are you sure you want to change the base?

Added a logic to disconnect users from ServiceNow on change of the encryption secret. #171

Conversation

Nityanand13 commented Feb 17, 2023 • edited Loading

Summary

ayusht2810 commented Jul 21, 2023

mickmister Aug 21, 2023

Choose a reason for hiding this comment

hanzei left a comment • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Sep 14, 2023 • edited Loading

Codecov Report

AayushChaudhary0001 left a comment

Choose a reason for hiding this comment

Kshitij-Katiyar commented Mar 21, 2024

hanzei commented Mar 22, 2024

mickmister Mar 22, 2024

Choose a reason for hiding this comment

hanzei Mar 25, 2024

Choose a reason for hiding this comment

hanzei Mar 25, 2024

Choose a reason for hiding this comment

mickmister May 14, 2024

Choose a reason for hiding this comment

Kshitij-Katiyar May 16, 2024

Choose a reason for hiding this comment

ayusht2810 commented Mar 29, 2024

ayusht2810 commented May 6, 2024

mickmister commented May 13, 2024

Kshitij-Katiyar commented May 14, 2024

mickmister left a comment

Choose a reason for hiding this comment

mickmister May 14, 2024

Choose a reason for hiding this comment

ayusht2810 commented Jun 4, 2024

mickmister left a comment

Choose a reason for hiding this comment

ayusht2810 commented Jun 5, 2024

fmartingr left a comment

Choose a reason for hiding this comment

ayusht2810 commented Jun 20, 2024

fmartingr left a comment

Choose a reason for hiding this comment

Nityanand13 commented Feb 17, 2023 •

edited

Loading

hanzei left a comment •

edited

Loading

codecov-commenter commented Sep 14, 2023 •

edited

Loading