Skip to content

A business logic bug within auth.scala #281

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

Closed
patseev opened this issue Feb 28, 2021 · 3 comments · Fixed by #282
Closed

A business logic bug within auth.scala #281

patseev opened this issue Feb 28, 2021 · 3 comments · Fixed by #282
Labels
bug Something isn't working

Comments

@patseev
Copy link

patseev commented Feb 28, 2021

Hey! Found a business logic bug within Auth algebra.

https://github.com/gvolpe/pfps-shopping-cart/blob/second-edition/modules/core/src/main/scala/shop/algebras/auth.scala#L68

Basically, users.find will return Some(User) only in case if caller has provided correct username & password. I suppose you need to introduce a different method that performs a search only via username.

Otherwise, given a username conflict it will go to None branch, then will attempt inserting a user with conflicting username and will raise a postgres-level exception instead of your custom one.

@gvolpe
Copy link
Owner

gvolpe commented Feb 28, 2021

Hey @patseev , thanks for the report!

Looking at it now I would definitely model that function to perform a lookup just by username and compare the encrypted password afterwards. The way I see it, this is a design issue. I will consider it for the 2nd edition but I think we're a bit too late for the first one.

@gvolpe
Copy link
Owner

gvolpe commented Feb 28, 2021

Oh @patseev, I think that is expected!

See https://github.com/gvolpe/pfps-shopping-cart/blob/second-edition/modules/core/src/main/scala/shop/algebras/users.scala#L30

The lookup is performed via username and the encrypted password is compared afterwards. That is in fact, how I would design it now, I guess that's some assurance 😄

Otherwise, given a username conflict it will go to None branch, then will attempt inserting a user with conflicting username and will raise a postgres-level exception instead of your custom one.

I'm afraid I don't understand this, what do you mean? A conflict can only happen if the same username already exists in the database.

@gvolpe
Copy link
Owner

gvolpe commented Feb 28, 2021

Oh I get it now (sorry, Sunday after a few beers 😄 ) So yeah that's the validation for creating a new user you're talking about, definitely a case that wasn't tested. I'll get it fixed soon!

@gvolpe gvolpe added the bug Something isn't working label Feb 28, 2021
@gvolpe gvolpe mentioned this issue Feb 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants