Skip to content

logout user in the case that the underlying connection gets disconnected#383

Merged
bigbrett merged 4 commits into
wolfSSL:mainfrom
JacobBarthelmeh:auth_connection
Jul 2, 2026
Merged

logout user in the case that the underlying connection gets disconnected#383
bigbrett merged 4 commits into
wolfSSL:mainfrom
JacobBarthelmeh:auth_connection

Conversation

@JacobBarthelmeh

@JacobBarthelmeh JacobBarthelmeh commented May 28, 2026

Copy link
Copy Markdown
Contributor

Follow up to item 4 from #270

@JacobBarthelmeh JacobBarthelmeh self-assigned this May 28, 2026
Copilot AI review requested due to automatic review settings May 28, 2026 04:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR ensures that an authenticated user session is properly logged out when the underlying communication channel is abruptly disconnected (not just on a clean COMM_CLOSE). It also defensively clears any stale auth session state during wh_Server_Init, in case the externally-owned auth context carries a leftover session from a prior connection.

Changes:

  • In wh_Server_SetConnected, invoke wh_Auth_Logout on the transition to WH_COMM_DISCONNECTED when a user is currently active.
  • In wh_Server_Init, after binding the externally-owned auth context, log out any stale active user; fall back to zeroing the user struct if logout fails.
  • Add _whTest_Auth_AbruptDisconnect test that uses a wrapping test_Logout callback to verify the disconnect path triggers exactly one logout and that repeated disconnects are no-ops.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/wh_server.c Adds logout-on-disconnect in wh_Server_SetConnected and stale-session cleanup in wh_Server_Init.
test/wh_test_auth.c Adds a logout-counting callback and a memory-transport test verifying logout fires on abrupt disconnect and is idempotent.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

@JacobBarthelmeh JacobBarthelmeh marked this pull request as ready for review May 29, 2026 21:29
@bigbrett bigbrett assigned rizlik and bigbrett and unassigned bigbrett and wolfSSL-Bot Jun 9, 2026
bigbrett
bigbrett previously approved these changes Jun 10, 2026

@bigbrett bigbrett left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. A few questions and a nit but nothing that would block merge

Comment thread src/wh_server.c Outdated
Comment thread src/wh_server.c Outdated
Comment thread test/wh_test_auth.c Outdated
@bigbrett bigbrett assigned billphipps and unassigned bigbrett Jun 10, 2026
Comment thread src/wh_server.c Outdated
billphipps
billphipps previously approved these changes Jun 10, 2026

@billphipps billphipps left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please consider unconditionally zeroing userid when logging out. Please resolve additional comments.

@JacobBarthelmeh JacobBarthelmeh dismissed stale reviews from billphipps and bigbrett via 15cb710 June 10, 2026 15:58
@JacobBarthelmeh

Copy link
Copy Markdown
Contributor Author

@bigbrett and @billphipps , I believe I've addressed the current feedback. Thank you for reviewing

Comment thread src/wh_auth.c
@JacobBarthelmeh JacobBarthelmeh removed their assignment Jun 11, 2026

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fenrir Automated Review — PR #383

Scan targets checked: wolfhsm-core-bugs, wolfhsm-crypto-bugs, wolfhsm-src

Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Findings are non-blocking.

Comment thread src/wh_server.c
Comment thread src/wh_server.c

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fenrir Automated Review — PR #383

Scan targets checked: wolfhsm-core-bugs, wolfhsm-crypto-bugs, wolfhsm-src

Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Findings are non-blocking.

Comment thread src/wh_server.c
Comment thread src/wh_server.c Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

Comment thread src/wh_server.c
Comment thread wolfhsm/wh_auth.h
Comment thread src/wh_auth.c
@JacobBarthelmeh

Copy link
Copy Markdown
Contributor Author

@bigbrett addressed the logging placement feedback, reviewed a re-kick off of AI reviews and rebased onto master. I think it's ready for re-review, thanks for your patience on this PR.

@bigbrett bigbrett merged commit 8062728 into wolfSSL:main Jul 2, 2026
108 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants