-
Notifications
You must be signed in to change notification settings - Fork 0
Anonymous: Add configurable device limit #10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: enhance-anonymous-access
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,10 +13,14 @@ import ( | |
| ) | ||
|
|
||
| const cacheKeyPrefix = "anon-device" | ||
| const anonymousDeviceExpiration = 30 * 24 * time.Hour | ||
|
|
||
| var ErrDeviceLimitReached = fmt.Errorf("device limit reached") | ||
|
|
||
| type AnonDBStore struct { | ||
| sqlStore db.DB | ||
| log log.Logger | ||
| sqlStore db.DB | ||
| log log.Logger | ||
| deviceLimit int64 | ||
| } | ||
|
|
||
| type Device struct { | ||
|
|
@@ -45,8 +49,8 @@ type AnonStore interface { | |
| DeleteDevicesOlderThan(ctx context.Context, olderThan time.Time) error | ||
| } | ||
|
|
||
| func ProvideAnonDBStore(sqlStore db.DB) *AnonDBStore { | ||
| return &AnonDBStore{sqlStore: sqlStore, log: log.New("anonstore")} | ||
| func ProvideAnonDBStore(sqlStore db.DB, deviceLimit int64) *AnonDBStore { | ||
| return &AnonDBStore{sqlStore: sqlStore, log: log.New("anonstore"), deviceLimit: deviceLimit} | ||
| } | ||
|
|
||
| func (s *AnonDBStore) ListDevices(ctx context.Context, from *time.Time, to *time.Time) ([]*Device, error) { | ||
|
|
@@ -65,9 +69,54 @@ func (s *AnonDBStore) ListDevices(ctx context.Context, from *time.Time, to *time | |
| return devices, err | ||
| } | ||
|
|
||
| // updateDevice updates a device if it exists and has been updated between the given times. | ||
| func (s *AnonDBStore) updateDevice(ctx context.Context, device *Device) error { | ||
| const query = `UPDATE anon_device SET | ||
| client_ip = ?, | ||
| user_agent = ?, | ||
| updated_at = ? | ||
| WHERE device_id = ? AND updated_at BETWEEN ? AND ?` | ||
|
|
||
| args := []interface{}{device.ClientIP, device.UserAgent, device.UpdatedAt.UTC(), device.DeviceID, | ||
| device.UpdatedAt.UTC().Add(-anonymousDeviceExpiration), device.UpdatedAt.UTC().Add(time.Minute), | ||
| } | ||
| err := s.sqlStore.WithDbSession(ctx, func(dbSession *sqlstore.DBSession) error { | ||
| args = append([]interface{}{query}, args...) | ||
| result, err := dbSession.Exec(args...) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| rowsAffected, err := result.RowsAffected() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if rowsAffected == 0 { | ||
| return ErrDeviceLimitReached | ||
| } | ||
|
|
||
| return nil | ||
| }) | ||
|
|
||
| return err | ||
| } | ||
|
|
||
| func (s *AnonDBStore) CreateOrUpdateDevice(ctx context.Context, device *Device) error { | ||
| var query string | ||
|
|
||
| // if device limit is reached, only update devices | ||
| if s.deviceLimit > 0 { | ||
| count, err := s.CountDevices(ctx, time.Now().UTC().Add(-anonymousDeviceExpiration), time.Now().UTC().Add(time.Minute)) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if count >= s.deviceLimit { | ||
| return s.updateDevice(ctx, device) | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Race Condition Bypasses Device LimitRace condition in device limit enforcement: the check-then-act pattern between counting devices and inserting new ones allows multiple concurrent requests to bypass the limit. When the count is near the limit, concurrent requests can all pass the check and insert devices, exceeding the configured |
||
| } | ||
|
|
||
| args := []any{device.DeviceID, device.ClientIP, device.UserAgent, | ||
| device.CreatedAt.UTC(), device.UpdatedAt.UTC()} | ||
| switch s.sqlStore.GetDBType() { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Old Devices Cannot Update Due to Limit
The
updateDevicefunction's WHERE clause restricts updates to devices modified within the last 30 days. When the device limit is reached, existing devices older than 30 days cannot be updated and incorrectly returnErrDeviceLimitReached, even though they should be updatable since they already exist in the database.