naturalcrit / homebrewery

Create authentic looking D&D homebrews using only markdown
https://homebrewery.naturalcrit.com
MIT License
1.1k stars 327 forks source link

User settings #1908

Open jeddai opened 2 years ago

jeddai commented 2 years ago

User Settings

Editor Settings

We've added a number of editor-related changes recently, and I think it would be beneficial for users to be able to modify certain editor settings and it be persistent in the application. A super easy solution there would be to have them stored in local storage and loaded when the editor is loaded.

These could be in an object type editorSettings value on the user schema. This would allow for a more gentle-handed approach to its schema as new settings are added.

As some examples of things that we might want to include in the editor settings, we could include the following:

Any other options/ideas people have are welcome!

calculuschild commented 2 years ago

Can probably use this to support CodeMirror themes on this PR as well, which is just missing the user interface to swap themes: https://github.com/naturalcrit/homebrewery/pull/1771

jeddai commented 2 years ago

Yeah we could include tags in this as well -- could be a second pane in the metadata editor section? Maybe below the metadata editor?

G-Ambatte commented 2 years ago

There was also the discussion of User Snippets, which would have it's own menu in the Editor (issue #1722). Some of this was the original impetus behind the UserInfo framework WIP PR (#1672), which is now closed, at least in part because it needed an Issue raised to guide the development of the PR. Bearing in mind that these settings were to be stored in Homebrewery, not local storage, so would persist across the account.

I suggest we either create a new overarching Issue, or use this Issue, to record the configurations and settings that should persist on a per-user basis, and to determine how best to progress. As has been mentioned previously, it's probably better to get this right at the beginning, rather than to implement half a solution and then be stuck supporting both options for eternity.

jeddai commented 2 years ago

That's a great idea, we can use the Object mongodb type for editorSettings or something on the user schema to allow for a more gentle-handed approach for this specifically instead of having individual schema values for each editor setting.

I'll edit the original issue to include that.

G-Ambatte commented 2 years ago

In terms of the UserInfo structure, I'd envisage it being something like follows:

{
username: "testuser",
passwordHash: "---",
createdAt: 2021-12-20 12:09:31,
updatedAt: 2021-12-20 12:09:31,
editorPrefs: {
    lineHighlights: true,
    otherEditorSettings: yes
    },
userSnippets: {
    [ menuTitle: "USER_SNIPPET_01", injectText: "USER SNIPPET TEXT"]
    },
otherStuffWeHaventEvenThoughtOfYet: {
    }
}

with username being a unique primary key within the database. However, with the knowledge that this may grow to supplant the NaturalCrit database, this may clash with it's current operation, so care will need to be taken to ensure they operate in the same fashion - a separate unique userId may be required.

The nature of MongoDB collections are very fluid, so the actual collection can expand over time as new requirements are developed. In terms of functionality, it could be as simple as two functions: one that saves the object to the MongoDB database, and one that retrieves it. However, I imagine a slightly more complex approach:

These can then be combined

G-Ambatte commented 2 years ago

Further to passwordHash, my understanding is that it is common to store the salt factor separate from the code and data bases. I imagine that this would reside in the environment variables, similar to NODE_ENV or the current Google service account settings.

UserInfo.generateHash : function (input, salt) { return hash; }

called by

const inputHash = UserInfo.generateHash(field.userInput, process.env.SALT);
if(inputHash === UserInfo.getUserInfo(user)?.passwordHash) { return true; };

Although I am by no means a security or DB expert, so feel free to let me know if there are better ways to do this.

jeddai commented 2 years ago

Something about salting passwords -- since you can't reverse a hash, the hashed password can only be compared against. Another option that is a little more secure and safe than having a single salt is to randomly generate a salt for each user and store it alongside the hashed password.

Example: https://auth0.com/blog/adding-salt-to-hashing-a-better-way-to-store-passwords/

ericscheid commented 2 years ago

Another option that is a little more secure and safe than having a single salt is to randomly generate a salt for each user

Yes. Given a known password, and the salted-hash of that password, it is pretty easy to discover what the salt is. If that salt is then used for all passwords then it is easy to mass-compute hashed-passwords of the most popular passwords (using that salt). You then have a big table of hashed-passwords you can reverse any stolen salted-hashed-passwords against.

If however each password has it's own salt, that makes the job exponentially harder. The rainbow table is only useful for that one password, of which you already know the unhashed password.

and store it alongside the hashed password.

or store it elsewhere, but that is truly getting into the weeds.

ericscheid commented 2 years ago

userSnippets: { [ menuTitle: "USER_SNIPPET_01", injectText: "USER SNIPPET TEXT"] },

This (of course) should be

userSnippets: [ { menuTitle: "USER_SNIPPET_01", injectText: "USER SNIPPET TEXT" } ],

calculuschild commented 2 years ago

How do we want to handle users who do not have accounts? Do we want editor options to persist between browser sessions? (i.e., do we want to use LocalStorage as a fallback still?)

