search

andrewlock / PwnedPasswords

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

Add support for the Add-Padding header #22

Open andrewlock opened 4 years ago

SeanFarrow commented 4 years ago

This looks good.

if the option to support padding is set, should we ensure the MinimumFrequencyToConsiderPwned is at least 1? In fact it feels like we should not allow 0 as a possible value for this property at all.

andrewlock commented 4 years ago

In fact it feels like we should not allow 0 as a possible value for this property at all.

I agree,it doesn't make any sense to be less than 1! I guess the best place to do that is in the PwnedPasswordsClient constructor, though that means we're checking it explicitly every time you use the client, which is a bit annoying.

Ideally they would be one place we'd check it - I normally run tasks like this on app startup. We could add a hosted service to do that, but that's maybe a bit heavy handed. What do you reckon? It's only a simple check, shall we just keep it in the client?

SeanFarrow commented 4 years ago

Ideally, we should keep this in the code that sets the property value in the options class. I’ll raise an issue and write a fix later today. Thanks, Sean. From: Andrew Lock [mailto:notifications@github.com] Sent: 17 April 2020 08:15 To: andrewlock/PwnedPasswords PwnedPasswords@noreply.github.com Cc: Sean Farrow sean.farrow@seanfarrow.co.uk; Comment comment@noreply.github.com Subject: Re: [andrewlock/PwnedPasswords] Add support for the Add-Padding header (#22)

In fact it feels like we should not allow 0 as a possible value for this property at all.

I agree,it doesn't make any sense to be less than 1! I guess the best place to do that is in the PwnedPasswordsClient constructor, though that means we're checking it explicitly every time you use the client, which is a bit annoying.

Ideally they would be one place we'd check it - I normally run tasks like this on app startup. We could add a hosted service to do that, but that's maybe a bit heavy handed. What do you reckon? It's only a simple check, shall we just keep it in the client?

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/andrewlock/PwnedPasswords/pull/22#issuecomment-615084637, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AALDK7XCMRJAA7THVIAHIYLRM76W3ANCNFSM4MKEYMCQ.