[SDP-1247] new API role for bypassing reCAPTCHA #368

ziyliu · 2024-07-26T01:40:13Z

What

Completed:

add role APIUserRole that could be concatenated with existing user roles to override reCAPTCHA validation
local testing for all three endpoints /login, /mfa, and /forgot-password that would usually require reCAPTCHA validation normally
fixes to existing unit tests and new tests for data access methods
refactoring fixed unit tests to be less repetitive

##Tests
The following calls were made referring to users with the api role and without the recaptcha token in the request body

/login
request

{
    "email": "[email protected]",
    "password": <REDACTED>
}

response

{
    "token": "eyJhbGciOiJFUzI1NiIsInR5cCI6IkpXVCJ9.eyJ1c2VyIjp7ImlkIjoiOWI3OTI4MWEtY2E1OS00MDZjLWIwNmMtYTE3NWNhYmQ0ZDA5IiwiZmlyc3RfbmFtZSI6ImpvaG4iLCJsYXN0X25hbWUiOiJkb2UiLCJlbWFpbCI6Im93bmVyQGJsdWVjb3JwLmxvY2FsIiwiaXNfYWN0aXZlIjpmYWxzZSwicm9sZXMiOlsib3duZXIiXX0sInRlbmFudF9pZCI6ImFhYzFlY2ZjLTQzZDItNDlkYy1iMzE3LTJjOTA4MzRmMjM5YiIsImV4cCI6MTcyMTk0MTk2NH0.O_dDRFXGV9tWhE8gIuxSPn8SUMsmJphJkAyVYEQ-5CBMiZ3egKKMEe_BHZLfc3bp3TVhMMB4Dc4JjGNs-Ez2nw"
}

26e4ca25-94b7-40a0-97e4-31c4d7271826 | 9b79281a-ca59-406c-b06c-a175cabd4d09 |  123  | 2024-07-26 00:31:24.611207+00 | 2024-07-26 09:40:56.524779+00 | 2024-07-28 00:31:24.611207+00 |  
-- | -- | -- | -- | -- | -- | --

/mfa

request

{
    "mfa_code": "123",
    "remember_me": false
}

response

{
    "token": "eyJhbGciOiJFUzI1NiIsInR5cCI6IkpXVCJ9.eyJ1c2VyIjp7ImlkIjoiOWI3OTI4MWEtY2E1OS00MDZjLWIwNmMtYTE3NWNhYmQ0ZDA5IiwiZmlyc3RfbmFtZSI6ImpvaG4iLCJsYXN0X25hbWUiOiJkb2UiLCJlbWFpbCI6Im93bmVyQGJsdWVjb3JwLmxvY2FsIiwiaXNfYWN0aXZlIjpmYWxzZSwicm9sZXMiOlsib3duZXIiXX0sInRlbmFudF9pZCI6ImFhYzFlY2ZjLTQzZDItNDlkYy1iMzE3LTJjOTA4MzRmMjM5YiIsImV4cCI6MTcyMTk1ODU0Nn0.aMqfzwTTt8tPTgbyyai5FurrWwol9LDnrI6KexnSPGshghGdtUlZCy_ssfGug96qS5D8RG3_qNnFW0-0lzGmpw"
}

/forgot-password
request

{
    "email": "[email protected]"
}

response

{
    "message": "Password reset requested. If the email is registered, you'll receive a reset link shortly. Check your inbox and spam folders."
}

Why

[TODO: Why this change is being made. Include any context required to understand the why.]

Known limitations

[TODO or N/A]

Checklist

PR Structure

This PR has a reasonably narrow scope (if not, break it down into smaller PRs).
This PR title and description are clear enough for anyone to review it.
This PR does not mix refactoring changes with feature changes (split into two PRs otherwise).

Thoroughness

This PR adds tests for the new functionality or fixes.
This PR contains the link to the Jira ticket it addresses.

Configs and Secrets

