Drop userauth implementation #1344
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
This PR removes the user authentication implementation from the SQLite Go driver, deprecating the feature and simplifying the codebase. The change converts all user authentication functions to no-ops that return unauthorized errors.
- User authentication functions now return errors instead of performing actual authentication
- Test suite simplified to verify authentication is disabled
- Documentation updated to mark the feature as deprecated
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| sqlite3_opt_userauth.go | Removes C bindings and implementation, converts all auth methods to return ErrUnauthorized |
| sqlite3_opt_userauth_test.go | Simplifies test suite to single test verifying auth is disabled |
| sqlite3.go | Adds conditional check before attempting authentication |
| error_test.go | Improves error handling with type assertion check |
| README.md | Adds deprecation notice for user authentication |
| .github/workflows/go.yaml | Updates Go versions and adds sqlite_userauth tag to CI |
sqlite3.go
Outdated
| if conn.AuthEnabled() { | ||
| // Preform Authentication | ||
| if err := conn.Authenticate(authUser, authPass); err != nil { | ||
| return nil, err | ||
| } | ||
| } |
There was a problem hiding this comment.
This conditional check is unnecessary since AuthEnabled() now always returns false. The authentication code inside this block will never execute, making it unreachable code.
| if conn.AuthEnabled() { | |
| // Preform Authentication | |
| if err := conn.Authenticate(authUser, authPass); err != nil { | |
| return nil, err | |
| } | |
| } | |
| // Authentication is disabled as AuthEnabled() always returns false. |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
No, we need to remain this code for users who uses Authenticate method.
| t.Fatalf("UserAuth not enabled: %s", err) | ||
| } | ||
| defer db2.Close() | ||
|
|
||
| // Delete user while logged in as normal user | ||
| // through SQL | ||
| rv, err = deleteUser(db2, "user") | ||
| if err != nil { | ||
| if !errors.Is(err, ErrUnauthorized) { | ||
| t.Fatal(err) | ||
| } |
There was a problem hiding this comment.
The test logic is incorrect. Since connect() now always fails (AuthEnabled returns false, but authentication is still attempted), the test should expect an error, but the error checking logic suggests it expects ErrUnauthorized specifically. However, the previous condition checks if err == nil and fails the test, creating contradictory expectations.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
This is a test for connect should return ErrUnauthorized always.
| default: | ||
| return c.lastError() | ||
| } | ||
| return ErrUnauthorized |
There was a problem hiding this comment.
I think we should return something like errors.New("sqlite3: the sqlite_userauth tag is no longer supported as the userauth extension is no longer supported by the SQLite authors, see <GitHub issue link>").
That way, users that have been relying on this extension will get a very clear message as to what the problem is, as opposed to a mysterious "not authorized" error.
sqlite3.go
Outdated
| // Preform Authentication | ||
| if err := conn.Authenticate(authUser, authPass); err != nil { | ||
| return nil, err | ||
| if conn.AuthEnabled() { |
There was a problem hiding this comment.
I don't think we can add this check. Now AuthEnabled() always returns false even if you use the build tag. Therefore, anyone who had been relying on it just would skip auth instead of getting the error.
There was a problem hiding this comment.
If enable sqlite_userauth tag, sqlite3 driver always fail since conn.Authenticate return error eventthough user/pass is not set. This means declaring to developers not to use sqlite_userauth. Is this what you intended?
There was a problem hiding this comment.
Yes. If the user doesn't need this extension, then the fix is simple - stop passing the build tag they don't need.
Closes #1341