Skip to content
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

[docs] How to setup Passwordless authentication #7650

Merged
merged 24 commits into from
Mar 29, 2023

Conversation

jacebenson
Copy link
Contributor

#6204 is an issue that @cannikin and @Olyno were discussing setting up passwordless auth on. I'm not sure if this makes sense to modify how dbauth works (and that's not in the core-repo anymore anyway!) or if adding a how-to to just make it work.

But, I went ahead and made it work and thought I'd add a very rough PR adding these notes.

I created the following repo from teh tutorial and checked in after installing dbauth, and then again after forcing it to be passwordless.

https://github.com/jacebenson/redwood-dbauth-passwordless

I know this is a WIP, but i'd rather put it out there then let it be forgotten on me.

@Olyno
Copy link
Contributor

Olyno commented Feb 17, 2023

Wow, thank you for sharing the passwordless documentation! It's awesome!

I did notice that the link-based approach may not work for users who check their emails on a phone and use the web platform on a computer. I think a code approach for passwordless could be a great alternative (or a very good addition).

What do you think? Thanks again for your work!

@jacebenson
Copy link
Contributor Author

@Olyno I'm not sure how to set that up.

I think that might be a great extra.

I guess my question is, is this good enough for a cookbook?

@Olyno
Copy link
Contributor

Olyno commented Feb 19, 2023

It looks good to me. I'll do a more thorough test early next week, and then I'll be able to give more concrete feedback on the documentation.

I'm also waiting to see if anyone from the project wants to give feedback on it, it would be a great addition.

@cannikin
Copy link
Member

Nice work @jacebenson! Thanks for putting this together. I'm all for making this a cookbook for now, rather than modifying dbAuth. I'm trying to keep dbAuth as simple as possible for as long as possible, and this article shows that you can extend its functionality without having to modify the Redwood codebase.

So you've actually run this code and it works? It's been a while since I worked on dbAuth, but I thought it wouldn't even call the login handler unless the user's email/password matched against the hashedPassword in the database (that check is internal to the dbAuth code). You're hardcoding hashedPassword and salt to just be loginToken and salt which would never pass the security check. Hmmm

Some questions/feedback:

  • I don't love saying that storing hashed passwords and salts is a security risk. We don't want to imply that just by using dbAuth you've got bad security. If your entire database was compromised then yes, a bad actor would have a headstart trying to crack passwords. But securing your database is kind of job number one when building an app!
  • You should use something other than Math.random() to create a random number, as it's not random enough for cryptographic stuff. Something like secure random is safer.
  • You should add a note in the text that devs need to make sure and never add the loginToken field to the SDL, or it will be available for anyone to query via GraphQL.
  • Having some screenshots of the front end pages that are being build would be awesome.
  • The article feels like it's missing a conclusion! It just suddenly ends after the last code example! :)

Great work!

@jacebenson
Copy link
Contributor Author

jacebenson commented Feb 20, 2023

@cannikin I'll make those changes.

I wasn't trying to disparage dbAuth. I love it, I just want my hands as clean as possible. I'll reword the bit about "storing hashed passwords and salts"

Regarding Math.random(), I'm not sure but I'm guessing I can use they Crypto thing to generate the id.

I need to remove the loginToken from the user's SDL (create/edit/read)

I'll add some screenshots throughout.

I'll add a conclusion

I am running this in production for a site I'm messing with but let me patch that SDL before I go linking things.

My Todos;

  • Reword the bit about "storing hashed passwords and salts" to be better
  • Use Crytpo to generate a number instead of Math.random() to generate the token
  • Remove the loginToken from the user's SDL and add a comment or reason why in the cookbook
  • Add some screenshots of the new login page in it's two states.
  • Add a conclusion

