search

andrewlock / PwnedPasswords

An ASP.NET Core Identity validator that checks for PwnedPasswords
MIT License
103 stars 12 forks source link

Simplify internal API #7

Closed andrewlock closed 5 years ago

andrewlock commented 6 years ago

As mentioned by @SeanFarrow on Twitter:

I wonder whether we have missed a trick with the pwned password validator client, we currently return a boolean and the number of instances the password was pwned, should we not just return the number of instances and 0 if the password was not found?

It's an internal API, so doesn't really matter, but I agree!

SeanFarrow commented 6 years ago

This looks good, however I wonder whether int is large enough, should we use either ULong- (we know the value is positive) or something larger?

andrewlock commented 6 years ago

Int is large enough currently by a factor of 100, but it's a fair point, so just using a long might be the easiest? I'm not personally a fan of using unsigned vals generally, though prob would be fine here as we're doing any arithmetic with it.

SeanFarrow commented 6 years ago

Exactly, we know the number is going to be positive, so unsigned makes sense—I don’t like them either btw, but why use a long when we can get more precision using ulong, then we’re future proof! I’ll let you update and merge.

From: Andrew Lock notifications@github.com Sent: 16 September 2018 10:16 To: andrewlock/PwnedPasswords PwnedPasswords@noreply.github.com Cc: Sean Farrow sean.farrow@seanfarrow.co.uk; Mention mention@noreply.github.com Subject: Re: [andrewlock/PwnedPasswords] Simplify internal API (#7)

Int is large enough currently by a factor of 100, but it's a fair point, so just using a long might be the easiest? I'm not personally a fan of using unsigned vals generally, though prob would be fine here as we're doing any arithmetic with it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/andrewlock/PwnedPasswords/pull/7#issuecomment-421729548, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ABY1fp6TODw13jMb5SRXFrJ-KDBj-4jtks5ubhbGgaJpZM4WqgJX.

andrewlock commented 6 years ago

Haha, I'm actually suggesting we stick with long! I don't think ulong is worth it for the extra precision - long gives every person on the planet a billion pwned passwords - we don't need to make that 2 billion 😉

SeanFarrow commented 6 years ago

Ok, I forget what the total number currently in the system is, but that should suffice!

From: Andrew Lock notifications@github.com Sent: 16 September 2018 14:33 To: andrewlock/PwnedPasswords PwnedPasswords@noreply.github.com Cc: Sean Farrow sean.farrow@seanfarrow.co.uk; Mention mention@noreply.github.com Subject: Re: [andrewlock/PwnedPasswords] Simplify internal API (#7)

Haha, I'm actually suggesting we stick with long! I don't think ulong is worth it for the extra precision - long gives every person on the planet a billion pwned passwords - we don't need to make that 2 billion 😉

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/andrewlock/PwnedPasswords/pull/7#issuecomment-421766154, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ABY1fmBb5mYloJB57FP_9IajiPzjrm7eks5ublMSgaJpZM4WqgJX.