Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

User settings #1908

Open
jeddai opened this issue Dec 19, 2021 · 34 comments · May be fixed by #2474
Open

User settings #1908

jeddai opened this issue Dec 19, 2021 · 34 comments · May be fixed by #2474
Assignees
Labels
Approved Has been discussed and an approach is agreed upon Epic P1 - high priority Obvious bug or popular features

Comments

@jeddai
Copy link
Collaborator

jeddai commented Dec 19, 2021

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:

  • enable/disable active line highlighting
  • enable/disable code folding
  • enable/disable line numbers?
  • change tab size
  • option to indent with tabs vs spaces
  • enable/disable auto-closing html/curly brace tags
  • enable/disable showing trailing spaces
  • editor theme?

Any other options/ideas people have are welcome!

@jeddai jeddai added the feature label Dec 19, 2021
@calculuschild
Copy link
Member

Can probably use this to support CodeMirror themes on this PR as well, which is just missing the user interface to swap themes: #1771

@jeddai
Copy link
Collaborator Author

jeddai commented Dec 19, 2021

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

@jeddai jeddai self-assigned this Dec 19, 2021
@G-Ambatte
Copy link
Collaborator

G-Ambatte commented Dec 19, 2021

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
Copy link
Collaborator Author

jeddai commented Dec 19, 2021

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.

@jeddai jeddai changed the title Persistent editor settings User settings Dec 19, 2021
@jeddai jeddai added Epic and removed feature labels Dec 19, 2021
@G-Ambatte
Copy link
Collaborator

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:

  • create new
  • update existing
  • get default object
  • get existing object

These can then be combined

@G-Ambatte
Copy link
Collaborator

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
Copy link
Collaborator Author

jeddai commented Dec 20, 2021

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
Copy link
Collaborator

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
Copy link
Collaborator

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

This (of course) should be

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

@calculuschild
Copy link
Member

calculuschild commented Dec 20, 2021

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
Copy link
Collaborator Author

jeddai commented Dec 20, 2021

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
Copy link
Member

calculuschild commented Dec 20, 2021

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
Copy link
Collaborator Author

jeddai commented Dec 20, 2021

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

Okay so it sounds like the plan is as follows:

  • a new UserSettings schema that has the following fields:
    • username (string, required)
    • editorPreferences (object, default: {})
  • When a user attempts to get their UserSettings, it creates it if it doesn't exist.
  • When the editorPreferences that are returned are an empty object it sets them to the default.
  • editorPreferences can be modified from any brew, and affect all brews.
  • editorPreferences are also stored in localStorage, but UserSettings.editorPreferences overrides the value in localStorage if it is set.

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

@ericscheid
Copy link
Collaborator

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
Copy link
Collaborator Author

jeddai commented Dec 20, 2021

Ah yes, very solid suggestion.

@calculuschild
Copy link
Member

I think this is a good framework to start on the PR. I am going to re-open #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
Copy link
Collaborator

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
Copy link
Member

calculuschild commented Dec 20, 2021

@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
Copy link
Collaborator

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
Copy link
Collaborator

@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
Copy link
Collaborator

G-Ambatte commented Dec 20, 2021

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
Copy link
Member

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
Copy link
Collaborator

G-Ambatte commented Dec 21, 2021

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
Copy link
Collaborator

lastActivity

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

@G-Ambatte
Copy link
Collaborator

G-Ambatte commented Dec 30, 2021

@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
Copy link
Collaborator

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

G-Ambatte added a commit to G-Ambatte/homebrewery that referenced this issue Mar 27, 2022
G-Ambatte added a commit to G-Ambatte/homebrewery that referenced this issue Apr 13, 2022
@G-Ambatte G-Ambatte linked a pull request Oct 28, 2022 that will close this issue
8 tasks
G-Ambatte added a commit to G-Ambatte/homebrewery that referenced this issue Oct 28, 2022
G-Ambatte added a commit to G-Ambatte/homebrewery that referenced this issue Oct 28, 2022
G-Ambatte added a commit to G-Ambatte/homebrewery that referenced this issue Nov 6, 2022
G-Ambatte added a commit to G-Ambatte/homebrewery that referenced this issue Nov 11, 2022
G-Ambatte added a commit to G-Ambatte/homebrewery that referenced this issue Nov 24, 2022
G-Ambatte added a commit to G-Ambatte/homebrewery that referenced this issue Dec 21, 2022
G-Ambatte added a commit to G-Ambatte/homebrewery that referenced this issue Dec 22, 2022
G-Ambatte added a commit to G-Ambatte/homebrewery that referenced this issue Dec 24, 2022
G-Ambatte added a commit to G-Ambatte/homebrewery that referenced this issue Dec 25, 2022
@5e-Cleric
Copy link
Member

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

@Gazook89
Copy link
Collaborator

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

G-Ambatte added a commit to G-Ambatte/homebrewery that referenced this issue Aug 29, 2023
@5e-Cleric
Copy link
Member

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
Copy link
Member

#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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Has been discussed and an approach is agreed upon Epic P1 - high priority Obvious bug or popular features
Projects
None yet
6 participants