Provide a possibility to disable flows in dbAuth completely #5851

Morishiri · 2022-06-29T14:46:26Z

netlify · 2022-06-29T14:46:34Z

✅ Deploy Preview for redwoodjs-docs ready!

Name	Link
🔨 Latest commit	`03c44ba`
🔍 Latest deploy log	https://app.netlify.com/sites/redwoodjs-docs/deploys/62d9b84c7e43120008f35d8e
😎 Deploy Preview	https://deploy-preview-5851--redwoodjs-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

packages/api/src/functions/dbAuth/DbAuthHandler.ts

jtoar · 2022-06-30T10:45:54Z

Thanks @Morishiri! @cannikin tagging you here, cause dbAuth!

dthyresson

@cannikin and @Morishiri does any documentation need updating? Perhaps in the configuration section?

cannikin · 2022-06-30T20:59:03Z

Awesome! Thanks for this @Morishiri ! As @dthyresson mentioned, the only thing missing is updated docs about these new options.

dac09 · 2022-07-01T16:07:25Z

I wanted to throw in a little curve ball here, I wonder why we're using the enabled flag, and not just a boolean?

I believe the desired behavior is that either:
a) We supply handler options
OR
b) We disable that handler all together

Given this, wouldn't it make more sense to have it in one of these forms?

// a little confusing, because why are options false?
const signUpHandlerOptions = false

// or maybe, where none of the other properties are supplied
const signUpHandlerOptions = {enabled: false}

I think we should throw if enabled is false, and other options are supplied here (and also updated the types to reflect this)

Just my 2p

cannikin · 2022-07-01T17:26:08Z

Hmm, isn't that what @Morishiri did in the PR? There's a new enabled key in the various option blocks (loginOptions, etc.) and you can set it to false there, skipping the rest of the options.

dac09 · 2022-07-01T18:08:57Z

false there, skipping the rest of the options.

I think it’s close, but:
a) the types suggest that you should still set the other properties - it should be enabled false OR the other options
b) I think we should throw at runtime if enabled false is supplied with other options, just to prevent hard to trace down bugs - I can already imagine “I supplied all these options, but it’s not working” issues

cannikin · 2022-07-01T18:13:36Z

Personally, I'm fine with letting you keep all of the options and then just flip a single boolean on/off if you want to enable/disable. Rather than deleting or commenting out all of the options AND setting it to false, and then when you want to turn it back on again you need to revert commits or uncomment or manually add all your options back in again.

It's like in my house if I need to work on the wiring in a room: I can flip off the circuit breaker to that room and everything is disabled. I don't need to also go around and unplug everything and unscrew every light bulb! ⚡️

Although for those people that like types, I agree that if you do want enabled: false then you shouldn't have to include the other options. Like you said, if enabled: true then all other keys are required, otherwise they're not.

dac09 · 2022-07-01T19:00:39Z

I can flip off the circuit breaker to that room

Hmmm yeah that makes sense - maybe we need to support a bit of both. Going by that analogy, we shouldn't need to plug in all your bulbs just to switch off the circuit breaker.

So I guess the type should look something like:

type DbAuthHandlerXOptions = OptionsWithEnabledFlag | {enabled: false}

That way you can just disable the handler with {enabled: false} and no other properties, but also have enabled just as a property when you supply other options too.

cannikin · 2022-07-01T20:17:44Z

I like that!

jtoar

@Morishiri I noticed a few mistakes in the comments, so I had a pass at improving it slightly.

packages/api/src/functions/dbAuth/DbAuthHandler.ts

Morishiri · 2022-07-16T15:44:45Z

@dac09 @jtoar I think I addressed your comments.

jtoar

Thanks @Morishiri, LGTM, but will let @cannikin give the final ok.

cannikin

Looks great @Morishiri thanks so much! This is a big change, 500 lines!

Morishiri · 2022-07-19T14:42:08Z

@cannikin The WebAuthn implementation got merged in the meantime. I added | {enabled: false} to it's types in the meantime while rebasing to keep in consistent with what is introduced here.

Is that fine?

cannikin · 2022-07-19T17:48:26Z

