Username validation refactor #1569

infomiho · 2023-11-10T15:16:15Z

moves username validation in the router
- this means that username validation is not performed every time user object is saved but rather when users login and signup
- this fixes the bug with validating the username as required even if it's an optional field on the model
removes username and password validation customisation
- I think is okay, since the validation customisation is not really used
- this makes it on par with email (who also didn't have the customisation)
- we should readd it with a better API for both username and email auth
puts all the validation login in validation.ts

Left to do

Remove the docs about username & password validation customization

Closes #1566
Closes #1563

infomiho · 2023-11-13T14:15:14Z

waspc/data/Generator/templates/server/src/auth/providers/email/utils.ts

@@ -0,0 +1,100 @@
+{{={= =}=}}


Moving email utils from utils.ts to providers/email/utils.ts

infomiho · 2023-11-13T14:15:34Z

waspc/data/Generator/templates/server/src/auth/providers/local/login.ts

@@ -28,3 +30,8 @@ export default handleRejection(async (req, res) => {

  return res.json({ token })
 })
+
+function ensureValidArgs(args: unknown): void {


Validating the username & password in the route

infomiho · 2023-11-13T14:16:23Z

waspc/data/Generator/templates/server/src/auth/validation.ts

@@ -0,0 +1,77 @@
+import HttpError from '../core/HttpError.js';


Putting username and email validations in one place

infomiho · 2023-11-13T14:16:37Z

waspc/data/Generator/templates/server/src/core/auth/prismaMiddleware.js

  registerPasswordHashing(prismaClient)
 }
-
-const userValidations = []


Removing username & password validation from Prisma middleware

Note: password hashing is still here

I guess password hashing is fine there!

What is our reasoning though about removing username & pass validation from there -> aren't we loosing something? I know you said we can reintroduce it back later once we do refactoring, but why not fix it now in such way that we keep having it there? Do you think it is not useful to have it there? Should we discuss the use cases a bit, pros and cons of having or not having it there?

Just to be clear: we are not removing the validation but rather moving it into the route 🙂

We are moving to route to make it work like email validation: connected to the login and signup process then connected to the Prisma model.

We are losing the customisation mechanism, but we should reintroduce it in a more user friendly way and in a way that enable also email validation customisation.

The current customisation mechanism felt a bit hacky and it you needed to specify the overrides on each usage with _waspCustomValidations. Also, I assume the feature is not really used. I got the feeling that people like the higher level API much more.

Sure that all makes sense, why I am asking is that that way it was before validation was enforced on any change to username / pass, not just the change via the routes. So if I as a developer do something more manually and want to set username/pass directly on the User entity, that would also be caught, while now it will not be caught. So I was curious why have you decided to drop that. For example, if I use https://wasp-lang.dev/docs/auth/username-and-pass#2-creating-your-custom-actions, I won't be getting these validations any more, right?

You say you think it wasn't used much -> I think there is certainly a case where it happens, which is https://wasp-lang.dev/docs/auth/username-and-pass#2-creating-your-custom-actions , and we since we are supporting that, I think we want to ensure good experience here. The question is, is it weird / bad if we drop these validations here? Is it expected by the developer? I think it might not be expected, so in that case we should mention this in the docs -> hey, you now have to do validation here on your own.
I wonder if they would rather have these validations here or not. If they are here, they can be annoying if they can't override them. But if there are none, they will have to implement their own.

I think I find it easier to think about if the concept of validating username/email is centralized per whole Wasp app, and you can either use it as it is default or customize it. I don't like the idea of them by accident not using it, or by accident going around it.

But I would like to hear your thoughts here, in more details!

For example, if I use https://wasp-lang.dev/docs/auth/username-and-pass#2-creating-your-custom-actions, I won't be getting these validations any more, right?

Yes, these validation won't apply here any more. The user will be responsible for validating the payload. And he can use our ensureValidUsername and ensureValidPassword helper functions. This is something I can write about in the docs in the section you linked (which I overlooked, sorry about that)

You say you think it wasn't used much -> I think there is certainly a case where it happens, which is https://wasp-lang.dev/docs/auth/username-and-pass#2-creating-your-custom-actions

"a case where it happens" != "users using the feature", I was talking about users not really customising the auth validation

and we since we are supporting that, I think we want to ensure good experience here.

I agree we should give them tools to validate auth related fields or any fields in general 👍

I think I find it easier to think about if the concept of validating username/email is centralized per whole Wasp app, and you can either use it as it is default or customize it.

I feel the validations are an auth feature and it should happen in that flow. If the users are building their custom auth flow, they can use our lower level validation helpers like mentioned above.

Alternatively, validation is hidden from the user and it happens at the Prisma level which is a bit too hidden for my taste. I'd rather instruct them to use ensureValidUsername then explain the details of validation happening in the Prisma middleware.

I don't like the idea of them by accident not using it, or by accident going around it.

Users can't accidentally write their own login and signup actions 😄 At that point they are responsible for validating the frontend payload before saving it in the database IMHO

Ok @infomiho this makes sense to me, let's do this as you described here!

infomiho · 2023-11-13T14:17:54Z

web/docs/auth/overview.md

@@ -429,65 +429,6 @@ If you use [Email](/docs/auth/email) authentication, the default validations are

 Note that `email`s are stored in a **case-insensitive** manner.

-### Customizing validations


Removing docs section around using auth validation customisation since it no longer exists.

I'd move the auth customization API to the DSL (like we have custom signup fields for example), but let's do as a new feature. #1571

Signed-off-by: Mihovil Ilakovac <[email protected]>

Martinsos

@infomiho looks good in general but I would like to hear more about the reasoning you used to decide on removing the validation from the middleware! I wrote a bit about it in the comment also.

Martinsos · 2023-11-13T14:23:18Z

waspc/data/Generator/templates/server/src/core/auth/prismaMiddleware.js

  registerPasswordHashing(prismaClient)
 }
