Add basic multiple admin users

[M1CK431]

Users list:

Confirm modal on disabling user:

Add an admin account:

Edit an admin account:

[CommanderStorm]

Just a general suggestion:

• the reason Feature: Multiple Users louislam/uptime-kuma#2506 never got done, was because everything was tried to be changed all at once
• then the person proposing the change disappeared

To prevent this, please:

• When proposing PRs upstream, try to keep the PR atomic
  • => keeping it test/reviewable (1 PR -> solves 1 Sub-Issue)
  • => keeping it large enough that, Louis reviewing it does not stall this feature too much.
• provide tests where it makes sense
  Rationale: adding tests would increase confidence
  => make merging a PR less risky for Louis

[M1CK431]

Hi @CommanderStorm, thanks for taking time giving me such interesting suggestion 👍🏼

the reason louislam#2506 never got done, was because everything was tried to be changed all at once
then the person proposing the change disappeared

That's a good reason to not merge it. My approach try to be far simpler so I guess it should avoid the first issue. And for the second one, I'm not that kind of people.

to prevent this [...]

You are perfectly right, my proposal will only contains code related to basic multi admin account support, nothing else.
About UT, I write some years ago, really hate that, so you can be absolutely sure I will restrict them to the strict minimum!

If you are okay, once my code will be working, could you please take a first look and indicate me the "tests [that] makes sense" I should add?

[M1CK431]

@CommanderStorm sorry to disturb you, I have a question about translations.

I have a few translations to add, I currently add them in en and fr-FR (the only languages I know). I have read in https://github.com/louislam/uptime-kuma/blob/master/CONTRIBUTING.md#can-i-create-a-pull-request-for-uptime-kuma that translations should be done using weblate. Since I never use this tool, to learn more about that, I try to open le link provided here: https://github.com/louislam/uptime-kuma/blob/master/CONTRIBUTING.md#translations but the link is broken (404).

At the end, I dont know how to properly handle translations in my PR 🤷🏼‍♂️
Could you explain me please? 🙏🏼

[CommanderStorm]

Please only add the strings which are translatable to en.json.
The reason for this is that otherwise this might cause a merge-conflict like louislam#3507

The translations can then (after merging a PR into master) be translated by our army of translators over at weblate

sorry to disturb you

No worries, ping me as many times as you want ^^

I dont know how to properly handle translations in my PR

What are you confused about (where do we need to update the docs?)

[M1CK431]

This is awesome, you are so reactive! Thanks a lot!
Ok I will remove the fr-FR translations, only keeping en.

No worries, ping me as many times as you want ^^

😘

What are you confused about (where do we need to update the docs?)

As I try to explain (sorry it was probably unclear), this section: https://github.com/louislam/uptime-kuma/blob/master/CONTRIBUTING.md#translations contains only one link which is broken (404):

So I never found how to handle translations "properly", I mean in a way it's acceptable at PR review time.

To be perfectly clear, the missing information is the one you gently provide:

Please only add the strings which are translatable to en.json.
The translations can then (after merging a PR into master) be translated by our army of translators over at weblate

I guess that information would be originally provided on the broken link page 🕳️

[CommanderStorm]

One thing that, I think, is worth commenting on despite you not being done with this PR:
Previously, the relationship between monitors/.. and users was that users “own” the monitors (despite us not having more than one user).
Now you are changing the relationship to not have these checks any more.

• Things which previously were namespaced to a user now are in a global scope.
• This makes some interactions impossible
  • one user not seeing all the monitors
  • one user granting another user access to some of their monitors.

I am fine with this change, but I am not the bdfl (Louis).
He will likely need a reason why all of these checks are removed.

[M1CK431]

is worth commenting on despite you not being done with this PR

No worries, your comments are more than welcome! 🤗

users was that users “own” the monitors (despite us not having more than one user)

My approach is: we store in the DB the user id of the ressource creator (here monitors) which is a different concept of ressource owner. After my change, ressources are totally user independent. Without that, "multiple admin user" makes no sense. To be honest, I was very surprised to discover that "user relationship" since, until now, UK assume that only one user can exist... 🤷🏼‍♂️

