Closed tralph3 closed 1 year ago
I would also like to note that it'd be preferable to split the DBConnectionString
env variable into multiple separate variables for each setting, since it's cleaner and easier to configure.
Left some feedback on the proposed changes above. I tested this functionally last night and can confirm this works as expected. Thank you for putting this together!
Please also do check #11 in case they did a better implementation.
@DavidLazarescu this looks good to me. I would suggest we open up follow up issues for:
* adding a sample docker-compose.yml to the documentation * supporting running the server without SMTP variables defined * supporting running the server without JWT variables defined * splitting the DBConnectionString into separate variables
I am afraid that the SMTP variables are required. While working on the current solution to self-hosting I had the idea of completely removing the SMTP stuff as well and not require any email verification, but I didn't find a solution to how we'd be able to securely implement a "reset password" functionality without sending an email to the account.
Please also do check #11 in case they did a better implementation.
I don't have the docker experience to judge about that. Looking through the files, both of the PRs seems to be doing similar things though.
I didn't find a solution to how we'd be able to securely implement a "reset password" functionality without sending an email to the account.
For people self-hosting the server, they could just reset the password manually :thinking:
Please also do check #11 in case they did a better implementation.
I don't have the docker experience to judge about that. Looking through the files, both of the PRs seems to be doing similar things though.
I have reviewed this one simply because it was up first, so I haven't functionally reviewed #11, but I would argue neither of the two is "better" necessarily. This PR builds the docker image via multi-stage process, which leads to a slimmer image, while #11 provides a sample docker-compose and documentation updates, so it's a bit more polished from that perspective.
I didn't find a solution to how we'd be able to securely implement a "reset password" functionality without sending an email to the account.
For people self-hosting the server, they could just reset the password manually 🤔
While that's true, it would mean that we wouldn't provide the full functionality to the users and every future feature that will have something to do with an email address will need to be manually taken care of. This is why I and the contributor I have worked together with on the initial self-hosting support agreed to keep the SMTP credentials in.
If we get the feedback that it's a big burden, we might look into it, but I'd propose to keep it as simple as possible for now and just get the docker support running.
Please also do check #11 in case they did a better implementation.
I don't have the docker experience to judge about that. Looking through the files, both of the PRs seems to be doing similar things though.
I have reviewed this one simply because it was up first, so I haven't functionally reviewed #11, but I would argue neither of the two is "better" necessarily. This PR builds the docker image via multi-stage process, which leads to a slimmer image, while #11 provides a sample docker-compose and documentation updates, so it's a bit more polished from that perspective.
I suppose it would be good if we could merge of these together.
If the SMPT stuff has to stay, then how do we create users? Is there a cli tool that we can use to create a user manually?
If the SMPT stuff has to stay, then how do we create users? Is there a cli tool that we can use to create a user manually?
I am not sure what you mean. Why wouldn't creating accounts via the register page work?
Both pull requests are functional and the end result of both differs primarily in stock configuration, the primary difference between #10 and #11;
I may be grading my own work, but I'd opt for #11, but not without credit to #10, I copied some ideas #10 had implemented such as disabling HTTPS by default.
If the SMPT stuff has to stay, then how do we create users? Is there a cli tool that we can use to create a user manually?
I am not sure what you mean. Why wouldn't creating accounts via the register page work?
The account gets created, but since there's no real SMTP server, the confirmation email never gets sent, so I can never use the account.
I would need a way to either confirm it manually, or create it manually.
Yes, that's why we require the user to input their SMTP credentials when selfhosting.
So once you add your SMTP credentials you should get the email
Well, that's the point, most people who self host do it for themselves and maybe a few others. There's no need for a system like this. I don't want to setup an SMPT server.
Well, that's the point, most people who self host do it for themselves and maybe a few others. There's no need for a system like this. I don't want to setup an SMPT server.
That was my original thought as well, but I was told that it is very easy to set up and non-selfhosted services like gmail also give you SMTP data.
I suppose we could remove the SMTP stuff completely and make the user reset their password via SQL in their DB directly.
I've seen most other services provide a CLI tool to manage it. In nextcloud for example, I can open a shell inside the container and use the occ tool to manage the installation. This inclides the creation, deletion, confirmation, and general management of users of course.
Something like that would be good.
Ok, so apart of the SMTP problems, this PR is working, right?
I would also argue we should credit #11 if possible, as many of the improvements made here were @dennis1248's ideas / suggestions.
I have just re-verified that the latest version works.
Great, I'll remove the SMTP stuff then and run a final test before merging it.
Sounds good to me, although that can even be a separate PR IMHO.
Yes, going to push that to dev/develop.
@tralph3 please change this PR to request a merge into dev/develop instead of main.
There we go.
Well, I just tried to deploy the container and for some reason it's giving me a permission error. The owner of the directory I'm mounting as a volume (/var/lib/librum-server/librum_storage
) is the user 1000. The user librum-server
is 999.
It's weird, since I had this problem before, but I fixed it by creating the librum_storage
directory before setting the volume, since according to the docs, the VOLUME directive inherits the already established permissions. And this worked at the time, and the Dockerfile has not been changed since, so I don't know why it's not working now.
Can anyone confirm? Is it just me?
The easiest solution would be to not have a librum-server
user at all and just run the whole thing as root. Does the server expect this user to be present? Does it expect its home to be in /var/lib/librum-server/srv
?
That's weird, I have tried the instructions (by simply copy pasting them) on multiple ubuntu machines from scratch and never ran into any such problems.
I think running it as root inside the Docker container shouldn't be any problem, so feel free to make a PR changing it if it fixes it.
It works on my end too. Typically running as non-root user is considered as better practice than running as root, from the security perspective, so I would advocate not changing that if possible.
This setup (volumes used with non-root container users) definitely works for many other projects out there so I suspect this is a problem isolated to @tralph3's setup. Which should be solvable by running sudo chown -R 999:999 <volume_folder>
.
Yes, it worked after changing permissions of the binded folder, but I had to recreate the container, simply restarting it wasn't working either. It's odd, because I tried the same Dockerfile before on the same system and it worked... anyway.
This PR adds a rough Dockerfile as an MVP. Features like user-creation do not work, but hopefully these issues can be ironed out.
The PR also includes a modified
appsettings.json
file which removes the https configuration, as it doesn't create its own certificate, letting you provide one with a reverse proxy. It also changes the HTTP url to0.0.0.0
to allow it to broadcast the server to the LAN (which in this case is the Docker network).Environment variables have been hard coded in order to work with a
docker-compose
file I've made for testing purposes. Better defaults should be proposed, and they can be easily overridden when creating the containers anyway.Usage
Firstly you need to build the docker image. Assuming you're in the repo's root directory:
This creates a docker image with the name "librum".
Now you need to create a container based off of this image, but running it by itself won't work. You need to have a MariaDB database running first. For this reason, I have made a
docker-compose
file that runs and configures a database to work with Librum.After creating the image, just run:
This will create the container for the database and for Librum, join both of them on their own network, and hopefully have them working.
The tag for the mariadb image should also be pinned to a specific version, but that's a decision I'll leave to the developers.