MySQL/PgSQL Authentication unit tests (Phase 2.2)#5485
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughA new unit test suite for authentication was added: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements Phase 2.2 of the Unit Testing Framework by adding comprehensive unit tests for Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a comprehensive suite of unit tests for the MySQL and PostgreSQL authentication subsystems. The tests are well-structured, cover a wide range of scenarios as detailed in the description, and significantly improve the test coverage for these critical components. My review focuses on a few areas for improvement: using C++-style casts for better type safety, clarifying a test failure message, and fixing a potential memory leak in one of the test cases. Overall, this is a valuable contribution to the project's testing infrastructure.
| "PgSQL: lookup() returns correct max_connections"); | ||
|
|
||
| if (password) free(password); | ||
| if (attrs) free(attrs); |
There was a problem hiding this comment.
There is a potential memory leak here. The lookup function for PgSQL_Authentication can allocate memory for sha1_pass if it exists for the user. The pointer sha1 is passed to lookup, but it's never freed. While in this specific test case sha1_pass is not set for the user pg_alice, so no memory is allocated, this is fragile. Future changes or other tests could introduce a leak.
You should add a check to free sha1 if it was allocated.
if (attrs) free(attrs);
if (sha1) free(sha1);| static bool mysql_add_frontend(MySQL_Authentication *auth, | ||
| const char *user, const char *pass, | ||
| int default_hg = 0, int max_conn = 100) | ||
| { | ||
| return auth->add( | ||
| (char *)user, (char *)pass, USERNAME_FRONTEND, | ||
| false, // use_ssl | ||
| default_hg, // default_hostgroup | ||
| (char *)"", // default_schema | ||
| false, // schema_locked | ||
| false, // transaction_persistent | ||
| false, // fast_forward | ||
| max_conn, // max_connections | ||
| (char *)"", // attributes | ||
| (char *)"" // comment | ||
| ); | ||
| } |
There was a problem hiding this comment.
Throughout this file, C-style casts like (char *) are used to cast away const from string literals and const char* parameters. This is risky and not idiomatic C++. It's better to use const_cast<char*>(). This makes the intent explicit and is type-safer than a C-style cast.
While the underlying API in MySQL_Authentication is not const-correct, using const_cast in the test code is a good practice.
This applies to other helper functions and direct API calls in this file as well.
static bool mysql_add_frontend(MySQL_Authentication *auth,
const char *user, const char *pass,
int default_hg = 0, int max_conn = 100)
{
return auth->add(
const_cast<char *>(user), const_cast<char *>(pass), USERNAME_FRONTEND,
false, // use_ssl
default_hg, // default_hostgroup
const_cast<char *>(""), // default_schema
false, // schema_locked
false, // transaction_persistent
false, // fast_forward
max_conn, // max_connections
const_cast<char *>(""), // attributes
const_cast<char *>("") // comment
);
}
test/tap/tests/unit/auth_unit-t.cpp
Outdated
| ok(memcmp(ad.sha1_pass, sha1_hash, SHA_DIGEST_LENGTH) == 0, | ||
| "MySQL: SHA1 hash matches what was set"); | ||
| } else { | ||
| ok(0, "MySQL: SHA1 hash matches (skipped - null)"); |
There was a problem hiding this comment.
The message MySQL: SHA1 hash matches (skipped - null) for a failing test (ok(0, ...) is a failure) is confusing. The message suggests the test was skipped, but it's actually reported as a failure. This can make debugging test failures more difficult.
A clearer message would state that the test failed because the hash was unexpectedly null.
ok(0, "MySQL: SHA1 hash was unexpectedly null");There was a problem hiding this comment.
Pull request overview
Adds a new TAP-based unit test binary to exercise the MySQL_Authentication and PgSQL_Authentication components in isolation using the Phase 2.1 unit-test harness.
Changes:
- Introduces
auth_unit-t.cppwith 58 TAP assertions spanning MySQL and PgSQL authentication CRUD, connection counting, bulk inactive patterns, checksums, and memory usage. - Extends the unit-test Makefile to build the new
auth_unit-ttarget alongside the existing smoke test.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
test/tap/tests/unit/auth_unit-t.cpp |
New unit test suite covering MySQL/PgSQL authentication behaviors via the harness globals. |
test/tap/tests/unit/Makefile |
Adds auth_unit-t to UNIT_TESTS and provides a build rule for it. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| static void test_mysql_clear_text_password() { | ||
| mysql_add_frontend(GloMyAuth, "ctpuser", "original"); | ||
|
|
||
| bool set_ok = GloMyAuth->set_clear_text_password( | ||
| (char *)"ctpuser", USERNAME_FRONTEND, | ||
| "clearpass", PASSWORD_TYPE::PRIMARY); | ||
| ok(set_ok == true, | ||
| "MySQL: set_clear_text_password(PRIMARY) returns true"); | ||
|
|
||
| set_ok = GloMyAuth->set_clear_text_password( | ||
| (char *)"ctpuser", USERNAME_FRONTEND, | ||
| "altpass", PASSWORD_TYPE::ADDITIONAL); | ||
| ok(set_ok == true, | ||
| "MySQL: set_clear_text_password(ADDITIONAL) returns true"); | ||
|
|
||
| bool set_fail = GloMyAuth->set_clear_text_password( | ||
| (char *)"nobody", USERNAME_FRONTEND, | ||
| "pass", PASSWORD_TYPE::PRIMARY); | ||
| ok(set_fail == false, | ||
| "MySQL: set_clear_text_password() returns false for nonexistent user"); | ||
| } |
There was a problem hiding this comment.
test_mysql_clear_text_password() only asserts that set_clear_text_password() returns true/false; it never verifies the stored PRIMARY/ADDITIONAL clear-text passwords are actually retrievable. To make this test meaningful (and match the stated intent), perform a lookup() after each set and assert ad.clear_text_password[PRIMARY] / [ADDITIONAL] match the expected values (and free the returned account_details_t).
test/tap/tests/unit/auth_unit-t.cpp
Outdated
| ok(strcmp(ad.password, "new_pass") == 0, | ||
| "MySQL: add() overwrites password on duplicate"); | ||
| ok(ad.default_hostgroup == 2, | ||
| "MySQL: add() overwrites default_hostgroup on duplicate"); | ||
| ok(ad.max_connections == 20, | ||
| "MySQL: add() overwrites max_connections on duplicate"); |
There was a problem hiding this comment.
This test calls strcmp(ad.password, ...) without checking ad.password is non-null. If add() or lookup() ever fails unexpectedly, the test will segfault and abort the whole suite instead of reporting a TAP failure. Prefer the pattern used elsewhere in this file (ad.password != nullptr && strcmp(...) == 0) so failures remain diagnosable.
test/tap/tests/unit/auth_unit-t.cpp
Outdated
| ok(strcmp(fe.password, "frontend_pass") == 0, | ||
| "MySQL: frontend lookup returns frontend password"); | ||
| ok(fe.default_hostgroup == 1, | ||
| "MySQL: frontend lookup returns frontend hostgroup"); | ||
|
|
||
| account_details_t be = GloMyAuth->lookup( | ||
| (char *)"dualuser", USERNAME_BACKEND, dup); | ||
| ok(strcmp(be.password, "backend_pass") == 0, |
There was a problem hiding this comment.
The frontend/backend separation test dereferences fe.password/be.password via strcmp() without a null check. If a lookup fails, this will crash the test binary rather than producing a TAP assertion failure. Add a null guard (as done in other tests here) before calling strcmp() so errors are reported cleanly.
| ok(strcmp(fe.password, "frontend_pass") == 0, | |
| "MySQL: frontend lookup returns frontend password"); | |
| ok(fe.default_hostgroup == 1, | |
| "MySQL: frontend lookup returns frontend hostgroup"); | |
| account_details_t be = GloMyAuth->lookup( | |
| (char *)"dualuser", USERNAME_BACKEND, dup); | |
| ok(strcmp(be.password, "backend_pass") == 0, | |
| ok(fe.password && strcmp(fe.password, "frontend_pass") == 0, | |
| "MySQL: frontend lookup returns frontend password"); | |
| ok(fe.default_hostgroup == 1, | |
| "MySQL: frontend lookup returns frontend hostgroup"); | |
| account_details_t be = GloMyAuth->lookup( | |
| (char *)"dualuser", USERNAME_BACKEND, dup); | |
| ok(be.password && strcmp(be.password, "backend_pass") == 0, |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
- Add null guards before strcmp() in test_mysql_add_overwrites and test_mysql_frontend_backend_separation to prevent segfaults on unexpected lookup failures - Fix misleading "skipped - null" message in SHA1 test fallback to clearly indicate an unexpected null - Free sha1 pointer in PgSQL lookup test to prevent potential memory leak if sha1_pass is ever set - Enhance test_mysql_clear_text_password to verify stored PRIMARY and ADDITIONAL passwords are actually retrievable via lookup(), not just that set_clear_text_password() returns true (2 new test cases, plan updated from 58 to 60)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/tap/tests/unit/auth_unit-t.cpp (2)
183-192: Minor: Consider callingfree_account_details(ad)for consistency.While the struct fields are null for a nonexistent user and the test process is short-lived (per ProxySQL test conventions), adding the cleanup call would maintain consistency with other test functions.
♻️ Optional fix for consistency
ok(ad.password == nullptr, "MySQL: lookup() returns null password for nonexistent user"); ok(ad.username == nullptr, "MySQL: lookup() returns null username for nonexistent user"); + + free_account_details(ad); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/unit/auth_unit-t.cpp` around lines 183 - 192, In test_mysql_lookup_not_found, after calling GloMyAuth->lookup(...) and asserting ad.password and ad.username are null, call free_account_details(ad) to free any allocated resources and match cleanup used in other tests; this uses the existing free_account_details function to release the account_details_t returned by GloMyAuth->lookup.
217-222: Good defensive check, but consider usingskip()for TAP compliance.The conditional avoids a crash if
sha1_passis unexpectedly null, which is good. However,ok(0, "...")reports a failure rather than a skip. If you want proper TAP semantics for conditional tests, you could use theskip()TAP directive.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/unit/auth_unit-t.cpp` around lines 217 - 222, The conditional currently uses ok(0, "...") which records a failure when ad.sha1_pass is null; change that branch to call the TAP skip directive instead (e.g., skip("MySQL: SHA1 hash matches (skipped - null)")) so the test is properly skipped; keep the positive branch as ok(memcmp(ad.sha1_pass, sha1_hash, SHA_DIGEST_LENGTH) == 0, "MySQL: SHA1 hash matches what was set") and only call skip() when ad.sha1_pass == nullptr.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/tap/tests/unit/auth_unit-t.cpp`:
- Around line 183-192: In test_mysql_lookup_not_found, after calling
GloMyAuth->lookup(...) and asserting ad.password and ad.username are null, call
free_account_details(ad) to free any allocated resources and match cleanup used
in other tests; this uses the existing free_account_details function to release
the account_details_t returned by GloMyAuth->lookup.
- Around line 217-222: The conditional currently uses ok(0, "...") which records
a failure when ad.sha1_pass is null; change that branch to call the TAP skip
directive instead (e.g., skip("MySQL: SHA1 hash matches (skipped - null)")) so
the test is properly skipped; keep the positive branch as
ok(memcmp(ad.sha1_pass, sha1_hash, SHA_DIGEST_LENGTH) == 0, "MySQL: SHA1 hash
matches what was set") and only call skip() when ad.sha1_pass == nullptr.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2cdf40fd-b096-421e-8af0-07db0cbd8122
📒 Files selected for processing (2)
test/tap/tests/unit/Makefiletest/tap/tests/unit/auth_unit-t.cpp
Comprehensive unit tests for MySQL_Authentication and PgSQL_Authentication covering 58 test cases across 13 test functions. Runs in <0.01s with no infrastructure dependencies. MySQL_Authentication coverage: - Core CRUD: add/exists/lookup/del with frontend and backend users - Duplicate handling: add() overwrites password, hostgroup, max_connections - exists() only checks frontends (not backends) — documented quirk - lookup() returns empty struct for nonexistent users - SHA1 credential storage and retrieval via set_SHA1() - Clear-text password management (PRIMARY and ADDITIONAL slots) - Connection counting: increase/decrease with max_connections enforcement - Bulk operations: set_all_inactive + remove_inactives config reload pattern - reset() clears all frontend and backend users - Runtime checksum: zero when empty, changes on add/modify - Memory tracking: increases on add, returns to baseline on reset - Frontend/backend separation: same username, different credentials PgSQL_Authentication coverage: - Core CRUD with PgSQL-specific lookup() signature (returns char*) - del() and reset() behavior - Connection counting (no PASSWORD_TYPE parameter) - Inactive pattern (set_all_inactive + remove_inactives)
Registers the Authentication unit test binary in the build system so it is built alongside smoke_test-t via 'make build_tap_tests'.
- Add null guards before strcmp() in test_mysql_add_overwrites and test_mysql_frontend_backend_separation to prevent segfaults on unexpected lookup failures - Fix misleading "skipped - null" message in SHA1 test fallback to clearly indicate an unexpected null - Free sha1 pointer in PgSQL lookup test to prevent potential memory leak if sha1_pass is ever set - Enhance test_mysql_clear_text_password to verify stored PRIMARY and ADDITIONAL passwords are actually retrievable via lookup(), not just that set_clear_text_password() returns true (2 new test cases, plan updated from 58 to 60)
|
Summary
Implements Phase 2.2 (#5474) of the Unit Testing Framework: comprehensive unit tests for
MySQL_AuthenticationandPgSQL_Authentication.Test Coverage
MySQL_Authentication (43 tests)
PgSQL_Authentication (15 tests)
Depends On
Test plan
Summary by CodeRabbit