[management] apply login filter only for setup key peers#4943
[management] apply login filter only for setup key peers#4943pascal-fischer merged 5 commits intomainfrom
Conversation
|
Warning Rate limit exceeded@pascal-fischer has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 23 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds Store and Manager APIs to map a peer key to a user ID, implements the lookup in the SQL store, exposes it via DefaultAccountManager, integrates the lookup into the gRPC Sync path (adjusting login gating and semaphore handling), and adds unit tests for the SQL lookup. Changes
Sequence DiagramsequenceDiagram
participant Client
participant GRPC as gRPC Server (Sync)
participant Acct as Account Manager
participant Store as SQL Store
participant DB as Database
Client->>GRPC: Sync Request (includes peerKey)
activate GRPC
GRPC->>Acct: GetUserIDByPeerKey(ctx, peerKey)
activate Acct
Acct->>Store: GetUserIDByPeerKey(ctx, LockingStrengthNone, peerKey)
activate Store
Store->>DB: SELECT user_id FROM nbpeer.Peer WHERE key = ?
activate DB
DB-->>Store: row (user_id) / no row / error
deactivate DB
alt query success (row)
Store-->>Acct: userID (may be empty)
else query error / no row
Store-->>Acct: error
end
deactivate Store
Acct-->>GRPC: userID or error
deactivate Acct
alt error returned
GRPC->>GRPC: decrement sync semaphore
GRPC-->>Client: mapped error
else userID non-empty OR loginFilter allows
GRPC->>GRPC: continue Sync processing
GRPC-->>Client: Sync response
else userID empty AND loginFilter denies
GRPC->>GRPC: decrement sync semaphore
GRPC-->>Client: mapped error
end
deactivate GRPC
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
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 |
# Conflicts: # management/server/store/sql_store_test.go
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
management/internals/shared/grpc/server.go (1)
187-205: Potential behavior change: unregistered peers may now returnNotFoundinstead ofPermissionDenied.
BecauseGetUserIDByPeerKeyis called beforeGetAccountIDForPeerKey, a missing peer key can short-circuit the old “peer is not registered” handling. Consider treating “not found” fromGetUserIDByPeerKeyasuserID == ""and continue.- userID, err := s.accountManager.GetUserIDByPeerKey(ctx, peerKey.String()) - if err != nil { - s.syncSem.Add(-1) - return mapError(ctx, err) - } + userID, err := s.accountManager.GetUserIDByPeerKey(ctx, peerKey.String()) + if err != nil { + if st, ok := internalStatus.FromError(err); ok && st.Type() == internalStatus.NotFound { + userID = "" + } else { + s.syncSem.Add(-1) + return mapError(ctx, err) + } + }management/server/account/manager.go (1)
26-127: MockAccountManager is missing multiple interface method implementations, including GetUserIDByPeerKey.The
MockAccountManagerinmanagement/server/mock_server/account_mock.gois missing 9 interface methods required byaccount.Manager:
GetUserIDByPeerKeyCreateGroupCreateGroupsUpdateGroupUpdateGroupsGetAccountIDByUserIDGetValidatedPeersOnPeerDisconnectedSyncUserJWTGroupsAdd the corresponding
Funcfields to the mock struct and implement the necessary method receivers to restore compilation.
🧹 Nitpick comments (2)
management/server/account.go (1)
2149-2151: Consider guarding against emptypeerKeyto avoid ambiguous NotFound behavior.
Right now this blindly forwards to the store; an earlyInvalidArgument(or at least a fast-path"") can make upstream error handling more deterministic.management/internals/shared/grpc/server.go (1)
194-246: If the goal is “login filter only for setup-key peers”, also gateloginFilter.addLogin(...)behinduserID == "".
Right now you bypassallowLoginfor user peers, but you still record them in the filter, which is inconsistent with the stated intent and may grow the in-memory filter unnecessarily.- metahash := metaHash(peerMeta, realIP.String()) - s.loginFilter.addLogin(peerKey.String(), metahash) + if userID == "" { + metahash := metaHash(peerMeta, realIP.String()) + s.loginFilter.addLogin(peerKey.String(), metahash) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
management/internals/shared/grpc/server.go(1 hunks)management/server/account.go(1 hunks)management/server/account/manager.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 (1)
management/server/store/sql_store.go (3)
management/server/store/store.go (2)
LockingStrength(40-40)LockingStrengthNone(47-47)management/server/peer/peer.go (1)
Peer(16-58)shared/management/status/error.go (3)
Error(54-57)Errorf(70-75)NewPeerNotFoundError(90-92)
🔇 Additional comments (1)
management/server/store/store.go (1)
50-207: Store interface change correctly implementsGetUserIDByPeerKeyin SqlStore.FileStore is not a Store implementation—it's legacy migration code used only in
MigrateFileStoreToSqlite(). The only active Store implementation is SqlStore, which already includes theGetUserIDByPeerKeymethod (line 4079). No compilation errors or breaking changes to other store backends will occur.
|
pascal-fischer
deleted the
feature/limit-login-filter-to-setup-key-peers
branch
2 participants
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
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.