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

Feature: Multiple Users #2506

Closed
wants to merge 24 commits into from
Closed

Feature: Multiple Users #2506

wants to merge 24 commits into from

Conversation

ItIzJR
Copy link

@ItIzJR ItIzJR commented Dec 29, 2022

⚠️⚠️⚠️ Since we do not accept all types of pull requests and do not want to waste your time. Please be sure that you have read pull request rules:
https://github.com/louislam/uptime-kuma/blob/master/CONTRIBUTING.md#can-i-create-a-pull-request-for-uptime-kuma

Tick the checkbox if you understand [x]:

  • I have read and understand the pull request rules.

Description

This feature, would add the ability to have the main user, (admin user) to add other users, and the main user has the ability to give these users permissions, so they can only view the monitors, status pages etc. Or they can have management permissions, such as add, remove, edit

Fixes #128

Type of change

Please delete any options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update (it could, but it really depends, will leave this here though)

( Just up for discussion and the okay from the project owner (@louislam) before I make any code, as the PR Rules go so none of these apply atm

Checklist

  • My code follows the style guidelines of this project
  • I ran ESLint and other linters for modified files
  • I have performed a self-review of my own code and tested it
  • I have commented my code, particularly in hard-to-understand areas
    (including JSDoc for methods)
  • My changes generate no new warnings
  • My code needed automated testing. I have added them (this is optional task)

Screenshots (if any)

Please do not use any external image service. Instead, just paste in or drag and drop the image here, and it will be uploaded automatically.

@ItIzJR ItIzJR changed the title PR For Multiple Users Feature: Multiple Users Dec 29, 2022
@ItIzJR ItIzJR marked this pull request as draft December 29, 2022 17:32
@louislam
Copy link
Owner

This feature I agree that will put Uptime Kuma into another level.

However, my answer is simply no. I don't recommend anyone working on it in this stage because it is a super super big task.

  • Server + Frontend more than 150+ files have to modify in order to handle different permissions I believe.
  • Add new components for managing users, permissions and user roles.
  • A lot of discussions on how to define permissions. (e.g. If User A created a monitor, should it be visible to Admin? If Admin created a monitor, should it be visible to User A?)
  • Security, there are a lot of API key such as notifications inside Uptime Kuma. Read-only user should not see it?
  • Testing and reviewing all changes are always hardest part.

Didn't mention there are still so many pull requests I haven't reviewed yet. I think I will put my time on them first.

@ItIzJR
Copy link
Author

ItIzJR commented Dec 30, 2022

Well, I will say I begun this as a test to see it was even possible, I created ways to add sub users, remove them and let them login, now I haven’t looked at permissions but the way I was going to add them, a proper way, would use way less than 150 files, but If you still want a pause on this I can close it, and when you mention the notifications my idea of read only user is that they will get notifications, and can see everything but not able to remove users, add monitors etc.

@Computroniks
Copy link
Contributor

@ItIzJR Would it be possible for you to share what you have done so far? It might help in making a decision if we can see how you are planning to implement this.

@ItIzJR
Copy link
Author

ItIzJR commented Dec 30, 2022

Would you like images or code examples?

@Computroniks
Copy link
Contributor

You could probably just commit it (you can always revert the commit later), or you could copy in some examples.

@ItIzJR
Copy link
Author

ItIzJR commented Dec 30, 2022

And you just want what I have done? Which is adding / deleting sub users and logging them in?

@Computroniks
Copy link
Contributor

Yeah, just something so that we can see in code how you are thinking about implementing multiple users.

@ItIzJR
Copy link
Author

ItIzJR commented Dec 31, 2022

It didn't import correctly for some reason, so until I get back on tomorrow it'll have to be this way, if you check the repo for this under the "main" branch, the files will be in the areas with the commit message "sub user files"

@ItIzJR
Copy link
Author

ItIzJR commented Dec 31, 2022

and... I just realized I forgot to include the most important part when I committed them, which was the login stuff, hopefully this will get you the main idea of how I will be planning on implementing these files though, also if you want I can also come up with the best way of implementing permissions, with as few changes as possible, while being secure as possible, I have actually done this before so I don't think it will be too hard.

@louislam
Copy link
Owner

I just realized I forgot to include the most important part when I committed them

I see nothing changed in File changed tab.

@ItIzJR
Copy link
Author

ItIzJR commented Dec 31, 2022

That would be due to the weird way GitHub imported it, whenever I get back from work if I have time I’ll fix it, but like I said the changes are in the files with “sub user files” in the main branch if you want to manually look at them, Ik it takes a lot more time, or you can wait till I get it to be fixed.

@ItIzJR
Copy link
Author

ItIzJR commented Dec 31, 2022

Alright I have added all the files into main and they show up under the changes, also it seems after merging whatever you pulled today it throws and error, in your commit checking.

This is what the files produce:

4ac66338175c06856f340976400a13b7.mp4
375f8a8819f0ff5fe080094cc2b82375.mp4

@Computroniks
Copy link
Contributor

I see the problem here. The reason we cannot see the commits is because they have been made on the main branch of your repo but this PR is for the master branch of your repo. If you just change the branch this PR comes from we will be able to see the changes

@ItIzJR
Copy link
Author

ItIzJR commented Dec 31, 2022

Oh I didn’t notice that I’ll update it real quick, then I have to go but that gives you guys time to take a look

@ItIzJR
Copy link
Author

ItIzJR commented Dec 31, 2022

I have to fix some stuff, I’ll have it updated as soon as possible

@acr-varonis
Copy link

just my 2cents.
If you build something like this you need to plan for support of different authentication methods in mind for example LDAP/AD etc

@Computroniks
Copy link
Contributor

just my 2cents. If you build something like this you need to plan for support of different authentication methods in mind for example LDAP/AD etc

Yes, definitely, the support (or at least designing/planning to support) of external authentication systems is a must, especially if we want to future proof.

@ItIzJR
Copy link
Author

ItIzJR commented Jan 2, 2023

Already had that in mind, and ill get the code up today for you to look at

@juzhiyuan
Copy link
Contributor

juzhiyuan commented Jan 5, 2023

Hi, because Uptime can only create one user, there is one case: if we can't contact the admin (especially 2FA is enabled), then it seems there is no way to log in to Uptime. Maybe provide a way to account recovery?

Just noticed this issue: #2238 :) It's helpful!

@ItIzJR
Copy link
Author

ItIzJR commented Jan 5, 2023

Sorry for my delay, I’ll get the code situated asap, been a busy week at work. I would definitely be able to implement that, thanks for the suggestion, and that is if they end up approving this.

-- You should not modify if this have pushed to Github, unless it does serious wrong with the db.
BEGIN TRANSACTION;

CREATE TABLE [sub_users](
Copy link
Owner

Choose a reason for hiding this comment

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

No idea why commits present here, but the File changed still shows nothing.

However, creating a new table sub_users which is not what I want to do unfortunately. As in this way, I believe all user related features have to re-implemented in order to handle both user and sub_users, which is not ideal. The best way should be keep using user table only.

As I said before, there will be so many discussions on how to design in code or in UI/UX, so I do not want anyone working on it.

If you want to contribute to this project, I would recommend that you should start from other smaller feature first.

Copy link
Contributor

@Computroniks Computroniks Jan 8, 2023

Choose a reason for hiding this comment

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

Yes, the users table is already set up for multiple users as each monitor has a foreign key referencing the user that created it. The only changes I think that need to be made to the actual database would be some sort of field for access level in the users table and possibly a visibility field in monitors/status pages. A group system might be something to look into to make management easier. @louislam, is there a discussion/issue that could be used for people to discuss the implementation of multi users to help keep everything in one place?

Copy link
Author

Choose a reason for hiding this comment

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

I mainly made that for testing because I wasn’t sure if the user part was for multiple or not, definitely should of asked about that, but I could easily just turn that into a permission implementation.

@kirtan403
Copy link

Is this going to be worked upon?

@apisword
Copy link

@louislam please add this❤️

@Computroniks
Copy link
Contributor

@louislam In terms of a permissions system, could we do something a bit like OAuth scopes? E.g: monitor:read, monitor:write and so on. For another example of these, you can have a look at the GitHub personal access tokens. This would allow us to just have all the scopes for a user in the database (schema to be determined) and then each page simply checks to see if the user has the required scope for that specific page. This would also probably make it fairly simple to implement the permissions levels for OAuth in future if that is desired. This would put the requirement to validate permissions on the individual pages instead some centralised place, making it more scalable (in my opinion). This would also allow admins fine grained control over user permissions. I would be interested to hear what you, and others think.

P.s. Should we use some sort of discussion system where people can discuss the big changes that don't quite fit into issues (like user permissions). Perhaps enabling GitHub discussions could be an option?

@401U
Copy link
Contributor

401U commented Mar 14, 2023

Why not just start without permission system and make all users admin at first?

  • Uptime kuma is light-weight enough to use multiple instance for different stuffs, and the minimal support for multi-user is enough for most(maybe up to 80%) cases.
  • This would be easier to impl, with less effort and less effect of current codebase(maybe less than 20% compared to with permssion system).
  • This would have less UI/UX or design work, make it much acceptable by louislam

@ItIzJR
Copy link
Author

ItIzJR commented Mar 14, 2023

Gonna be honest forgot this was open, Louis didn’t seem to be a fan of having someone make this so I privately made it myself so that I could use the feature I wanted, if anyone else were to want this I could help them with implementing it, but it has multiple user and permission system already.

Why not just start without permission system and make all users admin at first?

  • Uptime kuma is light-weight enough to use multiple instance for different stuffs, and the minimal support for multi-user is enough for most(maybe up to 80%) cases.
  • This would be easier to impl, with less effort and less effect of current codebase(maybe less than 20% compared to with permssion system).
  • This would have less UI/UX or design work, make it much acceptable by louislam

@401U
Copy link
Contributor

401U commented Mar 15, 2023

Just make my thought clear: we need to push things forward, and build from small part maybe useful enough, also acceptable by louislam with less UI/UX and design stuff.
What I really want is introduce this awsome feature into Uptime Kuma, in a way louislam would like it.
Sorry to bother you, but let me ping @louislam cause I want to know your opinion.

@ItIzJR
Copy link
Author

ItIzJR commented Mar 15, 2023

Just make my thought clear: we need to push things forward, and build from small part maybe useful enough, also acceptable by louislam with less UI/UX and design stuff.
What I really want is introduce this awsome feature into Uptime Kuma, in a way louislam would like it.
Sorry to bother you, but let me ping @louislam cause I want to know your opinion.

Yeah I gotcha, I’ve got a working version and the only UI added is a sub user management system and permission system, all we would need is Louis’s approval and then I could move the private repo into the one for this pr.

@HellStorm666
Copy link

please add this feature.

@kokofixcomputers
Copy link
Contributor

How do i install ItIzJR's multi user uptime-kuma it just says:

error: pathspec '1.19.2' did not match any file(s) known to git

@CommanderStorm
Copy link
Collaborator

@ItIzJR Could you close this PR?
#3571 implements the same feature, but is more mature
⇒ has a chance of getting merged ^^

@CommanderStorm
Copy link
Collaborator

Closing in favour of #3571, as the other PR is more mature and thus has a chance of getting merged

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.

Allow basic User management without permissions