[FIX] update when adding operator key, updated before adding the signing key to the jwt

aricart · aricart · commit b39b6f4be5c7 · 2023-11-14T12:25:40.000-06:00
[FIX] changed save logic depended which depended on `Loaded` field which only has second resolution to be based out of `Modified`
[FIX] nscprovider expected an account key to have an operator prefix instead of an account prefix
[FIX] nscprovider saving of users depended on the account being modified
diff --git a/accounts.go b/accounts.go
@@ -25,6 +25,7 @@ func (a *AccountData) issue(key *Key) error {
 	}
 	a.Claim = claim
 	a.Token = token
+	a.Modified = true
 	return nil
 }
 
diff --git a/operator.go b/operator.go
@@ -148,6 +148,7 @@ func (o *OperatorData) update() error {
 	}
 	o.Claim = claims
 	o.Token = token
+	o.Modified = true
 
 	return nil
 }
diff --git a/operator_signingkeys.go b/operator_signingkeys.go
@@ -19,13 +19,13 @@ func (os *operatorSigningKeys) add() (*Key, error) {
 	if err != nil {
 		return nil, err
 	}
+	os.data.Claim.SigningKeys.Add(key.Public)
 	err = os.data.update()
 	if err != nil {
 		return nil, err
 	}
 	os.data.AddedKeys = append(os.data.AddedKeys, key)
 	os.data.OperatorSigningKeys = append(os.data.OperatorSigningKeys, key)
-	os.data.Claim.SigningKeys = append(os.data.Claim.SigningKeys, key.Public)
 	return key, nil
 }
 
diff --git a/providers/kv/kv.go b/providers/kv/kv.go
@@ -225,6 +225,7 @@ func (p *KvProvider) LoadOperators() ([]*ab.OperatorData, error) {
 			return nil, err
 		}
 		o.Claim = oc
+		o.Modified = false
 		o.Loaded = o.Claim.IssuedAt
 		o.EntityName = o.Claim.Name
 		o.Key, err = p.GetKey(o.Claim.Subject)
