feat(auth): Added flexible validation checks for User entity email and password #376

shayneczyzewski · 2021-12-02T16:20:15Z

Description

Adds an additional Prisma middleware for User email/password validation checks that the user can disable and/or extend. This builds upon the existing pattern for hashing the password. While technically a monkey patch of Prisma's create/update/etc. functions just for User, it feels like the cleanest approach to ensure:

sane default validations always run by default
users can disable default validations
users can add their own custom validations

The goal is to provide basic checks out of the box, and allow users to use just ours (default), ours and theirs, or just theirs, like so:

const newUser = context.entities.User.create({
  data: { email: 'some@email.com', password: 'this will be hashed!' },
  _waspSkipDefaultValidations: false, // can be omitted if false (default), or explicitly set true
  _waspCustomValidations: [
    {
      validates: 'password',
      message: 'password must contain an uppercase letter',
      validator: password => /[A-Z]/.test(password)
    },
  ]
})

These are the default validations we will check (unless disabled):

const defaultValidations = [
  { validates: EMAIL_FIELD, message: 'email must be present', validator: email => !!email },
  { validates: PASSWORD_FIELD, message: 'password must be present', validator: password => !!password },
  { validates: PASSWORD_FIELD, message: 'password must be at least 8 characters', validator: password => password.length >= 8 },
  { validates: PASSWORD_FIELD, message: 'password must contain a number', validator: password => /\d/.test(password) },
]

Documentation has been updated to reflect user facing concerns.

Closes #88 and fixes #100

Type of change

Please select the option(s) that is more relevant.

Code cleanup
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

matijaSos

This is an amazing first PR! I really like how you handled the docs too and the code is really clean.

This also made me think - all of these validations are now performed on the server, as they should be (we don't want somebody to be able to bypass our client and send direct API calls with the invalid data to the backend). But is there a use case where we'd also want to be able to perform some checks on the client too? E.g. maybe if we want to have an instant feedback as the user types email/password in etc., anything that doesn't require a database.

It might be interesting to think about how we could enable this while also avoiding the duplication of code from the perspective of Wasp developer. This is definitely out of the scope for now, but if we can think of a viable use case maybe we could write it down in issues so we can reference it in the future.

waspc/data/Generator/templates/react-app/src/auth/forms/Login.js

waspc/data/Generator/templates/server/src/dbClient.js

waspc/data/Generator/templates/server/src/routes/auth/signup.js

waspc/src/Wasp/Generator/ServerGenerator.hs

web/docs/language/basic-elements.md

Martinsos

Awesome @shayneczyzewski ! Really thorough and complete solution to the issue, makes complete sense to me and I like it a lot.

I think it is on the spot regarding the main ideas - I only left some minor comments, they shouldn't cause any big changes.

waspc/data/Generator/templates/react-app/src/auth/forms/Signup.js

waspc/data/Generator/templates/server/src/core/EntityValidationError.js

waspc/data/Generator/templates/server/src/dbClient.js

waspc/data/Generator/templates/server/src/routes/auth/signup.js

waspc/src/Wasp/Generator/ServerGenerator.hs

web/docs/language/basic-elements.md

waspc/data/Generator/templates/server/src/dbClient.js

matijaSos

almost done! let us know when ready and we'll finalize it :)

waspc/data/Generator/templates/server/src/dbClient.js

waspc/data/Generator/templates/server/src/routes/auth/signup.js

shayneczyzewski · 2021-12-03T17:48:07Z

Thank you @Martinsos and @matijaSos so much for your thoughtful feedback. I believe I have addressed all of them except one, which I had an open question on. It was about moving HttpError.js and AuthError.js under core/error. I was worried that may be a breaking change since I saw in many example apps they import as follows: import HttpError from '@wasp/core/HttpError.js' If so, do we want to tackle that in a future, breaking PR?

Also, if you have any thoughts on ways I can improve the PR back and for for my first time, please let me know. I tried to keep the commits focused to the comments for easier resolving. At the very end, once you verify/resolve all comments and I get the final 👍🏻 I will squash all my PR fixes into a single commit.

But no rush on finalizing this, as I have taken lots of your time already yesterday. In the meantime, I plan to do more extensive local testing of this branch, including against some existing sample apps. I can then transition to the next task and come back to it once it is ready to be merged. Thanks

waspc/data/Generator/templates/react-app/src/utils.js

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

Martinsos · 2021-12-03T21:20:02Z

waspc/data/Generator/templates/server/src/dbClient.js

-    return result
-  })
+
+  registerAuthMiddleware(prismaClient)


waspc/data/Generator/templates/server/src/routes/auth/signup.js

Martinsos · 2021-12-03T21:28:28Z

waspc/src/Wasp/Generator/ServerGenerator/AuthG.hs

@@ -40,6 +41,19 @@ genCoreAuth auth = C.makeTemplateFD tmplFile dstFile (Just tmplData)
              "userEntityLower" .= Util.toLowerFirst userEntity
            ]

+genAuthMiddleware :: Wasp.Auth.Auth -> FileDraft


Ha nice job :D! A bit boilerplatish, this whole file, but nothing we should worry about right now, it is not worth the effort at the moment.

waspc/data/Generator/templates/react-app/src/utils.js

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

waspc/data/Generator/templates/server/src/utils.js

web/docs/language/basic-elements.md

matijaSos · 2021-12-06T17:26:14Z

web/docs/language/basic-elements.md