IMHO, that user ownership implementation is nothing else over-engineering:

• Or UK implement a real access rights mechanism (multiple users, ACL and/or RBAC, ...), and so it can't be limited to just "namespaced" ressources
• Or UK assume to not have access rights management (for now) and so we should not have related code in the app

wdyt?

[CommanderStorm]

My thoughts on this are kind of irrelevant. (as stated above: I am not the bdfl...) Only Louis vote really matters.
If you want a reliable (might not be quick) answer before wasting a lot of effort, please open an empty discussion pull-request

I think that removing core-code without a good reason needs an explanation.
I see reasons why keeping some relationships user-scoped might be advantageous (see above).
Likewise, I also see reasons why removing the code simplifies the product in a good way.

Note that not having access rights management and user ownership is not an either or choice.
Keeping this in the GitHub-analogy can have Org-Admins, which can see private repositories of all users and normal users which can only if permitted.
I think that such a change would likely overcomplicate things, making this a good avenue of security risks.

Asking the community in louislam#128 might also be helpful:

Hello community, we need some feedback.
@M1CK431 is currently developing a draft of user-management, and we have found that the current code makes monitors and many other dependant resources like notifications (used for storing certificate expiry notifications) a user-scoped resource (as in a user owns a monitor).

Assuming granting access to other users' monitors is a thing that can be done with acceptable complexity:

