From b8cab2882b6e314180b208a5f6ddbb728174ff80 Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Thu, 16 Mar 2023 15:57:44 +0100 Subject: [PATCH 01/12] storing and retrieving PATs --- management/server/account.go | 52 ++++++++- management/server/account_test.go | 56 ++++++++++ management/server/file_store.go | 56 +++++++++- management/server/file_store_test.go | 119 +++++++++++++++++++++ management/server/personal_access_token.go | 1 + management/server/store.go | 4 +- management/server/testdata/store.json | 13 ++- management/server/user.go | 24 +++++ management/server/user_test.go | 67 ++++++++++++ 9 files changed, 383 insertions(+), 9 deletions(-) create mode 100644 management/server/user_test.go diff --git a/management/server/account.go b/management/server/account.go index 5b9c9402d..135142b6b 100644 --- a/management/server/account.go +++ b/management/server/account.go @@ -2,7 +2,9 @@ package server import ( "context" + "crypto/sha256" "fmt" + "hash/crc32" "math/rand" "net" "net/netip" @@ -12,17 +14,19 @@ import ( "sync" "time" + "codeberg.org/ac/base62" "github.com/eko/gocache/v3/cache" cacheStore "github.com/eko/gocache/v3/store" + gocache "github.com/patrickmn/go-cache" + "github.com/rs/xid" + log "github.com/sirupsen/logrus" + nbdns "github.com/netbirdio/netbird/dns" "github.com/netbirdio/netbird/management/server/activity" "github.com/netbirdio/netbird/management/server/idp" "github.com/netbirdio/netbird/management/server/jwtclaims" "github.com/netbirdio/netbird/management/server/status" "github.com/netbirdio/netbird/route" - gocache "github.com/patrickmn/go-cache" - "github.com/rs/xid" - log "github.com/sirupsen/logrus" ) const ( @@ -50,6 +54,7 @@ type AccountManager interface { GetSetupKey(accountID, userID, keyID string) (*SetupKey, error) GetAccountByUserOrAccountID(userID, accountID, domain string) (*Account, error) GetAccountFromToken(claims jwtclaims.AuthorizationClaims) (*Account, *User, error) + GetAccountFromPAT(pat string) (*Account, *User, error) IsUserAdmin(claims jwtclaims.AuthorizationClaims) (bool, error) AccountExists(accountId string) (*bool, error) GetPeerByKey(peerKey string) (*Peer, error) @@ -1112,6 +1117,47 @@ func (am *DefaultAccountManager) redeemInvite(account *Account, userID string) e return nil } +// GetAccountFromPAT returns Account and User associated with a personal access token +func (am *DefaultAccountManager) GetAccountFromPAT(token string) (*Account, *User, error) { + if len(token) != PATLength { + return nil, nil, fmt.Errorf("token invalid") + } + + prefix := token[:len(PATPrefix)] + if prefix != PATPrefix { + return nil, nil, fmt.Errorf("token invalid") + } + secret := token[len(PATPrefix):len(PATPrefix)] + encodedChecksum := token[34:40] + + verificationChecksum, err := base62.Decode(encodedChecksum) + if err != nil { + return nil, nil, fmt.Errorf("token invalid") + } + + secretChecksum := crc32.ChecksumIEEE([]byte(secret)) + if secretChecksum != verificationChecksum { + return nil, nil, fmt.Errorf("token invalid") + } + + hashedToken := sha256.Sum256([]byte(token)) + tokenID, err := am.Store.GetTokenIDByHashedToken(string(hashedToken[:])) + if err != nil { + return nil, nil, fmt.Errorf("token invalid") + } + + user, err := am.Store.GetUserByTokenID(tokenID) + if err != nil { + return nil, nil, fmt.Errorf("token invalid") + } + + account, err := am.Store.GetAccountByUser(user.Id) + if err != nil { + return nil, nil, fmt.Errorf("token invalid") + } + return account, user, nil +} + // GetAccountFromToken returns an account associated with this token func (am *DefaultAccountManager) GetAccountFromToken(claims jwtclaims.AuthorizationClaims) (*Account, *User, error) { if claims.UserId == "" { diff --git a/management/server/account_test.go b/management/server/account_test.go index e40d5e5b8..b602f4231 100644 --- a/management/server/account_test.go +++ b/management/server/account_test.go @@ -1,6 +1,7 @@ package server import ( + "crypto/sha256" "fmt" "net" "reflect" @@ -458,6 +459,61 @@ func TestDefaultAccountManager_GetAccountFromToken(t *testing.T) { } } +func TestAccountManager_GetAccountFromPAT(t *testing.T) { + store := newStore(t) + account := newAccountWithId("account_id", "testuser", "") + account.Peers["testpeer"] = &Peer{ + Key: "peerkey", + SetupKey: "peerkeysetupkey", + IP: net.IP{127, 0, 0, 1}, + Meta: PeerSystemMeta{}, + Name: "peer name", + Status: &PeerStatus{Connected: true, LastSeen: time.Now()}, + } + + token := "nbp_9999EUDNdkeusjentDLSJEn1902u84390W6W" + hashedToken := sha256.Sum256([]byte(token)) + pat := PersonalAccessToken{ + ID: "tokenId", + Description: "some Description", + HashedToken: string(hashedToken[:]), + ExpirationDate: time.Time{}, + CreatedBy: "testuser", + CreatedAt: time.Time{}, + LastUsed: time.Time{}, + } + account.Users["someUser"] = &User{ + Id: "someUser", + Role: "", + AutoGroups: nil, + PATs: []PersonalAccessToken{pat}, + } + store.SaveAccount(account) + + am := DefaultAccountManager{ + Store: store, + cacheMux: sync.Mutex{}, + cacheLoading: nil, + peersUpdateManager: nil, + idpManager: nil, + cacheManager: nil, + ctx: nil, + eventStore: nil, + singleAccountMode: false, + singleAccountModeDomain: "", + dnsDomain: "", + peerLoginExpiry: nil, + } + + account, user, err := am.GetAccountFromPAT(token) + if err != nil { + t.Fatalf("Error when getting Account from PAT: %s", err) + } + + assert.Equal(t, "account_id", account.Id) + assert.Equal(t, "someUser", user.Id) +} + func TestAccountManager_PrivateAccount(t *testing.T) { manager, err := createManager(t) if err != nil { diff --git a/management/server/file_store.go b/management/server/file_store.go index e3ec5af44..07e5bc9b0 100644 --- a/management/server/file_store.go +++ b/management/server/file_store.go @@ -7,10 +7,11 @@ import ( "sync" "time" - "github.com/netbirdio/netbird/management/server/status" "github.com/rs/xid" log "github.com/sirupsen/logrus" + "github.com/netbirdio/netbird/management/server/status" + "github.com/netbirdio/netbird/util" ) @@ -25,6 +26,8 @@ type FileStore struct { PeerID2AccountID map[string]string `json:"-"` UserID2AccountID map[string]string `json:"-"` PrivateDomain2AccountID map[string]string `json:"-"` + HashedPAT2TokenID map[string]string `json:"-"` + TokenID2UserID map[string]string `json:"-"` InstallationID string // mutex to synchronise Store read/write operations @@ -57,6 +60,8 @@ func restore(file string) (*FileStore, error) { UserID2AccountID: make(map[string]string), PrivateDomain2AccountID: make(map[string]string), PeerID2AccountID: make(map[string]string), + HashedPAT2TokenID: make(map[string]string), + TokenID2UserID: make(map[string]string), storeFile: file, } @@ -80,6 +85,8 @@ func restore(file string) (*FileStore, error) { store.UserID2AccountID = make(map[string]string) store.PrivateDomain2AccountID = make(map[string]string) store.PeerID2AccountID = make(map[string]string) + store.HashedPAT2TokenID = make(map[string]string) + store.TokenID2UserID = make(map[string]string) for accountID, account := range store.Accounts { if account.Settings == nil { @@ -103,9 +110,10 @@ func restore(file string) (*FileStore, error) { } for _, user := range account.Users { store.UserID2AccountID[user.Id] = accountID - } - for _, user := range account.Users { - store.UserID2AccountID[user.Id] = accountID + for _, pat := range user.PATs { + store.TokenID2UserID[pat.ID] = user.Id + store.HashedPAT2TokenID[string(pat.HashedToken[:])] = pat.ID + } } if account.Domain != "" && account.DomainCategory == PrivateCategory && @@ -258,6 +266,10 @@ func (s *FileStore) SaveAccount(account *Account) error { for _, user := range accountCopy.Users { s.UserID2AccountID[user.Id] = accountCopy.Id + for _, pat := range user.PATs { + s.TokenID2UserID[pat.ID] = user.Id + s.HashedPAT2TokenID[string(pat.HashedToken[:])] = pat.ID + } } if accountCopy.DomainCategory == PrivateCategory && accountCopy.IsDomainPrimaryAccount { @@ -312,6 +324,42 @@ func (s *FileStore) GetAccountBySetupKey(setupKey string) (*Account, error) { return account.Copy(), nil } +// GetTokenIDByHashedToken returns the id of a personal access token by its hashed secret +func (s *FileStore) GetTokenIDByHashedToken(token string) (string, error) { + s.mux.Lock() + defer s.mux.Unlock() + + tokenID, tokenIDFound := s.HashedPAT2TokenID[token] + if !tokenIDFound { + return "", status.Errorf(status.NotFound, "tokenID not found: provided token doesn't exists") + } + + return tokenID, nil +} + +// GetUserByTokenID returns a User object a tokenID belongs to +func (s *FileStore) GetUserByTokenID(tokenID string) (*User, error) { + s.mux.Lock() + defer s.mux.Unlock() + + userID, userIDFound := s.TokenID2UserID[tokenID] + if !userIDFound { + return nil, status.Errorf(status.NotFound, "user not found: provided tokenID doesn't exists") + } + + accountID, accountIDFound := s.UserID2AccountID[userID] + if !accountIDFound { + return nil, status.Errorf(status.NotFound, "accountID not found: provided userID doesn't exists") + } + + account, err := s.getAccount(accountID) + if err != nil { + return nil, err + } + + return account.Users[userID].Copy(), nil +} + // GetAllAccounts returns all accounts func (s *FileStore) GetAllAccounts() (all []*Account) { s.mux.Lock() diff --git a/management/server/file_store_test.go b/management/server/file_store_test.go index 287f043d9..030749501 100644 --- a/management/server/file_store_test.go +++ b/management/server/file_store_test.go @@ -1,6 +1,7 @@ package server import ( + "crypto/sha256" "net" "path/filepath" "testing" @@ -71,6 +72,14 @@ func TestNewStore(t *testing.T) { if store.UserID2AccountID == nil || len(store.UserID2AccountID) != 0 { t.Errorf("expected to create a new empty UserID2AccountID map when creating a new FileStore") } + + if store.HashedPAT2TokenID == nil || len(store.HashedPAT2TokenID) != 0 { + t.Errorf("expected to create a new empty HashedPAT2TokenID map when creating a new FileStore") + } + + if store.TokenID2UserID == nil || len(store.TokenID2UserID) != 0 { + t.Errorf("expected to create a new empty TokenID2UserID map when creating a new FileStore") + } } func TestSaveAccount(t *testing.T) { @@ -239,11 +248,17 @@ func TestRestore(t *testing.T) { require.NotNil(t, account.SetupKeys["A2C8E62B-38F5-4553-B31E-DD66C696CEBB"], "failed to restore a FileStore file - missing Account SetupKey A2C8E62B-38F5-4553-B31E-DD66C696CEBB") + require.Len(t, account.Users["f4f6d672-63fb-11ec-90d6-0242ac120003"].PATs, 1, "failed to restore a FileStore wrong PATs length") + require.Len(t, store.UserID2AccountID, 2, "failed to restore a FileStore wrong UserID2AccountID mapping length") require.Len(t, store.SetupKeyID2AccountID, 1, "failed to restore a FileStore wrong SetupKeyID2AccountID mapping length") require.Len(t, store.PrivateDomain2AccountID, 1, "failed to restore a FileStore wrong PrivateDomain2AccountID mapping length") + + require.Len(t, store.HashedPAT2TokenID, 1, "failed to restore a FileStore wrong HashedPAT2TokenID mapping length") + + require.Len(t, store.TokenID2UserID, 1, "failed to restore a FileStore wrong TokenID2UserID mapping length") } func TestRestorePolicies_Migration(t *testing.T) { @@ -348,6 +363,110 @@ func TestFileStore_GetAccount(t *testing.T) { assert.Len(t, account.NameServerGroups, len(expected.NameServerGroups)) } +func TestFileStore_GetTokenIDByHashedToken(t *testing.T) { + storeDir := t.TempDir() + storeFile := filepath.Join(storeDir, "store.json") + err := util.CopyFileContents("testdata/store.json", storeFile) + if err != nil { + t.Fatal(err) + } + + accounts := &accounts{} + _, err = util.ReadJson(storeFile, accounts) + if err != nil { + t.Fatal(err) + } + + store, err := NewFileStore(storeDir) + if err != nil { + t.Fatal(err) + } + + hashedToken := accounts.Accounts["bf1c8084-ba50-4ce7-9439-34653001fc3b"].Users["f4f6d672-63fb-11ec-90d6-0242ac120003"].PATs[0].HashedToken + tokenID, err := store.GetTokenIDByHashedToken(hashedToken) + + expectedTokenID := accounts.Accounts["bf1c8084-ba50-4ce7-9439-34653001fc3b"].Users["f4f6d672-63fb-11ec-90d6-0242ac120003"].PATs[0].ID + assert.Equal(t, expectedTokenID, tokenID) +} + +func TestFileStore_GetTokenIDByHashedToken_Failure(t *testing.T) { + storeDir := t.TempDir() + storeFile := filepath.Join(storeDir, "store.json") + err := util.CopyFileContents("testdata/store.json", storeFile) + if err != nil { + t.Fatal(err) + } + + accounts := &accounts{} + _, err = util.ReadJson(storeFile, accounts) + if err != nil { + t.Fatal(err) + } + + store, err := NewFileStore(storeDir) + if err != nil { + t.Fatal(err) + } + + wrongToken := sha256.Sum256([]byte("someNotValidTokenThatFails1234")) + _, err = store.GetTokenIDByHashedToken(string(wrongToken[:])) + + assert.Error(t, err, "GetTokenIDByHashedToken should throw error if token invalid") +} + +func TestFileStore_GetUserByTokenID(t *testing.T) { + storeDir := t.TempDir() + storeFile := filepath.Join(storeDir, "store.json") + err := util.CopyFileContents("testdata/store.json", storeFile) + if err != nil { + t.Fatal(err) + } + + accounts := &accounts{} + _, err = util.ReadJson(storeFile, accounts) + if err != nil { + t.Fatal(err) + } + + store, err := NewFileStore(storeDir) + if err != nil { + t.Fatal(err) + } + + tokenId := accounts.Accounts["bf1c8084-ba50-4ce7-9439-34653001fc3b"].Users["f4f6d672-63fb-11ec-90d6-0242ac120003"].PATs[0].ID + user, err := store.GetUserByTokenID(tokenId) + if err != nil { + t.Fatal(err) + } + + assert.Equal(t, "f4f6d672-63fb-11ec-90d6-0242ac120003", user.Id) +} + +func TestFileStore_GetUserByTokenID_Failure(t *testing.T) { + storeDir := t.TempDir() + storeFile := filepath.Join(storeDir, "store.json") + err := util.CopyFileContents("testdata/store.json", storeFile) + if err != nil { + t.Fatal(err) + } + + accounts := &accounts{} + _, err = util.ReadJson(storeFile, accounts) + if err != nil { + t.Fatal(err) + } + + store, err := NewFileStore(storeDir) + if err != nil { + t.Fatal(err) + } + + wrongTokenID := "someNonExistingTokenID" + _, err = store.GetUserByTokenID(wrongTokenID) + + assert.Error(t, err, "GetUserByTokenID should throw error if tokenID invalid") +} + func TestFileStore_SavePeerStatus(t *testing.T) { storeDir := t.TempDir() diff --git a/management/server/personal_access_token.go b/management/server/personal_access_token.go index e7ee05dad..307f0a27a 100644 --- a/management/server/personal_access_token.go +++ b/management/server/personal_access_token.go @@ -15,6 +15,7 @@ const ( // PATPrefix is the globally used, 4 char prefix for personal access tokens PATPrefix = "nbp_" secretLength = 30 + PATLength = 40 ) // PersonalAccessToken holds all information about a PAT including a hashed version of it for verification diff --git a/management/server/store.go b/management/server/store.go index 02041dfda..3577a9f59 100644 --- a/management/server/store.go +++ b/management/server/store.go @@ -6,8 +6,10 @@ type Store interface { GetAccountByUser(userID string) (*Account, error) GetAccountByPeerPubKey(peerKey string) (*Account, error) GetAccountByPeerID(peerID string) (*Account, error) - GetAccountBySetupKey(setupKey string) (*Account, error) //todo use key hash later + GetAccountBySetupKey(setupKey string) (*Account, error) // todo use key hash later GetAccountByPrivateDomain(domain string) (*Account, error) + GetTokenIDByHashedToken(secret string) (string, error) + GetUserByTokenID(tokenID string) (*User, error) SaveAccount(account *Account) error GetInstallationID() string SaveInstallationID(ID string) error diff --git a/management/server/testdata/store.json b/management/server/testdata/store.json index 8eddeca23..7eed7f4c0 100644 --- a/management/server/testdata/store.json +++ b/management/server/testdata/store.json @@ -33,7 +33,18 @@ }, "f4f6d672-63fb-11ec-90d6-0242ac120003": { "Id": "f4f6d672-63fb-11ec-90d6-0242ac120003", - "Role": "user" + "Role": "user", + "PATs":[ + { + "ID":"9dj38s35-63fb-11ec-90d6-0242ac120003", + "Description":"some Description", + "HashedToken":"SoMeHaShEdToKeN", + "ExpirationDate":"2023-02-27T00:00:00Z", + "CreatedBy":"user", + "CreatedAt":"2023-01-01T00:00:00Z", + "LastUsed":"2023-02-01T00:00:00Z" + } + ] } } } diff --git a/management/server/user.go b/management/server/user.go index a9aaa1b61..682749475 100644 --- a/management/server/user.go +++ b/management/server/user.go @@ -189,6 +189,30 @@ func (am *DefaultAccountManager) CreateUser(accountID, userID string, invite *Us } +// AddPATToUser takes the userID and the accountID the user belongs to and assigns a provided PersonalAccessToken to that user +func (am *DefaultAccountManager) AddPATToUser(accountID string, userID string, pat PersonalAccessToken) error { + unlock := am.Store.AcquireAccountLock(accountID) + defer unlock() + + account, err := am.Store.GetAccount(accountID) + if err != nil { + return err + } + + user := account.Users[userID] + if user == nil { + return status.Errorf(status.NotFound, "user not found") + } + + user.PATs = append(user.PATs, pat) + + if err = am.Store.SaveAccount(account); err != nil { + return err + } + + return nil +} + // SaveUser saves updates a given user. If the user doesn't exit it will throw status.NotFound error. // Only User.AutoGroups field is allowed to be updated for now. func (am *DefaultAccountManager) SaveUser(accountID, userID string, update *User) (*UserInfo, error) { diff --git a/management/server/user_test.go b/management/server/user_test.go new file mode 100644 index 000000000..41f009ee3 --- /dev/null +++ b/management/server/user_test.go @@ -0,0 +1,67 @@ +package server + +import ( + "net" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/assert" +) + +func TestUser_AddPATToUser(t *testing.T) { + store := newStore(t) + account := newAccountWithId("account_id", "testuser", "") + account.Peers["testpeer"] = &Peer{ + Key: "peerkey", + SetupKey: "peerkeysetupkey", + IP: net.IP{127, 0, 0, 1}, + Meta: PeerSystemMeta{}, + Name: "peer name", + Status: &PeerStatus{Connected: true, LastSeen: time.Now()}, + } + store.SaveAccount(account) + + am := DefaultAccountManager{ + Store: store, + cacheMux: sync.Mutex{}, + cacheLoading: nil, + peersUpdateManager: nil, + idpManager: nil, + cacheManager: nil, + ctx: nil, + eventStore: nil, + singleAccountMode: false, + singleAccountModeDomain: "", + dnsDomain: "", + peerLoginExpiry: nil, + } + + token := "someToken" + pat := PersonalAccessToken{ + ID: "tokenId", + Description: "some Description", + HashedToken: token, + ExpirationDate: time.Time{}, + CreatedBy: "testuser", + CreatedAt: time.Time{}, + LastUsed: time.Time{}, + } + + am.AddPATToUser("account_id", "testuser", pat) + + fileStore := am.Store.(*FileStore) + tokenId := fileStore.HashedPAT2TokenID[string(token[:])] + + if tokenId == "" { + t.Fatal("GetTokenIDByHashedToken failed after adding PAT") + } + + assert.Equal(t, "tokenId", tokenId) + + userId := fileStore.TokenID2UserID[tokenId] + if userId == "" { + t.Fatal("GetUserByTokenId failed after adding PAT") + } + assert.Equal(t, "testuser", userId) +} From 453643683ddeb140bd5222e55659090678e342bf Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Thu, 16 Mar 2023 16:44:05 +0100 Subject: [PATCH 02/12] add method to account mock --- management/server/mock_server/account_mock.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/management/server/mock_server/account_mock.go b/management/server/mock_server/account_mock.go index 9da748757..4f2580679 100644 --- a/management/server/mock_server/account_mock.go +++ b/management/server/mock_server/account_mock.go @@ -47,6 +47,7 @@ type MockAccountManager struct { DeletePolicyFunc func(accountID, policyID, userID string) error ListPoliciesFunc func(accountID, userID string) ([]*server.Policy, error) GetUsersFromAccountFunc func(accountID, userID string) ([]*server.UserInfo, error) + GetAccountFromPATFunc func(pat string) (*server.Account, *server.User, error) UpdatePeerMetaFunc func(peerID string, meta server.PeerSystemMeta) error UpdatePeerSSHKeyFunc func(peerID string, sshKey string) error UpdatePeerFunc func(accountID, userID string, peer *server.Peer) (*server.Peer, error) @@ -175,6 +176,14 @@ func (am *MockAccountManager) GetPeerByIP(accountId string, peerIP string) (*ser return nil, status.Errorf(codes.Unimplemented, "method GetPeerByIP is not implemented") } +// GetAccountFromPAT mock implementation of GetAccountFromPAT from server.AccountManager interface +func (am *MockAccountManager) GetAccountFromPAT(pat string) (*server.Account, *server.User, error) { + if am.GetAccountFromPATFunc != nil { + return am.GetAccountFromPATFunc(pat) + } + return nil, nil, status.Errorf(codes.Unimplemented, "method GetAccountFromPAT is not implemented") +} + // GetNetworkMap mock implementation of GetNetworkMap from server.AccountManager interface func (am *MockAccountManager) GetNetworkMap(peerKey string) (*server.NetworkMap, error) { if am.GetNetworkMapFunc != nil { From 628a201e310c6914a66c1963ed62f6df70b329a9 Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Thu, 16 Mar 2023 16:59:32 +0100 Subject: [PATCH 03/12] fix PAT array split --- management/server/account.go | 4 ++-- management/server/personal_access_token.go | 9 +++++---- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/management/server/account.go b/management/server/account.go index 135142b6b..f098dfc68 100644 --- a/management/server/account.go +++ b/management/server/account.go @@ -1127,8 +1127,8 @@ func (am *DefaultAccountManager) GetAccountFromPAT(token string) (*Account, *Use if prefix != PATPrefix { return nil, nil, fmt.Errorf("token invalid") } - secret := token[len(PATPrefix):len(PATPrefix)] - encodedChecksum := token[34:40] + secret := token[len(PATPrefix) : len(PATPrefix)+PATsecretLength] + encodedChecksum := token[len(PATPrefix)+PATsecretLength : len(PATPrefix)+PATsecretLength+PATChecksumLength] verificationChecksum, err := base62.Decode(encodedChecksum) if err != nil { diff --git a/management/server/personal_access_token.go b/management/server/personal_access_token.go index 307f0a27a..27456ec08 100644 --- a/management/server/personal_access_token.go +++ b/management/server/personal_access_token.go @@ -13,9 +13,10 @@ import ( const ( // PATPrefix is the globally used, 4 char prefix for personal access tokens - PATPrefix = "nbp_" - secretLength = 30 - PATLength = 40 + PATPrefix = "nbp_" + PATsecretLength = 30 + PATLength = 40 + PATChecksumLength = 6 ) // PersonalAccessToken holds all information about a PAT including a hashed version of it for verification @@ -50,7 +51,7 @@ func CreateNewPAT(description string, expirationInDays int, createdBy string) (* } func generateNewToken() (string, string, error) { - secret, err := b.Random(secretLength) + secret, err := b.Random(PATsecretLength) if err != nil { return "", "", err } From b852198f67a7d2093a642e498abba351f759e181 Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Mon, 20 Mar 2023 11:44:12 +0100 Subject: [PATCH 04/12] codacy and lint hints --- management/server/account.go | 4 ++-- management/server/account_test.go | 5 ++++- management/server/file_store_test.go | 10 ++++++--- management/server/http/pat_handler.go | 16 +++++++++++++++ management/server/personal_access_token.go | 11 ++++++---- management/server/user.go | 7 ++----- management/server/user_test.go | 24 ++++++++++++++-------- 7 files changed, 53 insertions(+), 24 deletions(-) create mode 100644 management/server/http/pat_handler.go diff --git a/management/server/account.go b/management/server/account.go index f098dfc68..d3f7df028 100644 --- a/management/server/account.go +++ b/management/server/account.go @@ -1127,8 +1127,8 @@ func (am *DefaultAccountManager) GetAccountFromPAT(token string) (*Account, *Use if prefix != PATPrefix { return nil, nil, fmt.Errorf("token invalid") } - secret := token[len(PATPrefix) : len(PATPrefix)+PATsecretLength] - encodedChecksum := token[len(PATPrefix)+PATsecretLength : len(PATPrefix)+PATsecretLength+PATChecksumLength] + secret := token[len(PATPrefix) : len(PATPrefix)+PATSecretLength] + encodedChecksum := token[len(PATPrefix)+PATSecretLength : len(PATPrefix)+PATSecretLength+PATChecksumLength] verificationChecksum, err := base62.Decode(encodedChecksum) if err != nil { diff --git a/management/server/account_test.go b/management/server/account_test.go index b602f4231..fa932023a 100644 --- a/management/server/account_test.go +++ b/management/server/account_test.go @@ -488,7 +488,10 @@ func TestAccountManager_GetAccountFromPAT(t *testing.T) { AutoGroups: nil, PATs: []PersonalAccessToken{pat}, } - store.SaveAccount(account) + err := store.SaveAccount(account) + if err != nil { + t.Fatalf("Error when saving account: %s", err) + } am := DefaultAccountManager{ Store: store, diff --git a/management/server/file_store_test.go b/management/server/file_store_test.go index 030749501..838af50fb 100644 --- a/management/server/file_store_test.go +++ b/management/server/file_store_test.go @@ -7,9 +7,10 @@ import ( "testing" "time" - "github.com/netbirdio/netbird/util" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/netbirdio/netbird/util" ) type accounts struct { @@ -384,6 +385,9 @@ func TestFileStore_GetTokenIDByHashedToken(t *testing.T) { hashedToken := accounts.Accounts["bf1c8084-ba50-4ce7-9439-34653001fc3b"].Users["f4f6d672-63fb-11ec-90d6-0242ac120003"].PATs[0].HashedToken tokenID, err := store.GetTokenIDByHashedToken(hashedToken) + if err != nil { + t.Fatal(err) + } expectedTokenID := accounts.Accounts["bf1c8084-ba50-4ce7-9439-34653001fc3b"].Users["f4f6d672-63fb-11ec-90d6-0242ac120003"].PATs[0].ID assert.Equal(t, expectedTokenID, tokenID) @@ -433,8 +437,8 @@ func TestFileStore_GetUserByTokenID(t *testing.T) { t.Fatal(err) } - tokenId := accounts.Accounts["bf1c8084-ba50-4ce7-9439-34653001fc3b"].Users["f4f6d672-63fb-11ec-90d6-0242ac120003"].PATs[0].ID - user, err := store.GetUserByTokenID(tokenId) + tokenID := accounts.Accounts["bf1c8084-ba50-4ce7-9439-34653001fc3b"].Users["f4f6d672-63fb-11ec-90d6-0242ac120003"].PATs[0].ID + user, err := store.GetUserByTokenID(tokenID) if err != nil { t.Fatal(err) } diff --git a/management/server/http/pat_handler.go b/management/server/http/pat_handler.go new file mode 100644 index 000000000..40199fcd6 --- /dev/null +++ b/management/server/http/pat_handler.go @@ -0,0 +1,16 @@ +package http + +import ( + "github.com/netbirdio/netbird/management/server" + "github.com/netbirdio/netbird/management/server/jwtclaims" +) + +// PATHandler is the nameserver group handler of the account +type PATHandler struct { + accountManager server.AccountManager + claimsExtractor *jwtclaims.ClaimsExtractor +} + +func NewPATsHandler(accountManager server.AccountManager, authCfg AuthCfg) { + +} diff --git a/management/server/personal_access_token.go b/management/server/personal_access_token.go index 27456ec08..7416a9e0b 100644 --- a/management/server/personal_access_token.go +++ b/management/server/personal_access_token.go @@ -13,10 +13,13 @@ import ( const ( // PATPrefix is the globally used, 4 char prefix for personal access tokens - PATPrefix = "nbp_" - PATsecretLength = 30 - PATLength = 40 + PATPrefix = "nbp_" + // PATSecretLength number of characters used for the secret inside the token + PATSecretLength = 30 + // PATChecksumLength number of characters used for the encoded checksum of the secret inside the token PATChecksumLength = 6 + // PATLength total number of characters used for the token + PATLength = 40 ) // PersonalAccessToken holds all information about a PAT including a hashed version of it for verification @@ -51,7 +54,7 @@ func CreateNewPAT(description string, expirationInDays int, createdBy string) (* } func generateNewToken() (string, string, error) { - secret, err := b.Random(PATsecretLength) + secret, err := b.Random(PATSecretLength) if err != nil { return "", "", err } diff --git a/management/server/user.go b/management/server/user.go index 682749475..fcebe006d 100644 --- a/management/server/user.go +++ b/management/server/user.go @@ -206,11 +206,8 @@ func (am *DefaultAccountManager) AddPATToUser(accountID string, userID string, p user.PATs = append(user.PATs, pat) - if err = am.Store.SaveAccount(account); err != nil { - return err - } - - return nil + err = am.Store.SaveAccount(account) + return err } // SaveUser saves updates a given user. If the user doesn't exit it will throw status.NotFound error. diff --git a/management/server/user_test.go b/management/server/user_test.go index 41f009ee3..1c81827ac 100644 --- a/management/server/user_test.go +++ b/management/server/user_test.go @@ -20,7 +20,10 @@ func TestUser_AddPATToUser(t *testing.T) { Name: "peer name", Status: &PeerStatus{Connected: true, LastSeen: time.Now()}, } - store.SaveAccount(account) + err := store.SaveAccount(account) + if err != nil { + t.Fatalf("Error when saving account: %s", err) + } am := DefaultAccountManager{ Store: store, @@ -39,7 +42,7 @@ func TestUser_AddPATToUser(t *testing.T) { token := "someToken" pat := PersonalAccessToken{ - ID: "tokenId", + ID: "tokenID", Description: "some Description", HashedToken: token, ExpirationDate: time.Time{}, @@ -48,20 +51,23 @@ func TestUser_AddPATToUser(t *testing.T) { LastUsed: time.Time{}, } - am.AddPATToUser("account_id", "testuser", pat) + err = am.AddPATToUser("account_id", "testuser", pat) + if err != nil { + t.Fatalf("Error when adding PAT to user: %s", err) + } fileStore := am.Store.(*FileStore) - tokenId := fileStore.HashedPAT2TokenID[string(token[:])] + tokenID := fileStore.HashedPAT2TokenID[token[:]] - if tokenId == "" { + if tokenID == "" { t.Fatal("GetTokenIDByHashedToken failed after adding PAT") } - assert.Equal(t, "tokenId", tokenId) + assert.Equal(t, "tokenID", tokenID) - userId := fileStore.TokenID2UserID[tokenId] - if userId == "" { + userID := fileStore.TokenID2UserID[tokenID] + if userID == "" { t.Fatal("GetUserByTokenId failed after adding PAT") } - assert.Equal(t, "testuser", userId) + assert.Equal(t, "testuser", userID) } From 511ba6d51f16daa3f3e2f733791eff16fb35c635 Mon Sep 17 00:00:00 2001 From: pascal-fischer <32096965+pascal-fischer@users.noreply.github.com> Date: Mon, 20 Mar 2023 11:47:54 +0100 Subject: [PATCH 05/12] Delete pat_handler.go --- management/server/http/pat_handler.go | 16 ---------------- 1 file changed, 16 deletions(-) delete mode 100644 management/server/http/pat_handler.go diff --git a/management/server/http/pat_handler.go b/management/server/http/pat_handler.go deleted file mode 100644 index 40199fcd6..000000000 --- a/management/server/http/pat_handler.go +++ /dev/null @@ -1,16 +0,0 @@ -package http - -import ( - "github.com/netbirdio/netbird/management/server" - "github.com/netbirdio/netbird/management/server/jwtclaims" -) - -// PATHandler is the nameserver group handler of the account -type PATHandler struct { - accountManager server.AccountManager - claimsExtractor *jwtclaims.ClaimsExtractor -} - -func NewPATsHandler(accountManager server.AccountManager, authCfg AuthCfg) { - -} From e1ef091d4545dd5f689e4b1de3b74b0e2d26fcfa Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Mon, 20 Mar 2023 12:08:01 +0100 Subject: [PATCH 06/12] remove unnecessary string conversion --- management/server/file_store.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/management/server/file_store.go b/management/server/file_store.go index 07e5bc9b0..50bfec1d6 100644 --- a/management/server/file_store.go +++ b/management/server/file_store.go @@ -112,7 +112,7 @@ func restore(file string) (*FileStore, error) { store.UserID2AccountID[user.Id] = accountID for _, pat := range user.PATs { store.TokenID2UserID[pat.ID] = user.Id - store.HashedPAT2TokenID[string(pat.HashedToken[:])] = pat.ID + store.HashedPAT2TokenID[pat.HashedToken[:]] = pat.ID } } @@ -268,7 +268,7 @@ func (s *FileStore) SaveAccount(account *Account) error { s.UserID2AccountID[user.Id] = accountCopy.Id for _, pat := range user.PATs { s.TokenID2UserID[pat.ID] = user.Id - s.HashedPAT2TokenID[string(pat.HashedToken[:])] = pat.ID + s.HashedPAT2TokenID[pat.HashedToken[:]] = pat.ID } } From e30def175b5bdf525e6dbcc28c4ebcb5e457ac00 Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Mon, 20 Mar 2023 16:14:55 +0100 Subject: [PATCH 07/12] switch PATs to map and add deletion --- management/server/account.go | 2 + management/server/account_test.go | 25 ++++++------ management/server/file_store.go | 18 ++++++++ management/server/file_store_test.go | 26 ++++++++++-- management/server/store.go | 2 + management/server/testdata/store.json | 9 ++-- management/server/user.go | 47 ++++++++++++++++++--- management/server/user_test.go | 59 ++++++++++++++++++++++++++- 8 files changed, 162 insertions(+), 26 deletions(-) diff --git a/management/server/account.go b/management/server/account.go index d3f7df028..e998b8d5c 100644 --- a/management/server/account.go +++ b/management/server/account.go @@ -66,6 +66,8 @@ type AccountManager interface { GetNetworkMap(peerID string) (*NetworkMap, error) GetPeerNetwork(peerID string) (*Network, error) AddPeer(setupKey, userID string, peer *Peer) (*Peer, *NetworkMap, error) + AddPATToUser(accountID string, userID string, pat *PersonalAccessToken) error + DeletePAT(accountID string, userID string, tokenID string) error UpdatePeerSSHKey(peerID string, sshKey string) error GetUsersFromAccount(accountID, userID string) ([]*UserInfo, error) GetGroup(accountId, groupID string) (*Group, error) diff --git a/management/server/account_test.go b/management/server/account_test.go index fa932023a..d211973c2 100644 --- a/management/server/account_test.go +++ b/management/server/account_test.go @@ -473,20 +473,21 @@ func TestAccountManager_GetAccountFromPAT(t *testing.T) { token := "nbp_9999EUDNdkeusjentDLSJEn1902u84390W6W" hashedToken := sha256.Sum256([]byte(token)) - pat := PersonalAccessToken{ - ID: "tokenId", - Description: "some Description", - HashedToken: string(hashedToken[:]), - ExpirationDate: time.Time{}, - CreatedBy: "testuser", - CreatedAt: time.Time{}, - LastUsed: time.Time{}, - } account.Users["someUser"] = &User{ Id: "someUser", Role: "", AutoGroups: nil, - PATs: []PersonalAccessToken{pat}, + PATs: map[string]*PersonalAccessToken{ + "pat1": { + ID: "tokenId", + Description: "some Description", + HashedToken: string(hashedToken[:]), + ExpirationDate: time.Time{}, + CreatedBy: "testuser", + CreatedAt: time.Time{}, + LastUsed: time.Time{}, + }, + }, } err := store.SaveAccount(account) if err != nil { @@ -1267,8 +1268,8 @@ func TestAccount_Copy(t *testing.T) { Id: "user1", Role: UserRoleAdmin, AutoGroups: []string{"group1"}, - PATs: []PersonalAccessToken{ - { + PATs: map[string]*PersonalAccessToken{ + "pat1": { ID: "pat1", Description: "First PAT", HashedToken: "SoMeHaShEdToKeN", diff --git a/management/server/file_store.go b/management/server/file_store.go index 50bfec1d6..cd98ca6b8 100644 --- a/management/server/file_store.go +++ b/management/server/file_store.go @@ -288,6 +288,24 @@ func (s *FileStore) SaveAccount(account *Account) error { return s.persist(s.storeFile) } +func (s *FileStore) DeleteHashedPAT2TokenIDIndex(hashedToken string) error { + s.mux.Lock() + defer s.mux.Unlock() + + delete(s.HashedPAT2TokenID, hashedToken) + + return s.persist(s.storeFile) +} + +func (s *FileStore) DeleteTokenID2UserIDIndex(tokenID string) error { + s.mux.Lock() + defer s.mux.Unlock() + + delete(s.TokenID2UserID, tokenID) + + return s.persist(s.storeFile) +} + // GetAccountByPrivateDomain returns account by private domain func (s *FileStore) GetAccountByPrivateDomain(domain string) (*Account, error) { s.mux.Lock() diff --git a/management/server/file_store_test.go b/management/server/file_store_test.go index 838af50fb..c2ccbf546 100644 --- a/management/server/file_store_test.go +++ b/management/server/file_store_test.go @@ -249,7 +249,7 @@ func TestRestore(t *testing.T) { require.NotNil(t, account.SetupKeys["A2C8E62B-38F5-4553-B31E-DD66C696CEBB"], "failed to restore a FileStore file - missing Account SetupKey A2C8E62B-38F5-4553-B31E-DD66C696CEBB") - require.Len(t, account.Users["f4f6d672-63fb-11ec-90d6-0242ac120003"].PATs, 1, "failed to restore a FileStore wrong PATs length") + require.NotNil(t, account.Users["f4f6d672-63fb-11ec-90d6-0242ac120003"].PATs["9dj38s35-63fb-11ec-90d6-0242ac120003"], "failed to restore a FileStore wrong PATs length") require.Len(t, store.UserID2AccountID, 2, "failed to restore a FileStore wrong UserID2AccountID mapping length") @@ -383,16 +383,34 @@ func TestFileStore_GetTokenIDByHashedToken(t *testing.T) { t.Fatal(err) } - hashedToken := accounts.Accounts["bf1c8084-ba50-4ce7-9439-34653001fc3b"].Users["f4f6d672-63fb-11ec-90d6-0242ac120003"].PATs[0].HashedToken + hashedToken := accounts.Accounts["bf1c8084-ba50-4ce7-9439-34653001fc3b"].Users["f4f6d672-63fb-11ec-90d6-0242ac120003"].PATs["9dj38s35-63fb-11ec-90d6-0242ac120003"].HashedToken tokenID, err := store.GetTokenIDByHashedToken(hashedToken) if err != nil { t.Fatal(err) } - expectedTokenID := accounts.Accounts["bf1c8084-ba50-4ce7-9439-34653001fc3b"].Users["f4f6d672-63fb-11ec-90d6-0242ac120003"].PATs[0].ID + expectedTokenID := accounts.Accounts["bf1c8084-ba50-4ce7-9439-34653001fc3b"].Users["f4f6d672-63fb-11ec-90d6-0242ac120003"].PATs["9dj38s35-63fb-11ec-90d6-0242ac120003"].ID assert.Equal(t, expectedTokenID, tokenID) } +func TestFileStore_DeleteHashedPAT2TokenIDIndex(t *testing.T) { + store := newStore(t) + store.HashedPAT2TokenID["someHashedToken"] = "someTokenId" + + store.DeleteHashedPAT2TokenIDIndex("someHashedToken") + + assert.Empty(t, store.HashedPAT2TokenID["someHashedToken"]) +} + +func TestFileStore_DeleteTokenID2UserIDIndex(t *testing.T) { + store := newStore(t) + store.TokenID2UserID["someTokenId"] = "someUserId" + + store.DeleteTokenID2UserIDIndex("someTokenId") + + assert.Empty(t, store.TokenID2UserID["someTokenId"]) +} + func TestFileStore_GetTokenIDByHashedToken_Failure(t *testing.T) { storeDir := t.TempDir() storeFile := filepath.Join(storeDir, "store.json") @@ -437,7 +455,7 @@ func TestFileStore_GetUserByTokenID(t *testing.T) { t.Fatal(err) } - tokenID := accounts.Accounts["bf1c8084-ba50-4ce7-9439-34653001fc3b"].Users["f4f6d672-63fb-11ec-90d6-0242ac120003"].PATs[0].ID + tokenID := accounts.Accounts["bf1c8084-ba50-4ce7-9439-34653001fc3b"].Users["f4f6d672-63fb-11ec-90d6-0242ac120003"].PATs["9dj38s35-63fb-11ec-90d6-0242ac120003"].ID user, err := store.GetUserByTokenID(tokenID) if err != nil { t.Fatal(err) diff --git a/management/server/store.go b/management/server/store.go index 3577a9f59..daad30eaa 100644 --- a/management/server/store.go +++ b/management/server/store.go @@ -11,6 +11,8 @@ type Store interface { GetTokenIDByHashedToken(secret string) (string, error) GetUserByTokenID(tokenID string) (*User, error) SaveAccount(account *Account) error + DeleteHashedPAT2TokenIDIndex(hashedToken string) error + DeleteTokenID2UserIDIndex(tokenID string) error GetInstallationID() string SaveInstallationID(ID string) error // AcquireAccountLock should attempt to acquire account lock and return a function that releases the lock diff --git a/management/server/testdata/store.json b/management/server/testdata/store.json index 7eed7f4c0..ecde766c3 100644 --- a/management/server/testdata/store.json +++ b/management/server/testdata/store.json @@ -29,13 +29,14 @@ "Users": { "edafee4e-63fb-11ec-90d6-0242ac120003": { "Id": "edafee4e-63fb-11ec-90d6-0242ac120003", - "Role": "admin" + "Role": "admin", + "PATs": {} }, "f4f6d672-63fb-11ec-90d6-0242ac120003": { "Id": "f4f6d672-63fb-11ec-90d6-0242ac120003", "Role": "user", - "PATs":[ - { + "PATs": { + "9dj38s35-63fb-11ec-90d6-0242ac120003": { "ID":"9dj38s35-63fb-11ec-90d6-0242ac120003", "Description":"some Description", "HashedToken":"SoMeHaShEdToKeN", @@ -44,7 +45,7 @@ "CreatedAt":"2023-01-01T00:00:00Z", "LastUsed":"2023-02-01T00:00:00Z" } - ] + } } } } diff --git a/management/server/user.go b/management/server/user.go index fcebe006d..8047c624c 100644 --- a/management/server/user.go +++ b/management/server/user.go @@ -46,7 +46,7 @@ type User struct { Role UserRole // AutoGroups is a list of Group IDs to auto-assign to peers registered by this user AutoGroups []string - PATs []PersonalAccessToken + PATs map[string]*PersonalAccessToken } // IsAdmin returns true if user is an admin, false otherwise @@ -94,8 +94,12 @@ func (u *User) toUserInfo(userData *idp.UserData) (*UserInfo, error) { func (u *User) Copy() *User { autoGroups := make([]string, len(u.AutoGroups)) copy(autoGroups, u.AutoGroups) - pats := make([]PersonalAccessToken, len(u.PATs)) - copy(pats, u.PATs) + pats := make(map[string]*PersonalAccessToken, len(u.PATs)) + for k, v := range u.PATs { + patCopy := new(PersonalAccessToken) + *patCopy = *v + pats[k] = patCopy + } return &User{ Id: u.Id, Role: u.Role, @@ -190,7 +194,7 @@ func (am *DefaultAccountManager) CreateUser(accountID, userID string, invite *Us } // AddPATToUser takes the userID and the accountID the user belongs to and assigns a provided PersonalAccessToken to that user -func (am *DefaultAccountManager) AddPATToUser(accountID string, userID string, pat PersonalAccessToken) error { +func (am *DefaultAccountManager) AddPATToUser(accountID string, userID string, pat *PersonalAccessToken) error { unlock := am.Store.AcquireAccountLock(accountID) defer unlock() @@ -204,7 +208,40 @@ func (am *DefaultAccountManager) AddPATToUser(accountID string, userID string, p return status.Errorf(status.NotFound, "user not found") } - user.PATs = append(user.PATs, pat) + user.PATs[pat.ID] = pat + + err = am.Store.SaveAccount(account) + return err +} + +func (am *DefaultAccountManager) DeletePAT(accountID string, userID string, tokenID string) error { + unlock := am.Store.AcquireAccountLock(accountID) + defer unlock() + + account, err := am.Store.GetAccount(accountID) + if err != nil { + return err + } + + user := account.Users[userID] + if user == nil { + return status.Errorf(status.NotFound, "user not found") + } + + pat := user.PATs["tokenID"] + if pat == nil { + return status.Errorf(status.NotFound, "PAT not found") + } + + err = am.Store.DeleteTokenID2UserIDIndex(pat.ID) + if err != nil { + return err + } + err = am.Store.DeleteHashedPAT2TokenIDIndex(pat.HashedToken) + if err != nil { + return err + } + delete(user.PATs, tokenID) err = am.Store.SaveAccount(account) return err diff --git a/management/server/user_test.go b/management/server/user_test.go index 1c81827ac..b3cd1db9a 100644 --- a/management/server/user_test.go +++ b/management/server/user_test.go @@ -51,7 +51,7 @@ func TestUser_AddPATToUser(t *testing.T) { LastUsed: time.Time{}, } - err = am.AddPATToUser("account_id", "testuser", pat) + err = am.AddPATToUser("account_id", "testuser", &pat) if err != nil { t.Fatalf("Error when adding PAT to user: %s", err) } @@ -71,3 +71,60 @@ func TestUser_AddPATToUser(t *testing.T) { } assert.Equal(t, "testuser", userID) } + +func TestUser_DeletePAT(t *testing.T) { + store := newStore(t) + account := newAccountWithId("account_id", "testuser", "") + account.Peers["testpeer"] = &Peer{ + Key: "peerkey", + SetupKey: "peerkeysetupkey", + IP: net.IP{127, 0, 0, 1}, + Meta: PeerSystemMeta{}, + Name: "peer name", + Status: &PeerStatus{Connected: true, LastSeen: time.Now()}, + } + account.Users["user1"] = &User{ + Id: "user1", + Role: "admin", + AutoGroups: nil, + PATs: map[string]*PersonalAccessToken{ + "tokenID": { + ID: "tokenID", + Description: "some Description", + HashedToken: "SoMeHaShEdToKeN", + ExpirationDate: time.Now().AddDate(0, 0, 7), + CreatedBy: "user1", + CreatedAt: time.Now(), + LastUsed: time.Now(), + }, + }, + } + err := store.SaveAccount(account) + if err != nil { + t.Fatalf("Error when saving account: %s", err) + } + + am := DefaultAccountManager{ + Store: store, + cacheMux: sync.Mutex{}, + cacheLoading: nil, + peersUpdateManager: nil, + idpManager: nil, + cacheManager: nil, + ctx: nil, + eventStore: nil, + singleAccountMode: false, + singleAccountModeDomain: "", + dnsDomain: "", + peerLoginExpiry: nil, + } + + err = am.DeletePAT("account_id", "user1", "tokenID") + if err != nil { + t.Fatalf("Error when adding PAT to user: %s", err) + } + + assert.Nil(t, store.Accounts["account_id"].Users["user1"].PATs["tokenID"]) + assert.Empty(t, store.HashedPAT2TokenID["SoMeHaShEdToKeN"]) + assert.Empty(t, store.TokenID2UserID["tokenID"]) +} From 41a47be379268705d4ba78297f9ba7a481281ea8 Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Mon, 20 Mar 2023 16:38:17 +0100 Subject: [PATCH 08/12] add function comments, implement account mock functions and added error handling in tests --- management/server/file_store.go | 2 ++ management/server/file_store_test.go | 10 ++++++++-- management/server/mock_server/account_mock.go | 18 ++++++++++++++++++ management/server/user.go | 1 + 4 files changed, 29 insertions(+), 2 deletions(-) diff --git a/management/server/file_store.go b/management/server/file_store.go index cd98ca6b8..974aa12ab 100644 --- a/management/server/file_store.go +++ b/management/server/file_store.go @@ -288,6 +288,7 @@ func (s *FileStore) SaveAccount(account *Account) error { return s.persist(s.storeFile) } +// DeleteHashedPAT2TokenIDIndex removes an entry from the indexing map HashedPAT2TokenID func (s *FileStore) DeleteHashedPAT2TokenIDIndex(hashedToken string) error { s.mux.Lock() defer s.mux.Unlock() @@ -297,6 +298,7 @@ func (s *FileStore) DeleteHashedPAT2TokenIDIndex(hashedToken string) error { return s.persist(s.storeFile) } +// DeleteTokenID2UserIDIndex removes an entry from the indexing map TokenID2UserID func (s *FileStore) DeleteTokenID2UserIDIndex(tokenID string) error { s.mux.Lock() defer s.mux.Unlock() diff --git a/management/server/file_store_test.go b/management/server/file_store_test.go index c2ccbf546..b2e9bff29 100644 --- a/management/server/file_store_test.go +++ b/management/server/file_store_test.go @@ -397,7 +397,10 @@ func TestFileStore_DeleteHashedPAT2TokenIDIndex(t *testing.T) { store := newStore(t) store.HashedPAT2TokenID["someHashedToken"] = "someTokenId" - store.DeleteHashedPAT2TokenIDIndex("someHashedToken") + err := store.DeleteHashedPAT2TokenIDIndex("someHashedToken") + if err != nil { + t.Fatal(err) + } assert.Empty(t, store.HashedPAT2TokenID["someHashedToken"]) } @@ -406,7 +409,10 @@ func TestFileStore_DeleteTokenID2UserIDIndex(t *testing.T) { store := newStore(t) store.TokenID2UserID["someTokenId"] = "someUserId" - store.DeleteTokenID2UserIDIndex("someTokenId") + err := store.DeleteTokenID2UserIDIndex("someTokenId") + if err != nil { + t.Fatal(err) + } assert.Empty(t, store.TokenID2UserID["someTokenId"]) } diff --git a/management/server/mock_server/account_mock.go b/management/server/mock_server/account_mock.go index 4f2580679..2ae71d1a9 100644 --- a/management/server/mock_server/account_mock.go +++ b/management/server/mock_server/account_mock.go @@ -60,6 +60,8 @@ type MockAccountManager struct { SaveSetupKeyFunc func(accountID string, key *server.SetupKey, userID string) (*server.SetupKey, error) ListSetupKeysFunc func(accountID, userID string) ([]*server.SetupKey, error) SaveUserFunc func(accountID, userID string, user *server.User) (*server.UserInfo, error) + AddPATToUserFunc func(accountID string, userID string, pat *server.PersonalAccessToken) error + DeletePATFunc func(accountID string, userID string, tokenID string) error GetNameServerGroupFunc func(accountID, nsGroupID string) (*nbdns.NameServerGroup, error) CreateNameServerGroupFunc func(accountID string, name, description string, nameServerList []nbdns.NameServer, groups []string, primary bool, domains []string, enabled bool, userID string) (*nbdns.NameServerGroup, error) SaveNameServerGroupFunc func(accountID, userID string, nsGroupToSave *nbdns.NameServerGroup) error @@ -184,6 +186,22 @@ func (am *MockAccountManager) GetAccountFromPAT(pat string) (*server.Account, *s return nil, nil, status.Errorf(codes.Unimplemented, "method GetAccountFromPAT is not implemented") } +// AddPATToUser mock implementation of AddPATToUser from server.AccountManager interface +func (am *MockAccountManager) AddPATToUser(accountID string, userID string, pat *server.PersonalAccessToken) error { + if am.AddPATToUserFunc != nil { + return am.AddPATToUserFunc(accountID, userID, pat) + } + return status.Errorf(codes.Unimplemented, "method AddPATToUser is not implemented") +} + +// DeletePAT mock implementation of DeletePAT from server.AccountManager interface +func (am *MockAccountManager) DeletePAT(accountID string, userID string, tokenID string) error { + if am.DeletePATFunc != nil { + return am.DeletePATFunc(accountID, userID, tokenID) + } + return status.Errorf(codes.Unimplemented, "method DeletePAT is not implemented") +} + // GetNetworkMap mock implementation of GetNetworkMap from server.AccountManager interface func (am *MockAccountManager) GetNetworkMap(peerKey string) (*server.NetworkMap, error) { if am.GetNetworkMapFunc != nil { diff --git a/management/server/user.go b/management/server/user.go index 8047c624c..36ba477e1 100644 --- a/management/server/user.go +++ b/management/server/user.go @@ -214,6 +214,7 @@ func (am *DefaultAccountManager) AddPATToUser(accountID string, userID string, p return err } +// DeletePAT deletes a specific PAT from a user func (am *DefaultAccountManager) DeletePAT(accountID string, userID string, tokenID string) error { unlock := am.Store.AcquireAccountLock(accountID) defer unlock() From 94d39ab48c464fa555ffc6899267078aa6de72e7 Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Tue, 21 Mar 2023 13:34:48 +0100 Subject: [PATCH 09/12] improve style for tests --- management/server/account_test.go | 34 ++-------- management/server/user_test.go | 100 ++++++++---------------------- 2 files changed, 31 insertions(+), 103 deletions(-) diff --git a/management/server/account_test.go b/management/server/account_test.go index d211973c2..5b4b1cc17 100644 --- a/management/server/account_test.go +++ b/management/server/account_test.go @@ -462,30 +462,15 @@ func TestDefaultAccountManager_GetAccountFromToken(t *testing.T) { func TestAccountManager_GetAccountFromPAT(t *testing.T) { store := newStore(t) account := newAccountWithId("account_id", "testuser", "") - account.Peers["testpeer"] = &Peer{ - Key: "peerkey", - SetupKey: "peerkeysetupkey", - IP: net.IP{127, 0, 0, 1}, - Meta: PeerSystemMeta{}, - Name: "peer name", - Status: &PeerStatus{Connected: true, LastSeen: time.Now()}, - } token := "nbp_9999EUDNdkeusjentDLSJEn1902u84390W6W" hashedToken := sha256.Sum256([]byte(token)) account.Users["someUser"] = &User{ - Id: "someUser", - Role: "", - AutoGroups: nil, + Id: "someUser", PATs: map[string]*PersonalAccessToken{ "pat1": { - ID: "tokenId", - Description: "some Description", - HashedToken: string(hashedToken[:]), - ExpirationDate: time.Time{}, - CreatedBy: "testuser", - CreatedAt: time.Time{}, - LastUsed: time.Time{}, + ID: "tokenId", + HashedToken: string(hashedToken[:]), }, }, } @@ -495,18 +480,7 @@ func TestAccountManager_GetAccountFromPAT(t *testing.T) { } am := DefaultAccountManager{ - Store: store, - cacheMux: sync.Mutex{}, - cacheLoading: nil, - peersUpdateManager: nil, - idpManager: nil, - cacheManager: nil, - ctx: nil, - eventStore: nil, - singleAccountMode: false, - singleAccountModeDomain: "", - dnsDomain: "", - peerLoginExpiry: nil, + Store: store, } account, user, err := am.GetAccountFromPAT(token) diff --git a/management/server/user_test.go b/management/server/user_test.go index b3cd1db9a..20f2ca4f1 100644 --- a/management/server/user_test.go +++ b/management/server/user_test.go @@ -1,101 +1,66 @@ package server import ( - "net" - "sync" "testing" - "time" "github.com/stretchr/testify/assert" ) +const ( + mockAccountID = "accountID" + mockUserID = "userID" + mockTokenID = "tokenID" + mockToken = "SoMeHaShEdToKeN" +) + func TestUser_AddPATToUser(t *testing.T) { store := newStore(t) - account := newAccountWithId("account_id", "testuser", "") - account.Peers["testpeer"] = &Peer{ - Key: "peerkey", - SetupKey: "peerkeysetupkey", - IP: net.IP{127, 0, 0, 1}, - Meta: PeerSystemMeta{}, - Name: "peer name", - Status: &PeerStatus{Connected: true, LastSeen: time.Now()}, - } + account := newAccountWithId(mockAccountID, mockUserID, "") + err := store.SaveAccount(account) if err != nil { t.Fatalf("Error when saving account: %s", err) } am := DefaultAccountManager{ - Store: store, - cacheMux: sync.Mutex{}, - cacheLoading: nil, - peersUpdateManager: nil, - idpManager: nil, - cacheManager: nil, - ctx: nil, - eventStore: nil, - singleAccountMode: false, - singleAccountModeDomain: "", - dnsDomain: "", - peerLoginExpiry: nil, + Store: store, } - token := "someToken" pat := PersonalAccessToken{ - ID: "tokenID", - Description: "some Description", - HashedToken: token, - ExpirationDate: time.Time{}, - CreatedBy: "testuser", - CreatedAt: time.Time{}, - LastUsed: time.Time{}, + ID: mockTokenID, + HashedToken: mockToken, } - err = am.AddPATToUser("account_id", "testuser", &pat) + err = am.AddPATToUser(mockAccountID, mockUserID, &pat) if err != nil { t.Fatalf("Error when adding PAT to user: %s", err) } fileStore := am.Store.(*FileStore) - tokenID := fileStore.HashedPAT2TokenID[token[:]] + tokenID := fileStore.HashedPAT2TokenID[mockToken[:]] if tokenID == "" { t.Fatal("GetTokenIDByHashedToken failed after adding PAT") } - assert.Equal(t, "tokenID", tokenID) + assert.Equal(t, mockTokenID, tokenID) userID := fileStore.TokenID2UserID[tokenID] if userID == "" { t.Fatal("GetUserByTokenId failed after adding PAT") } - assert.Equal(t, "testuser", userID) + assert.Equal(t, mockUserID, userID) } func TestUser_DeletePAT(t *testing.T) { store := newStore(t) - account := newAccountWithId("account_id", "testuser", "") - account.Peers["testpeer"] = &Peer{ - Key: "peerkey", - SetupKey: "peerkeysetupkey", - IP: net.IP{127, 0, 0, 1}, - Meta: PeerSystemMeta{}, - Name: "peer name", - Status: &PeerStatus{Connected: true, LastSeen: time.Now()}, - } - account.Users["user1"] = &User{ - Id: "user1", - Role: "admin", - AutoGroups: nil, + account := newAccountWithId(mockAccountID, mockUserID, "") + account.Users[mockUserID] = &User{ + Id: mockUserID, PATs: map[string]*PersonalAccessToken{ - "tokenID": { - ID: "tokenID", - Description: "some Description", - HashedToken: "SoMeHaShEdToKeN", - ExpirationDate: time.Now().AddDate(0, 0, 7), - CreatedBy: "user1", - CreatedAt: time.Now(), - LastUsed: time.Now(), + mockTokenID: { + ID: mockTokenID, + HashedToken: mockToken, }, }, } @@ -105,26 +70,15 @@ func TestUser_DeletePAT(t *testing.T) { } am := DefaultAccountManager{ - Store: store, - cacheMux: sync.Mutex{}, - cacheLoading: nil, - peersUpdateManager: nil, - idpManager: nil, - cacheManager: nil, - ctx: nil, - eventStore: nil, - singleAccountMode: false, - singleAccountModeDomain: "", - dnsDomain: "", - peerLoginExpiry: nil, + Store: store, } - err = am.DeletePAT("account_id", "user1", "tokenID") + err = am.DeletePAT(mockAccountID, mockUserID, mockTokenID) if err != nil { t.Fatalf("Error when adding PAT to user: %s", err) } - assert.Nil(t, store.Accounts["account_id"].Users["user1"].PATs["tokenID"]) - assert.Empty(t, store.HashedPAT2TokenID["SoMeHaShEdToKeN"]) - assert.Empty(t, store.TokenID2UserID["tokenID"]) + assert.Nil(t, store.Accounts[mockAccountID].Users[mockUserID].PATs[mockTokenID]) + assert.Empty(t, store.HashedPAT2TokenID[mockToken]) + assert.Empty(t, store.TokenID2UserID[mockTokenID]) } From 311b67fe5a1c2f300f5522ddfac04ea838c5cefb Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Tue, 21 Mar 2023 13:56:31 +0100 Subject: [PATCH 10/12] change error messages --- management/server/account.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/management/server/account.go b/management/server/account.go index e998b8d5c..1d4c10721 100644 --- a/management/server/account.go +++ b/management/server/account.go @@ -1122,40 +1122,40 @@ func (am *DefaultAccountManager) redeemInvite(account *Account, userID string) e // GetAccountFromPAT returns Account and User associated with a personal access token func (am *DefaultAccountManager) GetAccountFromPAT(token string) (*Account, *User, error) { if len(token) != PATLength { - return nil, nil, fmt.Errorf("token invalid") + return nil, nil, fmt.Errorf("token has wrong length") } prefix := token[:len(PATPrefix)] if prefix != PATPrefix { - return nil, nil, fmt.Errorf("token invalid") + return nil, nil, fmt.Errorf("token has wrong prefix") } secret := token[len(PATPrefix) : len(PATPrefix)+PATSecretLength] encodedChecksum := token[len(PATPrefix)+PATSecretLength : len(PATPrefix)+PATSecretLength+PATChecksumLength] verificationChecksum, err := base62.Decode(encodedChecksum) if err != nil { - return nil, nil, fmt.Errorf("token invalid") + return nil, nil, fmt.Errorf("token checksum decoding failed: %w", err) } secretChecksum := crc32.ChecksumIEEE([]byte(secret)) if secretChecksum != verificationChecksum { - return nil, nil, fmt.Errorf("token invalid") + return nil, nil, fmt.Errorf("token checksum does not match") } hashedToken := sha256.Sum256([]byte(token)) tokenID, err := am.Store.GetTokenIDByHashedToken(string(hashedToken[:])) if err != nil { - return nil, nil, fmt.Errorf("token invalid") + return nil, nil, err } user, err := am.Store.GetUserByTokenID(tokenID) if err != nil { - return nil, nil, fmt.Errorf("token invalid") + return nil, nil, err } account, err := am.Store.GetAccountByUser(user.Id) if err != nil { - return nil, nil, fmt.Errorf("token invalid") + return nil, nil, err } return account, user, nil } From 82af60838ed5e727bafdd20c2e679df1d6310b3b Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Tue, 21 Mar 2023 14:00:59 +0100 Subject: [PATCH 11/12] use "ok" convention for check variables throughout files_store --- management/server/file_store.go | 36 ++++++++++++++++----------------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/management/server/file_store.go b/management/server/file_store.go index 974aa12ab..4f8092cfb 100644 --- a/management/server/file_store.go +++ b/management/server/file_store.go @@ -313,8 +313,8 @@ func (s *FileStore) GetAccountByPrivateDomain(domain string) (*Account, error) { s.mux.Lock() defer s.mux.Unlock() - accountID, accountIDFound := s.PrivateDomain2AccountID[strings.ToLower(domain)] - if !accountIDFound { + accountID, ok := s.PrivateDomain2AccountID[strings.ToLower(domain)] + if !ok { return nil, status.Errorf(status.NotFound, "account not found: provided domain is not registered or is not private") } @@ -331,8 +331,8 @@ func (s *FileStore) GetAccountBySetupKey(setupKey string) (*Account, error) { s.mux.Lock() defer s.mux.Unlock() - accountID, accountIDFound := s.SetupKeyID2AccountID[strings.ToUpper(setupKey)] - if !accountIDFound { + accountID, ok := s.SetupKeyID2AccountID[strings.ToUpper(setupKey)] + if !ok { return nil, status.Errorf(status.NotFound, "account not found: provided setup key doesn't exists") } @@ -349,8 +349,8 @@ func (s *FileStore) GetTokenIDByHashedToken(token string) (string, error) { s.mux.Lock() defer s.mux.Unlock() - tokenID, tokenIDFound := s.HashedPAT2TokenID[token] - if !tokenIDFound { + tokenID, ok := s.HashedPAT2TokenID[token] + if !ok { return "", status.Errorf(status.NotFound, "tokenID not found: provided token doesn't exists") } @@ -362,13 +362,13 @@ func (s *FileStore) GetUserByTokenID(tokenID string) (*User, error) { s.mux.Lock() defer s.mux.Unlock() - userID, userIDFound := s.TokenID2UserID[tokenID] - if !userIDFound { + userID, ok := s.TokenID2UserID[tokenID] + if !ok { return nil, status.Errorf(status.NotFound, "user not found: provided tokenID doesn't exists") } - accountID, accountIDFound := s.UserID2AccountID[userID] - if !accountIDFound { + accountID, ok := s.UserID2AccountID[userID] + if !ok { return nil, status.Errorf(status.NotFound, "accountID not found: provided userID doesn't exists") } @@ -393,8 +393,8 @@ func (s *FileStore) GetAllAccounts() (all []*Account) { // getAccount returns a reference to the Account. Should not return a copy. func (s *FileStore) getAccount(accountID string) (*Account, error) { - account, accountFound := s.Accounts[accountID] - if !accountFound { + account, ok := s.Accounts[accountID] + if !ok { return nil, status.Errorf(status.NotFound, "account not found") } @@ -419,8 +419,8 @@ func (s *FileStore) GetAccountByUser(userID string) (*Account, error) { s.mux.Lock() defer s.mux.Unlock() - accountID, accountIDFound := s.UserID2AccountID[userID] - if !accountIDFound { + accountID, ok := s.UserID2AccountID[userID] + if !ok { return nil, status.Errorf(status.NotFound, "account not found") } @@ -437,8 +437,8 @@ func (s *FileStore) GetAccountByPeerID(peerID string) (*Account, error) { s.mux.Lock() defer s.mux.Unlock() - accountID, accountIDFound := s.PeerID2AccountID[peerID] - if !accountIDFound { + accountID, ok := s.PeerID2AccountID[peerID] + if !ok { return nil, status.Errorf(status.NotFound, "provided peer ID doesn't exists %s", peerID) } @@ -463,8 +463,8 @@ func (s *FileStore) GetAccountByPeerPubKey(peerKey string) (*Account, error) { s.mux.Lock() defer s.mux.Unlock() - accountID, accountIDFound := s.PeerKeyID2AccountID[peerKey] - if !accountIDFound { + accountID, ok := s.PeerKeyID2AccountID[peerKey] + if !ok { return nil, status.Errorf(status.NotFound, "provided peer key doesn't exists %s", peerKey) } From 8e4710763e5fdacd01b66133dd78cf61e8dcb8b7 Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Tue, 21 Mar 2023 14:02:34 +0100 Subject: [PATCH 12/12] use single line return for SaveAccount --- management/server/user.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/management/server/user.go b/management/server/user.go index 36ba477e1..c3011c317 100644 --- a/management/server/user.go +++ b/management/server/user.go @@ -210,8 +210,7 @@ func (am *DefaultAccountManager) AddPATToUser(accountID string, userID string, p user.PATs[pat.ID] = pat - err = am.Store.SaveAccount(account) - return err + return am.Store.SaveAccount(account) } // DeletePAT deletes a specific PAT from a user @@ -244,8 +243,7 @@ func (am *DefaultAccountManager) DeletePAT(accountID string, userID string, toke } delete(user.PATs, tokenID) - err = am.Store.SaveAccount(account) - return err + return am.Store.SaveAccount(account) } // SaveUser saves updates a given user. If the user doesn't exit it will throw status.NotFound error.