Skip to content

Commit dc93472

Browse files
ForceuForceu
committed
Fixed race conditions and limit e2e info
1 parent 9c0171e commit dc93472

File tree

7 files changed

+255
-21
lines changed

7 files changed

+255
-21
lines changed

‎internal/webserver/api/Api.go‎

Lines changed: 36 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"fmt"
77
"io"
88
"net/http"
9+
"strconv"
910
"strings"
1011
"time"
1112

@@ -21,6 +22,7 @@ import (
2122
"github.com/forceu/gokapi/internal/storage/chunking/chunkreservation"
2223
"github.com/forceu/gokapi/internal/storage/filerequest"
2324
"github.com/forceu/gokapi/internal/storage/presign"
25+
"github.com/forceu/gokapi/internal/webserver/api/apiMutex"
2426
"github.com/forceu/gokapi/internal/webserver/api/errorcodes"
2527
"github.com/forceu/gokapi/internal/webserver/authentication/users"
2628
"github.com/forceu/gokapi/internal/webserver/fileupload"
@@ -86,6 +88,9 @@ func apiEditFile(w http.ResponseWriter, r requestParser, user models.User) {
8688
if !ok {
8789
panic("invalid parameter passed")
8890
}
91+
apiMutex.Lock(apiMutex.TypeMetaData, request.Id)
92+
defer apiMutex.Unlock(apiMutex.TypeMetaData, request.Id)
93+
8994
file, ok := database.GetMetaDataById(request.Id)
9095
if !ok {
9196
sendError(w, http.StatusNotFound, errorcodes.NotFound, "Invalid file ID provided.")
@@ -171,6 +176,9 @@ func apiModifyApiKey(w http.ResponseWriter, r requestParser, user models.User) {
171176
if !ok {
172177
panic("invalid parameter passed")
173178
}
179+
apiMutex.Lock(apiMutex.TypeApiKey, request.KeyId)
180+
defer apiMutex.Unlock(apiMutex.TypeApiKey, request.KeyId)
181+
174182
apiKeyOwner, apiKey, ok := isValidKeyForEditing(request.KeyId)
175183
if !ok {
176184
sendError(w, http.StatusNotFound, errorcodes.NotFound, "Invalid key ID provided.")
@@ -284,6 +292,7 @@ func apiChangeFriendlyName(w http.ResponseWriter, r requestParser, user models.U
284292
if !ok {
285293
panic("invalid parameter passed")
286294
}
295+
287296
ownerApiKey, apiKey, ok := isValidKeyForEditing(request.KeyId)
288297
if !ok {
289298
sendError(w, http.StatusNotFound, errorcodes.NotFound, "Invalid key ID provided.")
@@ -304,6 +313,10 @@ func renameApiKeyFriendlyName(id string, newName string) error {
304313
if newName == "" {
305314
newName = "Unnamed key"
306315
}
316+
317+
apiMutex.Lock(apiMutex.TypeApiKey, id)
318+
defer apiMutex.Unlock(apiMutex.TypeApiKey, id)
319+
307320
key, ok := database.GetApiKey(id)
308321
if !ok {
309322
return errors.New("could not modify API key")
@@ -727,6 +740,10 @@ func apiChangeFileOwner(w http.ResponseWriter, r requestParser, user models.User
727740
if !ok {
728741
panic("invalid parameter passed")
729742
}
743+
744+
apiMutex.Lock(apiMutex.TypeMetaData, request.Id)
745+
defer apiMutex.Unlock(apiMutex.TypeMetaData, request.Id)
746+
730747
file, ok := storage.GetFile(request.Id)
731748
if !ok {
732749
sendError(w, http.StatusNotFound, errorcodes.NotFound, "Invalid id provided.")
@@ -818,6 +835,10 @@ func apiModifyUser(w http.ResponseWriter, r requestParser, user models.User) {
818835
if !ok {
819836
panic("invalid parameter passed")
820837
}
838+
idStr := strconv.Itoa(request.Id)
839+
apiMutex.Lock(apiMutex.TypeUser, idStr)
840+
defer apiMutex.Unlock(apiMutex.TypeUser, idStr)
841+
821842
userEdit, ok := isValidUserForEditing(w, request.Id)
822843
if !ok {
823844
return
@@ -854,6 +875,10 @@ func apiChangeUserRank(w http.ResponseWriter, r requestParser, user models.User)
854875
if !ok {
855876
panic("invalid parameter passed")
856877
}
878+
idStr := strconv.Itoa(request.Id)
879+
apiMutex.Lock(apiMutex.TypeUser, idStr)
880+
defer apiMutex.Unlock(apiMutex.TypeUser, idStr)
881+
857882
userEdit, ok := isValidUserForEditing(w, request.Id)
858883
if !ok {
859884
return
@@ -965,7 +990,16 @@ func apiDeleteUser(w http.ResponseWriter, r requestParser, user models.User) {
965990
return
966991
}
967992
logging.LogUserDeletion(userToDelete, user)
968-
database.DeleteUser(userToDelete.Id)
993+
994+
database.DeleteAllSessionsByUser(userToDelete.Id)
995+
996+
for _, apiKey := range database.GetAllApiKeys() {
997+
if apiKey.UserId == userToDelete.Id {
998+
database.DeleteApiKey(apiKey.Id)
999+
}
1000+
}
1001+
1002+
database.DeleteEnd2EndInfo(userToDelete.Id)
9691003

9701004
for _, fRequest := range database.GetAllFileRequests() {
9711005
if fRequest.UserId == userToDelete.Id {
@@ -988,13 +1022,7 @@ func apiDeleteUser(w http.ResponseWriter, r requestParser, user models.User) {
9881022
}
9891023
}
9901024
}
991-
for _, apiKey := range database.GetAllApiKeys() {
992-
if apiKey.UserId == userToDelete.Id {
993-
database.DeleteApiKey(apiKey.Id)
994-
}
995-
}
996-
database.DeleteAllSessionsByUser(userToDelete.Id)
997-
database.DeleteEnd2EndInfo(userToDelete.Id)
1025+
database.DeleteUser(userToDelete.Id)
9981026
}
9991027

10001028
func apiLogsDelete(_ http.ResponseWriter, r requestParser, user models.User) {

‎internal/webserver/api/apiMutex/ApiMutex.go‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
package apiMutex
2+
3+
import (
4+
"hash/fnv"
5+
"sync"
6+
)
7+
8+
const numStripes = 256
9+
10+
var stripes [numStripes]sync.Mutex
11+
12+
func getStripe(objectType int, key string) *sync.Mutex {
13+
switch objectType {
14+
case TypeUser, TypeApiKey, TypeMetaData:
15+
// valid
16+
default:
17+
panic("invalid object type")
18+
}
19+
20+
h := fnv.New32a()
21+
_, _ = h.Write([]byte{byte(objectType)})
22+
_, _ = h.Write([]byte(key))
23+
return &stripes[h.Sum32()%numStripes]
24+
}
25+
26+
const (
27+
TypeUser = iota
28+
TypeApiKey
29+
TypeMetaData
30+
)
31+
32+
func Lock(objectType int, key string) {
33+
getStripe(objectType, key).Lock()
34+
}
35+
36+
func Unlock(objectType int, key string) {
37+
getStripe(objectType, key).Unlock()
38+
}

‎internal/webserver/api/apiMutex/ApiMutex_test.go‎

Lines changed: 150 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,150 @@
1+
package apiMutex
2+
3+
import (
4+
"fmt"
5+
"sync"
6+
"testing"
7+
8+
"github.com/forceu/gokapi/internal/test"
9+
)
10+
11+
// TestInvalidObjectTypePanics verifies that an unknown objectType causes a panic.
12+
func TestInvalidObjectTypePanics(t *testing.T) {
13+
defer func() {
14+
if r := recover(); r == nil {
15+
t.Error("expected panic for invalid object type, but did not panic")
16+
}
17+
}()
18+
19+
getStripe(999, "any")
20+
}
21+
22+
// TestAllValidTypesDoNotPanic verifies that all defined types are accepted.
23+
func TestAllValidTypesDoNotPanic(t *testing.T) {
24+
types := []int{TypeUser, TypeApiKey, TypeMetaData}
25+
for _, objectType := range types {
26+
func() {
27+
defer func() {
28+
if r := recover(); r != nil {
29+
t.Errorf("unexpected panic for objectType %d", objectType)
30+
}
31+
}()
32+
getStripe(objectType, "key")
33+
}()
34+
}
35+
}
36+
37+
// TestSameKeyReturnsSameStripe verifies that the same arguments always map to the same stripe.
38+
func TestSameKeyReturnsSameStripe(t *testing.T) {
39+
m1 := getStripe(TypeUser, "abc")
40+
m2 := getStripe(TypeUser, "abc")
41+
42+
test.IsEqual(t, m1, m2)
43+
}
44+
45+
// TestDifferentTypesSameKeyMayDiffer verifies that objectType is factored into the hash.
46+
func TestDifferentTypesSameKeyMayDiffer(t *testing.T) {
47+
collisions := 0
48+
keys := []string{"1", "2", "3", "4", "5"}
49+
for _, key := range keys {
50+
if getStripe(TypeUser, key) == getStripe(TypeApiKey, key) {
51+
collisions++
52+
}
53+
}
54+
if collisions == len(keys) {
55+
t.Error("objectType does not appear to affect stripe selection — all keys collided across types")
56+
}
57+
}
58+
59+
// TestLockUnlock verifies that Lock and Unlock work without deadlocking.
60+
func TestLockUnlock(t *testing.T) {
61+
done := make(chan struct{})
62+
go func() {
63+
Lock(TypeUser, "1")
64+
Unlock(TypeUser, "1")
65+
close(done)
66+
}()
67+
<-done
68+
}
69+
70+
// TestLockBlocksUntilUnlocked verifies that a second Lock on the same key blocks
71+
// until the first caller calls Unlock.
72+
func TestLockBlocksUntilUnlocked(t *testing.T) {
73+
// Find two keys that hash to the same stripe so we can test real blocking.
74+
key := "1"
75+
stripe := getStripe(TypeUser, key)
76+
77+
stripe.Lock()
78+
79+
acquired := make(chan struct{})
80+
go func() {
81+
Lock(TypeUser, key)
82+
close(acquired)
83+
Unlock(TypeUser, key)
84+
}()
85+
86+
select {
87+
case <-acquired:
88+
t.Error("second Lock acquired while first was still held")
89+
default:
90+
// expected: goroutine is blocked
91+
}
92+
93+
stripe.Unlock()
94+
<-acquired
95+
}
96+
97+
// TestConcurrentWritesSerialized verifies that concurrent writers on the same key
98+
// do not race and that the counter reaches the expected value.
99+
func TestConcurrentWritesSerialized(t *testing.T) {
100+
const goroutines = 100
101+
counter := 0
102+
var wg sync.WaitGroup
103+
wg.Add(goroutines)
104+
105+
for i := 0; i < goroutines; i++ {
106+
go func() {
107+
defer wg.Done()
108+
Lock(TypeUser, "counter")
109+
counter++
110+
Unlock(TypeUser, "counter")
111+
}()
112+
}
113+
114+
wg.Wait()
115+
116+
test.IsEqualInt(t, counter, goroutines)
117+
}
118+
119+
// TestIndependentTypesDoNotNecessarilyBlock verifies that different object types
120+
// with different keys are handled independently across the stripe space.
121+
func TestIndependentTypesDoNotNecessarilyBlock(t *testing.T) {
122+
// Only meaningful if they hash to different stripes — find such a pair.
123+
var keyA, keyB string
124+
found := false
125+
for i := 0; i < 1000; i++ {
126+
a := fmt.Sprintf("key-%d", i)
127+
b := fmt.Sprintf("key-%d", i+1)
128+
if getStripe(TypeUser, a) != getStripe(TypeApiKey, b) {
129+
keyA, keyB = a, b
130+
found = true
131+
break
132+
}
133+
}
134+
135+
if !found {
136+
t.Skip("could not find two keys hashing to different stripes")
137+
}
138+
139+
Lock(TypeUser, keyA)
140+
defer Unlock(TypeUser, keyA)
141+
142+
done := make(chan struct{})
143+
go func() {
144+
Lock(TypeApiKey, keyB)
145+
Unlock(TypeApiKey, keyB)
146+
close(done)
147+
}()
148+
149+
<-done
150+
}

‎internal/webserver/api/routing.go‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -582,15 +582,21 @@ type paramE2eStore struct {
582582
}
583583

584584
func (p *paramE2eStore) ProcessParameter(r *http.Request) error {
585+
const maxBodySize = 5 * 1024 * 1024 // 5MB in bytes
586+
bodyReader := http.MaxBytesReader(nil, r.Body, maxBodySize)
587+
defer bodyReader.Close()
588+
585589
type expectedInput struct {
586590
Content string `json:"content"`
587591
}
588592
var input expectedInput
589593

590-
err := json.NewDecoder(r.Body).Decode(&input)
594+
err := json.NewDecoder(bodyReader).Decode(&input)
591595
if err != nil {
596+
// If body is larger than 5MB, this will be returned here as an error
592597
return err
593598
}
599+
594600
content, err := base64.StdEncoding.DecodeString(input.Content)
595601
if err != nil {
596602
return err

‎internal/webserver/web/static/js/admin_ui_users.js‎

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -266,8 +266,8 @@ const PermissionDefinitions = [
266266
}
267267
];
268268

269-
function hasPermission(userPermissions, permissionBit) {
270-
return (userPermissions & permissionBit) !== 0;
269+
function hasPermission(userPerm, permissionBit) {
270+
return (userPerm & permissionBit) !== 0;
271271
}
272272

273273

@@ -321,7 +321,11 @@ function addRowUser(userid, name, permissions) {
321321
btnPromote.type = "button";
322322
btnPromote.className = "btn btn-outline-light btn-sm";
323323
btnPromote.title = "Promote User";
324-
btnPromote.onclick = () => changeRank(userid, 'ADMIN', `changeRank_${userid}`);
324+
if (isAdmin) {
325+
btnPromote.onclick = () => changeRank(userid, 'ADMIN', `changeRank_${userid}`);
326+
} else {
327+
btnPromote.disabled = true;
328+
}
325329
btnPromote.innerHTML = `<i class="bi bi-chevron-double-up"></i>`;
326330
btnGroup.appendChild(btnPromote);
327331

@@ -341,15 +345,21 @@ function addRowUser(userid, name, permissions) {
341345

342346
// Permissions
343347
cellPermissions.innerHTML = PermissionDefinitions.map(perm => {
344-
const granted = hasPermission(permissions, perm.bit)
345-
? "perm-granted"
346-
: "perm-notgranted";
347-
348+
let granted = "perm-notgranted";
349+
if (hasPermission(permissions, perm.bit)) {
350+
granted = "perm-granted";
351+
}
348352
const id = perm.htmlId(userid);
353+
let nochangeClass = "";
354+
if (!hasPermission(userPermissions, perm.bit)) {
355+
nochangeClass = "perm-nochange";
356+
}
357+
358+
349359

350360
return `
351361
<i id="${id}"
352-
class="${perm.icon} ${granted}"
362+
class="${perm.icon} ${granted} ${nochangeClass}"
353363
title="${perm.title}"
354364
onclick='changeUserPermission(${userid}, "${perm.apiName}", "${id}")'>
355365
</i>`;

‎internal/webserver/web/static/js/min/admin.min.d76f71bf62.js‎

Lines changed: 4 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎internal/webserver/web/templates/html_users.tmpl‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -210,6 +210,8 @@
210210
<script src="./js/min/admin.min.{{ template "js_admin_version"}}.js"></script>
211211
<script>
212212
var isInternalAuth = {{.IsInternalAuth}};
213+
var isAdmin = {{.ActiveUser.IsAdmin}};
214+
var userPermissions = {{.ActiveUser.Permissions}};
213215
</script>
214216
{{ template "pagename" "OverviewUsers"}}
215217
{{ template "customjs" .}}

0 commit comments

Comments
 (0)