Add account support to the server #62

Merged
luca0N merged 14 commits from dev_server_accounts into dev 2026-03-31 23:26:53 +00:00
Owner

This pull request adds support for user accounts in the Lanbassador server.

Due to the fact that database support is just now being included, this pull request will be quite lengthy and complex. An HTTP API must also be created to supply account creation and authentication endpoints.

  • Database schema
    • Check database schema on server startup
    • Create database schema during startup if it doesn't exist
  • Server account API
    • Allow account creation
    • Allow authentication
    • Allow session invalidation (logging off)
    • Securely store and validate account passwords (bcrypt)
    • Tidy up responses (standardize responses with JSON, create common functions)
    • Enhance error handling (use panic sparingly)
This pull request adds support for user accounts in the Lanbassador server. Due to the fact that database support is just now being included, this pull request will be quite lengthy and complex. An HTTP API must also be created to supply account creation and authentication endpoints. - [x] Database schema - [x] Check database schema on server startup - [x] Create database schema during startup if it doesn't exist - [x] Server account API - [x] Allow account creation - [x] Allow authentication - [x] Allow session invalidation (logging off) - [x] Securely store and validate account passwords (bcrypt) - [x] Tidy up responses (standardize responses with JSON, create common functions) - [x] Enhance error handling (use `panic` sparingly)
luca0N self-assigned this 2026-03-05 23:51:47 +00:00
During server initialization, establish a connection with the database,
check whether the database schema version is supported, and close the
connection once the server is closing.

