Add account support to the server #62
No reviewers
Labels
No labels
Compat/Breaking
Kind/Bug
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Blocks
#26 Add authentication support
luca0N/lanbassador
#41 WIP: Add account authentication support to web app
luca0N/lanbassador
Reference
luca0N/lanbassador!62
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "dev_server_accounts"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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.
panicsparingly)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.
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.
@ -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';")The database name must not be hardcoded.
Some servers also support prefixes for table names. I will evaluate whether implementing this will be useful.
984340b843toc0ef9a4941Latest force-push from
984340b843toc0ef9a4941fixes a bug which caused the server to ignore the database account password set inserver.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. MostThere's a typo here in database.
Note to self: put
spellin vimrc.@luca0N wrote in #62 (comment):
The
net/httppackage will be used for the HTTP API since it's already being used to upgrade the/liveAPI.I think I will also rename that path to
/api/v1/liveto keep it consistent with the naming convention of the (soon to be committed) API routes.7c6c65a309tod1c2531768Latest 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.
Before adding any of the server-side account authentication code, I will use a Go package for secure password hashing (e.g., Argon21).
https://en.wikipedia.org/wiki/Argon2 ↩︎
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 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.gofile, when it comes to basic request validation (Content-Typechecks, JSON unmarshalling...).740cb1012bto1b408a1a40Force-push from
740cb1012bto1b408a1a40fixes 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 = 4096Add godoc here.
This constant actually appears to be unused.
@ -0,0 +27,4 @@const LANBASSADOR_DATABASE_SCHEMA_VERSION = 1var db *sql.DBAdd godoc comment here.
@ -49,2 +44,3 @@}func setCommonResponse(w *http.ResponseWriter) {(*w).Header().Set("Server", "Lanbassador/0.1.0")}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.
@ -80,0 +93,4 @@w.WriteHeader(http.StatusUnauthorized)setCommonResponse(&w)fmt.Fprintln(w, "Missing or invalid session token.")return falseThis
setCommonResponseline 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.@ -0,0 +232,4 @@err = query.Scan(&userId, &username)if err != nil {panic(err)Error handling on this pull request should be reviewed, and improved if possible.
@ -0,0 +65,4 @@`CREATE TABLE IF NOT EXISTS users (id INT UNSIGNED NOT NULL PRIMARY KEY AUTO_INCREMENT,username TINYTEXT NOT NULL,This needs to be a
UNIQUE KEY.63b69b3752tofa80029602Latest force-push from
63b69b3752tofa80029602made 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.WIP: Add account support to the serverto Add account support to the server01ca00b316to433ef97765Latest force-push from
01ca00b316to433ef97765added missing Godoc documentation in the database schema version const.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.StatusOKswitch responseCode {There are several consts missing here.
The response here should be a JSON string.
@ -0,0 +46,4 @@panic(err)}return err == nilThe 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.
@ -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."))Missing Godoc comments.
@ -0,0 +113,4 @@}// Closes the opened database engine. Must only be called after initDatabase.func closeDatabase() {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 standardquery.Error() != nilcheck (as seen throughout the rest of this source file) does not apply here: this function expects the existence of the database schema table.Failure to store a new session record in the database does not mean the server should panic.