SMQ-3093 - User email verification #3101

arvindh123 · 2025-08-29T10:19:18Z

What type of PR is this?

What does this do?

Which issue(s) does this PR fix/relate to?

Resolves NOISSUE - Handle callout error #3039

Have you included tests for your changes?

Did you document any new/modified feature?

Notes

pkg/authn/middleware.go

users/postgres/users.go

users/postgres/init.go

users/api/users.go

users/users.go

users/api/requests.go

users/api/users.go

dborovcanin

CI is failing.

codecov · 2025-09-02T09:50:56Z

Codecov Report

❌ Patch coverage is 60.31519% with 277 lines in your changes missing coverage. Please review.
✅ Project coverage is 43.81%. Comparing base (2261691) to head (30bf733).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/sdk/mocks/sdk.go	0.00%	72 Missing ⚠️
users/verification.go	45.90%	22 Missing and 11 partials ⚠️
cli/users.go	0.00%	26 Missing ⚠️
users/events/streams.go	0.00%	26 Missing ⚠️
users/emailer/emailer.go	0.00%	18 Missing ⚠️
users/postgres/users.go	68.51%	12 Missing and 5 partials ⚠️
users/service.go	81.17%	11 Missing and 5 partials ⚠️
users/events/events.go	0.00%	15 Missing ⚠️
pkg/sdk/users.go	0.00%	14 Missing ⚠️
users/postgres/verfications.go	86.51%	8 Missing and 4 partials ⚠️
... and 5 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3101      +/-   ##
==========================================
+ Coverage   35.15%   43.81%   +8.65%     
==========================================
  Files         323      212     -111     
  Lines       47509    35520   -11989     
==========================================
- Hits        16704    15562    -1142     
+ Misses      29977    19206   -10771     
+ Partials      828      752      -76

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

users/README.md

users/postgres/init.go

auth/jwt/tokenizer.go

cmd/users/main.go

pkg/authn/middleware.go

pkg/errors/service/types.go

dborovcanin · 2025-09-02T09:27:01Z

users/service.go


+const (
+	verificationTokenExpiryDuration = 24 * time.Hour
+	verificationTokenSeparator      = "."


Remove if unused.

dborovcanin · 2025-09-02T09:27:57Z

users/service.go

 	"github.com/absmach/supermq/pkg/policies"
 )