This commit also introduces a new section to the configuration file,
`database'.  It allows administrators to define the database server host
and credentials that should be used by the server.

The database schema is not yet being created, the server merely checks
whether the schema table exists, and whether the current database schema
version matches that of the server implementation.  If the database
connection fails, then the server panics.

Additionally, even though some code to close the database connection was
added to the server, it is technically never reached, as the server runs
forever.  However, it's still handy to implement this once graceful
shutdown support is implemented.
Author
Owner

I was thinking about whether implementing (a) database support, (b) account support, and (c) HTTP API support all in the same pull request was a good idea.

Since the database and HTTP API support code won't be too lengthy, I think it might be fine if they're all kept in this pull request. I might revisit this thought in the future to keep everything tidy and reduce code complexity.

I was thinking about whether implementing (a) database support, (b) account support, and (c) HTTP API support all in the same pull request was a good idea. Since the database and HTTP API support code won't be _too_ lengthy, I think it might be fine if they're all kept in this pull request. I might revisit this thought in the future to keep everything tidy and reduce code complexity.
Author
Owner

Seems like Go already has a native package for SQL, and that it supports multiple SQL drivers. For now, I will be focusing on working with MariaDB, and this will be the only supported engine for now.

Seems like Go already has a native package for SQL, and that it supports multiple SQL drivers. For now, I will be focusing on working with MariaDB, and this will be the only supported engine for now.
server/db.go Outdated
@ -0,0 +34,4 @@
// This function panics if it's unable to perform its basic task of determining
// whether the schema table exists (e.g., due to an SQL error).
func checkDatabaseSchemaExists() bool {
query, err := db.Query("SELECT COUNT(ENGINE) FROM information_schema.TABLES WHERE TABLE_SCHEMA='lanbassador' AND TABLE_NAME='schema';")
Author
Owner

The database name must not be hardcoded.

The database name must not be hardcoded.
luca0N marked this conversation as resolved
Author
Owner

Some servers also support prefixes for table names. I will evaluate whether implementing this will be useful.

Some servers also support prefixes for table names. I will evaluate whether implementing this will be useful.
luca0N force-pushed dev_server_accounts from 984340b843 to c0ef9a4941 2026-03-08 21:06:42 +00:00 Compare
Author
Owner

Latest force-push from 984340b843 to c0ef9a4941 fixes a bug which caused the server to ignore the database account password set in server.cfg.

Latest force-push from 984340b843 to c0ef9a4941 fixes a bug which caused the server to ignore the database account password set in `server.cfg`.
@ -55,0 +65,4 @@
; Defines the password for the database account the server should log in as.
;password=
; Defines the name of the databse that should be used by the server. Most
Author
Owner

There's a typo here in database.

Note to self: put spell in vimrc.

There's a typo here in _database_. Note to self: put `spell` in vimrc.
luca0N marked this conversation as resolved
Author
Owner

@luca0N wrote in #62 (comment):

I was thinking about whether implementing (a) database support, (b) account support, and (c) HTTP API support all in the same pull request was a good idea.

Since the database and HTTP API support code won't be too lengthy, I think it might be fine if they're all kept in this pull request. I might revisit this thought in the future to keep everything tidy and reduce code complexity.

The net/http package will be used for the HTTP API since it's already being used to upgrade the /live API.

I think I will also rename that path to /api/v1/live to keep it consistent with the naming convention of the (soon to be committed) API routes.

@luca0N wrote in https://git.luca0n.com/luca0N/lanbassador/pulls/62#issuecomment-1428: > I was thinking about whether implementing (a) database support, (b) account support, and (c) HTTP API support all in the same pull request was a good idea. > > Since the database and HTTP API support code won't be _too_ lengthy, I think it might be fine if they're all kept in this pull request. I might revisit this thought in the future to keep everything tidy and reduce code complexity. The `net/http` package will be used for the HTTP API since it's already being used to upgrade the `/live` API. I think I will also rename that path to `/api/v1/live` to keep it consistent with the naming convention of the (soon to be committed) API routes.
Create the database schema on server (if it's not already present in the
database server).  The database creation function also inserts the
current schema ID to the database.
Add a new Go dependency to handle MariaDB connections via a driver
package.  This driver is also used for MySQL servers, but this project
currently only supports MariaDB.
luca0N force-pushed dev_server_accounts from 7c6c65a309 to d1c2531768 2026-03-11 23:54:33 +00:00 Compare
Author
Owner

Latest force-push fixes a typo in a comment from the server configuration file, and also uses a prepared statement to remove a hardcoded database name.

Latest force-push fixes a typo in a comment from the server configuration file, and also uses a prepared statement to remove a hardcoded database name.
Author
Owner

Before adding any of the server-side account authentication code, I will use a Go package for secure password hashing (e.g., Argon21).

Before adding any of the server-side account authentication code, I will use a Go package for secure password hashing (e.g., Argon2[^1]). [^1]: https://en.wikipedia.org/wiki/Argon2
Author
Owner

I think this is also a good opportunity to create a Swagger OpenAPI-compatible page. I will address this in a separate pull request.

I think this is also a good opportunity to create a Swagger OpenAPI-compatible page. I will address this in a separate pull request.
Rename the WebSocket API source file from `api.go' to `ws.go', as this
filename will be used by the HTTP API in the Lanbassador server.
In the server code, add support for creating and deleting user account
sessions.  Creating sessions require clients to supply a correct
username and password.

