fix(site): require explicit email verification setting Replace OptionalBool with an explicit require_email_verification value for the login settings save request while keeping legacy read defaults intact. Use positive RequireEmailVerification naming through the registration flow and inline the site setting mapping. Add validation, save-path, and registration coverage for the explicit email verification setting.
diff --git a/docs/docs.go b/docs/docs.go index 29e3a96..4e48c88 100644 --- a/docs/docs.go +++ b/docs/docs.go
@@ -12081,6 +12081,9 @@ }, "schema.SiteLoginReq": { "type": "object", + "required": [ + "require_email_verification" + ], "properties": { "allow_email_domains": { "type": "array",
diff --git a/docs/swagger.json b/docs/swagger.json index 2cc8f13..a075dfe 100644 --- a/docs/swagger.json +++ b/docs/swagger.json
@@ -12054,6 +12054,9 @@ }, "schema.SiteLoginReq": { "type": "object", + "required": [ + "require_email_verification" + ], "properties": { "allow_email_domains": { "type": "array",
diff --git a/docs/swagger.yaml b/docs/swagger.yaml index 2f7e375..b3416a1 100644 --- a/docs/swagger.yaml +++ b/docs/swagger.yaml
@@ -2518,6 +2518,8 @@ type: boolean require_email_verification: type: boolean + required: + - require_email_verification type: object schema.SiteLoginResp: properties:
diff --git a/internal/controller/user_controller.go b/internal/controller/user_controller.go index 9552182..531d88b 100644 --- a/internal/controller/user_controller.go +++ b/internal/controller/user_controller.go
@@ -279,7 +279,7 @@ handler.HandleResponse(ctx, errors.BadRequest(reason.EmailIllegalDomainError), nil) return } - applyEmailVerificationSetting(req, siteInfo) + req.RequireEmailVerification = siteInfo.RequireEmailVerification req.IP = ctx.ClientIP() isAdmin := middleware.GetUserIsAdminModerator(ctx) if !isAdmin { @@ -306,10 +306,6 @@ } } -func applyEmailVerificationSetting(req *schema.UserRegisterReq, siteInfo *schema.SiteLoginResp) { - req.SkipEmailVerification = !siteInfo.RequireEmailVerification -} - // UserVerifyEmail godoc // @Summary UserVerifyEmail // @Description UserVerifyEmail
diff --git a/internal/controller/user_controller_test.go b/internal/controller/user_controller_test.go deleted file mode 100644 index e48dc86..0000000 --- a/internal/controller/user_controller_test.go +++ /dev/null
@@ -1,37 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -package controller - -import ( - "testing" - - "github.com/apache/answer/internal/schema" - "github.com/stretchr/testify/assert" -) - -func TestApplyEmailVerificationSetting(t *testing.T) { - req := &schema.UserRegisterReq{} - applyEmailVerificationSetting(req, &schema.SiteLoginResp{RequireEmailVerification: false}) - assert.True(t, req.SkipEmailVerification) - - req = &schema.UserRegisterReq{} - applyEmailVerificationSetting(req, &schema.SiteLoginResp{RequireEmailVerification: true}) - assert.False(t, req.SkipEmailVerification) -}
diff --git a/internal/migrations/v34.go b/internal/migrations/v34.go index 1cc1895..245bb97 100644 --- a/internal/migrations/v34.go +++ b/internal/migrations/v34.go
@@ -67,6 +67,8 @@ } requireEmailVerification, exists := loginConfig["require_email_verification"] + // Legacy configs that predate this setting should keep the safer behavior. + // Treat a missing or null value as requiring email verification. if !exists || bytes.Equal(bytes.TrimSpace(requireEmailVerification), []byte("null")) { loginConfig["require_email_verification"] = json.RawMessage("true") } else {
diff --git a/internal/schema/siteinfo_schema.go b/internal/schema/siteinfo_schema.go index 3cd2787..1d0b27f 100644 --- a/internal/schema/siteinfo_schema.go +++ b/internal/schema/siteinfo_schema.go
@@ -21,7 +21,6 @@ import ( "context" - "encoding/json" "fmt" "net/mail" "net/url" @@ -217,39 +216,13 @@ AllowUpdateLocation bool `json:"allow_update_location"` } -// OptionalBool preserves whether a JSON boolean field was omitted, set to null, -// or set to a concrete boolean value. -type OptionalBool struct { - Set bool `json:"-"` - Null bool `json:"-"` - Value bool `json:"-"` -} - -func (b *OptionalBool) UnmarshalJSON(data []byte) error { - b.Set = true - if string(data) == "null" { - b.Null = true - b.Value = false - return nil - } - b.Null = false - return json.Unmarshal(data, &b.Value) -} - -func (b OptionalBool) MarshalJSON() ([]byte, error) { - if !b.Set || b.Null { - return []byte("null"), nil - } - return json.Marshal(b.Value) -} - // SiteLoginReq site login request type SiteLoginReq struct { - AllowNewRegistrations bool `json:"allow_new_registrations"` - AllowEmailRegistrations bool `json:"allow_email_registrations"` - AllowPasswordLogin bool `json:"allow_password_login"` - AllowEmailDomains []string `json:"allow_email_domains"` - RequireEmailVerification OptionalBool `json:"require_email_verification" swaggertype:"boolean"` + AllowNewRegistrations bool `json:"allow_new_registrations"` + AllowEmailRegistrations bool `json:"allow_email_registrations"` + AllowPasswordLogin bool `json:"allow_password_login"` + AllowEmailDomains []string `json:"allow_email_domains"` + RequireEmailVerification *bool `validate:"required" json:"require_email_verification" swaggertype:"boolean"` } // SiteLoginResp site login response
diff --git a/internal/schema/siteinfo_schema_test.go b/internal/schema/siteinfo_schema_test.go new file mode 100644 index 0000000..e5413f4 --- /dev/null +++ b/internal/schema/siteinfo_schema_test.go
@@ -0,0 +1,72 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package schema + +import ( + "encoding/json" + "testing" + + "github.com/apache/answer/internal/base/validator" + "github.com/segmentfault/pacman/i18n" + "github.com/stretchr/testify/require" +) + +func TestSiteLoginReqRequireEmailVerificationValidation(t *testing.T) { + tests := []struct { + name string + payload string + expectError bool + }{ + { + name: "omitted is invalid", + payload: `{"allow_new_registrations":true,"allow_email_registrations":true,"allow_password_login":true,"allow_email_domains":[]}`, + expectError: true, + }, + { + name: "null is invalid", + payload: `{"allow_new_registrations":true,"allow_email_registrations":true,"allow_password_login":true,"allow_email_domains":[],"require_email_verification":null}`, + expectError: true, + }, + { + name: "false is valid", + payload: `{"allow_new_registrations":true,"allow_email_registrations":true,"allow_password_login":true,"allow_email_domains":[],"require_email_verification":false}`, + expectError: false, + }, + { + name: "true is valid", + payload: `{"allow_new_registrations":true,"allow_email_registrations":true,"allow_password_login":true,"allow_email_domains":[],"require_email_verification":true}`, + expectError: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + req := &SiteLoginReq{} + require.NoError(t, json.Unmarshal([]byte(tt.payload), req)) + + _, err := validator.GetValidatorByLang(i18n.DefaultLanguage).Check(req) + if tt.expectError { + require.Error(t, err) + } else { + require.NoError(t, err) + } + }) + } +}
diff --git a/internal/schema/user_schema.go b/internal/schema/user_schema.go index 7d0c55a..0683a5a 100644 --- a/internal/schema/user_schema.go +++ b/internal/schema/user_schema.go
@@ -219,13 +219,13 @@ // UserRegisterReq user register request type UserRegisterReq struct { - Name string `validate:"required,gte=2,lte=30" json:"name"` - Email string `validate:"required,email,gt=0,lte=500" json:"e_mail" ` - Pass string `validate:"required,gte=8,lte=32" json:"pass"` - CaptchaID string `json:"captcha_id"` - CaptchaCode string `json:"captcha_code"` - IP string `json:"-" ` - SkipEmailVerification bool `json:"-"` + Name string `validate:"required,gte=2,lte=30" json:"name"` + Email string `validate:"required,email,gt=0,lte=500" json:"e_mail" ` + Pass string `validate:"required,gte=8,lte=32" json:"pass"` + CaptchaID string `json:"captcha_id"` + CaptchaCode string `json:"captcha_code"` + IP string `json:"-" ` + RequireEmailVerification bool `json:"-"` } func (u *UserRegisterReq) Check() (errFields []*validator.FormErrorField, err error) {
diff --git a/internal/service/content/user_service.go b/internal/service/content/user_service.go index 1d7866f..d0ebe87 100644 --- a/internal/service/content/user_service.go +++ b/internal/service/content/user_service.go
@@ -518,7 +518,7 @@ log.Errorf("set default user notification config failed, err: %v", err) } - err = applyRegistrationVerification(userInfo, registerUserInfo.SkipEmailVerification, registrationVerificationActions{ + err = applyRegistrationVerification(userInfo, registerUserInfo.RequireEmailVerification, registrationVerificationActions{ sendActivationEmail: func() error { return us.sendRegistrationActivationEmail(ctx, userInfo) }, @@ -569,10 +569,10 @@ } func applyRegistrationVerification( - userInfo *entity.User, skipEmailVerification bool, actions registrationVerificationActions, + userInfo *entity.User, requireEmailVerification bool, actions registrationVerificationActions, ) error { userInfo.MailStatus = entity.EmailStatusToBeVerified - if !skipEmailVerification { + if requireEmailVerification { return actions.sendActivationEmail() }
diff --git a/internal/service/content/user_service_test.go b/internal/service/content/user_service_test.go index 0d77b38..d77a1c4 100644 --- a/internal/service/content/user_service_test.go +++ b/internal/service/content/user_service_test.go
@@ -29,11 +29,11 @@ ) func TestApplyRegistrationVerification(t *testing.T) { - t.Run("enabled sends activation email and leaves user inactive", func(t *testing.T) { + t.Run("required sends activation email and leaves email pending", func(t *testing.T) { userInfo := &entity.User{} calls := map[string]int{} - err := applyRegistrationVerification(userInfo, false, registrationVerificationActions{ + err := applyRegistrationVerification(userInfo, true, registrationVerificationActions{ sendActivationEmail: func() error { calls["sendActivationEmail"]++ return nil @@ -55,11 +55,11 @@ assert.Zero(t, calls["markEmailAvailable"]) }) - t.Run("disabled activates once and marks email available", func(t *testing.T) { + t.Run("not required activates once and marks email available", func(t *testing.T) { userInfo := &entity.User{} calls := map[string]int{} - err := applyRegistrationVerification(userInfo, true, registrationVerificationActions{ + err := applyRegistrationVerification(userInfo, false, registrationVerificationActions{ sendActivationEmail: func() error { calls["sendActivationEmail"]++ return nil @@ -81,11 +81,11 @@ assert.Equal(t, 1, calls["markEmailAvailable"]) }) - t.Run("disabled user activation failure falls back to email verification", func(t *testing.T) { + t.Run("not required user activation failure falls back to email verification", func(t *testing.T) { userInfo := &entity.User{} calls := map[string]int{} - err := applyRegistrationVerification(userInfo, true, registrationVerificationActions{ + err := applyRegistrationVerification(userInfo, false, registrationVerificationActions{ sendActivationEmail: func() error { calls["sendActivationEmail"]++ return nil @@ -107,11 +107,11 @@ assert.Zero(t, calls["markEmailAvailable"]) }) - t.Run("disabled email status failure falls back to email verification", func(t *testing.T) { + t.Run("not required email status failure falls back to email verification", func(t *testing.T) { userInfo := &entity.User{} calls := map[string]int{} - err := applyRegistrationVerification(userInfo, true, registrationVerificationActions{ + err := applyRegistrationVerification(userInfo, false, registrationVerificationActions{ sendActivationEmail: func() error { calls["sendActivationEmail"]++ return nil @@ -133,11 +133,11 @@ assert.Equal(t, 1, calls["markEmailAvailable"]) }) - t.Run("fallback email failure returns before active status", func(t *testing.T) { + t.Run("fallback email failure returns before available email status", func(t *testing.T) { userInfo := &entity.User{} expectedErr := errors.New("email failed") - err := applyRegistrationVerification(userInfo, true, registrationVerificationActions{ + err := applyRegistrationVerification(userInfo, false, registrationVerificationActions{ sendActivationEmail: func() error { return expectedErr },
diff --git a/internal/service/siteinfo/siteinfo_service.go b/internal/service/siteinfo/siteinfo_service.go index 7656a14..8b32b72 100644 --- a/internal/service/siteinfo/siteinfo_service.go +++ b/internal/service/siteinfo/siteinfo_service.go
@@ -291,17 +291,8 @@ // SaveSiteLogin save site legal configuration func (s *SiteInfoService) SaveSiteLogin(ctx context.Context, req *schema.SiteLoginReq) (err error) { - requireEmailVerification := true - if req.RequireEmailVerification.Set { - if !req.RequireEmailVerification.Null { - requireEmailVerification = req.RequireEmailVerification.Value - } - } else { - currentLogin, err := s.GetSiteLogin(ctx) - if err != nil { - return err - } - requireEmailVerification = currentLogin.RequireEmailVerification + if req.RequireEmailVerification == nil { + return errors.BadRequest(reason.RequestFormatError) } loginConfig := &schema.SiteLoginResp{ @@ -309,7 +300,7 @@ AllowEmailRegistrations: req.AllowEmailRegistrations, AllowPasswordLogin: req.AllowPasswordLogin, AllowEmailDomains: req.AllowEmailDomains, - RequireEmailVerification: requireEmailVerification, + RequireEmailVerification: *req.RequireEmailVerification, } content, _ := json.Marshal(loginConfig) data := &entity.SiteInfo{
diff --git a/internal/service/siteinfo/siteinfo_service_test.go b/internal/service/siteinfo/siteinfo_service_test.go index fc4bbdf..a3f834a 100644 --- a/internal/service/siteinfo/siteinfo_service_test.go +++ b/internal/service/siteinfo/siteinfo_service_test.go
@@ -28,7 +28,6 @@ "github.com/apache/answer/internal/entity" "github.com/apache/answer/internal/schema" "github.com/apache/answer/internal/service/mock" - "github.com/apache/answer/internal/service/siteinfo_common" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" @@ -37,33 +36,17 @@ func TestSiteInfoService_SaveSiteLoginRequireEmailVerification(t *testing.T) { tests := []struct { name string - currentContent string - requestPayload string - expectGet bool + requireEmail bool expectedRequire bool }{ { - name: "omitted preserves normalized default", - currentContent: `{"allow_new_registrations":true,"allow_email_registrations":true,"allow_password_login":true}`, - requestPayload: `{"allow_new_registrations":true,"allow_email_registrations":true,"allow_password_login":true,"allow_email_domains":[]}`, - expectGet: true, - expectedRequire: true, - }, - { - name: "omitted preserves current false", - currentContent: `{"allow_new_registrations":true,"allow_email_registrations":true,"allow_password_login":true,"require_email_verification":false}`, - requestPayload: `{"allow_new_registrations":true,"allow_email_registrations":true,"allow_password_login":true,"allow_email_domains":[]}`, - expectGet: true, - expectedRequire: false, - }, - { - name: "null normalizes true", - requestPayload: `{"allow_new_registrations":true,"allow_email_registrations":true,"allow_password_login":true,"allow_email_domains":[],"require_email_verification":null}`, + name: "explicit true persists true", + requireEmail: true, expectedRequire: true, }, { name: "explicit false persists false", - requestPayload: `{"allow_new_registrations":true,"allow_email_registrations":true,"allow_password_login":true,"allow_email_domains":[],"require_email_verification":false}`, + requireEmail: false, expectedRequire: false, }, } @@ -74,11 +57,6 @@ defer ctl.Finish() repo := mock.NewMockSiteInfoRepo(ctl) - if tt.expectGet { - repo.EXPECT().GetByType(gomock.Any(), constant.SiteTypeLogin). - Return(&entity.SiteInfo{Content: tt.currentContent}, true, nil) - } - var savedContent string repo.EXPECT().SaveByType(gomock.Any(), constant.SiteTypeLogin, gomock.Any()). DoAndReturn(func(_ context.Context, _ string, data *entity.SiteInfo) error { @@ -86,12 +64,15 @@ return nil }) - req := &schema.SiteLoginReq{} - require.NoError(t, json.Unmarshal([]byte(tt.requestPayload), req)) - service := &SiteInfoService{ - siteInfoRepo: repo, - siteInfoCommonService: siteinfo_common.NewSiteInfoCommonService(repo), + siteInfoRepo: repo, + } + req := &schema.SiteLoginReq{ + AllowNewRegistrations: true, + AllowEmailRegistrations: true, + AllowPasswordLogin: true, + AllowEmailDomains: []string{}, + RequireEmailVerification: &tt.requireEmail, } require.NoError(t, service.SaveSiteLogin(context.TODO(), req)) assert.NotContains(t, savedContent, `"require_email_verification":null`) @@ -102,3 +83,15 @@ }) } } + +func TestSiteInfoService_SaveSiteLoginRequiresEmailVerificationValue(t *testing.T) { + service := &SiteInfoService{} + req := &schema.SiteLoginReq{ + AllowNewRegistrations: true, + AllowEmailRegistrations: true, + AllowPasswordLogin: true, + AllowEmailDomains: []string{}, + } + + require.Error(t, service.SaveSiteLogin(context.TODO(), req)) +}