Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds account provisioning to the Snowflake connector: connector metadata now exposes an AccountCreationSchema; userBuilder gains CreateAccount, Delete, and capability discovery; a REST Snowflake user client and doRequest helper were added; several client methods drop http.Response returns and response-body cleanup was centralized. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant userBuilder
participant SnowflakeREST as SnowflakeClient(REST)
participant SnowflakeSQL as SnowflakeClient(SQL)
participant HTTP
Caller->>userBuilder: CreateAccount(ctx, accountInfo, credentialOptions)
userBuilder->>userBuilder: extractProfileFields / build CreateUserRequest
userBuilder->>SnowflakeREST: CreateUserREST(ctx, CreateUserRequest)
SnowflakeREST->>HTTP: POST /api/v2/users (JSON)
HTTP-->>SnowflakeREST: 200/201/202 + headers (rate-limit)
SnowflakeREST-->>userBuilder: success + rate-limit
userBuilder->>SnowflakeSQL: fetchUserWithSQLRetry(ctx, username)
SnowflakeSQL->>HTTP: SQL API query
HTTP-->>SnowflakeSQL: statement response (may return 422 -> retry)
SnowflakeSQL-->>userBuilder: user record
userBuilder-->>Caller: CreateAccountResponse, PlaintextData, Annotations
sequenceDiagram
participant Caller
participant userBuilder
participant SnowflakeREST as SnowflakeClient(REST)
participant HTTP
Caller->>userBuilder: Delete(ctx, resourceId, parentResourceID)
userBuilder->>SnowflakeREST: DeleteUserREST(ctx, username, options)
SnowflakeREST->>HTTP: DELETE /api/v2/users/{name}
HTTP-->>SnowflakeREST: 200/204 + headers
SnowflakeREST-->>userBuilder: rate-limit
userBuilder-->>Caller: Annotations, error?
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/connector/connector.go (1)
201-248:⚠️ Potential issue | 🟠 MajorAdd Connector.Close() to release connector-owned clients.
Please add a Close() method that shuts down any clients created by the connector (e.g., delegate to snowflake.Client or close idle connections) and returns an error when the client is non-nil. As per coding guidelines: pkg/connector/connector.go: Implement the Close() method on Connector to close any clients created by the connector, returning error if client is not nil.
🤖 Fix all issues with AI agents
In `@pkg/snowflake/user_rest.go`:
- Around line 97-115: The error messages in pkg/snowflake/user_rest.go must
include the "baton-snowflake" prefix and wrap underlying errors with %w; update
every fmt.Errorf in this file (including the fmt.Errorf after c.NewRequest, the
one after c.Do, and the other fmt.Errorf calls around lines referenced) to use a
message like "baton-snowflake: <context>: %w" so the underlying err is wrapped,
and ensure all error returns consistently use %w wrapping; check
functions/methods that call c.NewRequest and c.Do and update their fmt.Errorf
invocations (e.g., the error created when NewRequest fails, the "request failed"
error after c.Do, and the two other fmt.Errorf locations noted) to follow this
pattern.
- Around line 116-122: The defer currently assumes response and response.Body
are non-nil which can panic on network errors; update the defer to first check
that response != nil and response.Body != nil before calling io.Copy/io.Discard
and response.Body.Close(). Locate the defer block that references response.Body
(in pkg/snowflake/user_rest.go) and change it so the closure guards against nil
(e.g., if response == nil || response.Body == nil { return } then perform
io.Copy and Close), ensuring the nil-guard matches the pattern used elsewhere
(like secrets.go).
agustin-conductor
force-pushed
the
feature/user_provisioning
branch
3 times, most recently
from
e0d4ba8 to
56708c1
Compare
| # - name: Check grant was re-granted | ||
| # run: | ||
| # baton grants --entitlement="${{ env.CONNECTOR_ENTITLEMENT }}" --output-format=json | jq --exit-status ".grants[].principal.id.resource == \"${{ env.CONNECTOR_PRINCIPAL }}\"" | ||
| - name: Test Account Provisioning |
There was a problem hiding this comment.
NOTE: our testing env expires soon as its a trail, when that happens credentials will no longer work and this test will fail. should I make this optional disable?
There was a problem hiding this comment.
You probably can keep this to validate the execution while we have an account. If new features are developed on the future, the credentials should be updated to the ones generated around testing that feature.
If not, you could create a test server that mimics the API and use that one for the CI tests
pkg/snowflake/user_rest.go
Outdated
| _, _ = io.Copy(io.Discard, response.Body) | ||
| closeErr := response.Body.Close() | ||
| if closeErr != nil { | ||
| log.Printf("baton-snowflake: warning: failed to close response body: %v", closeErr) |
There was a problem hiding this comment.
I think we want to avoid log.Printf. Should this be log.Warn() or log.Debug() maybe?
pkg/connector/users.go
Outdated
| // Using default options (ifExists=false) | ||
| _, err := o.client.DeleteUserREST(ctx, userName, nil) |
There was a problem hiding this comment.
Do we want this to fail if the user no longer exists?
My understanding is that this will 404 when ifExists=false and there is no user (I assume they are already deleted). If we set ifExist=true, it will succeed. I think that would be better.
What do you think?
There was a problem hiding this comment.
my first though was that setting it to true might hide an error if we send an incorrect user id, this is not found and we return success. but that could be an edge case and in most scenarios, given c1 provides the id to delete, it should be fine to use ifExists=true like you suggests. changing it
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@pkg/connector/users.go`:
- Around line 404-409: The current quoting of quotedUserName in
pkg/connector/users.go blindly wraps userName with double quotes but does not
escape any embedded double quotes, causing malformed identifiers; update the
construction of quotedUserName (the variable used in the DeleteUserREST call) to
escape internal double quotes by replacing each " with "" (e.g., use
strings.ReplaceAll(userName, "\"", "\"\"") and then wrap the result with
surrounding quotes) so DeleteUserREST receives a correctly escaped,
case-sensitive identifier.
- Around line 340-347: The GetUser retry loop fails to close resp.Body leading
to leaks; update the loop around o.client.GetUser(ctx, userName) to close the
response body whenever resp != nil: close resp.Body before returning on success
(after checking resp.StatusCode == http.StatusOK) and also close it on error
paths before continuing retries or returning. Use an explicit resp.Body.Close()
guarded by a nil check (do not defer inside the loop) so each attempt cleans up
its response body.
- Around line 248-254: The quotedUserName construction in the create user flow
doesn't escape embedded double quotes, which breaks Snowflake's quoted
identifier rules; update the logic that builds quotedUserName (used when
constructing the snowflake.CreateUserRequest{Name: quotedUserName}) to first
replace each internal `"` in userName with two `""` per Snowflake escaping and
then wrap the result in surrounding double quotes so embedded quotes are doubled
and the final identifier is valid.
In `@pkg/snowflake/account_role.go`:
- Around line 87-100: The first HTTP response stored in resp is leaked because
resp is reassigned later; before calling c.GetStatementResponse and reassigning
resp you must explicitly close the original response body (call
closeResponseBody(resp)) or use a separate variable for the second response to
avoid shadowing; update the block around the c.GetStatementResponse and c.Do
calls (references: resp, defer closeResponseBody(resp), c.GetStatementResponse,
c.Do) so the first response is closed before reassigning or use a new resp2 for
the second request and defer closeResponseBody(resp2) instead.
In `@pkg/snowflake/database.go`:
- Around line 64-77: The first HTTP response stored in resp is being re-assigned
later, so the initial defer closeResponseBody(resp) captures the variable and
ends up closing only the second response; fix by capturing the first response by
value before reassigning (e.g., r1 := resp; defer closeResponseBody(r1)) right
after the first Do call (and likewise ensure any subsequent resp defers capture
their own local copies), locate this around the sequence using
GetStatementResponse, Do, and closeResponseBody in the ListDatabases flow to
prevent the initial response body from leaking.
In `@pkg/snowflake/user.go`:
- Around line 204-214: The first HTTP response `resp` is reassigned before its
body is closed because both defers capture the same `resp` variable; fix by
capturing the first response value in a new local variable and deferring close
on that value before calling `GetStatementResponse`/`Do` again. Concretely,
after the initial `resp, err = c.Do(...)` succeed, create a new variable (e.g.
`firstResp := resp`) and call `defer closeResponseBody(firstResp)`, then proceed
to call `req, err = c.GetStatementResponse(...)` and the subsequent `resp, err =
c.Do(...)`; ensure the final response is closed with its own defer (e.g. `defer
closeResponseBody(resp)`) so each response body is closed exactly once.
🧹 Nitpick comments (2)
pkg/connector/users.go (1)
350-356: Fragile error string parsing for 422 detection.Parsing error messages with string matching is brittle. If the error message format changes or is localized, this detection will fail silently.
💡 Consider a more robust approach
Consider creating a typed error or using error wrapping in
GetUserthat can be checked witherrors.Isorerrors.As, rather than parsing the error string. Alternatively, rely solely on the status code check which is more reliable:// Check if we got a 422 error - is422 := false - if resp != nil && resp.StatusCode == http.StatusUnprocessableEntity { - is422 = true - } else if err != nil { - errStr := strings.ToLower(err.Error()) - is422 = strings.Contains(errStr, "422") || strings.Contains(errStr, "unprocessable entity") - } + is422 := resp != nil && resp.StatusCode == http.StatusUnprocessableEntitypkg/snowflake/user_rest.go (1)
133-135: Consider using structured logger instead of standard log.Printf.The rest of the codebase uses
ctxzapfor structured logging, but this useslog.Printf. For consistency and to avoid mixing log formats, consider using the context logger. However, since this is inside a deferred cleanup, accessing ctx might be awkward.
| // Build create user request | ||
| // name is the only required field for the create user request | ||
| // Quote the username to preserve case sensitivity (Snowflake stores unquoted identifiers in uppercase) | ||
| quotedUserName := fmt.Sprintf("\"%s\"", userName) | ||
| createReq := &snowflake.CreateUserRequest{ | ||
| Name: quotedUserName, | ||
| } |
There was a problem hiding this comment.
Username quoting doesn't escape embedded double quotes.
If userName contains a double quote character (e.g., foo"bar), the resulting identifier "foo"bar" will be malformed. Snowflake requires double quotes within quoted identifiers to be escaped by doubling them.
🛠️ Proposed fix
+ // Escape double quotes in the username by doubling them (Snowflake convention)
+ escapedUserName := strings.ReplaceAll(userName, `"`, `""`)
// Quote the username to preserve case sensitivity (Snowflake stores unquoted identifiers in uppercase)
- quotedUserName := fmt.Sprintf("\"%s\"", userName)
+ quotedUserName := fmt.Sprintf("\"%s\"", escapedUserName)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Build create user request | |
| // name is the only required field for the create user request | |
| // Quote the username to preserve case sensitivity (Snowflake stores unquoted identifiers in uppercase) | |
| quotedUserName := fmt.Sprintf("\"%s\"", userName) | |
| createReq := &snowflake.CreateUserRequest{ | |
| Name: quotedUserName, | |
| } | |
| // Build create user request | |
| // name is the only required field for the create user request | |
| // Escape double quotes in the username by doubling them (Snowflake convention) | |
| escapedUserName := strings.ReplaceAll(userName, `"`, `""`) | |
| // Quote the username to preserve case sensitivity (Snowflake stores unquoted identifiers in uppercase) | |
| quotedUserName := fmt.Sprintf("\"%s\"", escapedUserName) | |
| createReq := &snowflake.CreateUserRequest{ | |
| Name: quotedUserName, | |
| } |
🤖 Prompt for AI Agents
In `@pkg/connector/users.go` around lines 248 - 254, The quotedUserName
construction in the create user flow doesn't escape embedded double quotes,
which breaks Snowflake's quoted identifier rules; update the logic that builds
quotedUserName (used when constructing the snowflake.CreateUserRequest{Name:
quotedUserName}) to first replace each internal `"` in userName with two `""`
per Snowflake escaping and then wrap the result in surrounding double quotes so
embedded quotes are doubled and the final identifier is valid.
There was a problem hiding this comment.
as the comment says we quote the username to preserve case sensitivity so that for example testUser is stored like so and not like TESTUSER.
There was a problem hiding this comment.
@agustin-conductor, I understand you're quoting to preserve case sensitivity (e.g., testUser instead of TESTUSER), which makes sense for Snowflake identifiers.
My comment was about a different aspect: if a username itself contains a " character (e.g., test"user), the current code would produce "test"user", which is syntactically invalid in Snowflake. The standard approach is to escape embedded quotes by doubling them ("test""user"), but if your usernames won't contain double quotes or you're handling this validation elsewhere, that's fine.
I'll leave this for your consideration based on your use case.
There was a problem hiding this comment.
Is possible that a user contains a ' " ' character? Is that allowed by Snowflake?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
The question was for you @coderabbitai . Does Snowflake allows quotes to be as part of the username?
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Snowflake user name identifier rules double quotes allowed
💡 Result:
In Snowflake, a user name is an identifier, so the normal Snowflake identifier rules apply:
- Unquoted user names must start with a letter (or
_per general identifier rules) and can contain only letters, digits,_, and$. Unquoted names are stored/resolved uppercase. [2] - Double quotes are allowed. If you enclose the entire user name in double quotes, it may contain spaces/special characters and becomes case-sensitive (must be referenced exactly, including quotes, thereafter). [1] [2]
- If you need a literal
"inside a quoted identifier, escape it by doubling it (""). [2]
Example:
CREATE USER "My User" PASSWORD='...';[1] [2]
@JavierCarnelli-ConductorOne, yes, Snowflake does allow a literal double quote character (") to be part of the actual username, but it must be escaped by doubling it within the quoted identifier.
Here's how it works:
- Usernames are identifiers in Snowflake and follow standard identifier rules
- When you wrap a username in double quotes (like
"testUser"), it becomes case-sensitive - If the actual username value contains a
"character (e.g.,test"user), you must escape it by doubling:"test""user"
For example, to create a user named test"user:
CREATE USER "test""user" PASSWORD='...';The current code doesn't handle this escaping. If someone provides test"user as input, the code would generate "test"user", which is syntactically invalid SQL. The original suggestion to add strings.ReplaceAll(userName, ", "") before quoting would handle this edge case.
Whether you need this depends on your validation strategy:
- If usernames are validated/sanitized elsewhere to prevent
"characters, the current code is fine - If you accept arbitrary usernames, the escaping is needed for correctness
There was a problem hiding this comment.
Knowing this @agustin-conductor , you will need to scape the inner quotes on usernames. I had to do something really similar for Microsoft Dynamics F&O, but with single quotes.
It's ugly but it is what it is.
If you have the access to a instance to actually test this situation, please do before making changes
| for attempt := 0; attempt <= maxRetries; attempt++ { | ||
| user, resp, err := o.client.GetUser(ctx, userName) | ||
| if err == nil && resp != nil && resp.StatusCode == http.StatusOK { | ||
| l.Debug("user fetched successfully via SQL API", | ||
| zap.String("user_name", userName), | ||
| ) | ||
| return user, nil | ||
| } |
There was a problem hiding this comment.
Response body not closed after GetUser call.
The static analyzer correctly identifies that resp.Body is never closed. Per coding guidelines, always close response bodies. The retry loop may leak multiple response bodies across retries.
🛠️ Proposed fix
for attempt := 0; attempt <= maxRetries; attempt++ {
user, resp, err := o.client.GetUser(ctx, userName)
+ if resp != nil && resp.Body != nil {
+ resp.Body.Close()
+ }
if err == nil && resp != nil && resp.StatusCode == http.StatusOK {
l.Debug("user fetched successfully via SQL API",
zap.String("user_name", userName),Based on learnings: "Always check if http.Response is not nil before attempting to close resp.Body, as resp can be nil on network errors"
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for attempt := 0; attempt <= maxRetries; attempt++ { | |
| user, resp, err := o.client.GetUser(ctx, userName) | |
| if err == nil && resp != nil && resp.StatusCode == http.StatusOK { | |
| l.Debug("user fetched successfully via SQL API", | |
| zap.String("user_name", userName), | |
| ) | |
| return user, nil | |
| } | |
| for attempt := 0; attempt <= maxRetries; attempt++ { | |
| user, resp, err := o.client.GetUser(ctx, userName) | |
| if resp != nil && resp.Body != nil { | |
| resp.Body.Close() | |
| } | |
| if err == nil && resp != nil && resp.StatusCode == http.StatusOK { | |
| l.Debug("user fetched successfully via SQL API", | |
| zap.String("user_name", userName), | |
| ) | |
| return user, nil | |
| } |
🧰 Tools
🪛 GitHub Check: go-lint
[failure] 341-341:
response body must be closed (bodyclose)
🤖 Prompt for AI Agents
In `@pkg/connector/users.go` around lines 340 - 347, The GetUser retry loop fails
to close resp.Body leading to leaks; update the loop around
o.client.GetUser(ctx, userName) to close the response body whenever resp != nil:
close resp.Body before returning on success (after checking resp.StatusCode ==
http.StatusOK) and also close it on error paths before continuing retries or
returning. Use an explicit resp.Body.Close() guarded by a nil check (do not
defer inside the loop) so each attempt cleans up its response body.
pkg/connector/users.go
Outdated
| // Quote the username to match the case-sensitive identifier created with quotes | ||
| // This ensures we delete the exact case-sensitive identifier | ||
| quotedUserName := fmt.Sprintf("\"%s\"", userName) | ||
| // Delete user via REST API | ||
| // Using default options (ifExists=false) | ||
| _, err := o.client.DeleteUserREST(ctx, quotedUserName, nil) |
There was a problem hiding this comment.
Same quoting issue as CreateAccount - embedded double quotes not escaped.
Apply the same fix here for consistency and correctness.
🛠️ Proposed fix
+ // Escape double quotes in the username by doubling them (Snowflake convention)
+ escapedUserName := strings.ReplaceAll(userName, `"`, `""`)
// Quote the username to match the case-sensitive identifier created with quotes
- quotedUserName := fmt.Sprintf("\"%s\"", userName)
+ quotedUserName := fmt.Sprintf("\"%s\"", escapedUserName)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Quote the username to match the case-sensitive identifier created with quotes | |
| // This ensures we delete the exact case-sensitive identifier | |
| quotedUserName := fmt.Sprintf("\"%s\"", userName) | |
| // Delete user via REST API | |
| // Using default options (ifExists=false) | |
| _, err := o.client.DeleteUserREST(ctx, quotedUserName, nil) | |
| // Escape double quotes in the username by doubling them (Snowflake convention) | |
| escapedUserName := strings.ReplaceAll(userName, `"`, `""`) | |
| // Quote the username to match the case-sensitive identifier created with quotes | |
| // This ensures we delete the exact case-sensitive identifier | |
| quotedUserName := fmt.Sprintf("\"%s\"", escapedUserName) | |
| // Delete user via REST API | |
| // Using default options (ifExists=false) | |
| _, err := o.client.DeleteUserREST(ctx, quotedUserName, nil) |
🤖 Prompt for AI Agents
In `@pkg/connector/users.go` around lines 404 - 409, The current quoting of
quotedUserName in pkg/connector/users.go blindly wraps userName with double
quotes but does not escape any embedded double quotes, causing malformed
identifiers; update the construction of quotedUserName (the variable used in the
DeleteUserREST call) to escape internal double quotes by replacing each " with
"" (e.g., use strings.ReplaceAll(userName, "\"", "\"\"") and then wrap the
result with surrounding quotes) so DeleteUserREST receives a correctly escaped,
case-sensitive identifier.
There was a problem hiding this comment.
same as other response, quoting is so that snowflake respects case sensitivity if we send testUser snowflakes process it as TESTUSER, so we send scaped quoted string "testUser"
There was a problem hiding this comment.
@agustin-conductor, thank you for the explanation! I understand the quoting is for preserving case sensitivity.
My concern was specifically about the edge case where a username itself contains a double-quote character (e.g., test"User). In that scenario, the current code would produce "test"User", which would be malformed. The standard Snowflake approach would be to escape it as "test""User" (doubling the internal quote).
However, if Snowflake usernames cannot contain double quotes (or if there's validation elsewhere that prevents this), then this edge case wouldn't occur in practice and the current implementation is fine. I'll defer to your judgment on whether this is a concern for your use case.
pkg/snowflake/account_role.go
Outdated
| defer closeResponseBody(resp) | ||
|
|
||
| l := ctxzap.Extract(ctx) | ||
| l.Debug("ListAccountRoles", zap.String("response.code", response.Code), zap.String("response.message", response.Message)) | ||
|
|
||
| req, err = c.GetStatementResponse(ctx, response.StatementHandle) | ||
| if err != nil { | ||
| return nil, resp, err | ||
| return nil, nil, err | ||
| } | ||
| resp, err = c.Do(req, uhttp.WithJSONResponse(&response)) | ||
| if err != nil { | ||
| return nil, resp, err | ||
| } | ||
| defer closeResponseBody(resp) |
There was a problem hiding this comment.
First HTTP response body may leak due to variable reassignment.
Same pattern issue: resp is reassigned at line 96, both defers will close only the second response.
🛠️ Proposed fix
resp, err := c.Do(req, uhttp.WithJSONResponse(&response))
if err != nil {
return nil, nil, err
}
- defer closeResponseBody(resp)
+ closeResponseBody(resp)
l := ctxzap.Extract(ctx)
l.Debug("ListAccountRoles", zap.String("response.code", response.Code), zap.String("response.message", response.Message))
req, err = c.GetStatementResponse(ctx, response.StatementHandle)
if err != nil {
return nil, nil, err
}
- resp, err = c.Do(req, uhttp.WithJSONResponse(&response))
+ resp2, err := c.Do(req, uhttp.WithJSONResponse(&response))
if err != nil {
- return nil, resp, err
+ return nil, resp2, err
}
- defer closeResponseBody(resp)
+ defer closeResponseBody(resp2)🤖 Prompt for AI Agents
In `@pkg/snowflake/account_role.go` around lines 87 - 100, The first HTTP response
stored in resp is leaked because resp is reassigned later; before calling
c.GetStatementResponse and reassigning resp you must explicitly close the
original response body (call closeResponseBody(resp)) or use a separate variable
for the second response to avoid shadowing; update the block around the
c.GetStatementResponse and c.Do calls (references: resp, defer
closeResponseBody(resp), c.GetStatementResponse, c.Do) so the first response is
closed before reassigning or use a new resp2 for the second request and defer
closeResponseBody(resp2) instead.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@pkg/connector/users.go`:
- Around line 212-221: CreateAccountCapabilityDetails advertises an unsupported
ENCRYPTED_PASSWORD option; update the SupportedCredentialOptions to only include
the options actually handled by CreateAccount (replace
v2.CapabilityDetailCredentialOption_CAPABILITY_DETAIL_CREDENTIAL_OPTION_ENCRYPTED_PASSWORD
with
v2.CapabilityDetailCredentialOption_CAPABILITY_DETAIL_CREDENTIAL_OPTION_PLAINTEXT_PASSWORD)
and ensure PreferredCredentialOption is one of those (e.g., keep
v2.CapabilityDetailCredentialOption_CAPABILITY_DETAIL_CREDENTIAL_OPTION_RANDOM_PASSWORD).
Modify the slice in CreateAccountCapabilityDetails and the
PreferredCredentialOption constant to match the real capabilities.
In `@pkg/snowflake/account_role.go`:
- Around line 82-99: Ensure any non-nil *http.Response returned alongside an
error from c.Do(...) is closed before returning: in the error branches after the
first and second calls to c.Do (the ones using
uhttp.WithJSONResponse(&response)), check if resp != nil and call
closeResponseBody(resp) prior to returning the error; also do the same for the
second c.Do call. This fix touches the error handling around c.Do,
GetStatementResponse, and uses closeResponseBody to avoid leaking response
bodies in ListAccountRoles.
- Around line 120-135: The response body defer is placed after the error check
so it won't run when c.Do returns a non-nil resp with an error; move the defer
closeResponseBody(resp) to immediately after each c.Do call (before checking
err) for the calls using c.Do(..., uhttp.WithJSONResponse(&response)) so that
resp is always closed even on error—apply this change for both the initial c.Do
and the subsequent c.Do following GetStatementResponse in the account role logic
(look for variables resp, err, c.Do, GetStatementResponse, response, and
closeResponseBody in pkg/snowflake/account_role.go).
- Around line 180-185: The response body can leak when c.Do(req) returns a
non-nil resp and an error; in the code around c.Do(req) and
closeResponseBody(resp) (inside the function handling the HTTP call), either
move the defer closeResponseBody(resp) to immediately after assigning resp
(i.e., right after resp, err := c.Do(req)) so it always runs, or explicitly call
closeResponseBody(resp) on the error path before returning when err != nil;
update the logic in that function to ensure resp is closed in both success and
error cases.
In `@pkg/snowflake/user.go`:
- Around line 233-241: When handling the error from c.Do(req,
uhttp.WithJSONResponse(&response)) in the GetUserRawResponse path, ensure you
close the HTTP response body to avoid leaks: if err != nil and resp != nil, call
closeResponseBody(resp) before determining statusCode and returning; keep the
existing statusCode extraction from resp.StatusCode and return the error as
before. This change should be made in the error branch after calling c.Do in the
function that uses GetUserRawResponse and uhttp.WithJSONResponse.
- Around line 198-213: The two c.Do calls that populate ListUsersRawResponse can
return a non-nil resp alongside an err, but currently we only defer
closeResponseBody(resp) on the success path; update both error checks after c.Do
to explicitly close the response body before returning (call
closeResponseBody(resp) when err != nil) so connections are not leaked, keeping
the existing defer closeResponseBody(resp) for the success path; change the
error branches around c.Do (the first call that sets response and the second
call after GetStatementResponse using response.StatementHandle) to close resp on
error.
| // CreateAccountCapabilityDetails returns the capability details for user account provisioning. | ||
| func (o *userBuilder) CreateAccountCapabilityDetails(ctx context.Context) (*v2.CredentialDetailsAccountProvisioning, annotations.Annotations, error) { | ||
| return &v2.CredentialDetailsAccountProvisioning{ | ||
| SupportedCredentialOptions: []v2.CapabilityDetailCredentialOption{ | ||
| v2.CapabilityDetailCredentialOption_CAPABILITY_DETAIL_CREDENTIAL_OPTION_RANDOM_PASSWORD, | ||
| v2.CapabilityDetailCredentialOption_CAPABILITY_DETAIL_CREDENTIAL_OPTION_ENCRYPTED_PASSWORD, | ||
| }, | ||
| PreferredCredentialOption: v2.CapabilityDetailCredentialOption_CAPABILITY_DETAIL_CREDENTIAL_OPTION_RANDOM_PASSWORD, | ||
| }, nil, nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether encrypted password handling exists
rg -n "GetEncryptedPassword|EncryptedPassword" -g "*.go"
rg -n "Decrypt" -g "*.go"Repository: ConductorOne/baton-snowflake
Length of output: 45200
🏁 Script executed:
#!/bin/bash
# Find and examine the CreateAccount method
rg -n "func.*CreateAccount" -g "*.go" pkg/
# Also check what credential handling exists in users.go
rg -n "CredentialOptions|RandomPassword|PlaintextPassword|EncryptedPassword" pkg/connector/users.go -A 3 -B 1Repository: ConductorOne/baton-snowflake
Length of output: 1457
🏁 Script executed:
#!/bin/bash
# Read the CreateAccount method more completely
sed -n '224,350p' pkg/connector/users.go | cat -nRepository: ConductorOne/baton-snowflake
Length of output: 5330
Remove unsupported encrypted password capability.
CreateAccountCapabilityDetails advertises CAPABILITY_DETAIL_CREDENTIAL_OPTION_ENCRYPTED_PASSWORD, but CreateAccount only handles random and plaintext passwords. The encrypted password option is never used, which creates a capability mismatch that could lead to silent misconfiguration.
🔧 Recommended fix
SupportedCredentialOptions: []v2.CapabilityDetailCredentialOption{
v2.CapabilityDetailCredentialOption_CAPABILITY_DETAIL_CREDENTIAL_OPTION_RANDOM_PASSWORD,
- v2.CapabilityDetailCredentialOption_CAPABILITY_DETAIL_CREDENTIAL_OPTION_ENCRYPTED_PASSWORD,
},🤖 Prompt for AI Agents
In `@pkg/connector/users.go` around lines 212 - 221,
CreateAccountCapabilityDetails advertises an unsupported ENCRYPTED_PASSWORD
option; update the SupportedCredentialOptions to only include the options
actually handled by CreateAccount (replace
v2.CapabilityDetailCredentialOption_CAPABILITY_DETAIL_CREDENTIAL_OPTION_ENCRYPTED_PASSWORD
with
v2.CapabilityDetailCredentialOption_CAPABILITY_DETAIL_CREDENTIAL_OPTION_PLAINTEXT_PASSWORD)
and ensure PreferredCredentialOption is one of those (e.g., keep
v2.CapabilityDetailCredentialOption_CAPABILITY_DETAIL_CREDENTIAL_OPTION_RANDOM_PASSWORD).
Modify the slice in CreateAccountCapabilityDetails and the
PreferredCredentialOption constant to match the real capabilities.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/snowflake/database.go`:
- Around line 97-101: The defer closeResponseBody(resp) is registered before
checking the error from c.Do, which risks calling closeResponseBody with a nil
resp; move the defer so it is executed only after confirming err == nil and resp
!= nil (i.e., call defer closeResponseBody(resp) immediately after the
successful return from c.Do), or alternatively wrap the close in a nil-check
closure, ensuring the response body is only closed when resp is non-nil; update
the call site around c.Do and the closeResponseBody usage to prevent nil
dereference.
🧹 Nitpick comments (2)
pkg/snowflake/user_rest.go (1)
130-132: Consider using structured logging instead oflog.Printf.The rest of the codebase uses
ctxzapfor structured logging (see lines 154, 169, 183, 201, 208). Usinglog.Printfhere is inconsistent.♻️ Suggested refactor
func (c *Client) doRequest( ctx context.Context, method string, endpoint *url.URL, target interface{}, body interface{}, opts ...uhttp.RequestOption, ) (*http.Header, *v2.RateLimitDescription, int, error) { + l := ctxzap.Extract(ctx) // ... existing code ... defer func() { if response == nil || response.Body == nil { return } _, _ = io.Copy(io.Discard, response.Body) closeErr := response.Body.Close() if closeErr != nil { - log.Printf("baton-snowflake: warning: failed to close response body: %v", closeErr) + l.Warn("baton-snowflake: failed to close response body", zap.Error(closeErr)) } }()pkg/snowflake/database.go (1)
108-112: Error messages missing connector prefix.Per coding guidelines, error messages should include the connector prefix "baton-snowflake" for consistency and easier debugging.
♻️ Proposed fix
if len(databases) == 0 { - return nil, fmt.Errorf("database with name %s not found", name) + return nil, fmt.Errorf("baton-snowflake: database with name %s not found", name) } else if len(databases) > 1 { - return nil, fmt.Errorf("expected 1 database with name %s, got %d", name, len(databases)) + return nil, fmt.Errorf("baton-snowflake: expected 1 database with name %s, got %d", name, len(databases)) }As per coding guidelines: "include the connector prefix (baton-*) for all error messages"
luisina-santos
left a comment
luisina-santos
left a comment
There was a problem hiding this comment.
LGTM, I know it is out of scope but I would love to have a task to improve the code and use doRequest as a helper function across all API calls
| func (o *userBuilder) CreateAccountCapabilityDetails(ctx context.Context) (*v2.CredentialDetailsAccountProvisioning, annotations.Annotations, error) { | ||
| return &v2.CredentialDetailsAccountProvisioning{ | ||
| SupportedCredentialOptions: []v2.CapabilityDetailCredentialOption{ | ||
| v2.CapabilityDetailCredentialOption_CAPABILITY_DETAIL_CREDENTIAL_OPTION_RANDOM_PASSWORD, |
There was a problem hiding this comment.
If I'm not wrong you need to add the encrypted password option here to be able to use it on account creation
pkg/snowflake/user_rest.go
Outdated
| } | ||
|
|
||
| statusCode := response.StatusCode | ||
| if statusCode >= 300 { |
There was a problem hiding this comment.
I think this is redundant since we are using uhttp
agustin-conductor
force-pushed
the
feature/user_provisioning
branch
from
64749e0 to
7ff122d
Compare
FeliLucero1
left a comment
FeliLucero1
left a comment
There was a problem hiding this comment.
I would add something to GetUser to prevent injection risk, but I understand these aren't your changes; everything else LGTM
| func closeResponseBody(resp *http.Response) { | ||
| if resp != nil && resp.Body != nil { | ||
| _, _ = io.Copy(io.Discard, resp.Body) | ||
| _ = resp.Body.Close() |
There was a problem hiding this comment.
Is this ignoring the error, right? Should we do that?
There was a problem hiding this comment.
i think its fine in this case, this function is to ensure the response body is closed, if there was an error with the api call that should be handled in its own.
agustin-conductor
force-pushed
the
feature/user_provisioning
branch
from
6154370 to
5a0394d
Compare
5 participants
Adds user provisioning and deprovisioning
Summary by CodeRabbit
New Features
Improvements