…d loginToken from SDLs, Added some screenshots, and added a concusion
Comment on lines +62 to +74
let randomNumber = (() => {
let random = CryptoJS.lib.WordArray.random(6)
let randomString = random.toString()
let sixDigitNumber = randomString.replace(/\D/g, '')
if (sixDigitNumber.length < 6) {
sixDigitNumber = sixDigitNumber.padStart(6, '0')
}
if (sixDigitNumber.length > 6) {
sixDigitNumber = sixDigitNumber.slice(0, 6)
}
return sixDigitNumber.toString()
})()
console.log({ randomNumber }) // email the user this number
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this is a great way to do this, if there's a better way I'm all ears. It works.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, what does WordArray return, letters and numbers? Is it possible it could return all letters, which would end up getting replaced with all zeros? That wouldn't be ideal. haha Is there a function that just returns random numbers? If not, maybe generate a way longer WordArray to start, so you're much more likely to end up with 6 digits?

Copy link
Contributor

@Olyno Olyno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your patience. I tried to follow your cookbook, but without success. Would it be possible to have more information about some parts?

Note that I have only used Redwood a few times, so I think I can help with the use of the passwordless as a beginner.

### 2. Setting up the generateToken function
Next, we need to create a function that will generate a token and an expiration date.

If you followed the tutorial, you might not have a `/api/src/services/users/users.js` file. If that's the case, you can create it.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do i get this file? Using the latest Redwood version (4.2.0), and following the tutorial to setup the authentication, i don't have this file generated

@jacebenson
Copy link
Contributor Author

@cannikin @Olyno i'd like to move this along. Anything I can do to move this along?

@cannikin
Copy link
Member

Hey @jacebenson it would be awesome if @Olyno can verify that everything works as written...sounds like they're having an issue in the previous comment?

Added extra text around how to generate the users service file.
Copy link
Contributor

@Olyno Olyno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really sorry for the delay of my review, here it is right now:

The cookbook is quite well documented and understandable. However, I would have some remarks for some parts of it.

docs/docs/how-to/dbauth-passwordless.md Show resolved Hide resolved
docs/docs/how-to/dbauth-passwordless.md Show resolved Hide resolved
docs/docs/how-to/dbauth-passwordless.md Outdated Show resolved Hide resolved
docs/docs/how-to/dbauth-passwordless.md Outdated Show resolved Hide resolved
docs/docs/how-to/dbauth-passwordless.md Outdated Show resolved Hide resolved
docs/docs/how-to/dbauth-passwordless.md Outdated Show resolved Hide resolved
docs/docs/how-to/dbauth-passwordless.md Outdated Show resolved Hide resolved
docs/docs/how-to/dbauth-passwordless.md Outdated Show resolved Hide resolved
docs/docs/how-to/dbauth-passwordless.md Outdated Show resolved Hide resolved
docs/docs/how-to/dbauth-passwordless.md Outdated Show resolved Hide resolved
@jacebenson jacebenson requested a review from Olyno March 14, 2023 15:04
Copy link
Contributor

@Olyno Olyno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@cannikin cannikin added the release:docs This PR only updates docs label Mar 14, 2023
@cannikin
Copy link
Member

Looks good except for my couple comments above!

@replay-io
Copy link

replay-io bot commented Mar 14, 2023

16 replays were recorded for e4bd240.

