diff --git a/docs/docs.go b/docs/docs.go index 29e3a962..4e48c88d 100644 --- a/docs/docs.go +++ b/docs/docs.go @@ -12081,6 +12081,9 @@ const docTemplate = `{ }, "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 2cc8f130..a075dfe4 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 2f7e3754..b3416a10 100644 --- a/docs/swagger.yaml +++ b/docs/swagger.yaml @@ -2518,6 +2518,8 @@ definitions: 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 9552182e..531d88b4 100644 --- a/internal/controller/user_controller.go +++ b/internal/controller/user_controller.go @@ -279,7 +279,7 @@ func (uc *UserController) UserRegisterByEmail(ctx *gin.Context) { 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 (uc *UserController) UserRegisterByEmail(ctx *gin.Context) { } } -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 e48dc863..00000000 --- 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 1cc1895b..245bb976 100644 --- a/internal/migrations/v34.go +++ b/internal/migrations/v34.go @@ -67,6 +67,8 @@ func backfillRequireEmailVerification(content string) (string, error) { } 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 3cd27873..1d0b27ff 100644 --- a/internal/schema/siteinfo_schema.go +++ b/internal/schema/siteinfo_schema.go @@ -21,7 +21,6 @@ package schema import ( "context" - "encoding/json" "fmt" "net/mail" "net/url" @@ -217,39 +216,13 @@ type SiteUsersReq struct { 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 00000000..e5413f4a --- /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 7d0c55a5..0683a5af 100644 --- a/internal/schema/user_schema.go +++ b/internal/schema/user_schema.go @@ -219,13 +219,13 @@ type UserEmailLoginReq struct { // 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 1d7866f3..d0ebe875 100644 --- a/internal/service/content/user_service.go +++ b/internal/service/content/user_service.go @@ -518,7 +518,7 @@ func (us *UserService) UserRegisterByEmail(ctx context.Context, registerUserInfo 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 @@ type registrationVerificationActions struct { } 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 0d77b383..d77a1c44 100644 --- a/internal/service/content/user_service_test.go +++ b/internal/service/content/user_service_test.go @@ -29,11 +29,11 @@ import ( ) 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 @@ func TestApplyRegistrationVerification(t *testing.T) { 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 @@ func TestApplyRegistrationVerification(t *testing.T) { 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 @@ func TestApplyRegistrationVerification(t *testing.T) { 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 @@ func TestApplyRegistrationVerification(t *testing.T) { 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 7656a140..8b32b722 100644 --- a/internal/service/siteinfo/siteinfo_service.go +++ b/internal/service/siteinfo/siteinfo_service.go @@ -291,17 +291,8 @@ func (s *SiteInfoService) SaveSiteSecurity(ctx context.Context, req *schema.Site // 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 @@ func (s *SiteInfoService) SaveSiteLogin(ctx context.Context, req *schema.SiteLog 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 fc4bbdf7..a3f834a5 100644 --- a/internal/service/siteinfo/siteinfo_service_test.go +++ b/internal/service/siteinfo/siteinfo_service_test.go @@ -28,7 +28,6 @@ import ( "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 @@ import ( 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 @@ func TestSiteInfoService_SaveSiteLoginRequireEmailVerification(t *testing.T) { 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 @@ func TestSiteInfoService_SaveSiteLoginRequireEmailVerification(t *testing.T) { 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_SaveSiteLoginRequireEmailVerification(t *testing.T) { }) } } + +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)) +}