Repository navigation
enhance: serialize user update methods - #1042
eternal-flame-AD wants to merge 1 commit into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1042 +/- ##
==========================================
+ Coverage 76.12% 76.21% +0.09%
==========================================
Files 67 67
Lines 3619 3679 +60
==========================================
+ Hits 2755 2804 +49
- Misses 653 659 +6
- Partials 211 216 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ddfa0cc to
08f571c
Compare
c031b29 to
e787139
Compare
d486c1b to
6e4f37c
Compare
3421a40 to
6445b84
Compare
1728f14 to
d7f2d0b
Compare
| } | ||
| if compat.HasSupport(instance, compat.Storager) { | ||
| instance.SetStorageHandler(dbStorageHandler{pluginConf.ID, m.db}) | ||
| instance.SetStorageHandler(dbStorageHandler{pluginConf.ID, tx}) |
There was a problem hiding this comment.
This shouldn't use the transaction, as the transaction is closed after the http requests finishes, but the plugin instance keeps going in the background.
There was a problem hiding this comment.
Good catch, but we need to defer this in a gorountine just in case the plugin side blocks on this call. I will add a sync barrier for unit tests to make sure the test has no races.
| defer m.mutex.Unlock() | ||
| maps.DeleteFunc(m.instances, func(id uint, instance *InstanceWrapper) (delete bool) { | ||
| delete = instance.userID == userID | ||
| if delete { |
There was a problem hiding this comment.
| if delete { | |
| if delete && instance.enabled { |
I think .Disable should only be called if the plugin was enabled. E.g. a plugin could create some maps, bigger resources in the Enable method, and then free these resources in Disable method, but in this case disable could try to free uninitialized resources which could produce errors.
THe instance should be removed from the instances map regardless if the plugin is enabled or not.
| // $ref: "#/definitions/Error" | ||
| func (a *UserAPI) DeleteUserByID(ctx *gin.Context) { | ||
| withID(ctx, "id", func(id uint) { | ||
| user, err := a.DB.GetUserByID(id) |
There was a problem hiding this comment.
(optional) Can we move this into the transaction too? Otherwise they can still be some race here. Similar for the UpdateUser api.
There was a problem hiding this comment.
I think this is fine as is, it's just an early filter making sure the ID exists. The risk would be someone deleted the user right before the check and your transaction runs, in such case returning a success instead of "user does not exist" is probably acceptable.
There was a problem hiding this comment.
For the update endpoint it caches oidc_id before the transaction and then set's it inside the transaction. There probably could be a race condition where a user is linked to an oidc identity & maybe updates the admin permissions, and at the same time the update method is called, this could unset the oidc_id and the change of permission.
For the delete endpoint it could be that at the same time another admin changes the admin flag of the user, so it's the last admin. The admin flag is false on the read outside of the transaction, but before the transaction is called, it was actually updated, and then the admin check isn't done correctly.
But yeah, it's optional, but it seems like a similar problem we are trying to fix here.
| i.enabled = false | ||
| err := i.instance.Disable() | ||
| if err != nil { | ||
| i.enabled = true |
There was a problem hiding this comment.
I'm not sure this is true. Disable could be called without the plugin being active. Mabye we could guard here against disabling a plugin that is already disabled. Or implement it as the other comment describes.
d7f2d0b to
01169b4
Compare
01169b4 to
60ec466
Compare
2 participants
Serializes user update actions to prevent race conditions leading to unexpected results.
I removed the 'Test_UpdateUserByID_EmptyPassword_Expect400' test as it seemed to be a mistake - it should return 200, it returned 400 in the test because there wasn't a second admin.