image 0 Failed
image 16 Passed
    requireAuth graphql checks
          ```
          locator.waitFor: Target closed
          =========================== logs ===========================
          waiting for locator('.rw-form-error-title').locator('text=You don\'t have permission to do that') to be visible
          ============================================================
          ```
        </ol>
      </details>
      <li><a href=https://app.replay.io/recording/cff5be3d-078e-4279-80c6-a4ead3f9605a>useAuth hook, auth redirects checks</a></li>
      <li><a href=https://app.replay.io/recording/f25556bf-ad60-4701-8487-2a954e3b2768>Check that a specific blog post is prerendered</a></li>
      <li><a href=https://app.replay.io/recording/5adc9c91-52bf-4c22-98fc-6162865fc4dd>Check that about is prerendered</a></li>
      <li><a href=https://app.replay.io/recording/d8097551-c023-4314-8cca-e7b8e69c0991>Check that homepage is prerendered</a></li>
      <li><a href=https://app.replay.io/recording/a63047a5-15f7-4dc3-9c36-64ae312e4611>Check that meta-tags are rendering the correct dynamic data</a></li>
      <li><a href=https://app.replay.io/recording/8f803a7a-4597-4515-bba3-23de59827b23>Check that you can navigate from home page to specific blog post</a></li>
      <li><a href=https://app.replay.io/recording/8321b200-5344-4192-ac4b-7dafece373bf>Waterfall prerendering (nested cells)</a></li>
      <li><a href=https://app.replay.io/recording/fa4aaca6-df56-4c15-af8b-08a9c247e755>RBAC: Admin user should be able to delete contacts</a></li>
      <li><a href=https://app.replay.io/recording/e602ba4c-1107-4c83-8996-287787a59d81>RBAC: Should not be able to delete contact as non-admin user</a></li>
      <li><a href=https://app.replay.io/recording/66ab2afc-b902-486a-a343-c29c5aaaa0ba>Smoke test with dev server</a></li>
      <li><a href=https://app.replay.io/recording/122f4e12-6011-40a0-b8f4-66ef41d8c5a5>Smoke test with rw serve</a></li>
      <li><a href=https://app.replay.io/recording/6b1675dd-5a24-4179-9915-5bc438e4dfdf>Loads Cell mocks when Cell is nested in another story</a></li>
      <li><a href=https://app.replay.io/recording/b3029107-25dd-477b-b37b-f1343bfb74c0>Loads Cell Stories</a></li>
      <li><a href=https://app.replay.io/recording/ab0882f0-27f3-48bf-b559-b14f17c6fcd5>Loads MDX Stories</a></li>
      <li><a href=https://app.replay.io/recording/68cbf93e-0850-4f3b-a045-51d62d0a7e42>Mocks current user, and updates UI while dev server is running</a></li>
      

View test run on Replay ↗︎

Copy link
Contributor

@Olyno Olyno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed a problem

docs/docs/how-to/dbauth-passwordless.md Outdated Show resolved Hide resolved
I missed that. Good catch.

Co-authored-by: Olyno <[email protected]>
@thedavidprice
Copy link
Contributor

@jacebenson it looks like you replied to everyone's comments. Are you just waiting for us and/or final review?

@Olyno let me know if your review is approved.

@jacebenson
Copy link
Contributor Author

@thedavidprice I think so. Let me know what else I can do to help move this along.

Removed a double "a".
Copy link
Contributor

@Olyno Olyno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah for me it was good, there was just a small change to make but everything seems good on my side now!

@thedavidprice thedavidprice merged commit 461294b into redwoodjs:main Mar 29, 2023
@redwoodjs-bot redwoodjs-bot bot added this to the next-release milestone Mar 29, 2023
@thedavidprice
Copy link
Contributor

Merged 🚀

Thanks, everyone!

jtoar pushed a commit that referenced this pull request Mar 31, 2023
* passwordless rough

* Updated how-to to reword the intro, use Crypto to gen. number, removed loginToken from SDLs, Added some screenshots, and added a concusion

* Updated intro again

* Update dbauth-passwordless.md

Added extra text around how to generate the users service file.

* Update docs/docs/how-to/dbauth-passwordless.md

👍

Co-authored-by: Olyno <[email protected]>

* Update docs/docs/how-to/dbauth-passwordless.md

This is just how I prefer it, but really it does not matter one way or the other to me.

Co-authored-by: Olyno <[email protected]>

* Updated file based on reviews

* Added import needed for service file

* Removed unused loading/error variables

* replaced send send wth send

* Update docs/docs/how-to/dbauth-passwordless.md

I missed that. Good catch.

Co-authored-by: Olyno <[email protected]>

* Update dbauth-passwordless.md

Removed a double "a".

---------

Co-authored-by: Olyno <[email protected]>
Co-authored-by: Rob Cameron <[email protected]>
Co-authored-by: David Price <[email protected]>
@jtoar jtoar modified the milestones: next-release, v4.5.0 Apr 2, 2023
@jacebenson
Copy link
Contributor Author

jacebenson commented Apr 5, 2023

I see some formatting issues with the post I made mistakes on I'can't seem to resolve the git differences in this branch locally,

I forgot two `` in the third section, and I can't count so there's two ### 7 sections.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:docs This PR only updates docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants