[ Refactor ] Explicit CRUD Operations and Expiration Integration #37

mw10013 · 2023-12-22T21:12:06Z

Closes #35 [ Refactor ] Decomplect handleTOTP() API

This refactor addresses the application API for totp data storage by

Making the CRUD operations explicit
- Separating handleTOTP() into readTOTP() and updateTOTP().
- Renaming storeTOTP() to createTOTP().
Adding expiresAt to createTOTP() and updateTOTP()

Totp data is ephemeral since it expires after some time. Session data is also ephemeral and this PR mimics Remix's API for session storage (https://remix.run/docs/en/main/utils/sessions#createsessionstorage).

Conveying expiresAt during create and update simplifies implementation using key-value stores where expiresAt may be metadata.

@dev-xo: Please let me know if you want to move forward with this PR and I'll incorporate any feedback, update README, and add migration notes since breaking change. It would also be helpful to have a dev version in npm for further testing with totp-starter-example and remix-auth-totp-cloudflare-example. And I'm open to learning how to deploy dev versions.

dev-xo · 2023-12-22T22:11:22Z

I will look into this as soon as I get half an hour or so to deeply check the implementation, @mw10013! I'm not sure if it will be today or tomorrow, but I promise I will look into it!

Also, I will invite you to remix-auth-totp-dev, where we could push releases to the dev-npm package, in case you want to test a few things. (I think it's not set up yet, I will need to look into its github/workflows.)

Otherwise, feel free to create a npm package and call it whatever you want, in case you want to test things on your own. Until I get the time to properly look into this and set up the github/workflows for pushing to npm on the dev repository.

Once again, thank you so much for the time and effort you are putting into this!!

dev-xo

Fantastic work, @mw10013! A similar approach has been taken by Kent C. Dodds on the Epic Stack with the @epic-web/totp package.

In this case, I wanted to keep the extra logic for us, the maintainers, and give the developers a simpler API: handleTotp, which was managing the read and update, if I'm not mistaken.

Simplifying it is a great choice and one that should have been made from the start. In any case, I'm happy to be at this point.

I will have to review the code on my editor, to ensure that Prettier is satisfied, imports are well situated, and some minor details are addressed, which are purely personal preferences. As the project grows and more people start handling it, these preferences will gradually disappear.

A migration guide would be great, and any documentation updates as well. I will also revisit them, so as long as you add the basics or whatever you think the end user needs to understand, that will be great.

I usually have the help of ChatGPT for docs, which can simplify them and make our writing easier to understand for everyone, so don't worry too much about the docs.

I will approve this and will wait for you to comment back on anything you think is missing or to move on to the next steps.

Thank you so much for this fantastic work!

dev-xo · 2023-12-23T20:18:22Z

src/index.ts

@@ -12,6 +12,29 @@ import {
  verifyJWT,
 } from './utils.js'
 import { STRATEGY_NAME, FORM_FIELDS, SESSION_KEYS, ERRORS } from './constants.js'
+import { CreateContextOptions } from 'vm'


It is CreateContextOptions used? ESLint indicates that it is defined but never used.

I'm at the beginning of the review, so I'm not sure yet, but I'm leaving this comment here in case we need to revisit it later.

Should be removed and will do so in the revision to this PR

dev-xo · 2023-12-23T20:22:50Z

src/index.ts

-   */
-  attempts: number
+export interface CreateTOTP {
+  (data: TOTPData, expiresAt: Date): Promise<void>


So, should expiresAt be a Date, and only a Date?
Could this be an inconvenience for some databases, or should it be suitable for most of them?

I haven't reached the implementation yet; I'm simply commenting on things I find interesting and that could be great for discussion.

Date should be suitable since it includes time. Note that an ORM like Prisma has way to map Date's to database types and vice versa.

https://www.prisma.io/docs/orm/reference/prisma-schema-reference#datetimeDateTime

Remarks
Prisma Client returns all DateTime as native Date objects.

Connector Default mapping
PostgreSQL timestamp(3)
SQL Server datetime2
MySQL DATETIME(3)
MongoDB Timestamp
SQLite NUMERIC
CockroachDB TIMESTAMP

dev-xo · 2023-12-23T20:58:27Z

src/index.ts

-  private async _storeTOTP(totp: StoreTOTPOptions) {
-    await this.storeTOTP(totp)
-  }
-
  private async _sendTOTP(data: SendTOTPOptions) {


Some private _methodName simply does what the original method passed to the strategy does. I encapsulated them into private _methodName in case we need to handle specific logic in the future, but we could remove some private _method names if they do not add value.

I added a few private _methodName methods that were not handling any extra logic, simply for consistency. I think I will end up refactoring this and simply call the original method passed to the strategy, unless we have to handle any extra logic, in which case it will be encapsulated as private.

I'll keep it like this for now, but revisiting it in the future would certainly be great.

I'm going to remove it in the next revision to this PR.

dev-xo · 2023-12-23T21:03:19Z

src/index.ts

      throw new Error(this.customErrors.invalidTotp)
    }
  }
-
-  private async _handleExpiresAt(


Happy to have this out of the code; it was somewhat of a headache to come up with a solution that could be beneficial in most cases, based on the provided user database.

I'm not sure if it was the best implementation, but it did its job in the meantime.

dev-xo · 2023-12-23T21:12:15Z

test/index.spec.ts


      const request = new Request(`${HOST_URL}`, {
        method: 'POST',
        headers: {
          cookie: await sessionStorage.commitSession(session),
        },
-        body: formData,
+        body: new FormData(), // Empty form data indicates re-send new TOTP


It's me who's learning, @mw10013! I'm happy about it!

dev-xo · 2023-12-23T21:15:27Z

test/index.spec.ts

@@ -723,6 +842,116 @@ describe('[ TOTP ]', () => {
      expect(session.data.user.name).toBe('John Doe')
    })
  })
+
+  describe('End to End', () => {


Love this! Also we required an End to End test that could help other developers understand the flow of the strategy just by looking at it!

dev-xo · 2023-12-23T21:19:52Z

test/index.spec.ts

@@ -14,14 +14,16 @@ import {
  DEFAULT_EMAIL,
  sessionStorage,
 } from './utils'
+import { Session } from '@remix-run/server-runtime'


Is Session used only as a type? If so, it should be imported as a type, and I think it wasn't. No worries though, I will look into it right now.

Also, are SendTOTPOptions, TOTPData, and TOTPStrategy used as types too, right?

I think they were not imported as types before, right? In that case, it would be my mistake, or I'm probably being wrong about it. (I'll have to revisit the code in my editor, that would help me a bit).

Session, SendTOTPOptions, and TOTPData are types and I'll import them as such in the next revision. TOTPStrategy is a class.

mw10013 · 2023-12-24T01:31:45Z

@dev-xo: Revised PR based on your review, update docs, and added migration guide. Please review and let me know how you want to proceed.

dev-xo

Lovely! Thank you so much for all the care you've put into this!

dev-xo

Amazing!

dev-xo · 2023-12-24T02:11:57Z

This was a fantastic rework, @mw10013! You also made it look so simple. I want to review the project in my editor, and after that, I think we could push the npm package to v2, create the Release, etc.

Not sure if you had the chance to test it on any of the templates. If you are up for it, I could push remix-auto-totp-dev to v2 on npm, and we could also test that everything works as expected on the templates (it should, tests pass).

Let me know about that.

mw10013 · 2023-12-24T03:03:22Z

@dev-xo: If you are able to push remix-auto-totp-dev, I'll update and test totp-starter-example and remix-auth-totp-cloudflare-example.

dev-xo · 2023-12-24T03:08:14Z

Sounds good to me, @mw10013! I will. I also need to update the other templates and the main totp.fly.dev website repository, but that runs on my side, so there's nothing to worry about. You've done more than enough already!

Mostly personal preferences for imports, comments, etc. Nothing important.

dev-xo · 2023-12-24T08:01:46Z

v2 published on remix-auth-totp-dev @mw10013 in case you want to give it a try on the templates! Also, this PR is ready to be merged too.

Not sure if we have to discuss anything more, but I will wait a bit in case you want to try the package on the starter template.

P.S: I was trying to reply to you on Discord, but I'm getting this message: "Your message could not be delivered. This is usually because you don't share a server with the recipient or the recipient is only accepting direct messages from friends."

I think we both share the Remix server, and I also have you added as a friend.
Who knows what could be going on.

Again, thanks a lot for this fantastic work!

mw10013 · 2023-12-24T20:12:17Z

@dev-xo: I've added more tweaks to the docs in this PR based on testing remix-auth-totp-dev. totp-starter-example and remix-auth-totp-cloudflare-example both have next branches that work with remix-auth-totp-dev. v2 is ready to go live from my perspective.

dev-xo · 2023-12-24T21:43:58Z

README.md

+        try {
+          // Delete expired TOTP records.
+          // Better if this were in scheduled task.
+          await prisma.totp.deleteMany({ where: { expiresAt: { lt: new Date() } } })


Uh, I'm not sure about this. If a database contains a large number of expired TOTPs, this step could take more time than necessary; the authentication phase will feel slow.. Also, if the removal of expired TOTPs fails, the authentication flow will fail, or am I wrong about this? (I'm probably wrong. The creation of TOTP is handled beforehand, although I still think this could be implemented somehow by the user.)

Should we document this somewhere else, instead of adding it directly to the authentication/creation of TOTP phase? Although it could work, I'm not sure if it would be the best move.

Another option could be to remove the await from the deleteMany call, right? The database would still handle that without blocking the authentication flow. Anyway, I think it's okay for now, and it could also provide a hint to other users on how to delete expired TOTPs.

For someone who knows what they are doing and are building an application to scale. They would never do this and would put it in a scheduled task or some other mechanism outside of the createTOTP.

For the beginner or someone who is in a hurry just to get something working and doesn't need to scale, this gets them going without filling up their database with expired TOTP's. With an index on expiresAt, this should not be an expensive operation especially at small scale.

The try/catch around it should prevent any delete errors from causing createTOTP to fail. Removing the await is very interesting idea.

prisma.totp.deleteMany({where: { expiresAt: { lt: new Date() } } }).catch((reason) => console.warning(reason))

The code starts to look a little more obscure to the beginner who may be more comfortable with await.

Based on your feedback, I think we should remove expiresAt from the README code and have an expiresAt section in Customization. I can make that change.

Yes, I think you are absolutely right. For the beginner or someone in a hurry, this is just fine, and it also gives the idea that it could be handled differently later at some point.

Nothing to change here, everything is just as good as it can be, @mw10013, and you thought about it very well and carefully!

Also, let's keep the await call in there, it's the most common way to call async functions and removing it could be somewhat a visual disruption for folks in a hurry to learn the basics of the Strategy.

Alright, with everything looking this good, I will look into creating the release on GitHub.

As I said before, a million thanks for the amazing work you've been doing, testing, etc.
Merry Christmas to you!

dev-xo · 2023-12-24T21:51:06Z

Amazing, so V2 is ready! I will do the required steps later today, and by tomorrow we should have it operational. Again, many thanks for the mind-blowing efforts and the time invested in this, @mw10013!

Merry Christmas to you!

mw10013 added 5 commits December 21, 2023 15:38

add TOTPData and CRUD interfaces

0dba3ed

remove storeTOTP

abe5bcc

test: should authenticate user with valid TOTP

40facfc

remove handleTOTP

22c2095

cleanup

5c11228

mw10013 requested a review from dev-xo December 22, 2023 21:12

dev-xo mentioned this pull request Dec 23, 2023

[ Request ] Allow the flow to continue when emails are invalid. #36

Closed

dev-xo added the enhancement New feature or request label Dec 23, 2023

dev-xo approved these changes Dec 23, 2023

View reviewed changes

mw10013 added 6 commits December 23, 2023 19:02

remove unused import

8adb665

remove sendTOTP() wrapper

e2a3b65

import types as types

f379d90

Update readme

7bc4e0e

Update customization doc

06c8bb2

Doc fixes

8954f44

dev-xo reviewed Dec 24, 2023

View reviewed changes

dev-xo added 2 commits December 24, 2023 08:35

docs: update migrations

c8a7047

chore: minor refactors

a0abdd7

Mostly personal preferences for imports, comments, etc. Nothing important.

dev-xo changed the title ~~CRUD with expires~~ [ Feat ] Explicit CRUD Operations and Expiration Integration Dec 24, 2023

mw10013 added 2 commits December 24, 2023 15:00

Tweak docs

a70c911

migration doc tweaks

ff08e7a

dev-xo reviewed Dec 24, 2023

View reviewed changes

dev-xo changed the title ~~[ Feat ] Explicit CRUD Operations and Expiration Integration~~ [ Refactor ] Explicit CRUD Operations and Expiration Integration Dec 25, 2023

dev-xo merged commit 681f057 into dev-xo:main Dec 25, 2023
4 checks passed

mw10013 deleted the crud-with-expires branch December 27, 2023 20:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ Refactor ] Explicit CRUD Operations and Expiration Integration #37

[ Refactor ] Explicit CRUD Operations and Expiration Integration #37

mw10013 commented Dec 22, 2023

dev-xo commented Dec 22, 2023 •

edited

dev-xo left a comment

dev-xo Dec 23, 2023

mw10013 Dec 23, 2023

dev-xo Dec 23, 2023

mw10013 Dec 23, 2023

dev-xo Dec 23, 2023

mw10013 Dec 24, 2023

dev-xo Dec 23, 2023

dev-xo Dec 23, 2023

dev-xo Dec 23, 2023 •

edited

dev-xo Dec 23, 2023

mw10013 Dec 24, 2023

mw10013 commented Dec 24, 2023

dev-xo left a comment

dev-xo left a comment

dev-xo commented Dec 24, 2023

mw10013 commented Dec 24, 2023 •

edited

dev-xo commented Dec 24, 2023

dev-xo commented Dec 24, 2023 •

edited

mw10013 commented Dec 24, 2023

dev-xo Dec 24, 2023 •

edited

mw10013 Dec 24, 2023

dev-xo Dec 25, 2023 •

edited

dev-xo commented Dec 24, 2023

[ Refactor ] Explicit CRUD Operations and Expiration Integration #37

[ Refactor ] Explicit CRUD Operations and Expiration Integration #37

Conversation

mw10013 commented Dec 22, 2023

dev-xo commented Dec 22, 2023 • edited

dev-xo 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dev-xo Dec 23, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mw10013 commented Dec 24, 2023

dev-xo left a comment

Choose a reason for hiding this comment

dev-xo left a comment

Choose a reason for hiding this comment

dev-xo commented Dec 24, 2023

mw10013 commented Dec 24, 2023 • edited

dev-xo commented Dec 24, 2023

dev-xo commented Dec 24, 2023 • edited

mw10013 commented Dec 24, 2023

dev-xo Dec 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dev-xo Dec 25, 2023 • edited

Choose a reason for hiding this comment

dev-xo commented Dec 24, 2023

dev-xo commented Dec 22, 2023 •

edited

dev-xo Dec 23, 2023 •

edited

mw10013 commented Dec 24, 2023 •

edited

dev-xo commented Dec 24, 2023 •

edited

dev-xo Dec 24, 2023 •

edited

dev-xo Dec 25, 2023 •

edited