+
+To disable default validations, or add your own, you can do:
+```js
+const newUser = context.entities.User.create({


It might be helpful to provide a more "realistic" example here: e.g. we leave _waspSkipDefaultValidations to be false (and comment it can be omitted or set explicitly to true) and for custom validations we use something like what you did in a feature proposal, e.g. "password must contain at least one digit". Just a thought - it took me a bit to realize that we turned off default validations and then re-written them as custom ones.

ah sorry, I see now that "one digit" is also part of default validations. Maybe something like "uppercase letter".

Good thinking on the variety and more realistic. I will do should contain an uppercase letter, thanks!

This has been addressed here: 9a545b7 Thanks!

matijaSos

LGTM!

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

matijaSos · 2021-12-06T17:36:28Z

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

+  registerPasswordHashing(prismaClient)
+}
+
+const validateUserData = (data, args, action) => {


data is pretty generic, any way we can be more specific? Or maybe just go with validateUser(user, args, action)? But not a big deal, I just remember from "Clean Code" that we should avoid "data", "info" and "container" :D.

You are right! I haven't paid attention to it but true. Data does have a bit of context here though but is it enough? Hard to say hm.

I think swapping validateUserData(data, args, action) to validateUser(user, args, action) here is a worthwhile change, even with the existing context, if nothing more than to cement it. Will change, thanks! 👍🏻

This has been addressed here: 9a545b7 Thanks!

matijaSos · 2021-12-06T17:40:00Z

web/docs/language/basic-elements.md

+
+To disable default validations, or add your own, you can do:
+```js
+const newUser = context.entities.User.create({


ah sorry, I see now that "one digit" is also part of default validations. Maybe something like "uppercase letter".

Martinsos

Awesome work all together @shayneczyzewski , LGTM! I think we really polished this one now and covered all the nooks and crannies, and you did a great job on grasping the whole situation here and solving it. I will approve, @matijaSos you also give your approve when you are ready and then we can merge this one (squash to 1 commit or multiple commits if they make sense and then rebase).

web/docs/language/basic-elements.md

…d password Adds an additional Prisma middleware for validation checks that the user can disable and/or extend Closes #88 and fixes #100

shayneczyzewski force-pushed the shayne-password-checks branch from b59b57b to 81000d5 Compare December 2, 2021 16:43

shayneczyzewski changed the title ~~WIP: feat(auth): Added flexible validation checks for User entity email and password~~ feat(auth): Added flexible validation checks for User entity email and password Dec 2, 2021

shayneczyzewski requested a review from Martinsos December 2, 2021 17:31

matijaSos reviewed Dec 2, 2021

View reviewed changes

Martinsos reviewed Dec 2, 2021

View reviewed changes

matijaSos reviewed Dec 2, 2021

View reviewed changes

waspc/data/Generator/templates/server/src/dbClient.js Outdated Show resolved Hide resolved

waspc/data/Generator/templates/server/src/dbClient.js Outdated Show resolved Hide resolved

waspc/data/Generator/templates/server/src/routes/auth/signup.js Outdated Show resolved Hide resolved

shayneczyzewski force-pushed the shayne-password-checks branch from 917e0b6 to 3ffbc62 Compare December 3, 2021 16:00

Martinsos reviewed Dec 3, 2021

View reviewed changes

shayneczyzewski force-pushed the shayne-password-checks branch from 72c8794 to 48d8eed Compare December 6, 2021 15:32

Martinsos reviewed Dec 6, 2021

View reviewed changes

waspc/data/Generator/templates/react-app/src/utils.js Outdated Show resolved Hide resolved

Martinsos reviewed Dec 6, 2021

View reviewed changes

matijaSos reviewed Dec 6, 2021

View reviewed changes

matijaSos approved these changes Dec 6, 2021

View reviewed changes

Martinsos approved these changes Dec 7, 2021

View reviewed changes

matijaSos reviewed Dec 7, 2021

View reviewed changes

web/docs/language/basic-elements.md Outdated Show resolved Hide resolved

feat(auth): Added flexible validation checks for User entity email an…

1187743

…d password Adds an additional Prisma middleware for validation checks that the user can disable and/or extend Closes #88 and fixes #100

shayneczyzewski force-pushed the shayne-password-checks branch from c43c2d4 to 1187743 Compare December 7, 2021 17:57

shayneczyzewski merged commit f444eaa into master Dec 7, 2021

shayneczyzewski deleted the shayne-password-checks branch December 7, 2021 18:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(auth): Added flexible validation checks for User entity email and password #376

feat(auth): Added flexible validation checks for User entity email and password #376

shayneczyzewski commented Dec 2, 2021 •

edited

matijaSos left a comment •

edited

Martinsos left a comment

matijaSos left a comment

shayneczyzewski commented Dec 3, 2021

Martinsos Dec 3, 2021

Martinsos Dec 3, 2021

matijaSos Dec 6, 2021

matijaSos Dec 6, 2021

shayneczyzewski Dec 7, 2021

shayneczyzewski Dec 7, 2021

matijaSos left a comment

matijaSos Dec 6, 2021

Martinsos Dec 6, 2021

shayneczyzewski Dec 7, 2021

shayneczyzewski Dec 7, 2021

matijaSos Dec 6, 2021

Martinsos left a comment

feat(auth): Added flexible validation checks for User entity email and password #376

feat(auth): Added flexible validation checks for User entity email and password #376

Conversation

shayneczyzewski commented Dec 2, 2021 • edited

Description

Type of change

matijaSos left a comment • edited

Choose a reason for hiding this comment

Martinsos left a comment

Choose a reason for hiding this comment

matijaSos left a comment

Choose a reason for hiding this comment

shayneczyzewski commented Dec 3, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matijaSos left a comment

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

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

shayneczyzewski commented Dec 2, 2021 •

edited

matijaSos left a comment •

edited