Skip to content

feat(site): allow disabling email verification#1540

Open
AsyncAssassin wants to merge 1 commit into
apache:devfrom
AsyncAssassin:feat/1517-disable-email-verification
Open

feat(site): allow disabling email verification#1540
AsyncAssassin wants to merge 1 commit into
apache:devfrom
AsyncAssassin:feat/1517-disable-email-verification

Conversation

@AsyncAssassin
Copy link
Copy Markdown

Fixes #1517

Proposed Changes

  • Add a require_email_verification login setting that defaults to true to preserve the current behavior.
  • Expose the setting through the admin login settings API and UI.
  • Allow newly registered email users to become active immediately when email verification is disabled.
  • Keep existing unverified users unchanged.
  • Add migration and regression tests for legacy login settings and registration behavior.

Testing

  • go test ./internal/migrations -run 'TestBackfillRequireEmailVerification|TestAddRequireEmailVerification|TestSplitLegalMenuKeepsRequireEmailVerificationDefaultTrue' -count=1 -v
  • go test ./...
  • make lint
  • make ui
  • make build

Copy link
Copy Markdown
Member

@LinkinStars LinkinStars left a comment

Choose a reason for hiding this comment

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

The compatibility and default-safe behavior in this change look solid, and I agree with the overall direction. That said, I think there are still a few backend design / maintainability issues worth revisiting before merge:

  1. OptionalBool feels too heavy for this use case. To handle the omitted / null / false cases for require_email_verification, the PR introduces a dedicated OptionalBool type in the schema layer. The issue is that this API does not read like a clearly defined patch-style endpoint; it looks more like a normal save/update API. Adding a special tri-state wrapper for a single field increases the cognitive load in both the schema and save logic.
    If this is intended to be a full save API, I would prefer requiring the client to send the full field set. If this is intended to be a partial update API, then it would be better to make that explicit and handle partial-update semantics consistently rather than special-casing one field.

  2. Treating null as true is safe, but not very intuitive semantically.
    In the current implementation, both the migration path and the runtime save path normalize null to true. I understand why this was done from a security/default-safety perspective, but from an API semantics perspective, null usually reads more like “unset” than “explicitly enabled”. If we want to keep this behavior, I think it should at least be documented more explicitly in code/comments; otherwise it is not obvious to future maintainers why null does not preserve the current value, fail validation, or behave like an omitted field.

  3. applyEmailVerificationSetting is a very thin abstraction. This helper currently does little more than req.SkipEmailVerification = !siteInfo.RequireEmailVerification. Extracting that into a separate function adds another jump for the reader without really encapsulating meaningful logic yet. Unless we expect more registration policy logic to live there soon, I think keeping this mapping inline at the call site would actually be clearer.

  4. The naming mixes a positive configuration flag with a negative execution flag. At the config level we have RequireEmailVerification, while at the request/service level we introduce SkipEmailVerification, which means we have to invert the meaning in between. This works, but it hurts readability and makes future changes easier to get wrong. I would strongly prefer keeping the naming direction consistent across layers, for example using RequireEmailVerification end-to-end, or using a single enabled/disabled wording consistently.

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.

2 participants