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

New User Management #9348

Merged
merged 5 commits into from Apr 23, 2019

Conversation

Projects
None yet
5 participants
@murrant
Copy link
Member

commented Oct 19, 2018

image

Waiting on #8868 and #9422 some things really need tests here.

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926
After you are done testing, you can remove the changes with ./scripts/github-remove. If there are schema changes, you can ask on discord how to revert.

Does not collide with #9313, utilizes edituser still to manage device permissions.

@murrant murrant added the WebUI label Oct 19, 2018

@TheGreatDoc

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2018

Are you commiting app/Jobs/PingCheck.php on purpose here?

@murrant

This comment has been minimized.

Copy link
Member Author

commented Oct 19, 2018

@TheGreatDoc nope, thanks :)

@murrant murrant changed the title WIP: New User Management New User Management Oct 19, 2018

@laf

This comment has been minimized.

Copy link
Member

commented Oct 20, 2018

Doesn't save when unselecting Can Modify Password.

I'm not a fan of the new user / edit user form - it looked better as it was when it was a bit more condensed and concise, just seems to be trying to take up the entire page for the sake of it now.

The old edit user / add user pages still exist.

Does anything need to be done with ./adduser.php or install.php add user?

Could you make it so that deleting a user doesn't refresh the page and just removes the row. We do this in the alert-rules if you want a code example to look at.

Aside from that, all other options adding / deleting saving / updating works for me.

@laf laf added this to the 1.45 milestone Oct 20, 2018

@murrant

This comment has been minimized.

Copy link
Member Author

commented Oct 20, 2018

I didn't look at the old one, just put the form together, I'll take a look. Could definitely remove without refresh. Perhaps you like modal style better?

@laf

This comment has been minimized.

Copy link
Member

commented Oct 21, 2018

Perhaps you like modal style better?

No opinion either way. Edit could work with it but add user won't as you could be on any page.

@murrant murrant changed the title New User Management WIP New User Management Oct 23, 2018

@laf

This comment has been minimized.

Copy link
Member

commented Oct 23, 2018

Looks better now :)

So just the not being able to unselect Allow user to change password option. You can enable it and it saves but not disable it.

I realise the old edituser page needs to stay as the perms are done through it but I think it would be good to redirect the standard access or /edituser/ to this new page + change the link for Change... in the permissions page to point to the new users mgmt page.

Same question about it affecting ./adduser.php or install.php stands just in case.

@murrant

This comment has been minimized.

Copy link
Member Author

commented Oct 23, 2018

@laf I also need to add 2fa unlock and remove.

@murrant

This comment has been minimized.

Copy link
Member Author

commented Oct 23, 2018

Does not affect ./adduser.php and install.php, doesn't touch that code.

@laf laf modified the milestones: 1.45, 1.46 Oct 26, 2018

@laf laf modified the milestones: 1.46, 1.47 Nov 28, 2018

@murrant murrant modified the milestones: 1.47, 1.48 Dec 21, 2018

@voipmeister

This comment has been minimized.

Copy link
Contributor

commented Jan 20, 2019

Hi @murrant , do you still need testers for these changes?

@murrant

This comment has been minimized.

Copy link
Member Author

commented Jan 20, 2019

@voipmeister yes, but this is out of date at the moment. I'll update it sometime. I was waiting so I could added automated testing of the two factor auth stuff since it has several tricky scenarios. That is almost ready.

@voipmeister

This comment has been minimized.

Copy link
Contributor

commented Jan 20, 2019

Understood, if you mention me here I'd be happy to do some testing 🙂

@murrant murrant modified the milestones: 1.48, 1.49 Jan 24, 2019

@murrant murrant modified the milestones: 1.49, 1.50 Mar 1, 2019

@murrant murrant force-pushed the murrant:user-management branch 2 times, most recently from 015ee15 to 97b4c69 Mar 18, 2019

@murrant murrant modified the milestones: 1.50, 1.51 Mar 27, 2019

Show resolved Hide resolved html/pages/edituser.inc.php Outdated
@murrant

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2019

Fixed all those (and some other items, check the log if curious)

Still not sure what to do with tests, I'll probably just remove them or do something simple.

@murrant murrant changed the title WIP New User Management New User Management Apr 11, 2019

@murrant murrant force-pushed the murrant:user-management branch 2 times, most recently from cdf35cf to a7a5a4a Apr 12, 2019

murrant added some commits Oct 17, 2018

Rewrite user management.
Error management

Revert edituser legacy page

Connect user permissions button to legacy page for now.

Implement user creation
Refine form

Remove PingCheck.php accidental add :)

Fixes for redirection and deletion

More fixes: realname accidental validation setting, hide can modify for read-only auths

Use a panel to improve style

Add icon to panel-title

Not allowed to delete own user (at least via the click of a button)

Use request validation to reduce complexity of controller.
Improve protection against users doing things they should not.

Switch to horizontal form and not nearly as wide of layout :)

delete without refresh.
Fix for buttons

Include all users (not just from this auth)
Hide the auth column if there is only one auth type

Show username if real name isn't set

Don't allow creation of demo users via the webui

a fix to the lnms user:add command, it didn't set auth_id

update edituser.inc.php to current
just redirect to users page

@murrant murrant force-pushed the murrant:user-management branch from a7a5a4a to 0fe2a3d Apr 12, 2019

@murrant

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2019

We have basic login tests with and without 2fa, so that is better than nothing :)

@murrant murrant requested a review from laf Apr 12, 2019

@murrant

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2019

@voipmeister ready for testing

@laf laf dismissed their stale review via 458b47e Apr 14, 2019

@voipmeister

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

@voipmeister ready for testing

I missed this, will try to test this weekend!

@murrant murrant merged commit 6e6e54c into librenms:master Apr 23, 2019

5 of 6 checks passed

codeclimate 11 issues to fix
Details
Inspection Summary
Details
Node: analysis
Details
Travis CI - Pull Request Build Passed
Details
WIP Ready for review
Details
license/cla Contributor License Agreement is signed.
Details

@murrant murrant deleted the murrant:user-management branch Apr 23, 2019

@chasgames

This comment has been minimized.

Copy link

commented Apr 30, 2019

Nice I like this view 👍

Just noting two minor issues on my instance, unfortunately i have no dev environment setup yet to test any fixes

1 - LibreNMS logo changes, i have dark mode set, so every other page pulls (https://LIBRENMS/images/librenms_logo_dark.svg), but on manage users it pulls a different logo (https://LIBRENMS/images/librenms_logo_light.svg)

2 - The custom menu doesn't work for anything under https://LIBRENMS/users , i'm not sure but i wonder if LegacyAuth::get()->canManageUsers() blocks it
https://community.librenms.org/t/how-to-add-a-custom-menu-item/3761/4?u=chas

funzoneq added a commit to funzoneq/librenms that referenced this pull request Apr 30, 2019

New User Management (librenms#9348)
* Rewrite user management.

Error management

Revert edituser legacy page

Connect user permissions button to legacy page for now.

Implement user creation
Refine form

Remove PingCheck.php accidental add :)

Fixes for redirection and deletion

More fixes: realname accidental validation setting, hide can modify for read-only auths

Use a panel to improve style

Add icon to panel-title

Not allowed to delete own user (at least via the click of a button)

Use request validation to reduce complexity of controller.
Improve protection against users doing things they should not.

Switch to horizontal form and not nearly as wide of layout :)

delete without refresh.
Fix for buttons

Include all users (not just from this auth)
Hide the auth column if there is only one auth type

Show username if real name isn't set

Don't allow creation of demo users via the webui

a fix to the lnms user:add command, it didn't set auth_id

update edituser.inc.php to current
just redirect to users page

* Remove TwoFactorTest for now

* Update edituser.inc.php

* Update .env.dusk.testing

* Enable 2fa for 2fa test...
@murrant

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2019

  1. Must be some disparity the way the code gets the logo...
  2. Custom menu support isn't in the new menu system. Could probably be added back...
@chasgames

This comment has been minimized.

Copy link

commented Apr 30, 2019

Ahh thanks, 2. is rather indispensable for me, hopefully for others too as currently the only way to share custom aggregations e.g https://librenms/iftype/type=customportdescription/ and other links which don't fall under any other menu.

Thanks for fixing my other code too! I wasn't sure if LegacyAuth was still a thing :)

@murrant

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2019

@chasgames you mean this menu?? (backhaul is custom)
image

@chasgames

This comment has been minimized.

Copy link

commented Apr 30, 2019

Ahh i remember now exploring custom_descr a year ago, i forgot it automatically adds to the menu.

I think the reason was we have around 20 different aggregations, so putting a little symbol next to it (Font Awesome icons) and naming the aggregation(s) (like Total Internet Traffic) which is a combination of different aggregators made it a bit more user friendly for the rest of the users so i put it in the custom menu.

I do use it for portal links to other applications like an ip manager, and to my weather animation (https://community.librenms.org/t/weathermap-histogram/1052/4?u=chas) but i suppose this could live somewhere else.

But good point i could switch back to the custom_descr in config.php if the new menu system is not supporting it

@murrant

This comment has been minimized.

Copy link
Member Author

commented May 1, 2019

@chasgames murrant@f47e5ea

Also, logo theme was already fixed in that branch before you reported :)

@chasgames

This comment has been minimized.

Copy link

commented May 2, 2019

Sorry to keep updating this one , just noticed another difference;

Majority of my username scheme have the dot between the firstname and lastname. Creating a new user today, but dots no longer are allowed. I'm struggling to see where this validator is in the code though.

image

@murrant

This comment has been minimized.

Copy link
Member Author

commented May 2, 2019

app/Http/Requests/StoreUserRequest.php

There are better spots than this. Often I miss comments on closed PRs ;)

spencerbutler added a commit to spencerbutler/librenms that referenced this pull request May 21, 2019

New User Management (librenms#9348)
* Rewrite user management.

Error management

Revert edituser legacy page

Connect user permissions button to legacy page for now.

Implement user creation
Refine form

Remove PingCheck.php accidental add :)

Fixes for redirection and deletion

More fixes: realname accidental validation setting, hide can modify for read-only auths

Use a panel to improve style

Add icon to panel-title

Not allowed to delete own user (at least via the click of a button)

Use request validation to reduce complexity of controller.
Improve protection against users doing things they should not.

Switch to horizontal form and not nearly as wide of layout :)

delete without refresh.
Fix for buttons

Include all users (not just from this auth)
Hide the auth column if there is only one auth type

Show username if real name isn't set

Don't allow creation of demo users via the webui

a fix to the lnms user:add command, it didn't set auth_id

update edituser.inc.php to current
just redirect to users page

* Remove TwoFactorTest for now

* Update edituser.inc.php

* Update .env.dusk.testing

* Enable 2fa for 2fa test...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.