Skip to content
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.

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

Verify function return values are semantically equivalent #40

Open
KendallWeihe opened this issue Feb 9, 2024 · 4 comments
Open

Verify function return values are semantically equivalent #40

KendallWeihe opened this issue Feb 9, 2024 · 4 comments

Comments

@KendallWeihe
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor

alecthomas commented Feb 9, 2024

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
Copy link
Contributor Author

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 KendallWeihe mentioned this issue Feb 12, 2024
@KendallWeihe
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

No branches or pull requests

3 participants