Disable/Enable edit mode

SeDemal commented 1 year ago

Environment

Docker

Version

0.13.2

Describe the problem

The "Disable/Enable edit mode" button state it requires the environment variable named "EDIT_MODE_PASSWORD". It then states that if none is set then toggling is not possible.

Problem: When the EDIT_MODE_PASSWORD is not set, toggling is not only possible, it will be done regardless of what password you enter. When it is set, it will also behave that way.

Optionally, I think this whole option should not even be accessible from the UI if EDIT_MODE_PASSWORD is not set, which would be a clear indicator that the variable hasn't been set correctly or at all.

Logs

No response

Context

No response

Please tick the boxes

[X] I confirm that I attached the proper logs
[X] I've read the docs
[X] I've checked for duplicate issues
[X] I've tried to debug myself

SeDemal commented 1 year ago

as seen here, setting the PASSWORD env var did indeed work and prompted the login page, which means EDIT_MODE_PASSWORD is supposedly also set correctly. This could Require further testing from someone more knowledgeable with docker as I am not certain that my setup is correct, however it is still clear that you can change the edit mode when you shouldn't when no password is set.

SnippetSpace commented 1 year ago

I can confirm this issue. I then set the variable, and the toggle still works regardless of the data entered as password

Gordi90 commented 1 year ago

Same here, using cosmos-cloud for PaaS. Tried to set EDIT_MODE_PASSWORD directly, tried to set it via file. Checked inside the container, file content was accessile.

Neither EDIT_MODE_PASSWORD nor DISABLE_EDIT_MODE worked.

Version: 0.13.2

What more info can I provide?

Willy-JL commented 1 year ago

I am gonna be 100% honest here and say that I am absolutely flabbergasted by the level of sheer incompetence with the implementation of the edit mode lock and other security practises in general. This is not "experimental", this is shit. @ajnart it honestly looks like you put more effort into writing documentation for how broken this option is, than into actually making it.

For a few main points:

https://github.com/ajnart/homarr/blob/52ccbb3938b5d920f564705c1b1c479584167ce4/src/pages/api/configs/tryPassword.tsx#L8 the actual check is if the result of (tried === process.env.EDIT_MODE_PASSWORD) is different from undefined. that operation will always return a defined value. you are basically asking if (true === true). ON A PASSWORD CHECK MY MAN. throwback to when that google executive went rogue and removed the password check from all google logins...

https://github.com/ajnart/homarr/blob/52ccbb3938b5d920f564705c1b1c479584167ce4/src/hooks/useEditModeInformation.ts#L8-L11 https://github.com/ajnart/homarr/blob/52ccbb3938b5d920f564705c1b1c479584167ce4/src/pages/_app.tsx#L98-L100 what the fuck is this naming???? the variable is called "editModeEnabled" but by your logic, it indicates whether editing is disabled. and in perfect foolish behavior, you shot yourself in the foot and didnt understand the code you yourself wrote, so part of it uses it as if it meant "enabled" while other parts as if it meant "disabled". booleans are not complicated dude

https://github.com/ajnart/homarr/blob/52ccbb3938b5d920f564705c1b1c479584167ce4/src/server/api/routers/notebook.ts#L16 environment variables are strings. not boolean. here you are just checking if the variable is empty

finally, the api endpoint for updating the main config has no check at all for edit mode being disabled

I have gone ahead and forked the project. All of this is fixed. "Fixed" as to say, that NOW it can be considered "experimental", after I fixed it up. I by no means claim that my changes are bullet proof from someone skilled enough. But atleast the fucking user interface works as intended.

You can find it all fixed here: Sluthub/homarr

SeDemal commented 1 year ago

@Willy-JL Hi, sorry you got so frustrated over this. This is a project made by people on their free time and anyone can make a contribution. That includes you btw, you can simply send a PR with the fix and it would be welcomed with open arms.

This feature does matter for the current version, but won't in the future since a way bigger auth system is being worked on as we speak and will be available shortly to test out in a beta.

Nobody's perfect but if you want to make a contribution to make homarr better, it's appreciated. Don't let that one feature discourage you from using homarr.

ajnart commented 1 year ago

Although these are fair points, I am also sorry you feel it is shit @SnippetSpace , as stated by @Tagaishi, I work on homarr on my free time, for free.

We didn't bother to fix the EDIT_PASSWORD part of homarr because it will be completely removed in the auth version that we've been working on for the past months.

