diff --git a/bbs/config.c b/bbs/config.c index 46dcccc..18f6839 100644 --- a/bbs/config.c +++ b/bbs/config.c @@ -298,13 +298,17 @@ int bbs_config_free(struct bbs_config *c) #define BEGINS_SECTION(s) (*s == '[') -static struct bbs_config *config_parse(const char *name) +/*! \brief Parse a config file, and optionally write a setting to it */ +static struct bbs_config *config_parse_or_write(const char *name, FILE **restrict write_fp_ptr, const char *write_section, const char *write_key, const char *write_value) { struct bbs_config *cfg; struct bbs_config_section *section = NULL; struct bbs_keyval *keyval; FILE *fp; char *line = NULL; + char *dupline = NULL; + const char *endl = NULL; + int has_line_ending = 0; size_t len = 0; ssize_t bytes_read; char *key, *value; @@ -349,12 +353,32 @@ static struct bbs_config *config_parse(const char *name) bbs_debug(3, "Parsing config %s\n", fullname); +#define COPY_EXISTING_LINE() fprintf(*write_fp_ptr, "%s", dupline); + while ((bytes_read = getline(&line, &len, fp)) != -1) { lineno++; - rtrim(line); if (strlen_zero(line)) { - continue; /* Skip blank/empty lines */ + continue; + } + if (write_fp_ptr) { + has_line_ending = strchr(line, '\n') ? 1 : 0; /* Whether CR or LF, it has an LF */ + if (!endl) { + /* Preserve whichever line endings this file uses. */ + endl = strchr(line, '\r') ? "\r\n" : "\n"; + } + /* Handle previous line */ + if (dupline) { + COPY_EXISTING_LINE(); + free(dupline); + } + dupline = strdup(line); + if (ALLOC_FAILURE(dupline)) { + /* Exit cleanly, but fail. */ + fclose(*write_fp_ptr); + *write_fp_ptr = NULL; + } } + rtrim(line); if (*line == '\r' || *line == '\n') { continue; /* Skip blank/empty lines */ } @@ -384,6 +408,25 @@ static struct bbs_config *config_parse(const char *name) #ifdef DEBUG_CONFIG_PARSING bbs_debug(7, "New section: %s\n", section_name); #endif + + if (write_fp_ptr && write_section && section && !strcmp(section->name, write_section)) { + /* Append keyval to previous section since it was not already present. + * The only downside of this logic is in a file formatted like this: + * [section] + * keyA=valA + * keyB=valB + * + * [section2] + * ... + * If we append, it will be on the line immediately before [section2], + * which is correct, but it will be AFTER the blank line. + * This issue doesn't come up when the value is being replaced, rather than added. + * There's nothing incorrect about this, it just looks weird, + * but it's not worth the added overhead to work around that to me... */ + fprintf(*write_fp_ptr, "%s = %s%s", write_key, write_value, endl); + bbs_verb(5, "Adding setting '%s' in existing config\n", write_key); + } + section = calloc(1, sizeof(*section)); if (ALLOC_FAILURE(section)) { continue; @@ -416,12 +459,31 @@ static struct bbs_config *config_parse(const char *name) *value++ = '\0'; trim(key); trim(value); - + if (!section) { bbs_warning("Failed to process %s=%s, not in a section (%s:%d)\n", key, value, name, lineno); continue; } + if (write_fp_ptr && write_section) { + /* At this point, we know if this key should be updated. + * Furthermore, keys cannot have empty values, so we are replacing some value. */ + if (!strcmp(key, write_key) && !strcmp(section->name, write_section)) { + char *tmp = strchr(dupline, '='); + bbs_assert_exists(tmp); /* We duplicated a string which contains '=', so it must exist */ + tmp++; /* There must be a value, so tmp must be nonempty at this point */ + tmp = strstr(dupline, value); + bbs_assert_exists(value); + tmp += strlen(value); + /* Copy remainder of line from tmp onwards, + * then free dupline since we already handled this line. */ + fprintf(*write_fp_ptr, "%s = %s%s", key, write_value, S_IF(tmp)); /* tmp also includes the line ending */ + bbs_verb(5, "Updating setting '%s' in existing config\n", write_key); + FREE(dupline); /* free and set to NULL */ + write_section = NULL; /* Do not do any further replacements/additions */ + } + } + keyval = calloc(1, sizeof(*keyval)); if (ALLOC_FAILURE(keyval)) { continue; @@ -447,18 +509,114 @@ static struct bbs_config *config_parse(const char *name) if (line) { free(line); /* Free only once at the end */ } + if (write_fp_ptr && dupline) { + COPY_EXISTING_LINE(); + FREE(dupline); + } + if (write_fp_ptr && write_section && section && !strcmp(section->name, write_section)) { + /* If the last line did not originally end with a newline, don't add one at the end now */ + fprintf(*write_fp_ptr, "%s = %s%s", write_key, write_value, has_line_ending ? endl : ""); + bbs_verb(5, "Adding setting '%s' in existing config\n", write_key); + } fclose(fp); /* Only at the end should we insert the config into the list. */ - RWLIST_WRLOCK(&configs); - RWLIST_INSERT_TAIL(&configs, cfg, entry); - RWLIST_UNLOCK(&configs); + if (!write_key) { + RWLIST_WRLOCK(&configs); + RWLIST_INSERT_TAIL(&configs, cfg, entry); + RWLIST_UNLOCK(&configs); + } bbs_verb(5, "Parsed config %s\n", fullname); return cfg; } +static struct bbs_config *config_parse(const char *name) +{ + return config_parse_or_write(name, NULL, NULL, NULL, NULL); +} + +int bbs_config_set_keyval(const char *filename, const char *section, const char *key, const char *value) +{ + FILE *oldfp, *newfp; + struct bbs_config *cfg; + size_t fsize; + struct stat st; + char tmpfile[256] = "/tmp/bbs_config_XXXXXX"; + + /* Unlike Asterisk, we do not parse the entire config into memory, + * modify config objects, and then serialize it back to disk. + * Instead, we just work with the INI config file directly. + * This avoids having to worry about preserving comments and formatting verbatim, + * resulting in "dumber" (less semantic/powerful) but ultimately much simpler code. + * This would be very inefficient for updating multiple key-value pairs, + * but for updating a single setting in a file, it is fairly efficient. */ + + /* The GNU and BSD versions of sed use different syntax, + * and we may want to either set or replace the config value + * (and may not know or care what the old value is). + * So, do a brute force copy and update/add/replace. */ + + oldfp = fopen(filename, "r"); + if (!oldfp) { + bbs_warning("Existing config file '%s' does not exist\n", filename); + /* If config file doesn't exist, we could create it, + * but more than likely something is wrong and we should just abort. */ + return -1; + } + newfp = bbs_mkftemp(tmpfile, 0660); + if (!newfp) { + fclose(oldfp); + return -1; + } + + cfg = config_parse_or_write(filename, &newfp, section, key, value); + + /* Finalize and cleanup */ + fclose(oldfp); + if (!newfp) { + /* Failure occured, and newfp has already been closed. */ + if (cfg) { + config_free(cfg); + bbs_delete_file(tmpfile); + } + return -1; + } + fflush(newfp); + fsize = (size_t) ftell(newfp); + fclose(newfp); + + /* We parsed the config, but don't want to keep it. + * For one, it's now outdated, and it may be duplicating the stale version already in the linked list. */ + if (!cfg) { + bbs_delete_file(tmpfile); + return -1; + } + config_free(cfg); /* Wasn't inserted into list, so don't use bbs_config_free */ + + /* Edge case, but check if the disk was full and we weren't actually able to write the file to disk. */ + if (stat(tmpfile, &st)) { + bbs_error("Failed to stat %s: %s\n", tmpfile, strerror(errno)); + bbs_delete_file(tmpfile); + return -1; + } + if ((size_t) st.st_size != fsize) { + bbs_error("File size mismatch: %lu != %lu\n", st.st_size, fsize); + bbs_delete_file(tmpfile); + return -1; + } + + /* Okay, now do the atomic rename, since we are confident file truncation did not occur. */ + if (rename(tmpfile, filename)) { + bbs_error("Failed to rename %s -> %s: %s\n", tmpfile, filename, strerror(errno)); + bbs_delete_file(tmpfile); + return -1; + } + + return 0; +} + static int __bbs_cached_config_outdated(struct bbs_config *cfg, const char *name) { /* Check if the config has changed since we parsed it. */ diff --git a/include/config.h b/include/config.h index 77ad24f..8e9b32f 100644 --- a/include/config.h +++ b/include/config.h @@ -152,6 +152,16 @@ int bbs_config_free(struct bbs_config *cfg); /*! \brief Destroy all existing configs (used at shutdown) */ void bbs_configs_free_all(void); +/*! + * \brief Write a single particular key-value setting to a config file, adding if not present and overwriting existing value if already present + * \param filename Config fil name + * \param section Config section name in which to add or update setting + * \param key Key name of setting to add or update + * \param value Setting value to add or update + * \retval 0 on success, -1 on failure + */ +int bbs_config_set_keyval(const char *filename, const char *section, const char *key, const char *value) __attribute__((nonnull (1, 2, 3, 4))); + /*! * \brief Check whether a config file has been updated since it was last parsed * \param name Config filename diff --git a/modules/mod_oauth.c b/modules/mod_oauth.c index 09bc81a..f12714e 100644 --- a/modules/mod_oauth.c +++ b/modules/mod_oauth.c @@ -38,12 +38,14 @@ struct oauth_client { const char *clientid; const char *clientsecret; const char *posturl; + const char *filename; char *accesstoken; char *refreshtoken; RWLIST_ENTRY(oauth_client) entry; time_t tokentime; time_t expires; unsigned int userid; + unsigned int accesstokeninitiallyempty:1; bbs_mutex_t lock; char data[0]; }; @@ -66,10 +68,10 @@ static void free_client(struct oauth_client *client) /*! \note clients must be WRLOCK'd when calling */ static int add_oauth_client(const char *name, const char *clientid, const char *clientsecret, const char *refreshtoken, const char *accesstoken, - const char *posturl, int expires, unsigned int userid) + const char *posturl, int expires, unsigned int userid, const char *filename) { struct oauth_client *client; - size_t namelen, idlen, secretlen, urllen; + size_t namelen, idlen, secretlen, urllen, filenamelen; char *pos; REQUIRE_SETTING(clientid); @@ -96,7 +98,8 @@ static int add_oauth_client(const char *name, const char *clientid, const char * idlen = strlen(clientid); secretlen = !strlen_zero(clientsecret) ? strlen(clientsecret) : 0; urllen = strlen(posturl); - client = calloc(1, sizeof(*client) + namelen + idlen + secretlen + urllen + 4); /* NULs for each of them */ + filenamelen = strlen(filename); + client = calloc(1, sizeof(*client) + namelen + idlen + secretlen + urllen + filenamelen + 5); /* NULs for each of them */ if (ALLOC_FAILURE(client)) { return -1; } @@ -121,6 +124,10 @@ static int add_oauth_client(const char *name, const char *clientid, const char * strcpy(pos, posturl); client->posturl = pos; + pos += urllen + 1; + strcpy(pos, filename); + client->filename = pos; + client->refreshtoken = strdup(refreshtoken); if (ALLOC_FAILURE(client->refreshtoken)) { free(client); @@ -136,6 +143,8 @@ static int add_oauth_client(const char *name, const char *clientid, const char * return -1; } client->tokentime = time(NULL); /* Assumed to be valid as of now. */ + } else { + client->accesstokeninitiallyempty = 1; } client->expires = expires; @@ -197,7 +206,6 @@ static int refresh_token(struct oauth_client *client) } client->tokentime = now; - REPLACE(client->accesstoken, newaccesstoken); if (!strlen_zero(newrefreshtoken) && strcmp(newrefreshtoken, client->refreshtoken)) { /* The authorization server MAY issue a new refresh token, in which case the client * MUST discard the old refresh token and replace it with the new refresh token. @@ -206,8 +214,23 @@ static int refresh_token(struct oauth_client *client) REPLACE(client->refreshtoken, newrefreshtoken); /* This is good as long as the BBS is running continously, but we also need to update the static configuration file, * or we'll lose the new refresh token the next time the BBS starts (or mod_oauth is reloaded). */ - /*! \todo XXX Persist the new token to the config file */ - bbs_warning("OAuth refresh token has changed, but is not persisted to configuration\n"); + if (bbs_config_set_keyval(client->filename, client->name, "refreshtoken", client->refreshtoken)) { + bbs_warning("OAuth refresh token has changed, but is not persisted to configuration\n"); + } + } + if (!strlen_zero(newaccesstoken) && !client->accesstokeninitiallyempty && (strlen_zero(client->accesstoken) || strcmp(newaccesstoken, client->accesstoken))) { + REPLACE(client->accesstoken, newaccesstoken); + /* Also update the config with the new access token, so we don't try to use + * an old access token in the future. + * This way, if there's an access token specified in the config file, + * when we write new refresh tokens to the file, we don't leave a stale access token there + * and load that back in when it's no longer valid. */ + if (bbs_config_set_keyval(client->filename, client->name, "accesstoken", client->accesstoken)) { + bbs_warning("OAuth access token has changed, but is not persisted to configuration\n"); + } + } else { + /* Just update in memory, we won't pull an old access token from the config in the future since it's not specified there to start with. */ + REPLACE(client->accesstoken, newaccesstoken); } bbs_verb(4, "Refreshed OAuth token '%s' (good for %ds)\n", client->name, expires); @@ -321,7 +344,7 @@ static int load_config_file(const char *filename, unsigned int forceuserid, cons if (match && !strcmp(bbs_config_section_name(section), match)) { namematch = 1; } - add_oauth_client(bbs_config_section_name(section), clientid, clientsecret, refreshtoken, accesstoken, posturl, expires, userid); + add_oauth_client(bbs_config_section_name(section), clientid, clientsecret, refreshtoken, accesstoken, posturl, expires, userid, filename); if (forceuserid && added++ > MAX_USER_OAUTH_TOKENS) { /* Prevent a user from loading an unbounded amount of token mappings into memory. */ bbs_warning("Maximum user OAuth token mappings exceeded, ignoring remaining mappings\n"); diff --git a/modules/mod_test_config.c b/modules/mod_test_config.c new file mode 100755 index 0000000..e58ff39 --- /dev/null +++ b/modules/mod_test_config.c @@ -0,0 +1,155 @@ +/* + * LBBS -- The Lightweight Bulletin Board System + * + * Copyright (C) 2024, Naveen Albert + * + * Naveen Albert + * + * This program is free software, distributed under the terms of + * the GNU General Public License Version 2. See the LICENSE file + * at the top of the source tree. + */ + +/*! \file + * + * \brief Config file parsing tests + * + * \author Naveen Albert + */ + +#include "include/bbs.h" + +#include +#include + +#include "include/module.h" +#include "include/test.h" +#include "include/config.h" +#include "include/utils.h" + +#define START_TEST() \ + char *str1 = NULL, *str2 = NULL; \ + char template[256] = "/tmp/test_config_XXXXXX"; \ + FILE *fp = bbs_mkftemp(template, 0660); \ + if (!fp) { \ + return -1; \ + } + +static int test_keyval_create(void) +{ + int mres, res = -1; + char *tmp; + int len2; + START_TEST(); + + fprintf(fp, "[general]\n"); + fprintf(fp, "foo = bar\n"); + fprintf(fp, "\n"); + fprintf(fp, "[settings]\n"); + fclose(fp); + + mres = bbs_config_set_keyval(template, "settings", "mykey", "test!"); /* Value same length for easy string replace */ + bbs_test_assert_equals(mres, 0); + str2 = bbs_file_to_string(template, 0, &len2); + bbs_test_assert_exists(str2); + tmp = strstr(str2, "mykey = test!"); + bbs_test_assert_exists(tmp); + + res = 0; + +cleanup: + bbs_delete_file(template); + free_if(str1); + free_if(str2); + return res; +} + +static int test_keyval_update(void) +{ + int mres, res = -1; + char *tmp; + int len1, len2; + START_TEST(); + + fprintf(fp, "[general]\n"); + fprintf(fp, "foo = bar\n"); + fprintf(fp, "\n"); + fprintf(fp, "[settings]\n"); + fprintf(fp, "mykey = myval\n"); + fclose(fp); + + str1 = bbs_file_to_string(template, 0, &len1); + bbs_test_assert_exists(str1); + mres = bbs_config_set_keyval(template, "settings", "mykey", "test!"); /* Value same length for easy string replace */ + bbs_test_assert_equals(mres, 0); + str2 = bbs_file_to_string(template, 0, &len2); + bbs_test_assert_exists(str2); + tmp = strstr(str1, "myval"); + bbs_test_assert_exists(tmp); + memcpy(tmp, "test!", STRLEN("test!")); /* Don't copy a NUL terminator after */ + bbs_test_assert_str_equals(str1, str2); + + res = 0; + +cleanup: + bbs_delete_file(template); + free_if(str1); + free_if(str2); + return res; +} + +static int test_keyval_update_crlf(void) +{ + int mres, res = -1; + char *tmp; + int len1, len2; + START_TEST(); + + fprintf(fp, "[general]\r\n"); + fprintf(fp, "foo = bar\r\n"); + fprintf(fp, "\r\n"); + fprintf(fp, "[settings]\r\n"); + fprintf(fp, "mykey = myval\r\n"); + fprintf(fp, "\r\n"); + fprintf(fp, "[settings2]\r\n"); + fclose(fp); + + str1 = bbs_file_to_string(template, 0, &len1); + bbs_test_assert_exists(str1); + mres = bbs_config_set_keyval(template, "settings", "mykey", "test!"); /* Value same length for easy string replace */ + bbs_test_assert_equals(mres, 0); + str2 = bbs_file_to_string(template, 0, &len2); + bbs_test_assert_exists(str2); + tmp = strstr(str1, "myval"); + bbs_test_assert_exists(tmp); + memcpy(tmp, "test!", STRLEN("test!")); /* Don't copy a NUL terminator after */ + bbs_test_assert_str_equals(str1, str2); + + res = 0; + +cleanup: + bbs_delete_file(template); + free_if(str1); + free_if(str2); + return res; +} + +static struct bbs_unit_test tests[] = +{ + { "Config Setting Create", test_keyval_create }, + { "Config Setting Update", test_keyval_update }, + { "Config Update Line Endings", test_keyval_update_crlf }, +}; + +static int unload_module(void) +{ + return bbs_unregister_tests(tests); +} + +static int load_module(void) +{ + int res = bbs_register_tests(tests); + REQUIRE_FULL_LOAD(res); +} + +BBS_MODULE_INFO_STANDARD("Config File Unit Tests");