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

Add basic multiple admin users support #3571

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

M1CK431
Copy link
Contributor

@M1CK431 M1CK431 commented Aug 13, 2023

⚠️⚠️⚠️ 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

Add a very basic multiple admin users support.

ℹ️ This PR is already reviewed by @CommanderStorm and @chakflying (at least partially for the later). I really would like to thanks them a lot for help and advises while on boarding a new contributor. It was a pleasure to work with them 🤗

Fixes #128 (in a very basic/partial way)

Per commit review hugely recommended due to this PR size. I really take care about git commits history to easy the review for @louislam

Type of change

  • User interface (UI)
  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update => I don't know 🤷🏼‍♂️

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)

Users list:
image

Confirm modal on disabling user:
image

Add an admin account:
image

Edit an admin account:
image

@M1CK431 M1CK431 changed the title Add basic multiple admin users Add basic multiple admin users support Aug 13, 2023
@M1CK431 M1CK431 force-pushed the add_basic_multiple_admin_users branch from 7eb733a to 72ae82d Compare August 13, 2023 21:51
Computroniks

This comment was marked as resolved.

Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

In the Proto-PR we had a discussion about the value and need for tests in this PR.
I believe that security-related PRs should always come with tests to catch future regressions.

Normally, I would offer to provide a PR next week, but next week will be hell work-wise already
⇒ If somebody from #128 wants to provide an audit via implementing some of the above tests, this would be really appreciated

TLDR:

Given that this is a security-relevant PR, I think these cases should be covered (all as cypress-e2e testcases which are verifed by looking at the database + UI):

  • adding a user adds it => user can log in
  • disabling user marks the user as disabled => user cannot log in
  • enabling user marks the user as disabled => user can log in
  • removing a user removes the user => user cannot log in

@CommanderStorm

This comment was marked as resolved.

@CommanderStorm CommanderStorm mentioned this pull request Aug 14, 2023
7 tasks
@M1CK431 M1CK431 force-pushed the add_basic_multiple_admin_users branch from 72ae82d to b53aaa4 Compare August 14, 2023 21:23
@M1CK431 M1CK431 force-pushed the add_basic_multiple_admin_users branch from b53aaa4 to 5f06084 Compare August 14, 2023 21:24
@M1CK431
Copy link
Contributor Author

M1CK431 commented Aug 14, 2023

Just a couple of things, mainly about JSDoc comments. A PR was recently merged into the version 2 branch (#3529) which added JSDoc rules to eslint.

Well, I'm opening a PR on v1.x series... I have do my best but please keep in mind that it's a bit hard to handle such review without the related eslint rules configured... In addition, it's the first time I use JSDoc (very nice).

The key ones from that were @params and @returns require descriptions in all cases (except @returns {void} and @returns {Promise<void>}, all JSDoc comments for methods and functions require an @returns, object is preferred over Object and optional arguments should not document their default values if the default values are given in es6 syntax. I haven't added a review comment for each as to avoid cluttered everything. https://github.com/gajus/eslint-plugin-jsdoc#readme

I just push an update on this PR where I try to exhaustively take care about that. As already explained, since there is no such eslint rule in v1.x series, please tell me if there is still something to change 🙏🏼

@Rovel
Copy link

Rovel commented Aug 25, 2023

I can help with some cypress tests, if still needed.

I saw the project have some Github codespaces config files, and saw some setups to test cypress with them I can open a PR on that too
cypress-io/cypress-documentation#2956
https://devopstar.com/2022/01/03/cypress-testing-in-devcontainers-and-github-codespaces/

I also recommend to setup cypress cloud with an OSS plan to help testing and debugging cypress tests: (easy setup) https://docs.cypress.io/guides/cloud/account-management/billing-and-usage#Open-Source-Plan

@M1CK431

This comment was marked as spam.

@Rovel
Copy link

Rovel commented Nov 4, 2023

@CommanderStorm @Rovel Is there still one of you hotter than 🔥 to write some test as requested to merge this PR? 🥺

M1CK431#2

I tried to find a branch to PR this on the main remote but couldn't find it

@M1CK431
Copy link
Contributor Author

M1CK431 commented Nov 5, 2023

@Rovel Just merged it so your commits are here now. THANKS A LOT!! 😘 ❤️
@CommanderStorm So now we have unit test. What is the next step?
I notice some conflits, should I resolve them now? Is that PR merged once conflits will be resolved?

@Rovel
Copy link

Rovel commented Nov 5, 2023

@M1CK431 I added a secondary PR with cy tests on the edit user form in M1CK431#3
There is one point in this test, the secondary user created can incativate de original/default user and kind of lock him out, not sure if is in the scope of this PR but avoiding the root user to be inactivated by others would be advisable, not that db access cannot solve but I needed to mention.

@BenjaminEHowe
Copy link

avoiding the root user to be inactivated by others would be advisable

IMO that's out of scope for this PR -- all users are admins, and admins should be able to create and delete accounts. That said, I suspect in future the idea of user roles might be added, and non-admins probably shouldn't be able to add or remove users.

@M1CK431
Copy link
Contributor Author

M1CK431 commented Nov 5, 2023

@Rovel Thanks a lot for your work, I just merged it.

To answer your question, once this PR will be merged, the concept of "root" user doesn't exist anymore. There is just (admin) users, period. As @BenjaminEHowe mention, we could imagine adding a rights management system (ACL, RBAC, etc...) in the futur, but this is totally out of scope of this PR.

As you know, it's already a big change and so I would like to keep things as simple as possible. Anyway, thanks for your question, it's a pleasure to work with invested people 😍

@CommanderStorm CommanderStorm added the needs:review this PR needs a review by maintainers or other community members label May 19, 2024
@github-actions github-actions bot added needs:resolve-merge-conflict A merge-conflict needs to be addressed before reviewing makes sense again and removed needs:resolve-merge-conflict A merge-conflict needs to be addressed before reviewing makes sense again labels May 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:user-management needs:resolve-merge-conflict A merge-conflict needs to be addressed before reviewing makes sense again needs:review this PR needs a review by maintainers or other community members
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow basic User management without permissions