Hmm, will that cause any issue for people that have no webAuthn config option at all? I want to make sure it’s backwards compatible so that if you use your existing config (which will have no webAuthn key) everything just keeps working.

Morishiri · 2022-07-19T17:51:46Z

Hmm, will that cause any issue for people that have no webAuthn config option at all? I want to make sure it’s backwards compatible so that if you use your existing config (which will have no webAuthn key) everything just keeps working.

I kept question mark in webAuthn?: so it shouldn't cause any issues I think.

cannikin · 2022-07-21T18:08:18Z

HMMM @jtoar Any idea with the E2E and smoke test failures? They both have this error message:

Error: Cannot find module '/tmp/redwood.TUGpWh/node_modules/@graphql-tools/url-loader/cjs/index.js'
    at createEsmNotFoundErr (internal/modules/cjs/loader.js:929:15)
    at finalizeEsmResolution (internal/modules/cjs/loader.js:922:15)
    at resolveExports (internal/modules/cjs/loader.js:450:14)
    at Function.Module._findPath (internal/modules/cjs/loader.js:490:31)
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:888:27)
    at Function.Module._load (internal/modules/cjs/loader.js:746:27)
    at Module.require (internal/modules/cjs/loader.js:974:19)
    at require (internal/modules/cjs/helpers.js:101:18)
    at Object.<anonymous> (/tmp/redwood.TUGpWh/node_modules/graphql-config/index.js:14:19)
    at Module._compile (internal/modules/cjs/loader.js:1085:14) {
  code: 'MODULE_NOT_FOUND',
  path: '/tmp/redwood.TUGpWh/node_modules/@graphql-tools/url-loader/package.json'
}

Pretty sure this PR has nothing to do with GraphQL tools. I'm trying a re-run now...

Doc changes made

nx-cloud · 2022-07-21T20:34:21Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 03c44ba. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

✅ Successfully ran 14 targets

Sent with 💌 from NxCloud.

cannikin · 2022-07-21T20:34:31Z

Nevermind @jtoar a re-run turned green!

Morishiri · 2022-07-21T21:21:49Z

Wohoo 🎉

…ctmode-gen * 'main' of github.com:redwoodjs/redwood: fix(deps): update dependency eslint-plugin-jsx-a11y to v6.6.1 (redwoodjs#6017) chore(deps): update dependency @playwright/test to v1.24.0 (redwoodjs#6019) chore(deps): update dependency @okta/jwt-verifier to v2.6.0 (redwoodjs#6018) fix(storybook): add `args` to auto generate docs (redwoodjs#5979) fix: `yarn rw setup auth clerk` fails in canary (redwoodjs#5998) Provide a possibility to disable flows in dbAuth completely (redwoodjs#5851)

toxsick · 2022-07-28T11:08:35Z

This has not landed in v2.2.0 right?

dac09 · 2022-07-28T11:10:23Z

Hi @toxsick - I don't believe so.

you will be able to see all the features included in each release in the releases tab https://github.com/redwoodjs/redwood/releases

toxsick · 2022-07-28T11:19:35Z

Hey @dac09. thanks for the answer, yeah I looked through that, but I thought it is perhaps just not mentioned there.

Anyway, I was asking because it was planned for this release in milestone v2.2.0, but i guess it didn't make it then :)

Thanks for the awesome work and looking forward to use this change when it lands.

jtoar · 2022-07-28T12:47:14Z

@toxsick my bad, I forgot to update the milestone. This change depended on another change that we had to take out of v2.20, so it didn't make it this time. It'll be in the next release!

Morishiri commented Jun 29, 2022

View reviewed changes

packages/api/src/functions/dbAuth/DbAuthHandler.ts Outdated Show resolved Hide resolved

jtoar added the release:feature This PR introduces a new feature label Jun 30, 2022

dthyresson requested a review from cannikin June 30, 2022 14:39

dthyresson previously requested changes Jun 30, 2022

View reviewed changes

jtoar added the action/add-to-cycle label Jul 1, 2022

redwoodjs-bot bot removed the action/add-to-cycle label Jul 1, 2022

jtoar assigned cannikin Jul 1, 2022

jtoar reviewed Jul 3, 2022

View reviewed changes

Morishiri force-pushed the issue-5792 branch from 1f97119 to f4e22ea Compare July 16, 2022 15:42

jtoar approved these changes Jul 17, 2022

View reviewed changes

cannikin approved these changes Jul 18, 2022

View reviewed changes

Morishiri added 2 commits July 19, 2022 16:40

Provide a possibility to disable flows in dbAuth completely

dfd74e9

Update configuration docs

1296a32

Morishiri force-pushed the issue-5792 branch from b1854ea to 1296a32 Compare July 19, 2022 14:41

Merge branch 'main' into issue-5792

03c44ba

cannikin merged commit b562305 into redwoodjs:main Jul 21, 2022

redwoodjs-bot bot added this to the next-release milestone Jul 21, 2022

jtoar modified the milestones: next-release, v2.2.0 Jul 22, 2022

jtoar modified the milestones: v2.2.0, next-release Jul 28, 2022

jtoar mentioned this pull request Aug 6, 2022

Revert auth provider refactor #6135

Merged

jtoar modified the milestones: next-release, v3.0.0 Sep 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide a possibility to disable flows in dbAuth completely #5851

Provide a possibility to disable flows in dbAuth completely #5851

Morishiri commented Jun 29, 2022

netlify bot commented Jun 29, 2022 •

edited

Loading

jtoar commented Jun 30, 2022

dthyresson left a comment

cannikin commented Jun 30, 2022

dac09 commented Jul 1, 2022

cannikin commented Jul 1, 2022

dac09 commented Jul 1, 2022

cannikin commented Jul 1, 2022 •

edited

Loading

dac09 commented Jul 1, 2022 •

edited

Loading

cannikin commented Jul 1, 2022

jtoar left a comment

Morishiri commented Jul 16, 2022

jtoar left a comment

cannikin left a comment

Morishiri commented Jul 19, 2022

cannikin commented Jul 19, 2022

Morishiri commented Jul 19, 2022

cannikin commented Jul 21, 2022

nx-cloud bot commented Jul 21, 2022 •

edited

Loading

cannikin commented Jul 21, 2022

Morishiri commented Jul 21, 2022

toxsick commented Jul 28, 2022

dac09 commented Jul 28, 2022 •

edited

Loading

toxsick commented Jul 28, 2022

jtoar commented Jul 28, 2022

Provide a possibility to disable flows in dbAuth completely #5851

Provide a possibility to disable flows in dbAuth completely #5851

Conversation

Morishiri commented Jun 29, 2022

netlify bot commented Jun 29, 2022 • edited Loading

✅ Deploy Preview for redwoodjs-docs ready!

jtoar commented Jun 30, 2022

dthyresson left a comment

Choose a reason for hiding this comment

cannikin commented Jun 30, 2022

dac09 commented Jul 1, 2022

cannikin commented Jul 1, 2022

dac09 commented Jul 1, 2022

cannikin commented Jul 1, 2022 • edited Loading

dac09 commented Jul 1, 2022 • edited Loading

cannikin commented Jul 1, 2022

jtoar left a comment

Choose a reason for hiding this comment

Morishiri commented Jul 16, 2022

jtoar left a comment

Choose a reason for hiding this comment

cannikin left a comment

Choose a reason for hiding this comment

Morishiri commented Jul 19, 2022

cannikin commented Jul 19, 2022

Morishiri commented Jul 19, 2022

cannikin commented Jul 21, 2022

nx-cloud bot commented Jul 21, 2022 • edited Loading

☁️ Nx Cloud Report

cannikin commented Jul 21, 2022

Morishiri commented Jul 21, 2022

toxsick commented Jul 28, 2022

dac09 commented Jul 28, 2022 • edited Loading

toxsick commented Jul 28, 2022

jtoar commented Jul 28, 2022

netlify bot commented Jun 29, 2022 •

edited

Loading

cannikin commented Jul 1, 2022 •

edited

Loading

dac09 commented Jul 1, 2022 •

edited

Loading

nx-cloud bot commented Jul 21, 2022 •

edited

Loading

dac09 commented Jul 28, 2022 •

edited

Loading