If homarr is not good enough for you to use as a free self hosted app, then you could submit PRs to polish the parts you think are lacking or simply not use it. Homarr was built as my first ever frontend project and has never been about pleasing users or having as many users as possible. I do not make any money from it and have no plan of monetizing it. I just built homarr to learn about web. dev and it's still the case.

Everyone's learning and so are you, and we all learn by building stuff. Instead of complaining about how things are you should've fixed the issue, submitted the PR and explained your reasoning for doing so, by highlighting the bad practices of the previous code. This is how open-source work is supposed to be.

Now you've just proved us your childishness by forking the project and "fixing" the issue without submitting a PR and then leaving this comment.

SnippetSpace commented 1 year ago

@ajnart wrong person tagged! I just said it didn’t work. I appreciate all the hard work lol.

manuel-rw commented 1 year ago

I am gonna be 100% honest here and say that I am absolutely flabbergasted by the level of sheer incompetence with the implementation of the edit mode lock and other security practises in general. This is not "experimental", this is shit. @ajnart it honestly looks like you put more effort into writing documentation for how broken this option is, than into actually making it.

For a few main points:

https://github.com/ajnart/homarr/blob/52ccbb3938b5d920f564705c1b1c479584167ce4/src/pages/api/configs/tryPassword.tsx#L8 the actual check is if the result of (tried === process.env.EDIT_MODE_PASSWORD) is different from undefined. that operation will always return a defined value. you are basically asking if (true === true). ON A PASSWORD CHECK MY MAN. throwback to when that google executive went rogue and removed the password check from all google logins...

https://github.com/ajnart/homarr/blob/52ccbb3938b5d920f564705c1b1c479584167ce4/src/hooks/useEditModeInformation.ts#L8-L11 https://github.com/ajnart/homarr/blob/52ccbb3938b5d920f564705c1b1c479584167ce4/src/pages/_app.tsx#L98-L100 what the fuck is this naming???? the variable is called "editModeEnabled" but by your logic, it indicates whether editing is disabled. and in perfect foolish behavior, you shot yourself in the foot and didnt understand the code you yourself wrote, so part of it uses it as if it meant "enabled" while other parts as if it meant "disabled". booleans are not complicated dude

https://github.com/ajnart/homarr/blob/52ccbb3938b5d920f564705c1b1c479584167ce4/src/server/api/routers/notebook.ts#L16 environment variables are strings. not boolean. here you are just checking if the variable is empty

finally, the api endpoint for updating the main config has no check at all for edit mode being disabled

I have gone ahead and forked the project. All of this is fixed. "Fixed" as to say, that NOW it can be considered "experimental", after I fixed it up. I by no means claim that my changes are bullet proof from someone skilled enough. But atleast the fucking user interface works as intended.

You can find it all fixed here: Sluthub/homarr

Do you know, what flabbergasted me? Your rude and aggressive behaviour.

implementation of the edit mode lock and other security practises in general

We label both features as experimental, because we do not really maintain them actively and both were a quick way of enabling the community to use it.

We are currently working on an authentication overhaul, that will remove the mentioned authentication variables.

It will also support server side sessions and we are incrementally replacing JSON configuration with a relational database.

what the fuck is this naming???? the variable is called "editModeEnabled" but by your logic, it indicates whether editing is disabled.

You are correct with that one, the naming is not ideal. @Tagaishi do you have capacity to rename quickly?

You can find it all fixed here

Thank you for forking and implementing fixes. You're welcome to contribute them to the original upstream repository if you want.

However, I would like to remind you something. Homarr is a project, that has been evolving over the year. A team of volunteers is working on it. We are not getting compensated for any of it and we do it for the fun.

The original Homarr was somewhat terrible. We still use many of the legacy code in today's version, although it has been improved in some places. We are removing said technical debt within the next few updates. This should also resolve a few of your issues, that you're having.

Lastly, I want to mention, that nobody will force you to use Homarr. Homarr is open source and free to use. If you're unhappy or concerned about it's security, do not use it... There are many alternatives, if you do not wish to use Homarr.

Please keep the discussion here less toxic and rude. A good developer gives constructive and detailed feedback, instead of a rant in a random discussion.

I am the person, that is responsible for much of the decisions and architecture of the current design. So please let me know if you have any suggestions.

Cheers 🥂

Willy-JL commented 1 year ago