No new CONFIG variables are required -OR- the new required ones were added to the helmchart's values.yaml file.
No new CONFIG variables are required -OR- the new required ones were added to the deployments (pr-preview, dev, demo, prd).
No new SECRETS variables are required -OR- the new required ones were mentioned in the helmchart's values.yaml file.
No new SECRETS variables are required -OR- the new required ones were added to the deployments (pr-preview secrets, dev secrets, demo secrets, prd secrets).

Release

This is not a breaking change.
This is ready for production.. If your PR is not ready for production, please consider opening additional complementary PRs using this one as the base. Only merge this into develop or main after it's ready for production!

Deployment

Does the deployment work after merging?

stellar-jenkins · 2024-07-26T01:45:15Z

stellar-disbursement-platform-backend-preview is available here:
SDP: https://sdp-backend-pr368.previews.kube001.services.stellar-ops.com/health
AP: https://sdp-ap-pr368.previews.kube001.services.stellar-ops.com/health
Frontend: https://sdp-backend-dashboard-pr368.previews.kube001.services.stellar-ops.com

stellar-jenkins · 2024-07-26T14:11:08Z

stellar-disbursement-platform-backend-preview is available here:
SDP: https://sdp-backend-pr368.previews.kube001.services.stellar-ops.com/health
AP: https://sdp-ap-pr368.previews.kube001.services.stellar-ops.com/health
Frontend: https://sdp-backend-dashboard-pr368.previews.kube001.services.stellar-ops.com

stellar-jenkins · 2024-07-26T19:52:28Z

stellar-disbursement-platform-backend-preview is available here:
SDP: https://sdp-backend-pr368.previews.kube001.services.stellar-ops.com/health
AP: https://sdp-ap-pr368.previews.kube001.services.stellar-ops.com/health
Frontend: https://sdp-backend-dashboard-pr368.previews.kube001.services.stellar-ops.com

stellar-jenkins · 2024-07-26T20:45:46Z

stellar-disbursement-platform-backend-preview is available here:
SDP: https://sdp-backend-pr368.previews.kube001.services.stellar-ops.com/health
AP: https://sdp-ap-pr368.previews.kube001.services.stellar-ops.com/health
Frontend: https://sdp-backend-dashboard-pr368.previews.kube001.services.stellar-ops.com

stellar-jenkins · 2024-07-27T01:26:11Z

stellar-disbursement-platform-backend-preview is available here:
SDP: https://sdp-backend-pr368.previews.kube001.services.stellar-ops.com/health
AP: https://sdp-ap-pr368.previews.kube001.services.stellar-ops.com/health
Frontend: https://sdp-backend-dashboard-pr368.previews.kube001.services.stellar-ops.com

stellar-jenkins · 2024-08-06T18:08:53Z

stellar-disbursement-platform-backend-preview is available here:
SDP: https://sdp-backend-pr368.previews.kube001.services.stellar-ops.com/health
AP: https://sdp-ap-pr368.previews.kube001.services.stellar-ops.com/health
Frontend: https://sdp-backend-dashboard-pr368.previews.kube001.services.stellar-ops.com

stellar-jenkins · 2024-08-06T18:10:37Z

Something went wrong with PR preview build please check

comments

stellar-jenkins · 2024-08-06T22:26:10Z

stellar-disbursement-platform-backend-preview is available here:
SDP: https://sdp-backend-pr368.previews.kube001.services.stellar-ops.com/health
AP: https://sdp-ap-pr368.previews.kube001.services.stellar-ops.com/health
Frontend: https://sdp-backend-dashboard-pr368.previews.kube001.services.stellar-ops.com

stellar-jenkins · 2024-08-06T23:46:17Z

stellar-disbursement-platform-backend-preview is available here:
SDP: https://sdp-backend-pr368.previews.kube001.services.stellar-ops.com/health
AP: https://sdp-ap-pr368.previews.kube001.services.stellar-ops.com/health
Frontend: https://sdp-backend-dashboard-pr368.previews.kube001.services.stellar-ops.com

