NEX-873 Admin Identity Certificates #8
Conversation
| @@ -115,7 +116,22 @@ func Run(approver oracle.ApproverInterface) { | |||
| } | |||
|
|
|||
| spyNode := spynode.NewNode(spyConfig, spyStorage) | |||
| spyNode.AddTxFilter(&TxFilter{}) | |||
|
|
|||
| // ------------------------------------------------------------------------- | |||
There was a problem hiding this comment.
Let's lose these / -----------------------.... lines
If these setup is that big, use funcs rather than comments to clarify the setup.
There was a problem hiding this comment.
That will make the code more complex and less readable. The functions would each require a fair number of parameters and would greatly increase the line count and code complexity. I don't see any benefit.
| @@ -90,11 +99,18 @@ func (v *Verify) XPubSignature(ctx context.Context, log logger.Logger, w http.Re | |||
| return translate(errors.Wrap(err, "unmarshal request")) | |||
| } | |||
|
|
|||
| for _, xpub := range requestData.XPubs { | |||
| if xpub.IsPrivate() { | |||
| web.Respond(ctx, log, w, "private keys not allowed", http.StatusUnprocessableEntity) | |||
There was a problem hiding this comment.
This web.Respond doesn't return an error. It creates a response no matter what.
| dbConn := v.MasterDB.Copy() | ||
| defer dbConn.Close() | ||
|
|
||
| // Verify that the public key is associated with the entity. | ||
| sigHash, height, approved, err := oracle.VerifyXPub(ctx, dbConn, v.BlockHandler, | ||
| sigHash, height, approved, description, err := oracle.VerifyXPub(ctx, dbConn, v.BlockHandler, |
There was a problem hiding this comment.
That's a lot of return elements.
How about a struct and an error ?
There was a problem hiding this comment.
It just kind of evolved like that. I will fix it.
| Expiration: expiration, | ||
| } | ||
|
|
||
| web.RespondData(ctx, log, w, response, http.StatusOK) |
| } | ||
|
|
||
| if err := web.Unmarshal(r.Body, &requestData); err != nil { | ||
| return translate(errors.Wrap(err, "unmarshal request")) | ||
| } | ||
|
|
||
| for _, xpub := range requestData.XPubs { | ||
| if xpub.IsPrivate() { | ||
| web.Respond(ctx, log, w, "private keys not allowed", http.StatusUnprocessableEntity) |
| } | ||
|
|
||
| if err := web.Unmarshal(r.Body, &requestData); err != nil { | ||
| return translate(errors.Wrap(err, "unmarshal request")) | ||
| } | ||
|
|
||
| for _, xpub := range requestData.XPubs { | ||
| if xpub.IsPrivate() { | ||
| web.Respond(ctx, log, w, "private keys not allowed", http.StatusUnprocessableEntity) |
| @@ -67,7 +74,7 @@ func (t *Transfers) TransferSignature(ctx context.Context, log logger.Logger, w | |||
| } | |||
| } | |||
|
|
|||
| expiration := uint64(time.Now().Add(time.Duration(t.ExpirationDurationSeconds) * time.Second).UnixNano()) | |||
| expiration := uint64(time.Now().Add(time.Duration(t.TransferExpirationDurationSeconds) * time.Second).UnixNano()) | |||
| @@ -119,8 +123,25 @@ github.com/tokenized/pkg v0.0.0-20200528005955-162f9c3d87f8/go.mod h1:qVlOZ6a37X | |||
| github.com/tokenized/pkg v0.0.0-20200724032845-ccddf7770c5a/go.mod h1:qVlOZ6a37XqvfYQFoWhxBUGvGgzY0EkvoCHDBs+cjSA= | |||
| github.com/tokenized/pkg v0.0.0-20200730051124-244d94211b53 h1:XTLCUNW8dRdfQrwBAq0bHsOlDOb3JJINuXtmJ5CwxgY= | |||
| github.com/tokenized/pkg v0.0.0-20200730051124-244d94211b53/go.mod h1:StHV+mlXyhmmYZSRghRb+3Zy7QSiJe0jDaQw05SYnyQ= | |||
| github.com/tokenized/pkg v0.0.0-20200814000235-256842f23ba4/go.mod h1:StHV+mlXyhmmYZSRghRb+3Zy7QSiJe0jDaQw05SYnyQ= | |||
internal/oracle/contracts.go
Outdated
| // Check for pre-existing | ||
| b, err := cm.st.Read(ctx, key) | ||
| if err == nil { | ||
| // Check timestamp vs current version to ensure we keep the latest. |
There was a problem hiding this comment.
I think we just need to be careful to not prioritize statement depth of logic complexity. It is difficult for me because I have always thought that positive if statements are simpler, but golang seems to want a lot more negative if statements (if not error). I subconsciously default to the if "condition I want" statement, which I think has merit. I have changed this code though because it is a 3 way if statement that is difficult to read in any form. Just something to think about.
| if err != nil { | ||
| t.Errorf("Entity didn't verify : %s", err) | ||
| } | ||
| } else { |
There was a problem hiding this comment.
No need for else.
Just t.Fatal above to stop this test.
There was a problem hiding this comment.
This else is needed to check for the cases that are supposed to cause an error. I will add log statements to make that more clear.
internal/oracle/identity.go
Outdated
| @@ -13,75 +13,163 @@ import ( | |||
| ) | |||
|
|
|||
| func VerifyPubKey(ctx context.Context, dbConn *db.DB, blockHandler *BlockHandler, | |||
| entity *actions.EntityField, xpub bitcoin.ExtendedKey, index uint32) ([]byte, uint32, bool, error) { | |||
| entity *actions.EntityField, xpub bitcoin.ExtendedKey, index uint32) ([]byte, uint32, bool, string, error) { | |||
There was a problem hiding this comment.
This deserves a struct, rather than so many elements in the return.
| // uint32 - block height of block hash included in signature hash | ||
| // bitcoin.Hash32 - block hash included in signature hash | ||
| // bool - true if approved | ||
| func CreateAdminCertificate(ctx context.Context, dbConn *db.DB, isTest bool, |
There was a problem hiding this comment.
Number of return elements. Use struct.
internal/oracle/identity.go
Outdated
| return nil, 0, false, "", errors.Wrap(err, "get contract formation") | ||
| } | ||
|
|
||
| if err := VerifyEntityIsSubset(cf.Issuer, userEntity); err != nil { |
There was a problem hiding this comment.
These blocks can be improved.
The previous block, and the last half of this one are identical.
| @@ -70,6 +78,7 @@ func unmarshalNested(data []byte, cfg *Config) error { | |||
| err = json.Unmarshal(data, &cfg.Storage) | |||
| err = json.Unmarshal(data, &cfg.SpyNode) | |||
| err = json.Unmarshal(data, &cfg.NodeStorage) | |||
| err = json.Unmarshal(data, &cfg.RpcNode) | |||
There was a problem hiding this comment.
This is a copy of the "flat json" idea which meant this had to be changed anytime the config changed.
Just use "normal" json. Take a look at nexus-api
There was a problem hiding this comment.
We have a separate ticket to update the configs to use a common system. NEX-851
| @@ -91,6 +100,9 @@ func SafeConfig(cfg Config) *Config { | |||
| if len(cfgSafe.NodeStorage.Secret) > 0 { | |||
| cfgSafe.NodeStorage.Secret = "*** Masked ***" | |||
| } | |||
| if len(cfgSafe.RpcNode.Password) > 0 { | |||
| cfgSafe.RpcNode.Password = "*** Masked ***" | |||
There was a problem hiding this comment.
Feel free to use the masked idea out of nexus-api
Implement admin identity certificate endpoint and improve identity data comparisons.