-
-const userValidations = []


I guess password hashing is fine there!

What is our reasoning though about removing username & pass validation from there -> aren't we loosing something? I know you said we can reintroduce it back later once we do refactoring, but why not fix it now in such way that we keep having it there? Do you think it is not useful to have it there? Should we discuss the use cases a bit, pros and cons of having or not having it there?

Martinsos

@infomiho I approved, but it seems to me one thing is still missing: explaining in docs at which point does default validation stop working for them and they have to use those built-in arg validators.
I imagine we could mention this in auth/overview.md, when we say "Wasp includes several basic validation mechanisms for the auth fields." -> we could mention here that these validations work only in such and such case?
Also, when we tell them they can do their own custom login/singup actions, we should mention there that they should make sure to do validation on their own + point them to the built-in helpers. And finally, at the built-in helpers docs, we also might want to mention that these are needed when you are not using our actions but instead going rouge :D.
I am not sure if any of these is already in the docs but I couldn't find it.

infomiho added 5 commits November 10, 2023 16:11

Username validation in router. Validation refactor

ebf8365

Extract email utils into a separate file

def5853

Inline isEmailValid validator

1e63194

Split signup and login validation

5c10b37

Updates e2e tests

103d37a

infomiho marked this pull request as ready for review November 13, 2023 13:49

infomiho added 2 commits November 13, 2023 15:05

Updates exports

9f86f78

Updates docs and changelog

9e30f93

infomiho commented Nov 13, 2023

View reviewed changes

Cleanup

9c85786

Signed-off-by: Mihovil Ilakovac <[email protected]>

Martinsos reviewed Nov 13, 2023

View reviewed changes

infomiho added 3 commits November 14, 2023 11:48

Custom sign-up docs

a9fb3e3

Cleanup docs

625ee68

Docs cleanup

c4c9c6c

infomiho requested a review from Martinsos November 14, 2023 10:53

Martinsos approved these changes Nov 14, 2023

View reviewed changes

Updates docs

0058e0a

infomiho merged commit ef6d6b1 into main Nov 15, 2023
4 checks passed

infomiho deleted the miho-username-validation branch November 15, 2023 12:05

martinovicdev pushed a commit to martinovicdev/wasp that referenced this pull request Nov 15, 2023

Username validation refactor (wasp-lang#1569)

33361bd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Username validation refactor #1569

Username validation refactor #1569

infomiho commented Nov 10, 2023 •

edited

Loading

infomiho Nov 13, 2023

infomiho Nov 13, 2023

infomiho Nov 13, 2023

infomiho Nov 13, 2023 •

edited

Loading

Martinsos Nov 13, 2023

infomiho Nov 13, 2023

Martinsos Nov 13, 2023

infomiho Nov 13, 2023 •

edited

Loading

Martinsos Nov 13, 2023

infomiho Nov 13, 2023 •

edited

Loading

Martinsos left a comment

Martinsos Nov 13, 2023

Martinsos left a comment

		@@ -0,0 +1,77 @@
		import HttpError from '../core/HttpError.js';

		@@ -429,65 +429,6 @@ If you use [Email](/docs/auth/email) authentication, the default validations are

		Note that `email`s are stored in a case-insensitive manner.

		### Customizing validations

Username validation refactor #1569

Username validation refactor #1569

Conversation

infomiho commented Nov 10, 2023 • edited Loading

Left to do

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infomiho Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infomiho Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infomiho Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

Martinsos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Martinsos left a comment

Choose a reason for hiding this comment

infomiho commented Nov 10, 2023 •

edited

Loading

infomiho Nov 13, 2023 •

edited

Loading

infomiho Nov 13, 2023 •

edited

Loading

infomiho Nov 13, 2023 •

edited

Loading