@@ -260,6 +261,7 @@ func (p *KvProvider) LoadAccounts(od *ab.OperatorData) error {
 		if err != nil {
 			return err
 		}
+		a.Modified = false
 		a.Claim = ac
 		a.Loaded = a.Claim.IssuedAt
 		a.EntityName = a.Claim.Name
@@ -297,6 +299,7 @@ func (p *KvProvider) LoadUsers(ad *ab.AccountData) error {
 			return err
 		}
 		u.Claim = uc
+		u.Modified = false
 		u.Loaded = u.Claim.IssuedAt
 		u.EntityName = u.Claim.Name
 		u.Key, err = p.GetKey(u.Claim.Subject)
@@ -400,7 +403,7 @@ func (p *KvProvider) Store(operators []*ab.OperatorData) error {
 }
 
 func (p *KvProvider) StoreOperator(o *ab.OperatorData) error {
-	if o.Loaded > 0 && o.Loaded > o.Claim.IssuedAt {
+	if !o.Modified {
 		return nil
 	}
 	_, err := p.Kv.Put(context.Background(), fmt.Sprintf("%s.%s", OperatorPrefix, o.Subject()), []byte(o.Token))
@@ -416,11 +419,12 @@ func (p *KvProvider) StoreOperator(o *ab.OperatorData) error {
 		}
 	}
 	o.Loaded = o.Claim.IssuedAt
+	o.Modified = false
 	return nil
 }
 
 func (p *KvProvider) StoreAccount(a *ab.AccountData) error {
-	if a.Loaded > 0 && a.Loaded > a.Claim.IssuedAt {
+	if !a.Modified {
 		return nil
 	}
 	_, err := p.Kv.Put(context.Background(),
@@ -438,11 +442,12 @@ func (p *KvProvider) StoreAccount(a *ab.AccountData) error {
 		}
 	}
 	a.Loaded = a.Claim.IssuedAt
+	a.Modified = false
 	return nil
 }
 
 func (p *KvProvider) StoreUser(u *ab.UserData) error {
-	if u.Loaded > 0 && u.Loaded > u.Claim.IssuedAt {
+	if !u.Modified {
 		return nil
 	}
 	_, err := p.Kv.Put(context.Background(),
@@ -452,6 +457,7 @@ func (p *KvProvider) StoreUser(u *ab.UserData) error {
 		return err
 	}
 	u.Loaded = u.Claim.IssuedAt
+	u.Modified = false
 	return nil
 }
 
diff --git a/providers/nsc/nsc.go b/providers/nsc/nsc.go
@@ -149,7 +149,7 @@ func (a *NscProvider) loadAccount(si store.IStore, ks store.KeyStore, name strin
 	for _, k := range keys {
 		skp, _ := ks.GetKeyPair(k)
 		if skp != nil {
-			sk, _ := authb.KeyFromNkey(skp, nkeys.PrefixByteOperator)
+			sk, _ := authb.KeyFromNkey(skp, nkeys.PrefixByteAccount)
 			if sk != nil {
 				ad.AccountSigningKeys = append(ad.AccountSigningKeys, sk)
 			}
@@ -229,7 +229,7 @@ func (a *NscProvider) Store(operators []*authb.OperatorData) error {
 			return err
 		}
 		// if the operator changed configuration save it
-		if o.Claim.IssuedAt > o.Loaded {
+		if o.Modified {
 			if err := s.StoreRaw([]byte(o.Token)); err != nil {
 				return err
 			}
@@ -252,22 +252,24 @@ func (a *NscProvider) Store(operators []*authb.OperatorData) error {
 		o.DeletedKeys = nil
 
 		for _, account := range o.AccountDatas {
-			if account.Claim.IssuedAt > account.Loaded {
+			if account.Modified {
+				//if account.Claim.IssuedAt > account.Loaded || account.Modified {
 				if err := s.StoreRaw([]byte(account.Token)); err != nil {
 					return err
 				}
 				// check that signing keys were not modified
 				account.Loaded = account.Claim.IssuedAt
+			}
 
-				for _, u := range account.UserDatas {
-					if u.Claim.IssuedAt > u.Loaded {
-						if err := s.StoreRaw([]byte(u.Token)); err != nil {
-							return err
-						}
-						u.Loaded = u.Claim.IssuedAt
+			for _, u := range account.UserDatas {
+				if u.Modified {
+					if err := s.StoreRaw([]byte(u.Token)); err != nil {
+						return err
 					}
+					u.Loaded = u.Claim.IssuedAt
 				}
 			}
+
 			for _, u := range account.DeletedUsers {
 				if err := s.Delete(store.Accounts, account.EntityName, store.Users, store.JwtName(u.EntityName)); err != nil {
 					return err
diff --git a/tests/accounts_test.go b/tests/accounts_test.go
@@ -551,3 +551,44 @@ func (suite *ProviderSuite) Test_AccountJetStreamLimits() {
 	require.NoError(t, err)
 	suite.testTier(auth, b, 1)
 }
+
+func (suite *ProviderSuite) Test_AccountSkUpdate() {
+	t := suite.T()
+	auth, err := authb.NewAuth(suite.Provider)
+	require.NoError(t, err)
+
+	operators := auth.Operators()
+	require.Empty(t, operators.List())
+
+	o, err := operators.Add("O")
+	require.NoError(t, err)
+	require.NotNil(t, o)
+
+	a, err := o.Accounts().Add("A")
+	require.NoError(t, err)
+	require.NotNil(t, a)
+
+	require.NoError(t, auth.Commit())
+	require.NoError(t, auth.Reload())
+
+	o = operators.Get("O")
+	require.NotNil(t, o)
+
+	a = o.Accounts().Get("A")
+	require.NotNil(t, a)
+
+	k, err := a.ScopedSigningKeys().Add()
+	require.NoError(t, err)
+	require.NotEmpty(t, k)
+
+	require.NoError(t, auth.Commit())
+	require.NoError(t, auth.Reload())
+
+	o = operators.Get("O")
+	require.NotNil(t, o)
+	a = o.Accounts().Get("A")
+	require.NotNil(t, a)
+	scope, ok := a.ScopedSigningKeys().GetScope(k)
+	require.Nil(t, scope)
+	require.True(t, ok)
+}
diff --git a/tests/operator_test.go b/tests/operator_test.go
@@ -39,6 +39,41 @@ func (suite *ProviderSuite) Test_OperatorBasics() {
 	require.Equal(t, oc.Subject, key.Public)
 }
 
+func (suite *ProviderSuite) Test_SkUpdate() {
+	t := suite.T()
+	auth, err := authb.NewAuth(suite.Provider)
+	require.NoError(t, err)
+
+	operators := auth.Operators()
+	require.Empty(t, operators.List())
+
+	o := auth.Operators().Get("O")
+	require.NoError(t, err)
+	require.Nil(t, o)
+	o, err = operators.Add("O")
+	require.NoError(t, err)
+	require.NotNil(t, o)
+
+	require.NoError(t, auth.Commit())
+	require.NoError(t, auth.Reload())
+
+	o = operators.Get("O")
+	require.NotNil(t, o)
+
+	k, err := o.SigningKeys().Add()
+	require.NoError(t, err)
+	require.NotEmpty(t, k)
+
+	require.NoError(t, auth.Commit())
+	require.NoError(t, auth.Reload())
+
+	o = operators.Get("O")
+	require.NotNil(t, o)
+	keys := o.SigningKeys().List()
+	require.Len(t, keys, 1)
+	require.Contains(t, keys, k)
+}
+
 func (suite *ProviderSuite) Test_OperatorValidation() {
 	t := suite.T()
 	auth, err := authb.NewAuth(suite.Provider)
diff --git a/tests/users_test.go b/tests/users_test.go
@@ -337,3 +337,35 @@ func (suite *ProviderSuite) Test_Creds() {
 	ud := u.(*authb.UserData)
 	require.Equal(t, int64(0), ud.Claim.Expires)
 }
+
+func (suite *ProviderSuite) Test_UsersAddedSave() {
+	t := suite.T()
+	auth, err := authb.NewAuth(suite.Provider)
+	require.NoError(t, err)
+	o, err := auth.Operators().Add("O")
+	require.NoError(t, err)
+	a, err := o.Accounts().Add("A")
+	require.NoError(t, err)
+
+	require.NoError(t, auth.Commit())
+	require.NoError(t, auth.Reload())
+
+	o = auth.Operators().Get("O")
+	require.NotNil(t, o)
+	a = o.Accounts().Get("A")
+	require.NotNil(t, a)
+
+	u, err := a.Users().Add("U", "")
+	require.NoError(t, err)
+	require.NotNil(t, u)
+
+	require.NoError(t, auth.Commit())
+	require.NoError(t, auth.Reload())
+
+	o = auth.Operators().Get("O")
+	require.NotNil(t, o)
+	a = o.Accounts().Get("A")
+	require.NotNil(t, a)
+	u = a.Users().Get("U")
+	require.NotNil(t, u)
+}
diff --git a/types.go b/types.go
@@ -30,9 +30,11 @@ type BaseData struct {
 	// Loaded matches the issue time of a loaded JWT (UTC in seconds). When
 	// the entity is new, it should be 0. The AuthProvider
 	// stores claims that have been modified and have
-	// an issue time greater than this value. On Store(),
+	// an issue time greater than this value or have been Modified. On Store(),
 	// it should be set to the tokens issue time.
 	Loaded int64
+	// Modified is true if the entity has been modified since it was loaded
+	Modified bool
 	// EntityName is the name for the entity - in some cases NSC
 	// will display simple name which differs from the actual name
 	// of the entity stored in the JWT.
diff --git a/user.go b/user.go
@@ -48,6 +48,8 @@ func (u *UserData) update() error {
 	}
 	u.Claim = claim
 	u.Token = token
+	u.Loaded = claim.IssuedAt
+	u.Modified = true
 	return nil
 }
 
diff --git a/users.go b/users.go
@@ -23,7 +23,7 @@ func (a *UsersImpl) Add(name string, key string) (User, error) {
 		return nil, err
 	}
 	d := &UserData{
-		BaseData:    BaseData{EntityName: name, Key: uk},
+		BaseData:    BaseData{EntityName: name, Key: uk, Modified: true},
 		AccountData: a.accountData,
 		Claim:       jwt.NewUserClaims(uk.Public),
 		RejectEdits: scoped,

Original file line number	Diff line number	Diff line change
`@@ -25,6 +25,7 @@ func (a AccountData) issue(key Key) error {`
`25`	`25`	`}`
`26`	`26`	`a.Claim = claim`
`27`	`27`	`a.Token = token`
	`28`	`+ a.Modified = true`
`28`	`29`	`return nil`
`29`	`30`	`}`
`30`	`31`
Original file line number	Diff line number	Diff line change
`@@ -148,6 +148,7 @@ func (o *OperatorData) update() error {`
`148`	`148`	`}`
`149`	`149`	`o.Claim = claims`
`150`	`150`	`o.Token = token`
	`151`	`+ o.Modified = true`
`151`	`152`
`152`	`153`	`return nil`
`153`	`154`	`}`
Original file line number	Diff line number	Diff line change
`@@ -19,13 +19,13 @@ func (os operatorSigningKeys) add() (Key, error) {`
`19`	`19`	`if err != nil {`
`20`	`20`	`return nil, err`
`21`	`21`	`}`
	`22`	`+ os.data.Claim.SigningKeys.Add(key.Public)`
`22`	`23`	`err = os.data.update()`
`23`	`24`	`if err != nil {`
`24`	`25`	`return nil, err`
`25`	`26`	`}`
`26`	`27`	`os.data.AddedKeys = append(os.data.AddedKeys, key)`
`27`	`28`	`os.data.OperatorSigningKeys = append(os.data.OperatorSigningKeys, key)`
`28`		`- os.data.Claim.SigningKeys = append(os.data.Claim.SigningKeys, key.Public)`
`29`	`29`	`return key, nil`
`30`	`30`	`}`
`31`	`31`
Original file line number	Diff line number	Diff line change
`@@ -225,6 +225,7 @@ func (p KvProvider) LoadOperators() ([]ab.OperatorData, error) {`
`225`	`225`	`return nil, err`
`226`	`226`	`}`
`227`	`227`	`o.Claim = oc`
	`228`	`+ o.Modified = false`
`228`	`229`	`o.Loaded = o.Claim.IssuedAt`
`229`	`230`	`o.EntityName = o.Claim.Name`
`230`	`231`	`o.Key, err = p.GetKey(o.Claim.Subject)`
`@@ -260,6 +261,7 @@ func (p KvProvider) LoadAccounts(od ab.OperatorData) error {`
`260`	`261`	`if err != nil {`
`261`	`262`	`return err`
`262`	`263`	`}`
	`264`	`+ a.Modified = false`
`263`	`265`	`a.Claim = ac`
`264`	`266`	`a.Loaded = a.Claim.IssuedAt`
`265`	`267`	`a.EntityName = a.Claim.Name`
`@@ -297,6 +299,7 @@ func (p KvProvider) LoadUsers(ad ab.AccountData) error {`
`297`	`299`	`return err`
`298`	`300`	`}`
`299`	`301`	`u.Claim = uc`
	`302`	`+ u.Modified = false`
`300`	`303`	`u.Loaded = u.Claim.IssuedAt`
`301`	`304`	`u.EntityName = u.Claim.Name`
`302`	`305`	`u.Key, err = p.GetKey(u.Claim.Subject)`
`@@ -400,7 +403,7 @@ func (p KvProvider) Store(operators []ab.OperatorData) error {`
`400`	`403`	`}`
`401`	`404`
`402`	`405`	`func (p KvProvider) StoreOperator(o ab.OperatorData) error {`
`403`		`- if o.Loaded > 0 && o.Loaded > o.Claim.IssuedAt {`
	`406`	`+ if !o.Modified {`
`404`	`407`	`return nil`
`405`	`408`	`}`
`406`	`409`	`_, err := p.Kv.Put(context.Background(), fmt.Sprintf("%s.%s", OperatorPrefix, o.Subject()), []byte(o.Token))`
`@@ -416,11 +419,12 @@ func (p KvProvider) StoreOperator(o ab.OperatorData) error {`
`416`	`419`	`}`
`417`	`420`	`}`
`418`	`421`	`o.Loaded = o.Claim.IssuedAt`
	`422`	`+ o.Modified = false`
`419`	`423`	`return nil`
`420`	`424`	`}`
`421`	`425`
`422`	`426`	`func (p KvProvider) StoreAccount(a ab.AccountData) error {`
`423`		`- if a.Loaded > 0 && a.Loaded > a.Claim.IssuedAt {`
	`427`	`+ if !a.Modified {`
`424`	`428`	`return nil`
`425`	`429`	`}`
`426`	`430`	`_, err := p.Kv.Put(context.Background(),`
`@@ -438,11 +442,12 @@ func (p KvProvider) StoreAccount(a ab.AccountData) error {`
`438`	`442`	`}`
`439`	`443`	`}`
`440`	`444`	`a.Loaded = a.Claim.IssuedAt`
	`445`	`+ a.Modified = false`
`441`	`446`	`return nil`
`442`	`447`	`}`
`443`	`448`
`444`	`449`	`func (p KvProvider) StoreUser(u ab.UserData) error {`
`445`		`- if u.Loaded > 0 && u.Loaded > u.Claim.IssuedAt {`
	`450`	`+ if !u.Modified {`
`446`	`451`	`return nil`
`447`	`452`	`}`
`448`	`453`	`_, err := p.Kv.Put(context.Background(),`
`@@ -452,6 +457,7 @@ func (p KvProvider) StoreUser(u ab.UserData) error {`
`452`	`457`	`return err`
`453`	`458`	`}`
`454`	`459`	`u.Loaded = u.Claim.IssuedAt`
	`460`	`+ u.Modified = false`
`455`	`461`	`return nil`
`456`	`462`	`}`
`457`	`463`
Original file line number	Diff line number	Diff line change
`@@ -48,6 +48,8 @@ func (u *UserData) update() error {`
`48`	`48`	`}`
`49`	`49`	`u.Claim = claim`
`50`	`50`	`u.Token = token`
	`51`	`+ u.Loaded = claim.IssuedAt`
	`52`	`+ u.Modified = true`
`51`	`53`	`return nil`
`52`	`54`	`}`
`53`	`55`