stellar-jenkins · 2024-08-07T16:41:49Z

stellar-disbursement-platform-backend-preview is available here:
SDP: https://sdp-backend-pr368.previews.kube001.services.stellar-ops.com/health
AP: https://sdp-ap-pr368.previews.kube001.services.stellar-ops.com/health
Frontend: https://sdp-backend-dashboard-pr368.previews.kube001.services.stellar-ops.com

stellar-jenkins · 2024-08-08T18:31:31Z

stellar-disbursement-platform-backend-preview is available here:
SDP: https://sdp-backend-pr368.previews.kube001.services.stellar-ops.com/health
AP: https://sdp-ap-pr368.previews.kube001.services.stellar-ops.com/health
Frontend: https://sdp-backend-dashboard-pr368.previews.kube001.services.stellar-ops.com

marcelosalloum

Thank you for working on that.

I still found a few issues. Comments below ⬇️

marcelosalloum · 2024-08-09T00:50:37Z

internal/serve/httphandler/user_handler.go

-	// NOTE: in the MVP, users should have only one role.
-	validator.Check(len(roles) == 1, "roles", "the number of roles required is exactly one")
-
+	validator.Check(len(roles) >= 1, "roles", "the number of roles required is greater than or equal to 1")


You should also be validating that the user will not end up with just the API role.

marcelosalloum · 2024-08-09T00:53:07Z

internal/serve/httphandler/forgot_password_handler.go

+	user, err := h.AuthManager.GetUserByEmail(ctx, strings.TrimSpace(forgotPasswordRequest.Email))
+	if err != nil {
+		// If user cannot be found, we will default to user requiring to validate reCAPTCHA
+		if errors.Is(err, auth.ErrUserNotFound) {
+			// If we don't find the user by email, we just return an ok response
+			// to prevent malicious client from searching accounts in the system
+			log.Ctx(ctx).Errorf("Email in request not found: %s", utils.TruncateString(forgotPasswordRequest.Email, 3))
+		}
+	}
+
+	userRoleCanBypassReCAPTCHA := false
+	if user != nil {
+		userRoleCanBypassReCAPTCHA = slices.Contains(user.Roles, data.APIUserRole.String())
+	}


💡 since we don't care much about this error, we could simplify this logic a bit:

Suggested change

user, err := h.AuthManager.GetUserByEmail(ctx, strings.TrimSpace(forgotPasswordRequest.Email))

if err != nil {

// If user cannot be found, we will default to user requiring to validate reCAPTCHA

if errors.Is(err, auth.ErrUserNotFound) {

// If we don't find the user by email, we just return an ok response

// to prevent malicious client from searching accounts in the system

log.Ctx(ctx).Errorf("Email in request not found: %s", utils.TruncateString(forgotPasswordRequest.Email, 3))

}

}

userRoleCanBypassReCAPTCHA := false

if user != nil {

userRoleCanBypassReCAPTCHA = slices.Contains(user.Roles, data.APIUserRole.String())

}

userRoleCanBypassReCAPTCHA := false

user, err := h.AuthManager.GetUserByEmail(ctx, strings.TrimSpace(forgotPasswordRequest.Email))

if err == nil {

userRoleCanBypassReCAPTCHA = slices.Contains(user.Roles, data.APIUserRole.String())

}

marcelosalloum · 2024-08-09T00:54:37Z

internal/serve/httphandler/forgot_password_handler.go

+	resetToken, err := h.AuthManager.ForgotPassword(ctx, strings.TrimSpace(forgotPasswordRequest.Email))
+	// If we don't find the user by email, we just return an ok response


This is the second time you'te triming the space. Why don't you do it once, right at the beginning, by updating the value in forgotPasswordRequest.Email with it's trimmed version?

marcelosalloum · 2024-08-09T00:58:31Z

stellar-auth/pkg/auth/authenticator_test.go

