Skip to content

FIR-9: Identity enhancements #549

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 71 commits into from
Mar 3, 2022
Merged

FIR-9: Identity enhancements #549

merged 71 commits into from
Mar 3, 2022

Conversation

@peterbroadhurst
Copy link
Contributor

@peterbroadhurst peterbroadhurst commented Feb 24, 2022

See hyperledger/firefly-fir#9

@awrichar - the E2E is passing on these changes, but leaving PR in review state as I'd like to add some more E2E tests and do migration testing before pulling it out of draft.

I will put comments in-line in the code to help orient your review

@peterbroadhurst
Initial migration work
8fc3049 
Signed-off-by: Peter Broadhurst <[email protected]>
@peterbroadhurst
Merge branch 'main' of github.com:hyperledger/firefly into identityv2
0431c35
@peterbroadhurst
More work on migrations
07d8b89 
Signed-off-by: Peter Broadhurst <[email protected]>
@peterbroadhurst
Merge branch 'main' of github.com:hyperledger/firefly into identityv2
233cc94
@peterbroadhurst
Updated migrations an initial typedefs
8ba8944 
Signed-off-by: Peter Broadhurst <[email protected]>
@peterbroadhurst
Merge branch 'main' of github.com:hyperledger/firefly into identityv2
deed601
@peterbroadhurst
Merge branch 'main' of github.com:hyperledger/firefly into identityv2
dacb4bd
@peterbroadhurst
Add identity SQL table
77d0f19 
Signed-off-by: Peter Broadhurst <[email protected]>
@peterbroadhurst
Verifier DTO
6616a4b 
Signed-off-by: Peter Broadhurst <[email protected]>
@peterbroadhurst
Interim commit
bcb739b 
Signed-off-by: Peter Broadhurst <[email protected]>
@peterbroadhurst
Identity manager lookup functions implemented
d3c568d 
Signed-off-by: Peter Broadhurst <[email protected]>
@peterbroadhurst
Add message references to identity object/tables
74ac994 
Signed-off-by: Peter Broadhurst <[email protected]>
@peterbroadhurst
Update identity object for claims/verifications
aa948e8 
Signed-off-by: Peter Broadhurst <[email protected]>
@peterbroadhurst
Add chain verification function for identities
6418856 
Signed-off-by: Peter Broadhurst <[email protected]>
@peterbroadhurst
Interface impl for Definitions
ca88c9d 
Signed-off-by: Peter Broadhurst <[email protected]>
@peterbroadhurst
Commit before find/replace
73e9981 
Signed-off-by: Peter Broadhurst <[email protected]>
@peterbroadhurst
Broadcast migrated
bdda63e 
Signed-off-by: Peter Broadhurst <[email protected]>
@peterbroadhurst
Progress on networkmap
eac3554 
Signed-off-by: Peter Broadhurst <[email protected]>
@peterbroadhurst
Network map registration
cbbeae4 
Signed-off-by: Peter Broadhurst <[email protected]>
@peterbroadhurst
Add generic identity query support
94c8cc4 
Signed-off-by: Peter Broadhurst <[email protected]>
@peterbroadhurst
Merge with main
a5c3bbb 
Signed-off-by: Peter Broadhurst <[email protected]>
@peterbroadhurst
Handle new identity structure for root check
3b87b55 
Signed-off-by: Peter Broadhurst <[email protected]>
@peterbroadhurst
Identity profile update
34616f5 
Signed-off-by: Peter Broadhurst <[email protected]>
@peterbroadhurst
Clean up DX interface - no need for separate ID
59dde9b 
Signed-off-by: Peter Broadhurst <[email protected]>
@peterbroadhurst
Updates to private messaging
769187b 
Signed-off-by: Peter Broadhurst <[email protected]>
@peterbroadhurst
Progress on Pm
2d90e68 
Signed-off-by: Peter Broadhurst <[email protected]>
@peterbroadhurst
UT complete on private messaging
38de541 
Signed-off-by: Peter Broadhurst <[email protected]>
@peterbroadhurst
Move to single copy of IsRootOrgBroadcast logic in IM
9c68ec7 
Signed-off-by: Peter Broadhurst <[email protected]>
@peterbroadhurst
Intermediate commit with events migration
a0002fc 
Signed-off-by: Peter Broadhurst <[email protected]>
@peterbroadhurst
Up to definitions
07f4486 
Signed-off-by: Peter Broadhurst <[email protected]>
@peterbroadhurst
Copy link
Contributor Author

peterbroadhurst commented Mar 1, 2022

Thanks Andrew for the detailed review, and excellent suggestions.

I've gone through each of the comments and addressed them with code/comment changes.

Before we merge I'm planning to re-run the migration test I mentioned above one last time, but would be great to get your feedback first (as the migration test takes a while to set up and run).