• Is a system where monitors belong to its creator a system which makes sense?
• What Resources should belong to users and what to the system itsself?
• Would simplifying the permission system to Admininstrator/Maintainer/Read-Only` with all users having access to all monitiors be a bad thing?

~ Frank (@CommanderStorm) and @M1CK431

TLDR: I think opening an PR and discussing the work plan now (or after further investigation) is a good idea.

[M1CK431]

My thoughts on this are kind of irrelevant. (as stated above: I am not the bdfl...) Only Louis vote really matters.

Yes I know, but you clearly very well knows UK (at least faaaar better than me) and so, with your experience, you act as a first "filter" 🚔

I think that removing core-code without a good reason needs an explanation.

Of course! The explanation is very simple however: following KISS philosophy. I'm trying to provide a very basic multiple admin users support. Since we are speaking about admin users only, they obviously have access to everything. That's why the current resources user-scoping can't be kept.

Note that not having access rights management and user ownership is not an either or choice

I kind of agree. But then there is more questions to solve, for example: what happens on user removal? Should we remove all associated resources in cascade? Probably not! So what to do? Transfer ownership to another user? Or perhaps implement user "soft delete" in DB (could be a deleted_on datetime field in user table) to respect DB integrity constraints and ability to display the resource owner in the app?

So many questions... changes... for a so small benefit... at the end I think that the simplest solution is to make resources independent from users.

But please keep in mind that my proposal is just a first step in the right direction. I really hope to see enhancements (like ACL and/or RBAC system) in the futur.

Asking the community in louislam#128 might also be helpful

I carefully read that issue, the community request is very clear: they want multi users support with rights management (in a way or another). I'm afraid a so open discussion might takes a lot of time because of the huge amount of people waiting for that feature. Because of that, my plan is to start with the very basic approach you know, then, if (😝) once accepted by Louis, open the discussion like you suggest for futur enhancements (mainly about the rights management system I guess). At least we'll have a foundation to build on 🏗️

TLDR: I think opening an PR and discussing the work plan now (or after further investigation) is a good idea.

As you know I have provide my WIP code ASAP because I'm always open to discussion and advise, especially as a new comer on a project 👶🏼
But I think that such discussion might be more constructive while based on a concret proposal. That's why I'm currently working on the webapp side in the hope to have something working this week and quickly opens a PR.

[M1CK431]

@CommanderStorm could you please take a look on my work? 🙏🏼

[CommanderStorm]

Sorry for the previous two comments. I did mean to include them into this review, but I tried https://github.dev/ (can recomend) for reviewing PRs ^^.
This is a partial review of the code only.
I still need to do a design/UX/functionality-pass respectively.
I am going to the cinema tomorrow evening, so I will get to test this deeper on Wednesday or Thursday.

• checkLogin now should also check for the disabled state of a user
• overall, the work quality is excellent. (no obvious defects from a code perspective, no obvious defects from a security perspective)

[ShadowGaming100]

I don’t know if this will be possible but I have some features you could add

1. Add limits
   • Add how many monitors it can create. Etc
2. Add admin able to edit user
   • Edit user monitor
   • Pause/Resume/Delete it
   • Allow specific admins to do it

Is ok if I add more features to edit

Very good feature, keep it up

which to be implemented soon to Uptime Kuma

[M1CK431]

I don’t know if this will be possible but I have some features you could add

1. Add limits

   • Add how many monitors it can create. Etc

2. Add admin able to edit user

   • Edit user monitor
   • Pause/Resume/Delete it
   • Allow specific admins to do it

Is ok if I add more features to edit

Very good feature, keep it up

which to be implemented soon to Uptime Kuma

Thanks for sharing your enthusiasm and glad to see it please you.
However, I will not add more feature in this PR because the goal is to add only a very basic multiple admin user management. Check here and here if you want more details.

[M1CK431]

@CommanderStorm Thanks A LOT for the time you spent reviewing my code. I just add a commit with some of your suggestions. I also reply to others comments with explanations.

• checkLogin now should also check for the disabled state of a user

But!? You are right! How I could even forgot about that! 🤦🏼‍♂️
However this means checking the DB and so I have to make the function async... this means a lot of lines modified everywhere (adding await). I really hope it will not be a reason to refuse my PR... 😨

I am going to the cinema tomorrow evening, so I will get to test this deeper on Wednesday or Thursday.

WHAT! You are going to the cinema instead of gracefully reviewing my code!? This is a SCANDAL! 😏😂
Take your time!! You already give me so many useful advise and I like to read a so serious review, thanks again 🤗 😘

NB: Some of my comments might be a bit opinionated, but don't worry: at the end, I will obviously follow this project guidelines 😘

[CommanderStorm]

I don't found any existing test in the webapp part...

The tests can be found here: https://github.com/M1CK431/uptime-kuma/tree/add_basic_multiple_admin_users/test/cypress

I agree that this project is widely undertested, but this is an important change ⇒ tests for this functionality are an important addition

[M1CK431]

I don't found any existing test in the webapp part...

The tests can be found here: https://github.com/M1CK431/uptime-kuma/tree/add_basic_multiple_admin_users/test/cypress

I agree that this project is widely undertested, but this is an important change ⇒ tests for this functionality are an important addition

I agree. I quickly try to understand how to write such test... understand nothing... 🤯 😢
I write UT years ago using Jest on webapp side, looks very different.

Reading that:

I'm really sorry I will not do it, but I will allow edit from maintainers so if someone wants to add them 🤷🏼‍♂️

I think this PR is basically ready.

I rework the git commits history to help Louis reviewing then open this PR upstream. Now praying for a merge... 🕯️ 🙏🏼 🕯️

[ShadowGaming100]

How can I run it?
I did git clone and npm run setup then how do I run it?

[M1CK431]

How can I run it? I did git clone and npm run setup then how do I run it?

Follow regular README instruction 😉

[ShadowGaming100]

I did but I can’t see the new changes I will try again and tell if I could do it

[ShadowGaming100]

This is the commands i will run to setup and run it

Is it correct??

npm install npm -g

git clone https://github.com/M1CK431/uptime-kuma.git
git checkout add_basic_multiple_admin_users
cd uptime-kuma
npm ci --production
node server/server.js

[M1CK431]

This is the commands i will run to setup and run it

Is it correct??

npm install npm -g

git clone https://github.com/M1CK431/uptime-kuma.git git checkout add_basic_multiple_admin_users cd uptime-kuma npm ci --production node server/server.js

I guess cd uptime-kuma should be just after git clone? 🤷🏼‍♂️

[chakflying]

I'm pretty sure you need to follow the development instructions instead of the production ones.

[ShadowGaming100]

Can you tell me what is the command because I am getting confused