In any case, we do have two separate databases currently in use, and I think resolving the following is going to be the starting point for everything else here: We have 1) NaturalCrit (usernames and passwords for login) and 2) Homebrewery (brew documents).

The original intent of the two separate databases, was that "NaturalCrit" would be home to a suite of separate apps, with the NaturalCrit database holding a central log-in service for all apps, and then the Homebrewery for instance having its own database holding documents created via that app.

If we are going to be storing Homebrewery-specific user settings, we need to clear up in which database that belongs. I see a few approaches, and I think we need to settle on one before moving on:

1) Leave the Naturalcrit Database as is, and add a second collection to the Homebrewery database which simply stores username as an index, and then a list of their preferred settings specific to the Homebrewery. There is no need, I think, to store password hashes in this "preferences" collection if it is solely for looking up user preferences. The NaturalCrit Database is only ever accessed on login, and then does not need to be touched during use of the Homebrewery app.

2) Convert the Naturalcrit Database into a collection within the Homebrewery database, and delete the NaturalCrit database. User logins as well as preferences for the Homebrewery would be centralized in one location. However, this muddies the possibility of developing future apps under the NaturalCrit account system which would all end up sharing the database.

3) Leave the Homebrewery Database as is, and include all of the user preferences in the existing NaturalCrit Database. This also centralizes all user info in one location, but makes access a little trickier; we would need to set up Mongoose to simultaneously connect to both NaturalCrit and Homebrewery to pull data from both databases at once.

I may be missing something here, but personally option 1 seems the easiest and "best" way to do this. Yes, we would have two separate "user" collections, but they don't need to hold anything in common other than the username as an id. This way future apps could also have their own user settings specific to that app without worry of one app's settings treading over the other. I think this is also closer to @G-Ambatte 's original vision?

Once this is decided, I think we can select the main essential elements of the UserPreferences schema, perhaps limiting ourselves to "editorPrefs" for now, and implement that to resolve this issue. If that is successful, we can break out the other proposed features (user snippets, etc.) into their own issues to be worked on, since we will now have a framework to build on.

jeddai commented 2 years ago

FWIW if we do option one and make the schema reference the naturalcrit collection across the database assuming it's in the same physical MongoDB server.

https://docs.mongodb.com/manual/reference/database-references/

calculuschild commented 2 years ago

I think MongoDB Atlas places each Database on a separate physical server. We would need two collections in the same Database to make them share hardware.

However, with option 1 I don't think there is any need to cross-reference between databases anyway. The user logs in at NaturalCrit and gets a JWT token from it. Then we just get the username from that token when it comes time to look up any preferences in the Homebrewery database. We never need to go back and update anything in the Naturalcrit side unless we are actually on the login page.

jeddai commented 2 years ago

Fair enough, the JWT does make that a solid option.

Okay so it sounds like the plan is as follows:

Anything else we need to cover when we go to implement this?

ericscheid commented 2 years ago

When the editorPreferences that are returned are an empty object it sets them to the default.

Better: let prefs = Object.assign( defaultPrefs, prefs );

This way we can add new properties to existing prefs, using the new property defaults.

(modulo shallow vs deep concerns, of course)

jeddai commented 2 years ago

Ah yes, very solid suggestion.

calculuschild commented 2 years ago

I think this is a good framework to start on the PR. I am going to re-open https://github.com/naturalcrit/homebrewery/pull/1672 by @G-Ambatte since he had already started working on this, and I think now we all have a better roadmap for how to develop that PR.

G-Ambatte commented 2 years ago

If we are going to be storing Homebrewery-specific user settings, we need to clear up in which database that belongs. I see a few approaches, and I think we need to settle on one before moving on:

  1. Leave the Naturalcrit Database as is, and add a second collection to the Homebrewery database which simply stores username as an index, and then a list of their preferred settings specific to the Homebrewery. There is no need, I think, to store password hashes in this "preferences" collection if it is solely for looking up user preferences. The NaturalCrit Database is only ever accessed on login, and then does not need to be touched during use of the Homebrewery app.

I may be missing something here, but personally option 1 seems the easiest and "best" way to do this. Yes, we would have two separate "user" collections, but they don't need to hold anything in common other than the username as an id. This way future apps could also have their own user settings specific to that app without worry of one app's settings treading over the other. I think this is also closer to @G-Ambatte 's original vision?

My original intent was only to store user specific information - snippets, themes, whatever. It was only later on in development that I thought about using such a system for user specific notifications: specifically, I was thinking about notifying users of DMCA takedown processes, but it also occurred to me that it could be expanded to a full internal user messaging system. However, such notifications and messaging should require password verification, to ensure that the correct user has received the messages.

But if the database is already storing a password hash, then it is already halfway to supplanting the NaturalCrit login system completely. Moving to an internal login system would be useful for any users running local installs.

calculuschild commented 2 years ago