This commit also makes some changes in the database schema creation
function to address some issues, and to create a new table for user
account sessions.  By default, the SQL driver that is being used does
not support multiple SQL statements in a single `Exec', so they are now
done individually, but with no transactions (at this moment).

IMPORTANT: The changes made in this commit do not include any secure
hash password algorithm.  The password verification is done by doing a
1:1 comparison with the database record, which is insecure.  A future
commit will fix this.
Author
Owner

I went ahead and pushed my changes so far. Passwords are currently supposed to be stored in plain text in the database, which is obviously insecure. I will introduce support for secure hashed passwords in a future commit (I'm already working on it, but it's not yet ready to be pushed).

There's also a lot of refactoring that I can do, particularly in the api.go file, when it comes to basic request validation (Content-Type checks, JSON unmarshalling...).

I went ahead and pushed my changes so far. Passwords are currently supposed to be stored in plain text in the database, which is obviously insecure. I will introduce support for secure hashed passwords in a future commit (I'm already working on it, but it's not yet ready to be pushed). There's also a lot of refactoring that I can do, particularly in the `api.go` file, when it comes to basic request validation (`Content-Type` checks, JSON unmarshalling...).
Fix a server panic that occurred when a user-agent made any attempt to
authenticate with a username that does not exist.

The SQL function called from Go's library returns false on error, or
when no records are available.  The changes introduced in this commit
check whether an error occurred, or whether no matching records were
found.
Interpret account passwords in the database as a standard bcrypt hashed
password, and check received passwords from clients, using a new Go
dependency for handling bcrypt hashing and checking.
luca0N force-pushed dev_server_accounts from 740cb1012b to 1b408a1a40 2026-03-19 23:52:22 +00:00 Compare
Author
Owner

Force-push from 740cb1012b to 1b408a1a40 fixes an erroneous removal of a comment, and removes a trailing tab on a line.

Force-push from 740cb1012b to 1b408a1a40 fixes an erroneous removal of a comment, and removes a trailing tab on a line.
@ -25,2 +26,4 @@
"net/http"
)
const LANBASSADOR_SERVER_BUFFER_LENGTH = 4096
Author
Owner

Add godoc here.

Add godoc here.
Author
Owner

This constant actually appears to be unused.

This constant actually appears to be unused.
luca0N marked this conversation as resolved
server/db.go Outdated
@ -0,0 +27,4 @@
const LANBASSADOR_DATABASE_SCHEMA_VERSION = 1
var db *sql.DB
Author
Owner

Add godoc comment here.

Add godoc comment here.
luca0N marked this conversation as resolved
Refactor server code by creating common API functions for validating
requests, and moving API route registration code to a separate function
in `main.go'.
Create API route for creating new accounts and deleting existing ones.
For now, there is no server configuration that allows this feature to be
turned off, and a configuration option to allow only administrators to
create user accounts is also lacking.  New accounts are always created
without administrator privileges.

The issues listed above will be addressed by a future commit.
server/api.go Outdated
@ -49,2 +44,3 @@
}
func setCommonResponse(w *http.ResponseWriter) {
(*w).Header().Set("Server", "Lanbassador/0.1.0")
}
Author
Owner

There should be a constant with the project name and version, instead of having it hardcoded here.

It would also be a good idea to somehow fetch the HEAD commit hash and display it here, unless the tree is checked out to a release tag.

There should be a constant with the project name and version, instead of having it hardcoded here. It would also be a good idea to somehow fetch the HEAD commit hash and display it here, unless the tree is checked out to a release tag.
luca0N marked this conversation as resolved
server/api.go Outdated
@ -80,0 +93,4 @@
w.WriteHeader(http.StatusUnauthorized)
setCommonResponse(&w)
fmt.Fprintln(w, "Missing or invalid session token.")
return false
Author
Owner

This setCommonResponse line is repeated throughout this file way too often. I believe a better way of approaching this would be to have one function that does all of this.

