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
| // $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.
d7f2d0b to
01169b4
Compare
01169b4 to
60ec466
Compare
| // Don't panic, disable for now and wait for user to update config | ||
| log.Warn().Err(err).Str("user", userCtx.Name).Msg("Plugin initialize failed, disabling now") | ||
| pluginConf.Enabled = false | ||
| if err = tx.UpdatePluginConf(pluginConf); err != nil { |
There was a problem hiding this comment.
This should use m.db too
| if err = tx.UpdatePluginConf(pluginConf); err != nil { | |
| if err = m.db.UpdatePluginConf(pluginConf); err != nil { |
| defer func() { | ||
| i.state.And(^stateBusyMask) | ||
| }() | ||
| prevState = i.state.Or(stateEnabledMask) |
There was a problem hiding this comment.
Do we need this with atomic? The plugin.Manager has a mutex which is called on all entry points. Only the enable in the goroutine isn't guarded by this.
I think adding this here, would remove the need for the atomic, and we could use a plain enabled boolean.
go func() {
if compat.HasSupport(instance, compat.Storager) {
instance.SetStorageHandler(dbStorageHandler{pluginConf.ID, m.db})
}
if pluginConf.Enabled {
m.mutex.Lock()
err := instanceWrapper.Enable()
m.mutex.Unlock()
There was a problem hiding this comment.
I think the property is hard to prove and we might want to relax that constraint for basic operations to reduce overhead after initialization (enable/disable, etc), so let's just do atomic just in case, wdyt?
There was a problem hiding this comment.
If I look at the atomic solution standalone, it doesn't seem 100% right. E.g. when enabling, the enabled state is set before the enabling of the actual plugin finishes. RemoveUser only checks the enabled flag, then Disable fails with "already in progress", this is only logged and the instance is removed from the map anyway. So the plugin finishes enabling and keeps running, but nothing can disable it anymore.
This probably cannot happen, because of the global mutex, but I'd rather not have this dependency. I'd be probably okay with another mutex on the instance level, this would force enable / disable to wait, to not have this race condition.
But overall, I'd probably prefer one mutex for all, as every other syncing mechanism increases complexity.
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.