I am terribly sorry for the rudeness of my previous comment, guess 5 am me that was awake for nearly 24 hours was on his last straw and snapped, I do tend to have some anger management issues, although I'm aware this doesn't justify it my any means.

I fully agree with all your points, and thank you for working on this awesome project. I guess I was just very shocked to see that this feature in particular was so rough around the edges while everything else seemed in great condition, both from functionality, code and looks too.

That said, I did intend on making a PR with these changes despite my unjustified outlash, would you all be interested in it for the time being while the authentication rework is in progress?

ClaraCrazy commented 1 year ago

Let me just start this by saying that I am disappointed, and sorry. As the manager of @Willy-JL on this project, I feel the need to say a few words.

I just woke up, seeing his message in here, expecting it to be like "Hey, i made improvements to this code, heres a PR" or just "Hey, I made improvements to this code, but dont have time for a PR right now, feel free to take it", but then I kept reading, and kept getting more angry before I even had my morning properly begin.

Is this code clearly not production ready? Yes. Should this code maybe have been inside a separate dev branch? Yes. Does this warrant such behaviour? Fuck. No. Not every project on github is made by a team of skilled developers, not every line of code can be perfect or functional, and not everyone even understands everything about what they try to achieve just yet. That is fine, and please do not let his behaviour get you down. We all started somewhen, and our first projects all "sucked" in comparison to what we (will) write years later. That is the joy of learning, and perfecting your skills. Behaviour like this can seriously damage or kill a project, and I do NOT want to see that happen.

Yesterday was a day of trying many github projects that are certainly not up to our standards, and that reminded me about how github is for everyone, not just professional developers. And I for my part welcome newbies into the community as well, and while their code might not be perfect as of right now, some day with enough handholding, guidance and support it will. Honestly, when I noticed that the env var to disable edit mode doesnt do anything I had the worst idea imaginable, and I still think its rather funny: "Just remove the button, lol.". Glad he fixed it now, but yeah.. not like this!

Once again, I'm sorry for his behaviour. I appreciate everything you did so far on this project, even if its buggy at times or parts are non-functional. The idea and the attempt is what counts and I hope that you keep trying and keep adding more to this.

I will have to talk to Willy now, as he apparently is awake given his comment just while I was making sure this text is fine, to see what he has to say, and then I will tell him to open a PR with constructive critisism, pointing out the flaws in a normal tone, and I hope that it will be all fine.

Thats all for now. Thanks, and love the project <3

manuel-rw commented 1 year ago

Hi 👋. Let me just thank you both for your honest and direct replies. I highly appreciate it.

I think a few things here went wrong:

We don't have any automated testing, that protects us against regressions. Therefore we also don't know, since when this feature has been broken.
I understand the security concerns and complications, when such features do not work. It's very hard to deal with and I understand the frustration of the initial comment.
Personally, I didn't even notice or hear of this issue. I get hundreds of emails daily and I probably overlooked it. I will adjust my Email rules so this hopefully doesn't happen again. Regarding the response time on this issue, I think we should implement a prioritization system for issues @ajnart . I don't have any overview on the current list of issues. We usually have something near to 100 issues constantly and I don't know, what has high priority.
We should label experimental features in the future more clearly. It seems that many people are confusing or misunderstanding (understandably).
We should hurry with the release of the authentication update. @ajnart please try to fix the CI and then we should merge quite soon. Currently, we have a ton of branches open, that we can't merge, because we're waiting for other changes. If we can finally close those and release authentication, we can tackle such issues faster.

Both I and @Meierschlumpf are somewhat knowledgeable about our technology stack. I have to admit, that we should have noticed this issue earlier.

Anyhow, I think everyone here can agree, that trying to blame a single person won't benefit anybody and rude comments don't provide any value. @Willy-JL we would be incredibly thankful if you could contribute your fixes. This way, we can concentrate on the authentication update. You can also check out the current state of our authentication at feat/add-basic-authentication , if you're interested.

I'm not sure about the other maintainers, but personally, I'm not satisfied with code quality and our current testing methodology. We should do better and learn.

@ajnart and @Meierschlumpf can we maybe coordinate a meeting, were we work together on the auth release and maybe work out a plan, to improve?

manuel-rw commented 1 year ago

Merged #1333 . Will be part of 0.13.3. Thank you everyone in this conversation for your help and patience. 👍

ajnart / homarr