Add live authentication support in web app client #103

Open
luca0N wants to merge 6 commits from dev_client_live_auth into dev
Owner

Authenticate with the session token as part of the live connection handshake, if the server indicates that authentication is required.

  • Evaluate whether unit testing is required
  • Add unit testing
  • Squash commits
Authenticate with the session token as part of the live connection handshake, if the server indicates that authentication is required. - [x] Evaluate whether unit testing is required - [ ] Add unit testing - [ ] Squash commits
luca0N self-assigned this 2026-08-17 11:25:52 +00:00
Add live authentication support in web app client
All checks were successful
/ Test client i18n (push) Successful in 41s
/ Test client (push) Successful in 34s
7bd4c744d4
Authenticate with the session token as part of the live connection
handshake, if the server indicates that authentication is required.
Author
Owner

This pull request depends on the functionality being implemented in the server. I have an early draft for this server code, but it's not ready to be committed yet.

This pull request depends on the functionality being implemented in the server. I have an early draft for this server code, but it's not ready to be committed yet.
luca0N force-pushed dev_client_live_auth from 7bd4c744d4
All checks were successful
/ Test client i18n (push) Successful in 41s
/ Test client (push) Successful in 34s
to 1e8f7a28ac
Some checks failed
/ Test client i18n (push) Failing after 40s
/ Test client (push) Failing after 30s
2026-08-18 11:26:47 +00:00
Compare
Author
Owner

Previous force-push from 7bd4c744d4 to 1e8f7a28ac was made to include missing changes. Those were amended into the previous commit.

Previous force-push from 7bd4c744d4 to 1e8f7a28ac was made to include missing changes. Those were amended into the previous commit.
luca0N force-pushed dev_client_live_auth from 1e8f7a28ac
Some checks failed
/ Test client i18n (push) Failing after 40s
/ Test client (push) Failing after 30s
to 809adb1bc2
Some checks failed
/ Test client i18n (push) Failing after 38s
/ Test client (push) Successful in 33s
2026-08-19 23:46:42 +00:00
Compare
Update web app client messages
All checks were successful
/ Test client i18n (push) Successful in 37s
/ Test client (push) Successful in 33s
f6466a4c2e
Update web app client i18n files.
Author
Owner

Force-push from 1e8f7a28ac to 809adb1bc2 was made to fix a syntax error in AccountSession.ts.

Force-push from 1e8f7a28ac to 809adb1bc2 was made to fix a syntax error in `AccountSession.ts`.
luca0N changed title from WIP: Add live authentication support in web app client to Add live authentication support in web app client 2026-08-20 00:02:55 +00:00
@ -31,4 +34,0 @@
* @param token The session token to store.
* @param rememberLogin If `true', then the session token is stored for
* upwards of `SESSION_MAX_TIME' milliseconds, otherwise, the session token
* can only be used during this session.
Author
Owner

This was erroneously removed.

This was erroneously removed.
luca0N marked this conversation as resolved
Restore erroneously erased comments
Some checks failed
/ Test client (push) Has been cancelled
/ Test client i18n (push) Has been cancelled
1714acc11b
Restore some commented lines that were erroneously removed in a previous
commit.  These comments are part of a function documentation header.
Remove unused code
Some checks failed
/ Test client (push) Has been cancelled
/ Test client i18n (push) Has been cancelled
3c7c821397
Remove some unused code that was erroneously commited as part of the
live connection authentication support in the web app client.
Remove unused injection in web app
Some checks failed
/ Test client i18n (push) Failing after 38s
/ Test client (push) Successful in 34s
7d422ed3f2
Remove the unused injected Router in the `LiveConnection.ts` web app
client source file.
Update web app client messages
All checks were successful
/ Test client i18n (push) Successful in 38s
/ Test client (push) Successful in 33s
20d80040e7
Update `messages.xlf` web app client files for i18n.
luca0N left a comment

Unit tests must be added for the new functionality before merging this pull request, particularly for the LiveConnection class, and the AccountSession class needs some special attention (see related comment).

