Conversation
There was a problem hiding this comment.
Pull request overview
Adds client-side detection for “password changed” events (via heartbeat passVer updates) so that existing connections start failing requests after a password update, and introduces a regression test to validate the behavior.
Changes:
- Introduce a
passChangedflag in the client connection state and set it when heartbeat observes a newer password version. - Reject new requests on a connection once
passChangedis set, returning a new client error codeTSDB_CODE_TSC_PASS_CHANGED. - Add a Python test that changes a user’s password via root and asserts the old connection can no longer query.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/cases/24-Users/test_user_basic.py | Adds a test case validating that an old connection fails after the user’s password is changed. |
| source/util/src/terror.c | Adds human-readable error string for TSDB_CODE_TSC_PASS_CHANGED. |
| source/client/src/clientHb.c | Sets passChanged when heartbeat reports passVer increased for the user. |
| source/client/src/clientEnv.c | Blocks request creation when passChanged is set, returning TSDB_CODE_TSC_PASS_CHANGED. |
| source/client/inc/clientInt.h | Extends notify/pass info struct with passChanged flag. |
| include/util/taoserror.h | Defines new error code TSDB_CODE_TSC_PASS_CHANGED. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| TAOS_DEFINE_ERROR(TSDB_CODE_TSC_INVALID_TOTP_CODE, "Invalid TOTP code") | ||
| TAOS_DEFINE_ERROR(TSDB_CODE_TSC_INVALID_TOKEN, "Invalid token") | ||
| TAOS_DEFINE_ERROR(TSDB_CODE_TSC_INSTANCE_API_RATE_LIMIT, "instance register/list API rate limit exceeded") | ||
| TAOS_DEFINE_ERROR(TSDB_CODE_TSC_PASS_CHANGED, "Password has been changed from another session, please reconnect") |
There was a problem hiding this comment.
The message says the password was changed "from another session", but the client-side detection is based solely on passVer changes and can also trigger when the same session/user changes the password. Consider rewording to a neutral/accurate message (e.g., "Password has been changed, please reconnect") or otherwise ensuring the condition really implies an external-session change.
| TAOS_DEFINE_ERROR(TSDB_CODE_TSC_PASS_CHANGED, "Password has been changed from another session, please reconnect") | |
| TAOS_DEFINE_ERROR(TSDB_CODE_TSC_PASS_CHANGED, "Password has been changed, please reconnect") |
| tdSql.execute("CREATE USER upasschk PASS 'AAbb1122'") | ||
| conn = taos.connect(user='upasschk', password='AAbb1122') | ||
| cursor = conn.cursor() | ||
| cursor.execute("SELECT SERVER_VERSION()") | ||
| tdLog.info("conn1 query before pass change: success") | ||
|
|
||
| tdSql.execute("ALTER USER upasschk PASS 'CCdd3344!!'") | ||
| tdLog.info("password changed via root conn, waiting for heartbeat...") | ||
| time.sleep(5) | ||
|
|
||
| try: | ||
| cursor.execute("SELECT SERVER_VERSION()") | ||
| tdLog.exit("query should have failed after password change but succeeded") | ||
| except Exception as e: | ||
| tdLog.info(f"query after pass change failed as expected: {e}") | ||
|
|
||
| cursor.close() | ||
| conn.close() | ||
| tdSql.execute("DROP USER upasschk") | ||
| print("do pass change detect ............. [passed]") | ||
|
|
There was a problem hiding this comment.
This test creates a user/connection and only drops/closes them on the success path. If any assertion/exception occurs before the end, the user may be left behind and impact subsequent test runs. Wrap the body in try/finally (and/or drop the user defensively) to guarantee cursor/conn close and user cleanup.
| tdSql.execute("CREATE USER upasschk PASS 'AAbb1122'") | |
| conn = taos.connect(user='upasschk', password='AAbb1122') | |
| cursor = conn.cursor() | |
| cursor.execute("SELECT SERVER_VERSION()") | |
| tdLog.info("conn1 query before pass change: success") | |
| tdSql.execute("ALTER USER upasschk PASS 'CCdd3344!!'") | |
| tdLog.info("password changed via root conn, waiting for heartbeat...") | |
| time.sleep(5) | |
| try: | |
| cursor.execute("SELECT SERVER_VERSION()") | |
| tdLog.exit("query should have failed after password change but succeeded") | |
| except Exception as e: | |
| tdLog.info(f"query after pass change failed as expected: {e}") | |
| cursor.close() | |
| conn.close() | |
| tdSql.execute("DROP USER upasschk") | |
| print("do pass change detect ............. [passed]") | |
| conn = None | |
| cursor = None | |
| tdSql.execute("CREATE USER upasschk PASS 'AAbb1122'") | |
| try: | |
| conn = taos.connect(user='upasschk', password='AAbb1122') | |
| cursor = conn.cursor() | |
| cursor.execute("SELECT SERVER_VERSION()") | |
| tdLog.info("conn1 query before pass change: success") | |
| tdSql.execute("ALTER USER upasschk PASS 'CCdd3344!!'") | |
| tdLog.info("password changed via root conn, waiting for heartbeat...") | |
| time.sleep(5) | |
| try: | |
| cursor.execute("SELECT SERVER_VERSION()") | |
| tdLog.exit("query should have failed after password change but succeeded") | |
| except Exception as e: | |
| tdLog.info(f"query after pass change failed as expected: {e}") | |
| print("do pass change detect ............. [passed]") | |
| finally: | |
| if cursor is not None: | |
| cursor.close() | |
| if conn is not None: | |
| conn.close() | |
| try: | |
| tdSql.execute("DROP USER upasschk") | |
| except Exception as e: | |
| tdLog.info(f"cleanup drop user upasschk failed: {e}") |
| tdLog.info("password changed via root conn, waiting for heartbeat...") | ||
| time.sleep(5) | ||
|
|
||
| try: | ||
| cursor.execute("SELECT SERVER_VERSION()") | ||
| tdLog.exit("query should have failed after password change but succeeded") | ||
| except Exception as e: | ||
| tdLog.info(f"query after pass change failed as expected: {e}") |
There was a problem hiding this comment.
Using a fixed sleep here adds a hard 5s to the suite and can still be timing-sensitive if heartbeat behavior changes. Prefer polling until the expected error is observed (or a bounded timeout), which both reduces runtime and makes the test more deterministic.
| tdLog.info("password changed via root conn, waiting for heartbeat...") | |
| time.sleep(5) | |
| try: | |
| cursor.execute("SELECT SERVER_VERSION()") | |
| tdLog.exit("query should have failed after password change but succeeded") | |
| except Exception as e: | |
| tdLog.info(f"query after pass change failed as expected: {e}") | |
| tdLog.info("password changed via root conn, waiting for old connection to be rejected...") | |
| deadline = time.monotonic() + 5 | |
| last_error = None | |
| while time.monotonic() < deadline: | |
| try: | |
| cursor.execute("SELECT SERVER_VERSION()") | |
| except Exception as e: | |
| last_error = e | |
| break | |
| time.sleep(0.1) | |
| if last_error is None: | |
| tdLog.exit("query should have failed after password change but succeeded") | |
| else: | |
| tdLog.info(f"query after pass change failed as expected: {last_error}") |
There was a problem hiding this comment.
Code Review
This pull request implements password change detection for client connections. It introduces a new error code TSDB_CODE_TSC_PASS_CHANGED, tracks password changes in the connection object via heartbeats, and ensures that subsequent requests on a connection with an outdated password fail. A new test case was added to verify this behavior. A critical issue was identified in the heartbeat processing logic where the reuse of response data across different user connections could lead to incorrect password change detection.
| atomic_store_8(&passInfo->passChanged, 1); | ||
| if (passInfo->fp) { | ||
| (*passInfo->fp)(passInfo->param, &pRsp->passVer, TAOS_NOTIFY_PASSVER); | ||
| } | ||
| tscDebug("update passVer of user %s from %d to %d, conn:%" PRIi64, pRsp->user, oldVer, | ||
| atomic_load_32(&passInfo->ver), pTscObj->id); | ||
| } |
There was a problem hiding this comment.
The pRsp pointer used here is declared outside the while loop (at line 131) and is not reset for each connection iteration. This creates a significant bug in multi-user scenarios: if a heartbeat response contains updates for multiple users, a connection for one user will incorrectly reuse the pRsp data from a previous user's connection processed in the same loop. This will lead to incorrect password change detection, session metric updates, and other auth-related logic for all subsequent connections in the loop. Although the declaration is outside this diff hunk, the new logic added here is directly affected by this issue.
Description
Issue(s)
Checklist
Please check the items in the checklist if applicable.