+const (


Once you remove unused const, make this inline and rename const verificationTokenDuration.

dborovcanin · 2025-09-02T09:31:05Z

users/service.go

+	if err != nil {
+		if err == repoerr.ErrNotFound {
+			return nil
+		}
+		return err


No need for nested if:

if err == repoerr.ErrNotFound { return nil } if err != nil { return err }

Why do we return nil in case of notFound?

dborovcanin · 2025-09-02T09:36:38Z

users/service.go

 	}

-	token, err := svc.token.Issue(ctx, &grpcTokenV1.IssueReq{UserId: dbUser.ID, UserRole: uint32(dbUser.Role + 1), Type: uint32(smqauth.AccessKey)})
+	token, err := svc.token.Issue(ctx, &grpcTokenV1.IssueReq{UserId: dbUser.ID, UserRole: uint32(dbUser.Role + 1), Type: uint32(smqauth.AccessKey), Verified: dbUser.Verified})


Not related to this PR, but Role+1 is too cryptic.

dborovcanin · 2025-09-02T09:44:36Z

users/service.go

+
+type tokenPayload struct {
+	UserID string `json:"user_id"`
+	Email  string `json:"email"`
+	Token  string `json:"token"`
+}
+
+func generateVerificationToken(userID, email string) (string, error) {
+	randomBytes := make([]byte, 32)
+	if _, err := rand.Read(randomBytes); err != nil {
+		return "", errors.Wrap(errFailedToEncodeToken, err)
+	}
+
+	payload := tokenPayload{
+		UserID: userID,
+		Email:  email,
+		Token:  base64.URLEncoding.EncodeToString(randomBytes),
+	}
+
+	jsonBytes, err := json.Marshal(payload)
+	if err != nil {
+		return "", errors.Wrap(errFailedToEncodeToken, err)
+	}
+
+	return base64.URLEncoding.EncodeToString(jsonBytes), nil
+}
+
+func getUserIDFromToken(token string) (tokenPayload, error) {
+	decodedPayload, err := base64.URLEncoding.DecodeString(token)
+	if err != nil {
+		return tokenPayload{}, errors.Wrap(errFailedToDecodeToken, err)
+	}
+
+	var payload tokenPayload
+	if err := json.Unmarshal(decodedPayload, &payload); err != nil {
+		return tokenPayload{}, errInvalidTokenFormat
+	}
+
+	return payload, nil
+}


Let's move this to a separate file.

dborovcanin · 2025-09-02T09:50:46Z

users/users.go

+	if u.VerificationTokenExpiresAt.IsZero() {
+		return true
+	}


Should we check if verificationToken is empty first?

I will think about this, What we should return if token is empty ?

dborovcanin · 2025-09-02T09:53:55Z

users/users.go

+	UpdateToUserVerificationDetails(ctx context.Context, user User) (User, error)
+


Update instead of UpdateTo.

dborovcanin · 2025-09-02T09:54:09Z

users/users.go


+	UpdateEmail(ctx context.Context, user User) (User, error)
+
+	UpdateRole(ctx context.Context, user User) (User, error)


Add godoc comments.

dborovcanin · 2025-09-02T09:54:31Z

users/users.go


+	SendVerification(ctx context.Context, session authn.Session) error
+
+	VerifyEmail(ctx context.Context, verificationToken string) (User, error)


Same, add godoc.

dborovcanin · 2025-09-02T09:55:28Z

users/service_test.go

 		policyCall := policies.On("AddPolicy", context.Background(), mock.Anything).Return(tc.addPolicyErr)
 		policyCall1 := policies.On("DeletePolicyFilter", context.Background(), mock.Anything).Return(tc.deletePolicyErr)
-		repoCall1 := cRepo.On("Update", context.Background(), mock.Anything, mock.Anything).Return(tc.updateRoleResponse, tc.updateRoleErr)
+		repoCall1 := cRepo.On("UpdateRole", context.Background(), mock.Anything).Return(tc.updateRoleResponse, tc.updateRoleErr)


We are missing tests for the added methods. Either do it now or open a ticket for the follow-up PR, since this one is already large.

dborovcanin · 2025-09-02T10:15:33Z

users/postgres/init.go

+				Id: "clients_07",
+				Up: []string{
+					`ALTER TABLE users
+						ADD COLUMN verified BOOLEAN DEFAULT FALSE,


Do we need this redundancy? Why don't we just use verified_at as nullable?

Yes. We use a verified (boolean) field in JWTs and for other condition checks. Relying only on verified_at would make the logic less clear.

For example, if we only had verified_at, we’d need to write something like:

verified: !verified_at.IsZero()

By keeping a verified field, the code is cleaner and easier to understand.

dborovcanin · 2025-09-02T10:17:06Z

users/api/users.go

+			r.Post("/tokens/refresh", otelhttp.NewHandler(kithttp.NewServer(
+				refreshTokenEndpoint(svc),
+				decodeRefreshToken,
+				api.EncodeResponse,
+				opts...,
+			), "refresh_token").ServeHTTP)
+			r.Patch("/{id}/email", otelhttp.NewHandler(kithttp.NewServer(
+				updateEmailEndpoint(svc),
+				decodeUpdateUserEmail,
+				api.EncodeResponse,
+				opts...,
+			), "update_user_email").ServeHTTP)
+		})


Why is this allowed for unverified user?

Change Email Endpoint:
If a user enters the wrong email address during signup, they must have the ability to correct it. Allowing unverified users to change their email ensures they can complete the verification process successfully.

Refresh Token Endpoint:
The refresh token enables users to request new access tokens regardless of their verification status. Both access and refresh tokens include a verified field. Before verification, this field is false. Once the user completes verification and refreshes their token, the newly issued tokens will correctly reflect verified = true

Change Email Endpoint:
If a user enters the wrong email address during signup, they must have the ability to correct it. Allowing unverified users to change their email ensures they can complete the verification process successfully.

No need -> they can restart the procedure process with the correct email.

Refresh Token Endpoint:
The refresh token enables users to request new access tokens regardless of their verification status. Both access and refresh tokens include a verified field. Before verification, this field is false. Once the user completes verification and refreshes their token, the newly issued tokens will correctly reflect verified = true

Why? Isn't it simpler to disable issuing tokens to who don't have VerifiedAt field?

No need -> they can restart the procedure process with the correct email.

A username cannot be reused until the associated account is deleted.

This ensures usernames can be properly recycled.

It helps prevent the buildup of unwanted or unverified user accounts.

We can provide some basic operation without verification of email
This provides flexibility.

During this basic operation time, If user want to reset password, then they need to verify the email.

felixgateru

dborovcanin · 2025-09-02T15:04:34Z

CI is failing.

dborovcanin · 2025-09-03T09:06:54Z

users/postgres/init.go

+				Down: []string{
+					`ALTER TABLE users
+						DROP COLUMN verified_at,
+						DROP COLUMN verification_token,


Is the token needed at all? Can we put user ID in JWT we send for verification?

dborovcanin · 2025-09-03T09:07:33Z

users/postgres/init.go

+					`ALTER TABLE users
+						DROP COLUMN verified_at,
+						DROP COLUMN verification_token,
+						DROP COLUMN verification_token_expires_at;`,


Is this needed if we are going to use JWT that already contains the expiration?

dborovcanin · 2025-09-04T08:21:35Z

users/postgres/init.go

+						email VARCHAR(254) NOT NULL,
+						otp VARCHAR(255),
+						created_at TIMESTAMPTZ,
+						expiry_at TIMESTAMPTZ,


Let's use expires_at since we used created_at.

dborovcanin · 2025-09-04T08:36:40Z

users/service.go

+		return User{}, errors.Wrap(svcerr.ErrMalformedEntity, err)
+	}
+
+	oguv, err := svc.users.RetrieveUserVerification(ctx, ruv.UserID, ruv.Email)


oguv - Original (OG) User Verification

Both ruvs and oguv are not descriptive names. Please update. Rename ruvs to user; and user to updatedUser or just updated. Rename ruvs to token, and ruv := UserVerification to uv := userVerification. Especially since this does not have to be registration (r) related.

users/service.go

dborovcanin · 2025-09-04T08:41:32Z

users/postgres/users_verfications.go

@@ -0,0 +1,131 @@
+// Copyright (c) Abstract Machines


Let's rename to verifications, no need for users prefix.

dborovcanin

CI is failing.

smithjilks

@arvindh123
I get no token/invalid token when I call sdk verify email method with the token gotten from link query params.

Signed-off-by: Arvindh <arvindh91@gmail.com>

smithjilks

You can update the template so that it says welcome to 'host' instead of 'host/endpoint'

dborovcanin · 2025-09-05T09:10:53Z

users/service.go

+		return User{}, errors.Wrap(svcerr.ErrMalformedEntity, err)
+	}
+
+	oguv, err := svc.users.RetrieveUserVerification(ctx, ruv.UserID, ruv.Email)


