CLOUDP-330561: Moves unauth error check to common errors #4043

cveticm · 2025-07-16T10:44:12Z

Proposed changes

Issues

Initial work to move unauth check from builder.go to transport.go (#4034) neglected digest and access token transport clients. This meant that if either of these transport methods received a 401 response, the error would not be wrapped with any additional messaging. For example, when a user's access token expires.

Solution

To fix this, I have moved the unauth check to atlas.go.
The flow now is:

a command finishes executing
if err
- check if 401
  - if 401, print custom error message to stdErr

Note that the raw transport error message will be printed to terminal before the command is fully complete. In practice, on terminal this looks much the same as the previous implementation:

Error: https://cloud-dev.mongodb.com/api/atlas/v2/groups/670cd17af33cea212ea1ed61/clusters GET: HTTP 401 Unauthorized (Error code: "") Detail: You are not authorized for this resource. Reason: Unauthorized. Params: []
this action requires authentication

To log in using your Atlas username and password, run: atlas auth login
To set credentials using API keys, run: atlas config init

Testing

Unit tests have been added.

As oauth authentication (Access Tokens) requires a browser-based flow, e2e testing could not be created to verify that access tokens are refreshed on expiry.
Manual testing was done instead. Access and Refresh Tokens are successfully refreshed when they expire.
Future work: E2E testing will be created once Service Accounts are implemented (CLOUDP-329808). An e2e test will be created to verify tokens are refreshed on expiry.

Jira ticket: CLOUDP-330561

Checklist

I have signed the MongoDB CLA
I have added tests that prove my fix is effective or that my feature works
I have added any necessary documentation in document requirements section listed in CONTRIBUTING.md (if appropriate)
I have addressed the @mongodb/docs-cloud-team comments (if appropriate)
I have updated test/README.md (if an e2e test has been added)
I have run make fmt and formatted my code

cveticm · 2025-07-16T10:52:23Z

internal/transport/transport.go

-type AuthRequiredRoundTripper struct {
-	Base http.RoundTripper
-}
-


transport wrapper no longer needed since its primary purpose, to check for unauth, is now done after the command executes

jeroenvervaeke · 2025-07-17T11:09:28Z

internal/cli/commonerrors/errors.go

@@ -49,6 +54,13 @@ func Check(err error) error {
 	return err
 }

+func CheckHTTPErrors(err error) error {
+	if strings.Contains(err.Error(), "401") && strings.Contains(err.Error(), "Unauthorized") {


I don't think this is the proper way to check the error code coming from the API.

I think you'll have to use:

admin.AsError(err)

I was first going to recommend using the errorcode: UNAUTHORIZED, but it looks like we have so many variations.

It's better to check the HTTP status code instead (through the error).

Unfortunately I don't fully remember how to get the status code from the SDK, let me know if you can't find it and want to look into it together

Unfortunately I don't fully remember how to get the status code from the SDK, let me know if you can't find it and want to look into it together

AsError returns the ApiError struct which contains the error code as param
https://github.com/mongodb/atlas-sdk-go/blob/main/admin/model_api_error.go#L6

Yes i tried this initially, but these unauth errors aren't GenericOpenAPIError types so admin.AsError(err) returns !ok.

I think we need a custom error, perhaps should we introduce error handling on transport and create an ErrUnauthorized? at least a generic http error that has code as a property

andreaangiolillo · 2025-07-17T11:09:58Z

cmd/atlas/atlas.go

+		if errMsg != nil {
+			fmt.Println(errMsg)
+		}


shouldn't we print this to stderr?

I think this prints it twice because cobra prints it

If we are moving the error check here we should remove each individual usage of it and move it here

…of sdk

github-actions · 2025-07-18T15:39:37Z

Coverage Report 📈

Branch	Commit	Coverage
SA_refactor_feature_branch	`5fbb105`	25.7%
CLOUDP-330561_commonerror_unauth_check	`1f2a84a`	25.8%
	Difference	.1%

cveticm · 2025-07-18T15:44:51Z

internal/validate/validate_test.go

@@ -169,7 +169,7 @@ func TestCredentials(t *testing.T) {
 	})
 }

-func TestNoAPIKeyss(t *testing.T) {
+func TestNoAPIKeys(t *testing.T) {


Drive-by fix

cveticm · 2025-07-18T16:03:54Z

e2e failure is plugin e2e tests due to GitHub rate limiting. These tests pass locally and should not be affected by the changes in this PR. Will rerun next week once rate limiting resets.

fmenezes · 2025-07-18T16:09:26Z

cmd/atlas/atlas.go

@@ -36,6 +37,10 @@ func execute(rootCmd *cobra.Command) {

 To learn more, see our documentation: https://www.mongodb.com/docs/atlas/cli/stable/connect-atlas-cli/`
 	if cmd, err := rootCmd.ExecuteContextC(ctx); err != nil {
+		errMsg := commonerrors.GetHumanFriendlyErrorMessage(err)
+		if errMsg != nil {
+			fmt.Fprintln(os.Stderr, errMsg)


we did not have this print before, how was cobra printing it?

I think errors might be printing twice, there is a SilenceErrors property in cobra, double check if you need it

cobra prints to stderr but only prints response error. errMsg only prints custom error (ie. "this action requires authentication ...").

Since errMsg has different content than err printed by cobra, we don't get duplication.

running this code this is what I get after my browser login is expired

Error: Get "https://cloud-dev.mongodb.com/api/atlas/v2/groups/686f96db2f08ee49a6da7ca8/clusters?includeCount=true&includeDeletedWithRetainedBackups=false&itemsPerPage=100&pageNum=1": POST https://cloud-dev.mongodb.com/api/private/unauth/account/device/token: 400 (request "INVALID_REFRESH_TOKEN") The refresh token is invalid or expired. exit status 1

on master I get

Error: session expired Please note that your session expires periodically. If you use Atlas CLI for automation, see https://www.mongodb.com/docs/atlas/cli/stable/atlas-cli-automate/ for best practices. To login, run: atlas auth login exit status 1

I prefer master's experience, can we keep it?

tested by running atlas clusters list

I know why that is, the error of not being able to refresh token is a 400 not a 401. I think this is a sort of chicken and egg problem.

I've managed to go back a to a good UX by:

updated internal/cli/commonerrors/errors.go

func Check(err error) error { if err == nil { return nil } + + apiErrorCode := "UNKNOWN" + + var sdkError *atlas.ErrorResponse + if ok := errors.As(err, &sdkError); ok { + apiErrorCode = sdkError.ErrorCode + } apiError, ok := atlasv2.AsError(err) if ok { - switch apiError.GetErrorCode() { + apiErrorCode = apiError.GetErrorCode() + } + + apiPinnedError, ok := atlasClustersPinned.AsError(err) + if ok { + apiErrorCode = apiPinnedError.GetErrorCode() + } + + switch apiErrorCode { case "TENANT_CLUSTER_UPDATE_UNSUPPORTED": return errClusterUnsupported case "GLOBAL_USER_OUTSIDE_SUBNET": return errOutsideVPN case asymmetricShardUnsupportedErrorCode: return errAsymmetricShardUnsupported + case "INVALID_REFRESH_TOKEN": + return ErrUnauthorized - } } + return err }

updated cmd/atlas/atlas.go

if cmd, err := rootCmd.ExecuteContextC(ctx); err != nil { - errMsg := commonerrors.GetHumanFriendlyErrorMessage(err) + errMsg := commonerrors.Check(err) if errMsg != nil {

updated internal/cli/root/builder.go

Example: ` # Display the help menu for the config command: atlas config --help `, SilenceUsage: true, + SilenceErrors: true, Annotations: map[string]string{ "toc": "true", },

fmenezes

check my comment on the thread unfortunately the UX here needs some more work, good news is I've managed the find the way I think we should move forward, take a look at my comments and let me know.

Moves error handling from transport to atlas.go

14aa81d

cveticm requested a review from a team as a code owner July 16, 2025 10:44

cveticm commented Jul 16, 2025

View reviewed changes

jeroenvervaeke reviewed Jul 17, 2025

View reviewed changes

andreaangiolillo reviewed Jul 17, 2025

View reviewed changes

fix naming

6bc5ad3

cveticm force-pushed the CLOUDP-330561_commonerror_unauth_check branch from f83fb70 to 6bc5ad3 Compare July 18, 2025 10:27

Fixes error getter to look for sdk errors for all supported versions …

1f2a84a

…of sdk

cveticm commented Jul 18, 2025

View reviewed changes

cveticm requested review from fmenezes, andreaangiolillo and jeroenvervaeke July 18, 2025 16:04

fmenezes reviewed Jul 18, 2025

View reviewed changes

fmenezes requested changes Jul 21, 2025

View reviewed changes

CLOUDP-330561: Moves unauth error check to common errors #4043

Are you sure you want to change the base?

CLOUDP-330561: Moves unauth error check to common errors #4043

Uh oh!

Conversation

cveticm commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Issues

Solution

Testing

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreaangiolillo Jul 17, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jul 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cveticm commented Jul 18, 2025

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!

fmenezes Jul 21, 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!

fmenezes Jul 21, 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!

fmenezes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cveticm commented Jul 16, 2025 •

edited

Loading

andreaangiolillo Jul 17, 2025 •

edited

Loading

fmenezes Jul 21, 2025 •

edited

Loading

fmenezes Jul 21, 2025 •

edited

Loading