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

Selfhosting #8

Merged
merged 49 commits into from
Nov 4, 2023
Merged

Conversation

3036662
Copy link
Contributor

@3036662 3036662 commented Oct 26, 2023

IF LIBRUM_SELFHOSTED != true - nothing will be changed.

IF LIBRUM_SELFHOSTED == true

  1. UserBlobStorage and BooksBlobStorage classes will be substituted by UserLocalStorage and BooksLocalStorage with Dependecy Injection mechanism
  2. EmailSender.cs will use variable SMTPMailFrom to fill email sender author.
  3. AIService.cs will send simple string informing that OpenAi not available if no API token exists, otherwise it will act normal.
  4. UserConroller will use it's own http methods to show simple "password reset" form.
  5. Program.cs will create local database structure if not exists.
    PS.
    BookRepository.cs string to Double parsing method was changed due problems with running on linux (it was throwing exception)

I would like you to take a look, I'm pretty shure we can work it out to run perfect!
For now we can run this server as linux service in fully self-hosted mode using install instruction from my fork.
At the same time azure services will continue their normal work if not self-hosted.

@DavidLazarescu
Copy link
Member

Thank you, this looks great. I'll look over it tomorrow and give you detailed feedback on it.

@DavidLazarescu DavidLazarescu changed the title Pr tech selection Selfhosting Oct 27, 2023
Copy link
Member

@DavidLazarescu DavidLazarescu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really good! I added some notes to your code, please look over them and change the PR respectively.

src/Application/Managers/BookLocalStorageManager.cs Outdated Show resolved Hide resolved
src/Application/Managers/BookLocalStorageManager.cs Outdated Show resolved Hide resolved
src/Application/Managers/UserLocalStorageManager.cs Outdated Show resolved Hide resolved
src/Application/Managers/UserLocalStorageManager.cs Outdated Show resolved Hide resolved
src/Application/Managers/BookLocalStorageManager.cs Outdated Show resolved Hide resolved
src/Application/Managers/UserLocalStorageManager.cs Outdated Show resolved Hide resolved
src/Application/Services/AIService.cs Outdated Show resolved Hide resolved
src/Application/Utility/EmailSender.cs Show resolved Hide resolved
@3036662
Copy link
Contributor Author

3036662 commented Oct 27, 2023

Hi, thanks, I'll do everything on Monday.

@3036662 3036662 marked this pull request as draft October 30, 2023 08:03
@3036662
Copy link
Contributor Author

3036662 commented Nov 2, 2023

nice to remove

It's always easy way to delete some functionality, but not very easy to add it.
I tried not to reduce the possibilities but to increase them.

For home use you can just push 1 value to whole database column (AspNetUsers->EmailConfirmed) all users will become verified. In my opinion - "delete unconfirmed users it's very good thing. I've created a dozen users while testing, and all of them were automatically deleted by server because of unconfirmed email.

But i think it's up on you to decide.

@DavidLazarescu
Copy link
Member

DavidLazarescu commented Nov 2, 2023

nice to remove

It's always easy way to delete some functionality, but not very easy to add it. I tried not to reduce the possibilities but to increase them.

For home use you can just push 1 value to whole database column (AspNetUsers->EmailConfirmed) all users will become verified. In my opinion - "delete unconfirmed users it's very good thing. I've created a dozen users while testing, and all of them were automatically deleted by server because of unconfirmed email.

Hm, I get your point, but I don't think that its worth the extra struggle for the end user. I think by far most people who want to self-host Librum will simply setup one account and be happy with it. If they want an account deleted, they can simply delete it by logging into it and clicking "Delete Account" in the settings.

Setting up an extra SMTP service to simply use Librum is a big burden imo. so keeping it as simple as possible would be good.

Also, by not requiring the user to set up an SMTP service, we literally just require the user to copy paste a few lines (which we provide in the documentation) and it will just work.

@3036662
Copy link
Contributor Author

3036662 commented Nov 2, 2023

Setting up an extra SMTP service to simply use Librum is a big burden imo. so keeping it as simple as possible would be good.

I Never can argue with "Keep it simple" ! )

@DavidLazarescu
Copy link
Member

