TBD54566975 / web5-go

Apache License 2.0
10 stars 6 forks source link

Verify function return values are semantically equivalent #40

Open KendallWeihe opened 6 months ago

KendallWeihe commented 6 months ago
ok, err := jwt.Verify(idToken)
if err != nil {
    return false, fmt.Errorf("failed to verify ID Token: %w", err)
}

if !ok {
    return false, fmt.Errorf("integrity compromised")
}

If ok is false then err will always be nil

Which raises the question, do we want this? Because as it is now requires one extra if statement for the same semantic situation, which is boilerplate

mistermoe commented 6 months ago

super good point @KendallWeihe. which makes me wonder whether we should even return ok? @alecthomas thoughts on all this from a go idioms perspective?

alecthomas commented 6 months ago

If you're always returning an error in the false case then it makes sense for the function to just return the error. It would make sense if you need to distinguish between invalid for some reason and any other error.

KendallWeihe commented 6 months ago

This is fixed for the jws.Verify() and jwt.Verify() which now return (Decoded, err) in this PR

Issue still present in the Verify() functions within the crypto/ directory

KendallWeihe commented 6 months ago

I had started down this path last week but didn't get all the way https://github.com/TBD54566975/web5-go/tree/kendallw/verify-return-types