+		u, err := authenticator.GetUserByEmail(ctx, randUser.Email)
+		require.NoError(t, err)
+
+		assert.Equal(t, randUser.ID, u.ID)
+		assert.Equal(t, randUser.Email, u.Email)


Ok, thanks for explaining it.

marcelosalloum · 2024-08-09T01:01:53Z

internal/serve/httphandler/forgot_password_handler_test.go

-			Once()
+		for _, userRoles := range usersRoles {
+			targetUser := &auth.User{
+				ID:    "user-ID",


A few lines above you used userID and here you have user-ID. Is that a typo? Is that field being used for anything? Should we add a test for it?

marcelosalloum · 2024-08-09T01:03:30Z

internal/serve/httphandler/forgot_password_handler_test.go

+	t.Run("Should return http status 400 when attempting to validate reCAPTCHA token after failing to find user by email", func(t *testing.T) {
+		requestBody := `
+		{ 
+			"email": "[email protected]"


Would this be a better naming? WDYT?

Suggested change

"email": "valid@email.com"

"email": "not-found@email.com"

marcelosalloum · 2024-08-09T01:06:02Z

internal/serve/httphandler/login_handler.go

+		// If user cannot be found, we will default to user requiring to validate reCAPTCHA
+		if errors.Is(err, auth.ErrUserNotFound) {
+			// If we don't find the user by email, we just return an ok response
+			// to prevent malicious client from searching accounts in the system
+			log.Ctx(ctx).Errorf("Email in request not found: %s", utils.TruncateString(reqBody.Email, 3))
+		}
+	}
+
+	userRoleCanBypassReCAPTCHA := false
+	if user != nil {
+		userRoleCanBypassReCAPTCHA = slices.Contains(user.Roles, data.APIUserRole.String())
+	}


Same here about simplifying this statement.

marcelosalloum · 2024-08-09T01:11:50Z

internal/serve/httphandler/login_handler_test.go

+	defaultReqBody := fmt.Sprintf(
+		`{"email": "%s", "password": "%s", "recaptcha_token": "%s"}`, email, password, defaultReCAPTCHAToken)


I think the naming here can be improved for better clarity. I mentioned this in a previous review on this same PR, but what does default mean vs non-default? If this is meant to be a login request body with or without recaptcha, you should improve make sure the const tame conveys that meaning.

marcelosalloum · 2024-08-09T01:13:54Z

internal/serve/httphandler/login_handler_test.go

+func authenticateSetup(
+	t *testing.T,
+	authenticatorMock *auth.AuthenticatorMock,
+	jwtManagerMock *auth.JWTManagerMock,
+	user *auth.User, password, userToken string,
+) {
+	t.Helper()
+	authenticatorMock.
+		On("ValidateCredentials", mock.Anything, user.Email, password).
+		Return(user, nil).
+		Once()
+	jwtManagerMock.
+		On("GenerateToken", mock.Anything, user, mock.AnythingOfType("time.Time")).
+		Return(userToken, nil).
+		Once()
+}
+


Can you be more specific on this method name? Maybe prepareMocksForLogin would be more accurate?

marcelosalloum · 2024-08-09T01:18:00Z

internal/serve/httphandler/mfa_handler.go

+		return
+	}
+
+	user, err := h.AuthManager.GetUserByDeviceID(ctx, strings.TrimSpace(deviceID))


🚨 It doesn't look like the implementation of GetUserByDeviceID is returning the user roles.

ziyliu had a problem deploying to Receiver Registration - E2E Integration Tests (Stellar) July 26, 2024 01:40 — with GitHub Actions Failure

ziyliu temporarily deployed to Receiver Registration - E2E Integration Tests (Stellar) July 26, 2024 01:40 — with GitHub Actions Inactive

ziyliu temporarily deployed to Anchor Integration Tests July 26, 2024 01:40 — with GitHub Actions Inactive

ziyliu temporarily deployed to Receiver Registration - E2E Integration Tests (Circle) July 26, 2024 01:40 — with GitHub Actions Inactive

ziyliu self-assigned this Jul 26, 2024

ziyliu temporarily deployed to Anchor Integration Tests July 26, 2024 14:08 — with GitHub Actions Inactive

ziyliu temporarily deployed to Receiver Registration - E2E Integration Tests (Stellar) July 26, 2024 14:08 — with GitHub Actions Inactive

ziyliu temporarily deployed to Receiver Registration - E2E Integration Tests (Circle) July 26, 2024 14:08 — with GitHub Actions Inactive

ziyliu had a problem deploying to Receiver Registration - E2E Integration Tests (Stellar) July 26, 2024 19:49 — with GitHub Actions Error

ziyliu temporarily deployed to Anchor Integration Tests July 26, 2024 19:49 — with GitHub Actions Inactive

ziyliu temporarily deployed to Receiver Registration - E2E Integration Tests (Stellar) July 26, 2024 19:49 — with GitHub Actions Inactive

ziyliu had a problem deploying to Receiver Registration - E2E Integration Tests (Circle) July 26, 2024 19:49 — with GitHub Actions Failure

ziyliu temporarily deployed to Anchor Integration Tests July 26, 2024 20:40 — with GitHub Actions Inactive

ziyliu had a problem deploying to Receiver Registration - E2E Integration Tests (Stellar) July 26, 2024 20:40 — with GitHub Actions Error

ziyliu temporarily deployed to Receiver Registration - E2E Integration Tests (Stellar) July 26, 2024 20:40 — with GitHub Actions Inactive

ziyliu had a problem deploying to Receiver Registration - E2E Integration Tests (Circle) July 26, 2024 20:40 — with GitHub Actions Failure

ziyliu force-pushed the erica/sdp-1247 branch from a53ca16 to cdbae54 Compare July 27, 2024 01:23

ziyliu temporarily deployed to Receiver Registration - E2E Integration Tests (Stellar) July 27, 2024 01:23 — with GitHub Actions Inactive

ziyliu temporarily deployed to Anchor Integration Tests July 27, 2024 01:23 — with GitHub Actions Inactive

ziyliu temporarily deployed to Receiver Registration - E2E Integration Tests (Stellar) July 27, 2024 01:23 — with GitHub Actions Inactive

ziyliu temporarily deployed to Receiver Registration - E2E Integration Tests (Circle) July 27, 2024 01:23 — with GitHub Actions Inactive

ziyliu had a problem deploying to Receiver Registration - E2E Integration Tests (Stellar) July 29, 2024 21:34 — with GitHub Actions Failure

ziyliu temporarily deployed to Receiver Registration - E2E Integration Tests (Stellar) July 29, 2024 21:34 — with GitHub Actions Inactive

ziyliu temporarily deployed to Anchor Integration Tests July 29, 2024 21:34 — with GitHub Actions Inactive

ziyliu temporarily deployed to Receiver Registration - E2E Integration Tests (Circle) August 6, 2024 18:07 — with GitHub Actions Inactive

oops

d9589e5

comments

ziyliu temporarily deployed to Receiver Registration - E2E Integration Tests (Stellar) August 6, 2024 22:23 — with GitHub Actions Inactive

ziyliu temporarily deployed to Anchor Integration Tests August 6, 2024 22:23 — with GitHub Actions Inactive

ziyliu temporarily deployed to Receiver Registration - E2E Integration Tests (Circle) August 6, 2024 22:23 — with GitHub Actions Inactive

oops

1c0ae24

ziyliu temporarily deployed to Receiver Registration - E2E Integration Tests (Stellar) August 6, 2024 23:43 — with GitHub Actions Inactive

ziyliu temporarily deployed to Anchor Integration Tests August 6, 2024 23:43 — with GitHub Actions Inactive

ziyliu temporarily deployed to Receiver Registration - E2E Integration Tests (Circle) August 6, 2024 23:43 — with GitHub Actions Inactive

ziyliu requested a review from marcelosalloum August 7, 2024 00:39

oops

2cfe421

ziyliu temporarily deployed to Anchor Integration Tests August 7, 2024 16:38 — with GitHub Actions Inactive

ziyliu had a problem deploying to Receiver Registration - E2E Integration Tests (Stellar) August 7, 2024 16:38 — with GitHub Actions Failure

ziyliu temporarily deployed to Receiver Registration - E2E Integration Tests (Stellar) August 7, 2024 16:38 — with GitHub Actions Inactive

ziyliu temporarily deployed to Receiver Registration - E2E Integration Tests (Circle) August 7, 2024 16:38 — with GitHub Actions Inactive

rm t

0e30aae

ziyliu had a problem deploying to Receiver Registration - E2E Integration Tests (Stellar) August 8, 2024 18:28 — with GitHub Actions Failure

ziyliu temporarily deployed to Anchor Integration Tests August 8, 2024 18:28 — with GitHub Actions Inactive

ziyliu had a problem deploying to Receiver Registration - E2E Integration Tests (Stellar) August 8, 2024 18:28 — with GitHub Actions Failure

ziyliu had a problem deploying to Receiver Registration - E2E Integration Tests (Circle) August 8, 2024 18:28 — with GitHub Actions Error

marcelosalloum reviewed Aug 9, 2024

View reviewed changes

ziyliu closed this Aug 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SDP-1247] new API role for bypassing reCAPTCHA #368

[SDP-1247] new API role for bypassing reCAPTCHA #368

ziyliu commented Jul 26, 2024 •

edited

Loading

stellar-jenkins commented Jul 26, 2024

stellar-jenkins commented Jul 26, 2024

stellar-jenkins commented Jul 26, 2024

stellar-jenkins commented Jul 26, 2024

stellar-jenkins commented Jul 27, 2024

stellar-jenkins commented Aug 6, 2024

stellar-jenkins commented Aug 6, 2024

stellar-jenkins commented Aug 6, 2024

stellar-jenkins commented Aug 6, 2024

stellar-jenkins commented Aug 7, 2024

stellar-jenkins commented Aug 8, 2024

marcelosalloum left a comment

marcelosalloum Aug 9, 2024

marcelosalloum Aug 9, 2024

marcelosalloum Aug 9, 2024

marcelosalloum Aug 9, 2024

marcelosalloum Aug 9, 2024

marcelosalloum Aug 9, 2024

marcelosalloum Aug 9, 2024

marcelosalloum Aug 9, 2024

marcelosalloum Aug 9, 2024

marcelosalloum Aug 9, 2024

		resetToken, err := h.AuthManager.ForgotPassword(ctx, strings.TrimSpace(forgotPasswordRequest.Email))
		// If we don't find the user by email, we just return an ok response

		defaultReqBody := fmt.Sprintf(
		`{"email": "%s", "password": "%s", "recaptcha_token": "%s"}`, email, password, defaultReCAPTCHAToken)

[SDP-1247] new API role for bypassing reCAPTCHA #368

[SDP-1247] new API role for bypassing reCAPTCHA #368

Conversation

ziyliu commented Jul 26, 2024 • edited Loading

What

Why

Known limitations

Checklist

PR Structure

Thoroughness

Configs and Secrets

Release

Deployment

stellar-jenkins commented Jul 26, 2024

stellar-jenkins commented Jul 26, 2024

stellar-jenkins commented Jul 26, 2024

stellar-jenkins commented Jul 26, 2024

stellar-jenkins commented Jul 27, 2024

stellar-jenkins commented Aug 6, 2024

stellar-jenkins commented Aug 6, 2024

stellar-jenkins commented Aug 6, 2024

stellar-jenkins commented Aug 6, 2024

stellar-jenkins commented Aug 7, 2024

stellar-jenkins commented Aug 8, 2024

marcelosalloum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ziyliu commented Jul 26, 2024 •

edited

Loading