-
Notifications
You must be signed in to change notification settings - Fork 26
[CFX-7608] Classify server errors on the dr auth check legs and split 403 from 401 #911
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
bc73f6b
5e0ec1b
50362fd
fac5234
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -16,12 +16,12 @@ package check | |||||||||||||||
|
|
||||||||||||||||
| import ( | ||||||||||||||||
| "bytes" | ||||||||||||||||
| "io" | ||||||||||||||||
| "net/http" | ||||||||||||||||
| "net/http/httptest" | ||||||||||||||||
| "os" | ||||||||||||||||
| "testing" | ||||||||||||||||
|
|
||||||||||||||||
| "github.com/datarobot/cli/internal/config" | ||||||||||||||||
| "github.com/datarobot/cli/internal/config/viperx" | ||||||||||||||||
| "github.com/stretchr/testify/assert" | ||||||||||||||||
| "github.com/stretchr/testify/require" | ||||||||||||||||
| ) | ||||||||||||||||
|
|
@@ -43,32 +43,82 @@ func TestCheckCLICredentials_QuotedEndpointNamesEndpointNotToken(t *testing.T) { | |||||||||||||||
| assert.NotContains(t, buf.String(), "DATAROBOT_API_TOKEN environment variable is invalid or expired") | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| // The '.env' leg consumes VerifyToken's error as an opaque non-nil; the typed | ||||||||||||||||
| // *config.HTTPStatusError must leave its message and verdict unchanged. | ||||||||||||||||
| func TestVerifyDotenvToken_StatusErrorKeepsMessage(t *testing.T) { | ||||||||||||||||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { | ||||||||||||||||
| w.WriteHeader(http.StatusUnauthorized) | ||||||||||||||||
| })) | ||||||||||||||||
| t.Cleanup(server.Close) | ||||||||||||||||
|
|
||||||||||||||||
| orig := os.Stdout | ||||||||||||||||
|
|
||||||||||||||||
| t.Cleanup(func() { os.Stdout = orig }) | ||||||||||||||||
|
|
||||||||||||||||
| r, w, err := os.Pipe() | ||||||||||||||||
| require.NoError(t, err) | ||||||||||||||||
|
|
||||||||||||||||
| os.Stdout = w | ||||||||||||||||
|
|
||||||||||||||||
| valid := verifyDotenvToken(server.URL+"/api/v2", "expired-token") | ||||||||||||||||
|
|
||||||||||||||||
| os.Stdout = orig | ||||||||||||||||
|
|
||||||||||||||||
| require.NoError(t, w.Close()) | ||||||||||||||||
|
|
||||||||||||||||
| out, err := io.ReadAll(r) | ||||||||||||||||
| require.NoError(t, err) | ||||||||||||||||
| // The stored-profile leg blames the token only on a real 401; 403 reports the | ||||||||||||||||
| // account lacking access, and other statuses blame the instance. | ||||||||||||||||
| func TestCheckCLICredentials_ClassifiesStoredProfileStatus(t *testing.T) { | ||||||||||||||||
| cases := []struct { | ||||||||||||||||
| name string | ||||||||||||||||
| status int | ||||||||||||||||
| wantContains string | ||||||||||||||||
| wantNotContains string | ||||||||||||||||
| }{ | ||||||||||||||||
| {"401 blames the token", http.StatusUnauthorized, "No valid API key found", ""}, | ||||||||||||||||
| {"403 reports lacking access", http.StatusForbidden, "lacks API access", "No valid API key found"}, | ||||||||||||||||
| {"503 blames the instance", http.StatusServiceUnavailable, "answered HTTP 503", "No valid API key found"}, | ||||||||||||||||
|
Comment on lines
+55
to
+57
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [P3] Add the stored-profile 404 row the commit message claims The commit says
Suggested change
|
||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| for _, c := range cases { | ||||||||||||||||
| t.Run(c.name, func(t *testing.T) { | ||||||||||||||||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { | ||||||||||||||||
| w.WriteHeader(c.status) | ||||||||||||||||
| })) | ||||||||||||||||
| t.Cleanup(server.Close) | ||||||||||||||||
|
|
||||||||||||||||
| t.Setenv("DATAROBOT_ENDPOINT", "") | ||||||||||||||||
| t.Setenv("DATAROBOT_API_ENDPOINT", "") | ||||||||||||||||
| t.Setenv("DATAROBOT_API_TOKEN", "") | ||||||||||||||||
|
|
||||||||||||||||
| viperx.Reset() | ||||||||||||||||
| viperx.Set(config.DataRobotURL, server.URL+"/api/v2") | ||||||||||||||||
| viperx.Set(config.DataRobotAPIKey, "stored-token") | ||||||||||||||||
| t.Cleanup(viperx.Reset) | ||||||||||||||||
|
|
||||||||||||||||
| var buf bytes.Buffer | ||||||||||||||||
|
|
||||||||||||||||
| valid := checkCLICredentials(&buf) | ||||||||||||||||
|
|
||||||||||||||||
| require.False(t, valid) | ||||||||||||||||
| assert.Contains(t, buf.String(), c.wantContains) | ||||||||||||||||
|
|
||||||||||||||||
| if c.wantNotContains != "" { | ||||||||||||||||
| assert.NotContains(t, buf.String(), c.wantNotContains) | ||||||||||||||||
| } | ||||||||||||||||
| }) | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| require.False(t, valid) | ||||||||||||||||
| assert.Contains(t, string(out), "DATAROBOT_API_TOKEN in '.env' is invalid or expired") | ||||||||||||||||
| // The '.env' leg used to blame the token for every non-200; now 401 blames the | ||||||||||||||||
| // token, 403 reports lacking access, and other statuses blame the instance. | ||||||||||||||||
| func TestVerifyDotenvToken_ClassifiesStatus(t *testing.T) { | ||||||||||||||||
| cases := []struct { | ||||||||||||||||
| name string | ||||||||||||||||
| status int | ||||||||||||||||
| wantContains string | ||||||||||||||||
| wantNotContains string | ||||||||||||||||
| }{ | ||||||||||||||||
| {"401 blames the token", http.StatusUnauthorized, "DATAROBOT_API_TOKEN in '.env' is invalid or expired", ""}, | ||||||||||||||||
| {"403 reports lacking access", http.StatusForbidden, "lacks API access", "is invalid or expired"}, | ||||||||||||||||
| {"404 blames the instance", http.StatusNotFound, "answered HTTP 404", "is invalid or expired"}, | ||||||||||||||||
| {"503 blames the instance", http.StatusServiceUnavailable, "answered HTTP 503", "is invalid or expired"}, | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| for _, c := range cases { | ||||||||||||||||
| t.Run(c.name, func(t *testing.T) { | ||||||||||||||||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { | ||||||||||||||||
| w.WriteHeader(c.status) | ||||||||||||||||
| })) | ||||||||||||||||
| t.Cleanup(server.Close) | ||||||||||||||||
|
|
||||||||||||||||
| var buf bytes.Buffer | ||||||||||||||||
|
|
||||||||||||||||
| valid := verifyDotenvToken(&buf, server.URL+"/api/v2", "some-token") | ||||||||||||||||
|
|
||||||||||||||||
| require.False(t, valid) | ||||||||||||||||
| assert.Contains(t, buf.String(), c.wantContains) | ||||||||||||||||
|
|
||||||||||||||||
| if c.wantNotContains != "" { | ||||||||||||||||
| assert.NotContains(t, buf.String(), c.wantNotContains) | ||||||||||||||||
| } | ||||||||||||||||
| }) | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.