bug fix for natural person validation #117
Conversation
pkg/rvasp/transfer.go
Outdated
| if err = identity.Beneficiary.BeneficiaryPersons[0].GetNaturalPerson().Validate(); err != nil { | ||
| log.Warn().Err(err).Msg("beneficiary person validation error") | ||
| return protocol.Errorf(protocol.ValidationError, "beneficiary person validation error: %s", err) | ||
| if naturalPerson := identity.Beneficiary.BeneficiaryPersons[0].GetNaturalPerson(); naturalPerson != nil { |
There was a problem hiding this comment.
If natural person is nil do we also want to consider that an error? In other words is the beneficiary person required for a valid beneficiary?
There was a problem hiding this comment.
The issue here is that Krupa's identity had a legal person under beneficiary_persons and not a natural person, which if I'm correct is entirely valid IVMS101
There was a problem hiding this comment.
Ok, here's what I propose. Let's make it a switch statement and check for either NaturalPerson or LegalPerson. In both of those cases validate the entity, otherwise return a validation error. That should correspond better to IVMS101. We should probably do the same for BeneficiaryVASP on line 250 while we're at it.
There was a problem hiding this comment.
That's a good idea, I implemented the changes, that should fix the issue
| if err = person.LegalPerson.Validate(); err != nil { | ||
| log.Warn().Err(err).Msg("beneficiary legal person validation error") | ||
| return protocol.Errorf(protocol.ValidationError, "beneficiary legal person validation error: %s", err) | ||
| } |
There was a problem hiding this comment.
I think we need a default case to handle the case where the beneficiary person is nil (or something that happens to fit the isPerson_Person interface but is not a Person_NaturalPerson or Person_LegalPerson).
See this example, if a nil object is type checked then the default case is triggered: https://go.dev/play/p/aHIX6AOtfJz
| if err = person.LegalPerson.Validate(); err != nil { | ||
| log.Warn().Err(err).Msg("beneficiary vasp legal person validation error") | ||
| return protocol.Errorf(protocol.ValidationError, "beneficiary vasp legal person validation error: %s", err) | ||
| } |
There was a problem hiding this comment.
Also add a default case here.
There was a problem hiding this comment.
Could you also create a follow up story to add a test for validateIdentityPayload? Specifically I want to make sure this fix avoids the panic.
There was a problem hiding this comment.
Default added and story created
Adds a nil check before validating the natural person to prevent panics when the beneficiary contains a legal person.
NOTE: I also needed to regenerate the test certs since they were expired as of December of last year.