[management] Approve all pending peers when peer approval is disabled #4806
Conversation
Signed-off-by: bcmmbaga <[email protected]>
Signed-off-by: bcmmbaga <[email protected]>
WalkthroughAdds a Store API and SQL implementation to approve pending peers, updates account settings flow to call it when Extra.PeerApprovalEnabled transitions true→false, changes integrated validator signature to remove peers map, and adds unit tests for store and account-manager behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AccountMgr as Account Manager
participant Store
participant DB
Client->>AccountMgr: UpdateAccountSettings(accountID, newSettings)
activate AccountMgr
AccountMgr->>AccountMgr: validate newSettings (integrated validator)
alt PeerApproval toggled true → false
AccountMgr->>Store: ApproveAccountPeers(ctx, accountID)
activate Store
Store->>DB: UPDATE peers SET requires_approval = false WHERE account_id = ? AND requires_approval = true
DB-->>Store: rowsAffected
Store-->>AccountMgr: approvedCount, nil
deactivate Store
AccountMgr->>AccountMgr: log "approved X peers" and set updateAccountPeers flag
end
AccountMgr-->>Client: response (success or error)
deactivate AccountMgr
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
management/server/store/store.go (1)
124-147: Interface extension for peer approval looks consistentAdding
ApproveAccountPeers(ctx context.Context, accountID string) (int, error)to the Store interface fits well alongside the existing peer operations and matches how the method is exercised from tests and the account manager. If you ever expect very large peer counts, you might consider switching the return type toint64for closer alignment with DB row counts, but that’s a minor future improvement.management/server/store/sql_store.go (1)
425-425: Add explicit type conversion for RowsAffected.
result.RowsAffectedisint64but the return type isint. While overflow is unlikely in practice, an explicit cast makes the conversion clear and prevents potential issues.Apply this diff:
- return int(result.RowsAffected), nil + return int(result.RowsAffected), nilNote: The current code already performs an implicit conversion, but making it explicit with
int(result.RowsAffected)is clearer. If the code doesn't already have the explicit cast, add it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
management/server/account.go(1 hunks)management/server/account_test.go(1 hunks)management/server/store/sql_store.go(1 hunks)management/server/store/sql_store_test.go(1 hunks)management/server/store/store.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
management/server/store/sql_store.go (2)
management/server/peer/peer.go (1)
Peer(16-58)shared/management/status/error.go (3)
Error(54-57)Errorf(70-75)Internal(24-24)
management/server/account_test.go (2)
management/server/types/settings.go (2)
Settings(10-55)ExtraSettings(82-99)management/server/store/store.go (2)
Store(50-207)LockingStrengthNone(47-47)
management/server/store/sql_store_test.go (1)
management/server/peer/peer.go (2)
Peer(16-58)PeerStatus(60-69)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Management / Benchmark (API) (amd64, postgres)
- GitHub Check: Management / Benchmark (API) (amd64, sqlite)
- GitHub Check: Management / Benchmark (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Benchmark (amd64, postgres)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Client / Unit (386)
- GitHub Check: JS / Lint
- GitHub Check: Client / Unit
- GitHub Check: release
- GitHub Check: Windows
- GitHub Check: Linux
- GitHub Check: Darwin
- GitHub Check: Client / Unit
- GitHub Check: Android / Build
- GitHub Check: Client / Unit
🔇 Additional comments (3)
management/server/store/sql_store_test.go (1)
3720-3796: Good coverage of ApproveAccountPeers semanticsThe test nicely exercises the main behaviours: approving only pending peers, ensuring idempotency on subsequent calls, and treating non-existent accounts as a no-op without error. The use of
runTestForAllEngineskeeps engine coverage consistent with the rest of the suite.management/server/account_test.go (1)
2058-2093: Peer-approval toggle flow is exercised correctlyThis test cleanly models the enable → mark peers pending → disable path and verifies via
GetAccountPeersthat allRequiresApprovalflags are cleared after disablingPeerApprovalEnabled. Usingaccount.Settings.Copy()before eachUpdateAccountSettingscall keeps other settings intact while focusing on the Extra flag.management/server/account.go (1)
308-317: LGTM! Correct implementation of auto-approval when peer approval is disabled.The logic correctly detects when
PeerApprovalEnabledtransitions from true to false, invokes the approval method within the transaction, handles errors appropriately, and logs the result. The conditional logging onapprovedCount > 0is a good practice.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
management/server/account.go (1)
302-312: LGTM! Auto-approval logic correctly implemented and triggers peer updates.The implementation correctly:
- Detects the transition from PeerApprovalEnabled true to false
- Approves pending peers atomically within the transaction
- Handles errors appropriately with proper wrapping
- Triggers downstream peer updates by setting
updateAccountPeers = trueat line 310, which later invokesUpdateAccountPeersat lines 380-382This addresses the past review comment from pascal-fischer about triggering update account peers.
Optional enhancements for auditability and visibility:
Consider adding an activity event to track the peer auto-approvals for better audit trails:
if approvedCount > 0 { log.WithContext(ctx).Debugf("approved %d pending peers in account %s", approvedCount, accountID) + eventMeta := map[string]any{"approved_count": approvedCount} + am.StoreEvent(ctx, userID, accountID, accountID, activity.PeerApprovalDisabled, eventMeta) updateAccountPeers = true }Additionally, consider using
Infolevel instead ofDebugfor line 309, as auto-approving peers is a significant administrative action:- log.WithContext(ctx).Debugf("approved %d pending peers in account %s", approvedCount, accountID) + log.WithContext(ctx).Infof("approved %d pending peers in account %s", approvedCount, accountID)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
management/server/account.go(1 hunks)management/server/account_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- management/server/account_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
management/server/account.go (1)
shared/management/status/error.go (1)
Errorf(70-75)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (24)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Benchmark (API) (amd64, sqlite)
- GitHub Check: Management / Benchmark (amd64, postgres)
- GitHub Check: Management / Benchmark (API) (amd64, postgres)
- GitHub Check: Management / Integration (amd64, postgres)
- GitHub Check: Management / Benchmark (amd64, sqlite)
- GitHub Check: Management / Integration (amd64, sqlite)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client / Unit (386)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Darwin
- GitHub Check: Windows
- GitHub Check: Linux
- GitHub Check: Client / Unit
- GitHub Check: release
- GitHub Check: release_ui_darwin
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
- GitHub Check: Android / Build
- GitHub Check: Client / Unit
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
go.mod(1 hunks)management/server/account.go(3 hunks)management/server/integrated_validator.go(1 hunks)management/server/integrations/integrated_validator/interface.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
management/server/integrations/integrated_validator/interface.go (1)
management/server/types/settings.go (1)
ExtraSettings(82-99)
management/server/integrated_validator.go (1)
management/server/types/settings.go (1)
ExtraSettings(82-99)
management/server/account.go (1)
management/server/types/settings.go (1)
Settings(10-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
- GitHub Check: Client / Unit
- GitHub Check: Darwin
- GitHub Check: Windows
- GitHub Check: Linux
- GitHub Check: release
- GitHub Check: Client / Unit
- GitHub Check: release_ui_darwin
- GitHub Check: Build Cache
- GitHub Check: Android / Build
- GitHub Check: Check External GPL/AGPL Licenses
- GitHub Check: iOS / Build
🔇 Additional comments (4)
management/server/integrations/integrated_validator/interface.go (1)
13-13: LGTM! Interface signature simplified.The removal of the
peersparameter fromValidateExtraSettingssimplifies the validation interface. This change is consistent with the PR's goal to handle peer approval separately from settings validation.management/server/integrated_validator.go (1)
130-132: LGTM! Mock correctly updated to match interface.The mock implementation properly reflects the updated interface signature by removing the peers parameter while maintaining the no-op behavior.
management/server/account.go (1)
387-402: LGTM! Validation simplified appropriately.The method signature has been correctly updated to remove the transaction parameter and no longer builds a peers map for validation. The validation logic now properly delegates to the integrated validator with the new signature.
Note: The method passes
newSettings.ExtraandoldSettings.Extradirectly to the validator. Ensure the validator implementation handles nilExtragracefully if needed.go.mod (1)
67-67: Dependency version confirmed and compatible.The commit d5400f03084766360f91d30c877c49f8b24ededb exists in the management-integrations repository (branch
update-setting-validator, PR #41). TheValidateExtraSettingsmethod signature in this version isValidateExtraSettings(context.Context, *types.ExtraSettings, *types.ExtraSettings, string, string) error—the peers parameter has been removed, which aligns with the interface changes in this PR.
Signed-off-by: bcmmbaga <[email protected]>
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
management/server/account.go (2)
302-313: Peer-approval disable path correctly approves peers; consider adding an activity event.The conditional around
PeerApprovalEnabledand the call toApproveAccountPeersinside the transaction look sound, and settingupdateAccountPeers = truewhenapprovedCount > 0ensures peers and network maps are refreshed appropriately.To make this change auditable in the same way as other settings toggles (e.g., login/inactivity expiration, DNS domain), consider also emitting an
activityevent when peer approval is disabled and pending peers are auto-approved, not just a debug log.
298-300: KeepvalidateSettingsUpdate/ValidateExtraSettingslightweight and read-only within the transaction.
UpdateAccountSettingsstill invokesvalidateSettingsUpdateinsideExecuteInTransaction, and that now delegates straight tointegratedPeerValidator.ValidateExtraSettings(without a transactional store parameter). This is fine as long asValidateExtraSettingsremains an inexpensive, read-only validator and doesn’t perform long-running I/O or writes; otherwise we risk holding the DB transaction open longer than necessary or depending on state outside the transaction’s view.If the validator needs heavier operations or its own store interactions, consider either:
- moving the call to
validateSettingsUpdatebefore starting the transaction, or- passing all necessary pre-fetched context into the validator so it can stay pure inside the transaction.
Also applies to: 388-403
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
management/server/account.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
management/server/account.go (1)
management/server/types/settings.go (1)
Settings(10-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (26)
- GitHub Check: Management / Integration (amd64, postgres)
- GitHub Check: Management / Integration (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Benchmark (API) (amd64, postgres)
- GitHub Check: Management / Benchmark (API) (amd64, sqlite)
- GitHub Check: Management / Benchmark (amd64, postgres)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Benchmark (amd64, sqlite)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Client / Unit (386)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Client / Unit
- GitHub Check: Windows
- GitHub Check: Linux
- GitHub Check: Darwin
- GitHub Check: JS / Lint
- GitHub Check: release_ui_darwin
- GitHub Check: Client / Unit
- GitHub Check: release
- GitHub Check: release_ui
- GitHub Check: Android / Build
- GitHub Check: Client / Unit
- GitHub Check: iOS / Build
Describe your changes
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.