@G-Ambatte You would advocate for option 2 then? You bring up a good point that local installs would be made easier, although I think we can work around that; Stolksdorf already created a dummy login system specifically for that case, hidden away in his V3 branch if we want to dig into that.

As far as messaging, I think that starts to move outside the boundaries of what the Homebrewery is (a text editor), and treading into social media territory, which I don't think we want to do. System notifications, (DMCA notices, etc.) could happen eventually, but I think inter-user messaging is a can of worms we don't want to get into.

As far as password verification for notices, that is already handled by the JWT token. You log in at Naturalcrit, get a token that verifies you have used a valid password, and then all communication with the userInfo database, etc. also sends that token (as we currently do to detect whether we should display unpublished brews or not). Nobody should be able to spoof your token and access your messages.

What do you think?

Gazook89 commented 2 years ago

I also agree that inter user interaction should not be part of HB. Building a community and allowing browsing and such is much better handled by other platforms.

G-Ambatte commented 2 years ago

@G-Ambatte You would advocate for option 2 then? You bring up a good point that local installs would be made easier, although I think we can work around that; Stolksdorf already created a dummy login system specifically for that case, hidden away in his V3 branch if we want to dig into that.

I wrote a much longer response, but the short answer is "yes".

In the short term, though, option 1 can be implemented, in such a way that adding password information can be added to the collection later, effectively turning it into option 2.

--OR--

As a random thought, option 2 is locked behind a process.env.NODE_ENV check, so it only functions that way on local installs - existing functionality is maintained on the PRODUCTION web site.

G-Ambatte commented 2 years ago

I also agree that inter user interaction should not be part of HB. Building a community and allowing browsing and such is much better handled by other platforms.

Agreed - while from a technical perspective, such a system could be used to allow user interactions, inter-user messaging allows for inter-user abuse, and then suddenly Community Management becomes a full time job.

To crib a line from Unix philosophy: The program shall do one job, and do it well. Homebrewery is an editor that creates awesome looking content, with minimal barrier to user entry. Someone else can make D&D Twitter, or RPG Facebook.

calculuschild commented 2 years ago

Alright. Thanks you @G-Ambatte for the feedback.

I think then we shall move forward with option 1 for now, and separately look at reviving Stolksdorfs local login system, perhaps behind an environment variable.

G-Ambatte commented 2 years ago

I just remembered as I was perusing some of the code in #1672, I was - amongst other things - looking to provide a "lastActivity" field, so that we can tell how long it has been since a user was last using the site. I was working on the theory that this information might then be useful in determining if an account can be archived/deleted/etc.

A potential extension of UserInfo for a later date.

Gazook89 commented 2 years ago

lastActivity

this might also be helpful in verifying account ownership when calculuschild is resetting passwords/doing manual changes for users.

G-Ambatte commented 2 years ago

this might also be helpful in verifying account ownership when calculuschild is resetting passwords/doing manual changes for users.

Indeed! "When did you last use the site?" could potentially become an account verification question.

G-Ambatte commented 2 years ago

A comment today on the subreddit made me realize that deleting a brew will only remove it from the Recently Edited/Viewed lists in local storage on the machine that it is deleted from - these lists are not account wide. Moving this data to live in the UserInfo object would make it account wide; we could even improve the functionality to remove the brew ID from ALL UserInfo recently edited/viewed lists.

calculuschild commented 2 years ago

Good thought. Although we probably want to still fall back to localstorage in the case that the user does not have an account.

5e-Cleric commented 2 years ago

@jeddai about the styles for the editor, i've been using darkbrewery for a long time and works fine for me, you can check the colors used here. Hope it helps.

G-Ambatte commented 2 years ago

@jeddai about the styles for the editor, i've been using darkbrewery for a long time and works fine for me, you can check the colors used here. Hope it helps.

The immediate intention is to implement CodeMirror themes, which will allow users to select from the 64 options seen here. However, extending to a user generated custom theme should be possible.

G-Ambatte commented 2 years ago

Default page size is a User setting that could be added.

5e-Cleric commented 1 year ago

@G-Ambatte once brew themes are out, shouldn't default theme be an option in the user settings?

Gazook89 commented 1 year ago

Another "user setting" that could be incorporated is a "default to US Letter or A4 on new brews?" question.

5e-Cleric commented 6 months ago

to disable line numbers, css will suffice:

.CodeMirror-linenumber,.CodeMirror-gutters {
    display:none;
}
/*this margin is hardcoded for whatever reason*/
.CodeMirror-sizer {
    margin-left:10px !important;
}

This still leaves space for folding, if one would wish to remove that as well, we can remove the folding buttons as well:

.CodeMirror-foldgutter-open,.CodeMirror-foldgutter-folded {
    display:none;
}
5e-Cleric commented 6 months ago

3498 fixes page size default, pretty easy to set up.

Can set up some others in separate PRs. Will wait for full approval and consensus in how the feature is implemented.