From what I see the server only sends emails in two places: When registering and when resetting the password.

I have already removed the email sending while registering. I'll now look into the password resetting.

@DavidLazarescu
Copy link
Member

The only option to avoid the email sending in the password reset is to open a browser with the form right from the server, but this is not a very nice way to handle this.
Do you have another idea?

@3036662
Copy link
Contributor Author

3036662 commented Nov 2, 2023

I believe that a user who can install and configure the database and server service in linux will not be confused by the mail setup, its basics, no rocket since.
Still, the ability to reset the password and a login tied to the mail is not bad, you can run a server, for example, for a whole house, for neighbours.
But, I won't argue.
If we continue with disabling simple checks, it will transform to simple local storage. I would consider to save full functionality.

IMHO, the program is primarily interesting for the possibility of launching a full-fledged server.

If we disable sending email when password reset, we can same way disable password, i think.
It's about local storage reader, or about server without password, i don't sure it's worth "super simple installation".

May be email confirmation for register is not necessary , but for password reset you need email or sms, it's basic way to keep data private.

@DavidLazarescu
Copy link
Member

I believe that a user who can install and configure the database and server service in linux will not be confused by the mail setup, its basics, no rocket since. Still, the ability to reset the password and a login tied to the mail is not bad, you can run a server, for example, for a whole house, for neighbours. But, I won't argue. If we continue with disabling simple checks, it will transform to simple local storage. I would consider to save full functionality.

IMHO, the program is primarily interesting for the possibility of launching a full-fledged server.

If we disable sending email when password reset, we can same way disable password, i think. It's about local storage reader, or about server without password, i don't sure it's worth "super simple installation".

You are right, I did not think about the fact that not sending an email to the account's owner on a password reset would basically make passwords useless and thus destroy all of the security.

I will revert the last commit I made and we will keep the email sending for self-hosted servers.

@DavidLazarescu
Copy link
Member

Ok, it is running just fine together with the client application.
I'll do some more testing tomorrow and if everything goes well we can merge this soon.

DavidLazarescu and others added 6 commits November 4, 2023 09:58
It seems like mariadb didn't like it that updating highlights just overwrites all of them with the new ones thus it threw an error. I have now added proper updating of highlights.
@DavidLazarescu
Copy link
Member

Ok, everything looks good so far. Have you tested if password resetting works fine for the self-hosted server? I don't have a self-hosted email, thus I can not test it.

Can you confirm that it works fine?

@3036662
Copy link
Contributor Author

3036662 commented Nov 4, 2023

It looks like you're putting things in order today. good job!

@3036662
Copy link
Contributor Author

3036662 commented Nov 4, 2023

Ok, everything looks good so far. Have you tested if password resetting works fine for the self-hosted server? I don't have a self-hosted email, thus I can not test it.

Can you confirm that it works fine?

You can test it with any public email service like gmail, aol, etc. I' ve made some tests, it works fine with my email server.

@3036662
Copy link
Contributor Author

3036662 commented Nov 4, 2023

Ok, everything looks good so far. Have you tested if password resetting works fine for the self-hosted server? I don't have a self-hosted email, thus I can not test it.
Can you confirm that it works fine?

You can test it with any public email service like gmail, aol, etc. I' ve made some tests, it works fine with my email server.

I mean you don't need a self-hosted email server. You can use any public email, if it allows smtp mail client usage.
Just for instance, i think it will fit:
https://support.google.com/a/answer/176600?hl=en

@DavidLazarescu
Copy link
Member

It looks like you're putting things in order today. good job!

Yes, pretty much everything should be done now. Could you change the PR request to merge into dev/develop instead of main though?

@3036662 3036662 changed the base branch from main to dev/develop November 4, 2023 14:31
@3036662
Copy link
Contributor Author

3036662 commented Nov 4, 2023

It looks like you're putting things in order today. good job!

Yes, pretty much everything should be done now. Could you change the PR request to merge into dev/develop instead of main though?

Done.

@DavidLazarescu DavidLazarescu merged commit da0a1f9 into Librum-Reader:dev/develop Nov 4, 2023
2 checks passed
@3036662 3036662 deleted the PR_techSelection branch November 7, 2023 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants