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

feat(authentication,config): remove Authentication's dependency on Email for unverified local email auth strategies #103

Merged
merged 9 commits into from
Apr 4, 2022

Conversation

kon14
Copy link
Contributor

@kon14 kon14 commented Mar 30, 2022

This PR greatly improves the experience of setting up a new deployment from scratch by removing Authentication`s Email dependency on a fresh database. Fixes #99.

This change is going to be extremely useful for newcomers trying out Conduit for the first time as the docs can now be simplified in that regard.
As far as backend contributors are concerned, it's also going to significantly reduce the amount of time spent on submitting configuration requests just to get an active local auth post db wipes.

The Authentication's configuration schema's local section has also been slightly altered so as to keep things clean and structured.
Check out the relevant changes below:

Routes depending on Email now only get registered if Email is online (previously threw 404 in handlers instead).
Auth reregisters its routes on email activation.
This is currently broken due to #104, but I'm opening a follow-up PR to address this so as to keep this one clean.

Also fixed VerifyChangePassword route being registered without 2FA.

Old:

local: {
  identifier: string,                  // removed
  enabled: boolean,
  sendVerificationEmail: boolean,      // changed
  verificationRequired: boolean,       // changed
  verification_redirect_uri: string,   // changed
  forgot_password_redirect_uri: string,
},

New:

local: {
  enabled: boolean,
  verification: {
    required:boolean,
    send_email: boolean,
    redirect_uri: string,
  },
  forgot_password_redirect_uri: string,
},

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change?

  • Yes
  • No

Local authentication identifier has been deprecated:
POST /admin/authentication/users body param 'identifier' -> 'email'

The PR fulfills these requirements:

  • It's submitted to the main branch
  • When resolving a specific issue, it's referenced in the PR's title

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

@kon14 kon14 marked this pull request as draft March 30, 2022 16:21
@kon14
Copy link
Contributor Author

kon14 commented Mar 30, 2022

On second thought, going to improve on this prior to merge in the following ways:

  • Move the migration to Authentication so as to remove the need for guided update
  • Phase out redundant "username" local auth identifier (breaking change, no handler-level compat)
  • Add a configurable option for allowing/preventing user self-registration
  • Convert config fields to camel_case (we should standardize this across our configs)

@kon14 kon14 marked this pull request as ready for review March 31, 2022 13:20
@kon14
Copy link
Contributor Author

kon14 commented Apr 1, 2022

This PR also introduces proper character validation for user email fields.

@kkopanidis kkopanidis merged commit 9574848 into main Apr 4, 2022
@kkopanidis kkopanidis deleted the auth-email-dep branch April 4, 2022 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEAT] Authentication module's initial run should not depend on Email or further configuration
4 participants