Unit tests must be added for the new functionality before merging this pull request, particularly for the `LiveConnection` class, and the `AccountSession` class needs some special attention (see related comment).
@ -70,0 +101,4 @@
resolve();
});
});
}
Author
Owner

There's no .spec file for the AccountSession class at this time, and adding a unit test for it would be slightly tricky because it modifies browser cookies directly. Thus, there are two ways to create unit testing for this class, as I see it:

  1. create a new class for dealing with browser cookies, or
  2. create a mock for AccountSession that mocks loadSessionToken().

This must be evaluated before merging.

There's no `.spec` file for the `AccountSession` class at this time, and adding a unit test for it would be slightly tricky because it modifies browser cookies directly. Thus, there are two ways to create unit testing for this class, as I see it: 1. create a new class for dealing with browser cookies, or 2. create a mock for `AccountSession` that mocks `loadSessionToken()`. This must be evaluated before merging.
Author
Owner

Commit 49a5487997 introduces the AccountSessionMock class that allows mocking session tokens. However, it still wouldn't make sense to create unit testing for the functions for AccountSession alone; the new mock class only helps with other unit testing sources.

As such, unit testing for AccountSession will not be implemented.

Commit 49a5487997 introduces the `AccountSessionMock` class that allows mocking session tokens. However, it still wouldn't make sense to create unit testing for the functions for `AccountSession` alone; the new mock class only helps with other unit testing sources. As such, unit testing for AccountSession will not be implemented.
luca0N marked this conversation as resolved
@ -297,0 +339,4 @@
return;
}
if (packet.action === PacketAction.RET_AUTHENTICATION_REQUIRED) {
Author
Owner

A unit test must be created for testing the client behavior when RET_AUTHENTICATION_REQUIRED is received.

A unit test must be created for testing the client behavior when `RET_AUTHENTICATION_REQUIRED` is received.
@ -297,0 +351,4 @@
.withData({ session_token: sessionToken })
.build();
this.loggingIn = true;
this.ws.send(JSON.stringify(packet));
Author
Owner

A unit test must be created for testing the LOGIN packet action.

A unit test must be created for testing the `LOGIN` packet action.
Author
Owner

Added by commit 49a5487997.

Added by commit 49a5487997.
luca0N marked this conversation as resolved
Author
Owner

Marking this pull request as a WIP once again (see comments in the previous code review).

Marking this pull request as a WIP once again (see comments in the previous code review).
Add unit test for client live authentication
All checks were successful
/ Test client i18n (push) Successful in 38s
/ Test client (push) Successful in 34s
49a5487997
Create a new unit test that checks whether `LiveConnection` is correctly
authenticating with the `LOGIN` packet action after being notified by
the server that authentication is required (via the
`RET_AUTHENTICATION_REQUIRED` packet action).

This commit introduces a new mock class for `AccountSession`, which
currently only allows for the mocking of account session tokens.  No
other functions were implemented at this time.
@ -105,0 +120,4 @@
wsMock.mockDataIncoming(JSON.stringify({ action: "QUERY_HISTORY" }));
wsMock.mockDataOutgoing(JSON.stringify({ action: "RET_AUTHENTICATION_REQUIRED", data: "" }));
wsMock.mockDataIncoming(JSON.stringify({ action: "LOGIN", data: { session_token: mockedSessionToken } }));
Author
Owner

I have had trouble mocking anything beyond this packet, for some reason. I don't have enough time to solve this at this time, but I will do some more investigation once I have some time.

I have had trouble mocking anything beyond this packet, for some reason. I don't have enough time to solve this at this time, but I will do some more investigation once I have some time.
Author
Owner

Fixed by 008e3298fd.

Fixed by 008e3298fd.
luca0N marked this conversation as resolved
luca0N force-pushed dev_client_live_auth from 49a5487997
All checks were successful
/ Test client i18n (push) Successful in 38s
/ Test client (push) Successful in 34s
to 7d5d4cecd2
Some checks failed
/ Test and build server (push) Successful in 1m15s
/ Test client i18n (push) Successful in 42s
/ Test client (push) Failing after 31s
2026-09-03 00:13:31 +00:00
Compare
Author
Owner

Previous force-push operation from 49a5487997 to 7d5d4cecd2 was made to solve multiple conflicts on multiple files that were introduced after merging !105.

Previous force-push operation from 49a5487997 to 7d5d4cecd2 was made to solve multiple conflicts on multiple files that were introduced after merging !105.
luca0N force-pushed dev_client_live_auth from 7d5d4cecd2
Some checks failed
/ Test and build server (push) Successful in 1m15s
/ Test client i18n (push) Successful in 42s
/ Test client (push) Failing after 31s
to bd532c03a0
Some checks failed
/ Test client i18n (push) Successful in 43s
/ Test client (push) Failing after 39s
2026-09-03 00:25:07 +00:00
Compare
Author
Owner

Previous force-push operation from 7d5d4cecd2 to bd532c03a0 was made to fix a build error that was introduced after rebasing this branch with dev, and to remove a trailing space in a line from AccountSessionMock.ts.

Previous force-push operation from 7d5d4cecd2 to bd532c03a0 was made to fix a build error that was introduced after rebasing this branch with `dev`, and to remove a trailing space in a line from `AccountSessionMock.ts`.
luca0N left a comment

A few issues were introduced after rebasing with dev.

A few issues were introduced after rebasing with dev.
@ -19,6 +19,8 @@
import { Injectable, } from "@angular/core";
import { ServerComms } from "../ServerComms";
Author
Owner

Missing trailing comma (see related comment).

Missing trailing comma ([see related comment](https://git.luca0n.com/luca0N/lanbassador/pulls/103/files#issuecomment-2196)).
Author
Owner

Fixed; see #103 (comment).

Fixed; see https://git.luca0n.com/luca0N/lanbassador/pulls/103#issuecomment-2202.
luca0N marked this conversation as resolved
@ -0,0 +20,4 @@
import { Injectable } from '@angular/core';
import { ServerComms } from "../ServerComms";
import { AccountSessionProvider, AccountSession } from "./AccountSession";
Author
Owner

Missing trailing comma; this violates the project coding conventions. See !105.

Missing trailing comma; this violates the project coding conventions. See !105.
Author
Owner

Fixed; see #103 (comment).

Fixed; see https://git.luca0n.com/luca0N/lanbassador/pulls/103#issuecomment-2202.
luca0N marked this conversation as resolved
@ -1,3 +1,4 @@
Author
Owner

This line break should not be in here.

This line break should not be in here.
Author
Owner

Fixed; see #103 (comment).

Fixed; see https://git.luca0n.com/luca0N/lanbassador/pulls/103#issuecomment-2202.
luca0N marked this conversation as resolved
luca0N force-pushed dev_client_live_auth from bd532c03a0
Some checks failed
/ Test client i18n (push) Successful in 43s
/ Test client (push) Failing after 39s
to a7cdd49d22
Some checks failed
/ Test client i18n (push) Successful in 45s
/ Test client (push) Failing after 5m25s
2026-09-03 23:32:40 +00:00
Compare
Author
Owner

Previous force-push operation from bd532c03a0 to a7cdd49d22 was made to fix previous review comments1 2 3.

Previous force-push operation from bd532c03a0 to a7cdd49d22 was made to fix previous review comments[^1] [^2] [^3]. [^1]: https://git.luca0n.com/luca0N/lanbassador/pulls/103/files#issuecomment-2199 [^2]: https://git.luca0n.com/luca0N/lanbassador/pulls/103/files#issuecomment-2196 [^3]: https://git.luca0n.com/luca0N/lanbassador/pulls/103/files#issuecomment-2198
luca0N force-pushed dev_client_live_auth from a7cdd49d22
Some checks failed
/ Test client i18n (push) Successful in 45s
/ Test client (push) Failing after 5m25s
to e0cd39a034
All checks were successful
/ Test client i18n (push) Successful in 43s
/ Test client (push) Successful in 38s
2026-09-05 23:01:19 +00:00
Compare
Author
Owner

Previous force-push operation from a7cdd49d22 to e0cd39a034 introduced a minor change in the packet builder class to fix a bug. The bug caused client packets to always have an undefined data field.

Previous force-push operation from a7cdd49d22 to e0cd39a034 introduced a minor change in the packet builder class to fix a bug. The bug caused client packets to always have an undefined `data` field.
Test complete live auth authentication
Some checks failed
/ Test client i18n (push) Failing after 41s
/ Test client (push) Successful in 35s
6fc165739a
Test all client actions in the live authentication test.  This commit
also fixes an issue in `LiveConnection.ts` that prevented the test from
working.
luca0N force-pushed dev_client_live_auth from 6fc165739a
Some checks failed
/ Test client i18n (push) Failing after 41s
/ Test client (push) Successful in 35s
to 008e3298fd
All checks were successful
/ Test client i18n (push) Successful in 39s
/ Test client (push) Successful in 37s
2026-09-09 23:57:52 +00:00
Compare
Author
Owner

Previous force-push operation from 6fc165739a to 008e3298fd was made to include changes that were left off in the messages files.

Previous force-push operation from 6fc165739a to 008e3298fd was made to include changes that were left off in the messages files.
luca0N left a comment

Revisiting this pull request after a several days-long hiatus as I was extremely busy with other projects. Hopefully this allows me to review this with a clean mind to catch some stuff that I didn't see earlier while working on commits made herein.

Because I have been inactive in this repository for a few weeks, some of the comments below may not be applicable. I will see this once I get back to coding.

Revisiting this pull request after a several days-long hiatus as I was extremely busy with other projects. Hopefully this allows me to review this with a clean mind to catch some stuff that I didn't see earlier while working on commits made herein. Because I have been inactive in this repository for a few weeks, some of the comments below may not be applicable. I will see this once I get back to coding.
@ -42,0 +39,4 @@
import { Host, LiveConnection, } from './services/LiveConnection';
import { PaginatorIntl, } from './services/PaginatorIntl';
import { PreferencesManager, PreferencesKey, } from './PreferencesManager';
import { ServerComms, } from './ServerComms';
Author
Owner

Double quotation marks have been changed to apostrophe characters; this should be reverted to preserve consistency with the rest of the codebase.

Double quotation marks have been changed to apostrophe characters; this should be reverted to preserve consistency with the rest of the codebase.
Author
Owner

Fixed; see #103 (comment)

Fixed; see https://git.luca0n.com/luca0N/lanbassador/pulls/103#issuecomment-2233
luca0N marked this conversation as resolved
@ -0,0 +17,4 @@
*/
// SPDX-License-Identifier: AGPL-3.0-only
import { Injectable } from '@angular/core';
Author
Owner

Missing trailing comma, and string literal should use quotation marks.

Missing trailing comma, and string literal should use quotation marks.
Author
Owner

Fixed, including the inconsistent string literal; see: #103 (comment)

Fixed, including the inconsistent string literal; see: https://git.luca0n.com/luca0N/lanbassador/pulls/103#issuecomment-2236
luca0N marked this conversation as resolved
@ -0,0 +52,4 @@
*/
public loadSessionToken(): string | undefined {
return this.stackSessionToken.pop();
}
Author
Owner

If this.stackSessionToken has its length equal to 0, then this would return undefined (as evidenced by the function return type). I think that this function should raise an error if the session token stack is empty, otherwise some errors might not be caught by the verify function.

For instance: imagine a faulty unit test in which the mocked session token was not pushed to the stack, even though it should.

Of course, throughout the web app client, having an undefined session token means that the user is not logged in, and should therefore be redirected to the authentication page (if authentication is declared to be required by the server). So, this mock should be modified in a way that allows pushing undefined to the stack.

If `this.stackSessionToken` has its length equal to `0`, then this would return `undefined` (as evidenced by the function return type). I think that this function should raise an error if the session token stack is empty, otherwise some errors might not be caught by the `verify` function. For instance: imagine a faulty unit test in which the mocked session token was not pushed to the stack, even though it should. Of course, throughout the web app client, having an `undefined` session token means that the user is not logged in, and should therefore be redirected to the authentication page (if authentication is declared to be required by the server). So, this mock should be modified in a way that allows pushing `undefined` to the stack.
@ -338,0 +352,4 @@
case PacketAction.RET_SUCCESS:
this.liveStage--;
this.doNextRequest();
break;
Author
Owner

Code quality/consistency/nitpick: all the other cases in this switch block are returning instead
of breaking, then returning at the end; should this not be the case here as well?

Code quality/consistency/nitpick: all the other `case`s in this `switch` block are `return`ing instead of `break`ing, then `return`ing at the end; should this not be the case here as well?
Author
Owner

Fixed; see: #103 (comment)

Fixed; see: https://git.luca0n.com/luca0N/lanbassador/pulls/103#issuecomment-2239
luca0N marked this conversation as resolved
@ -338,0 +356,4 @@
case PacketAction.RET_ERROR_NO_SUCH_SESSION:
this.close();
this.onAuthenticationRequired?.();
return;
Author
Owner

Perhaps add some debugging message here to the console to indicate that the session has expired.

Perhaps add some debugging message here to the console to indicate that the session has expired.
@ -338,0 +372,4 @@
this.close();
this.onAuthenticationRequired?.();
return;
}
Author
Owner

Add some helpful debugging message here to the console as well to help users in troubleshooting/technical support.

Add some helpful debugging message here to the console as well to help users in troubleshooting/technical support.
luca0N force-pushed dev_client_live_auth from 008e3298fd
All checks were successful
/ Test client i18n (push) Successful in 39s
/ Test client (push) Successful in 37s
to 690edd23f3
All checks were successful
/ Test client i18n (push) Successful in 51s
/ Test client (push) Successful in 44s
2026-10-09 02:52:02 +00:00
Compare
Author
Owner

Previous force-push operation from 008e3298fd to 690edd23f3 reverts an erroneous change that deviated from the string literal convention (which is to use quotation marks for string literals, as opposed to apostrophes).

Previous force-push operation from 008e3298fd to 690edd23f3 reverts an erroneous change that deviated from the string literal convention (which is to use quotation marks for string literals, as opposed to apostrophes).
luca0N force-pushed dev_client_live_auth from 690edd23f3
All checks were successful
/ Test client i18n (push) Successful in 51s
/ Test client (push) Successful in 44s
to 58dc38d96c
All checks were successful
/ Test client i18n (push) Successful in 46s
/ Test client (push) Successful in 40s
2026-10-09 10:49:57 +00:00
Compare
Author
Owner

Previous force-push operation from 690edd23f3 to 58dc38d96c fixes inconsistent string literal delimiters, and adds a missing trailing comma in an import line.

Previous force-push operation from 690edd23f3 to 58dc38d96c fixes inconsistent string literal delimiters, and adds a missing trailing comma in an `import` line.
luca0N force-pushed dev_client_live_auth from 58dc38d96c
All checks were successful
/ Test client i18n (push) Successful in 46s
/ Test client (push) Successful in 40s
to 4f7b950ddf
All checks were successful
/ Test client i18n (push) Successful in 46s
/ Test client (push) Successful in 40s
2026-10-09 10:56:30 +00:00
Compare
Author
Owner

Previous force-push operation from 58dc38d96c to 4f7b950ddf fixes inconsistent switch breaks.

Previous force-push operation from 58dc38d96c to 4f7b950ddf fixes inconsistent switch `break`s.
All checks were successful
/ Test client i18n (push) Successful in 46s
/ Test client (push) Successful in 40s
This pull request can be merged automatically.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin dev_client_live_auth:dev_client_live_auth
git switch dev_client_live_auth

Merge

Merge the changes and update on Forgejo.
git switch dev
git merge --no-ff dev_client_live_auth
git switch dev_client_live_auth
git rebase dev
git switch dev
git merge --ff-only dev_client_live_auth
git switch dev_client_live_auth
git rebase dev
git switch dev
git merge --no-ff dev_client_live_auth
git switch dev
git merge --squash dev_client_live_auth
git switch dev
git merge --ff-only dev_client_live_auth
git switch dev
git merge dev_client_live_auth
git push origin dev
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Reference
luca0N/lanbassador!103
No description provided.