Add Auth0 AppMetdata Struct #632
Conversation
Codecov Report
@@ Coverage Diff @@
## main #632 +/- ##
==========================================
- Coverage 45.85% 45.78% -0.07%
==========================================
Files 543 544 +1
Lines 16151 16147 -4
Branches 1202 1201 -1
==========================================
- Hits 7406 7393 -13
- Misses 7621 7628 +7
- Partials 1124 1126 +2
Continue to review full report at Codecov.
|
| testnetID := claims.VASPs[testnet] | ||
| mainnetID := claims.VASPs[mainnet] | ||
| testnetID := claims.VASPs.TestNet | ||
| mainnetID := claims.VASPs.MainNet |
There was a problem hiding this comment.
This makes a lot more sense than indexing into the map, can you update the above comment to reflect that this is a struct with default values now rather than a map?
| if err = s.SaveAuth0AppMetadata(*user.ID, *appdata); err != nil { | ||
| log.Error().Err(err).Str("user_id", *user.ID).Msg("could not save user app_metadata") | ||
| c.JSON(http.StatusInternalServerError, "could not complete user login") | ||
| return |
There was a problem hiding this comment.
Since we are now syncing the user app metadata at the end, this protects us from partial updates?
There was a problem hiding this comment.
There should only be two possible cases:
- The user is logging in for the first time and an organization is created for them
- The user needs the VASP ids updated from their organization
In case #1 - the following data is saved:
{
"orgid": "uuid",
"vasps": {
"testnet": "",
"mainnet": "",
}
}Case #1 should only happen once.
Because I moved the syncing to the end, case #2 is going to happen on every single login since I removed the MapEqual check. I debated whether or not we should do this -- it is safer, and logins are infrequent. However, we will eventually need some change detection to alert the front-end that the user needs to login again; so this is likely temporary.
Agreed, if we add more fields we should add more cases; though I do think I went a little overboard with all the different combinations -- we don't have to test every single combination of empty and filled field! |
Scope of changes
This PR should fix the panic caused when logging in a user that has a
nilapp_metadataand ensure that the record is correctly populated. It introduces a structAppMetadatathat will allow us to serialize theapp_metadataJSON in a structured fashion, ensuring keys and parsing are correct.Type of change
Acceptance criteria
This PR could really use an intense sanity check; particularly second eyes on the user-login logic. Happy to discuss if that would make things easier.
I still need to test that this fixes the panic.Author checklist
Reviewer(s) checklist
app_metadata