@peterbroadhurst
Address review comments
1145279 
Signed-off-by: Peter Broadhurst <[email protected]>
@peterbroadhurst peterbroadhurst requested a review from awrichar March 2, 2022 02:20
awrichar
awrichar reviewed Mar 2, 2022
View reviewed changes
awrichar
awrichar reviewed Mar 2, 2022
View reviewed changes
awrichar
awrichar reviewed Mar 2, 2022
View reviewed changes
awrichar
awrichar reviewed Mar 2, 2022
View reviewed changes
internal/apiserver/route_get_verifier_by_id.go
Method: http.MethodGet,
PathParams: []*oapispec.PathParam{
{Name: "ns", ExampleFromConf: config.NamespacesDefault, Description: i18n.MsgTBD},
{Name: "vid", Example: "id", Description: i18n.MsgTBD},
Copy link
Contributor

@awrichar awrichar Mar 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just want to note that this became a "hash" rather than an "id" in the latest pass. Might suggest a tweak to reflect that on the Swagger?

Copy link
Contributor Author

@peterbroadhurst peterbroadhurst Mar 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually thought about that and didn't do it. My thinking was that it's actually a resource identifier, just one that's deterministically generated on all nodes. We don't rely on it being a hash for processing, and due to an edge case on migration there's a small number of environments where it might not actually be the Hash.

Copy link
Contributor

@awrichar awrichar Mar 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK - I'm mainly worried about the case where I query /verifiers and get a list of things, how do I know which one to put in vid in order to look up that one item? I have to know that hash maps to vid. Of course this is a detail that can easily be tweaked later.

awrichar
awrichar reviewed Mar 2, 2022
View reviewed changes
awrichar
awrichar reviewed Mar 2, 2022
View reviewed changes
awrichar
awrichar reviewed Mar 2, 2022
View reviewed changes
awrichar
awrichar reviewed Mar 2, 2022
View reviewed changes
awrichar
awrichar reviewed Mar 2, 2022
View reviewed changes
internal/definitions/definition_handler_identity_verification.go
// It might be being processed in the same pin batch as us - so we can't

// See if the message has already arrived, if so we need to queue a rewind to it
claimMsg, err := dh.database.GetMessageByID(ctx, verification.Claim.ID)
Copy link
Contributor

@awrichar awrichar Mar 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to check the pending messages in the batch before hitting the database? Unless we expect the database to be the more common case. Probably getting into micro-optimization at this point anyway.

Copy link
Contributor Author

@peterbroadhurst peterbroadhurst Mar 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your suggestion feels right to me - will do it.

Copy link
Contributor Author

@peterbroadhurst peterbroadhurst Mar 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it did make the logic a lot less clean... so going to defer this for now (the database case is more likely, as my best guess):

	// See if the message has already arrived, if so we need to queue a rewind to it
	claimMsg := state.GetPendingConfirm()[*verification.Claim.ID]
	if claimMsg == nil {
		claimMsg, err := dh.database.GetMessageByID(ctx, verification.Claim.ID)
		if err != nil {
			return HandlerResult{Action: ActionRetry}, err
		}
		if claimMsg != nil && claimMsg.State != fftypes.MessageStateConfirmed {
			claimMsg = nil
		}
	}

Copy link
Contributor

@awrichar awrichar Mar 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me; thanks for giving it a pass.

awrichar
awrichar reviewed Mar 2, 2022
View reviewed changes
Copy link
Contributor

@awrichar awrichar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates - this is looking even better and I think I can fully understand the flow now.

I marked most of my comments resolved, although there are 1 or 2 that may still need a look. Also left a handful more on small items. After one more pass to confirm all of this, I think it's good to go.

@peterbroadhurst
Tweaks from review
c6614aa 
Signed-off-by: Peter Broadhurst <[email protected]>
@peterbroadhurst
Swagger
75f63be 
Signed-off-by: Peter Broadhurst <[email protected]>
@peterbroadhurst
Copy link
Contributor Author

peterbroadhurst commented Mar 2, 2022

Moving onto the migration test re-run after the hash change

@peterbroadhurst
PSQL requires UUID to text
88da6d6 
Signed-off-by: Peter Broadhurst <[email protected]>
@peterbroadhurst
Copy link
Contributor Author

peterbroadhurst commented Mar 2, 2022 •
edited
Loading

Sadly the migration cheat of using a UUID to fill in the bytes that should be a hash work. First problem was UUID cannot be used in a REPLACE as a string, then the second problem was after I did REPLACE(o.id::text, '-', '') I ended up with too few characters c0f5e5b543a7477daf1e33f96702d995 (UUID is 16B, not 32).

postgres=# select hash from verifiers;
                               hash
------------------------------------------------------------------
 c0f5e5b543a7477daf1e33f96702d995
 f4ac3f2dab2b4da096ee70a13d2c21ba
 bc0ae3c7d7574fac9e0ef49017cf972b
 5f6003a1461d4c37a68ddf88dfe07f3f
(4 rows)

Then the REST API code on retrieval failed with "FF10121: Database resultset read error from table 'verifiers': sql: Scan error on column index 0, name \"hash\": encoding/hex: invalid byte: U+0020 ' '":

So I can write them twice, but it's feeling very messy at this point 🤔

@peterbroadhurst
Copy link
Contributor Author

peterbroadhurst commented Mar 2, 2022

I added the double-UUID extension to the migration code, and tested the migration e2e.

Racked my brains, and couldn't think of another simpler solution to supporting up+down migrations for this table split. I considered allocating a eyecatcher prefixed ID in the up migration, and then doing a Go based proper Hash calculation. But this brakes the ability to down migration.

So let me know if you have any further inspiration @awrichar - otherwise, I believe this is all tested and good to go.

@awrichar
Copy link
Contributor

awrichar commented Mar 2, 2022

Yea, I was suspicious that the UUID to hash hack wouldn't quite work. I think where it's landed is fine though.

I considered the fact that Postgres could probably do actual hashing - but it would be a problem for sqlite, and would make the down migration harder in any case.

Looks good to me now - will leave the button push to you!

@peterbroadhurst peterbroadhurst merged commit 25d5800 into hyperledger:main Mar 3, 2022
@peterbroadhurst peterbroadhurst deleted the identityv2 branch March 3, 2022 01:02
@peterbroadhurst peterbroadhurst mentioned this pull request Mar 3, 2022
Merged
@awrichar awrichar mentioned this pull request Mar 4, 2022
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@awrichar awrichar awrichar approved these changes

@nguyer nguyer Awaiting requested review from nguyer

@nickgaski nickgaski Awaiting requested review from nickgaski

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@peterbroadhurst @codecov-commenter @awrichar