Both ruvs and oguv are not descriptive names. Please update. Rename ruvs to user; and user to updatedUser or just updated. Rename ruvs to token, and ruv := UserVerification to uv := userVerification. Especially since this does not have to be registration (r) related.

dborovcanin · 2025-09-05T12:03:48Z

groups/api/http/transport.go


 // MakeHandler returns a HTTP handler for Groups API endpoints.
-func MakeHandler(svc groups.Service, authn authn.Authentication, mux *chi.Mux, logger *slog.Logger, instanceID string, idp supermq.IDProvider) *chi.Mux {
+func MakeHandler(svc groups.Service, authnMW authn.AuthNMiddleware, mux *chi.Mux, logger *slog.Logger, instanceID string, idp supermq.IDProvider) *chi.Mux {


Let's just use authn for this variable.

users/service.go

Signed-off-by: Arvindh <arvindh91@gmail.com>

dborovcanin · 2025-09-05T12:46:13Z

users/service.go

-	var ruv UserVerification
-	if err := ruv.Decode(ruvs); err != nil {
+func (svc service) VerifyEmail(ctx context.Context, token string) (User, error) {
+	var uv UserVerification


Let's use received and stored instead, so we have stored.Match(received).

Signed-off-by: Arvindh <arvindh91@gmail.com>

arvindh123 requested a review from a team as a code owner August 29, 2025 10:19

arvindh123 force-pushed the user-verification branch from bd2650c to a7e5b8a Compare August 29, 2025 10:20

felixgateru requested changes Sep 1, 2025

View reviewed changes

felixgateru reviewed Sep 1, 2025

View reviewed changes

users/api/requests.go Outdated Show resolved Hide resolved

felixgateru requested changes Sep 1, 2025

View reviewed changes

users/api/requests.go Outdated Show resolved Hide resolved

users/api/users.go Outdated Show resolved Hide resolved

users/api/users.go Outdated Show resolved Hide resolved

dborovcanin force-pushed the user-verification branch from 8792374 to c47b711 Compare September 1, 2025 13:04

arvindh123 force-pushed the user-verification branch from c47b711 to 9160a8b Compare September 1, 2025 16:44

dborovcanin force-pushed the user-verification branch from 8565ae5 to 5c0cce2 Compare September 2, 2025 09:03

dborovcanin requested changes Sep 2, 2025

View reviewed changes

arvindh123 force-pushed the user-verification branch 2 times, most recently from 1bdf4a4 to fbcb871 Compare September 2, 2025 09:21

felixgateru requested changes Sep 2, 2025

View reviewed changes

dborovcanin requested changes Sep 2, 2025

View reviewed changes

felixgateru reviewed Sep 2, 2025

View reviewed changes

This was referenced Sep 2, 2025

Not related to this PR, but Role+1 is too cryptic. #3113

Open

Add tests for user registration verification #3114

Open

arvindh123 force-pushed the user-verification branch from cfa419f to e476d42 Compare September 3, 2025 08:40

dborovcanin requested changes Sep 3, 2025

View reviewed changes

dborovcanin force-pushed the user-verification branch from 23fff36 to 01a5310 Compare September 3, 2025 14:31

arvindh123 force-pushed the user-verification branch from 77b1d8a to af725c0 Compare September 3, 2025 15:52

felixgateru previously approved these changes Sep 4, 2025

View reviewed changes

dborovcanin requested changes Sep 4, 2025

View reviewed changes

arvindh123 dismissed felixgateru’s stale review via db2b939 September 4, 2025 11:03

arvindh123 force-pushed the user-verification branch from d10bd83 to fdca173 Compare September 4, 2025 11:06

dborovcanin force-pushed the user-verification branch from 60a2907 to 9083ba0 Compare September 4, 2025 17:56

dborovcanin requested changes Sep 4, 2025

View reviewed changes

smithjilks requested changes Sep 5, 2025

View reviewed changes

arvindh123 added 14 commits September 5, 2025 13:38

fix make user email unverified on email update

7822b1a

Signed-off-by: Arvindh <arvindh91@gmail.com>

add AuthNMiddleware to all service

a6f0021

Signed-off-by: Arvindh <arvindh91@gmail.com>

add allow unverified env to composes

35e86e9

Signed-off-by: Arvindh <arvindh91@gmail.com>

remove verified reused verifiedAt

06b618e

Signed-off-by: Arvindh <arvindh91@gmail.com>

remove lints

a186afd

Signed-off-by: Arvindh <arvindh91@gmail.com>

fix test

86df85b

Signed-off-by: Arvindh <arvindh91@gmail.com>

fix test

2e40ee0

Signed-off-by: Arvindh <arvindh91@gmail.com>

update user verification backend logic

fbc34d9

Signed-off-by: Arvindh <arvindh91@gmail.com>

fix user verification backend logic

d18256b

Signed-off-by: Arvindh <arvindh91@gmail.com>

rename expiry_at to expires_at

639d913

Signed-off-by: Arvindh <arvindh91@gmail.com>

rename file

7915ce2

Signed-off-by: Arvindh <arvindh91@gmail.com>

defaut allow unverified user in docker

114c351

Signed-off-by: Arvindh <arvindh91@gmail.com>

defaut allow unverified user in docker

cf36722

Signed-off-by: Arvindh <arvindh91@gmail.com>

fix linter errors

acf452b

Signed-off-by: Arvindh <arvindh91@gmail.com>

arvindh123 force-pushed the user-verification branch from 9083ba0 to acf452b Compare September 5, 2025 08:08

arvindh123 added 2 commits September 5, 2025 14:28

add verification cli and fix sdk

f912fec

Signed-off-by: Arvindh <arvindh91@gmail.com>

add verification cli and fix sdk

808645c

Signed-off-by: Arvindh <arvindh91@gmail.com>

felixgateru previously approved these changes Sep 5, 2025

View reviewed changes

smithjilks requested changes Sep 5, 2025

View reviewed changes

smithjilks previously approved these changes Sep 5, 2025

View reviewed changes

dborovcanin requested changes Sep 5, 2025

View reviewed changes

rename variable name in SendVerificaiton

2de1dd1

Signed-off-by: Arvindh <arvindh91@gmail.com>

arvindh123 dismissed stale reviews from smithjilks and felixgateru via 2de1dd1 September 5, 2025 12:36

rename variable authMW to authn

5a3151f

Signed-off-by: Arvindh <arvindh91@gmail.com>

dborovcanin reviewed Sep 5, 2025

View reviewed changes

Fix variable names

30bf733

Signed-off-by: Arvindh <arvindh91@gmail.com>

dborovcanin approved these changes Sep 5, 2025

View reviewed changes

dborovcanin merged commit e57ad79 into absmach:main Sep 5, 2025
8 of 9 checks passed

dborovcanin deleted the user-verification branch September 5, 2025 13:24

		UpdateToUserVerificationDetails(ctx context.Context, user User) (User, error)


		UpdateEmail(ctx context.Context, user User) (User, error)

		UpdateRole(ctx context.Context, user User) (User, error)


		SendVerification(ctx context.Context, session authn.Session) error

		VerifyEmail(ctx context.Context, verificationToken string) (User, error)

SMQ-3093 - User email verification #3101

SMQ-3093 - User email verification #3101

Uh oh!

Conversation

arvindh123 commented Aug 29, 2025 • edited by dborovcanin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

What does this do?

Which issue(s) does this PR fix/relate to?

Have you included tests for your changes?

Did you document any new/modified feature?

Notes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dborovcanin left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arvindh123 Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixgateru left a comment

Choose a reason for hiding this comment

Uh oh!

dborovcanin commented Sep 2, 2025

arvindh123 commented Aug 29, 2025 •

edited by dborovcanin

Loading

codecov bot commented Sep 2, 2025 •

edited

Loading

arvindh123 Sep 2, 2025 •

edited

Loading