This `setCommonResponse` line is repeated throughout this file way too often. I believe a better way of approaching this would be to have one function that does all of this.
luca0N marked this conversation as resolved
server/db.go Outdated
@ -0,0 +232,4 @@
err = query.Scan(&userId, &username)
if err != nil {
panic(err)
Author
Owner

Error handling on this pull request should be reviewed, and improved if possible.

Error handling on this pull request should be reviewed, and improved if possible.
luca0N marked this conversation as resolved
server/db.go Outdated
@ -0,0 +65,4 @@
`CREATE TABLE IF NOT EXISTS users (
id INT UNSIGNED NOT NULL PRIMARY KEY AUTO_INCREMENT,
username TINYTEXT NOT NULL,
Author
Owner

This needs to be a UNIQUE KEY.

This needs to be a `UNIQUE KEY`.
luca0N marked this conversation as resolved
Make the `username' column in the `users' database table unique.  This
is required, as usernames are unique identifiers user by users to
authenticate as part of their credentials.
Create new section in the server configuration file that controls
support for user accounts and account registration.
luca0N force-pushed dev_server_accounts from 63b69b3752 to fa80029602 2026-03-24 21:29:24 +00:00 Compare
Author
Owner

Latest force-push from 63b69b3752 to fa80029602 made the new server account options commented by default, so as to keep it consistent with the rest of the sample configuration file. Options that are not explicitly required (e.g., those with default values) are commented out and have their default values in the commented line.

Latest force-push from 63b69b3752 to fa80029602 made the new server account options commented by default, so as to keep it consistent with the rest of the sample configuration file. Options that are not explicitly required (e.g., those with default values) are commented out and have their default values in the commented line.
Perform code refactoring in the server API code by moving repetitive
code to their own functions, enhance error handling, and add some more
Godoc documentation.
luca0N changed title from WIP: Add account support to the server to Add account support to the server 2026-03-25 23:26:22 +00:00
luca0N force-pushed dev_server_accounts from 01ca00b316 to 433ef97765 2026-03-26 23:29:16 +00:00 Compare
Author
Owner

Latest force-push from 01ca00b316 to 433ef97765 added missing Godoc documentation in the database schema version const.

Latest force-push from 01ca00b316 to 433ef97765 added missing Godoc documentation in the database schema version const.
luca0N left a comment
Author
Owner

There's a few minor fixes that are still missing before this PR is ready to be merged.

I am all out of time to work on this tonight sadly, so I will revisit this soon.

There's a few minor fixes that are still missing before this PR is ready to be merged. I am all out of time to work on this tonight sadly, so I will revisit this soon.
@ -57,2 +101,2 @@
Action: action,
Data: data,
responseStatusCode := http.StatusOK
switch responseCode {
Author
Owner

There are several consts missing here.

There are several consts missing here.
luca0N marked this conversation as resolved
server/api.go Outdated
Author
Owner

The response here should be a JSON string.

The response here should be a JSON string.
luca0N marked this conversation as resolved
server/crypto.go Outdated
@ -0,0 +46,4 @@
panic(err)
}
return err == nil
Author
Owner

The password checking logic here is too primitive. User account passwords should include at least a number.

I believe enforcing too much of a complex password on users does the exact opposite of securing accounts, as users will probably be more likely to just append a special character to the end of their existing password, for instance. As such, the password complexity verification code here should be minimal, but should enforce a reasonably secure password for user accounts.

The password checking logic here is too primitive. User account passwords should include at least a number. I believe enforcing too much of a complex password on users does the exact opposite of securing accounts, as users will probably be more likely to just append a special character to the end of their existing password, for instance. As such, the password complexity verification code here should be minimal, but should enforce a reasonably secure password for user accounts.
luca0N marked this conversation as resolved
server/db.go Outdated
@ -0,0 +36,4 @@
LanbassadorDbErrorUnknown = errors.New("An unknown internal error occurred.")
LanbassadorDbErrorNoSuchUser = errors.New("No such user.")
LanbassadorDbErrorDuplicateEntry = errors.New("A conflicting entry already exists.")
)
Author
Owner

Missing Godoc comments.

Missing Godoc comments.
luca0N marked this conversation as resolved
@ -0,0 +113,4 @@
}
// Closes the opened database engine. Must only be called after initDatabase.
func closeDatabase() {
Author
Owner

A comment here would be nice for context: this function is only called if the schema exists (by checking the result value of checkDatabaseSchemaExists). This is why the standard query.Error() != nil check (as seen throughout the rest of this source file) does not apply here: this function expects the existence of the database schema table.

A comment here would be nice for context: this function is only called if the schema exists (by checking the result value of `checkDatabaseSchemaExists`). This is why the standard `query.Error() != nil` check (as seen throughout the rest of this source file) does not apply here: this function expects the existence of the database schema table.
luca0N marked this conversation as resolved
Author
Owner

Failure to store a new session record in the database does not mean the server should panic.

Failure to store a new session record in the database does not mean the server should panic.
luca0N marked this conversation as resolved
Perform minor code refactoring in theaccount creation server code.  The
changes made involve the enhancement of error handling, adding missing
comments, and general code refactoring.
Enhance the checking of secure passwords in the server code.  The server
will now consider passwords as secure when they include at least a
letter (uppercase or lowercase), and a number.  The existing requirement
of a minimum of 8 characters was kept.
luca0N deleted branch dev_server_accounts 2026-03-31 23:26:53 +00:00
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!62
No description provided.