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

WebUI: edituser page with new Auth system #9313

Merged
merged 5 commits into from Oct 29, 2018

Conversation

PipoCanaja
Copy link
Contributor

@PipoCanaja PipoCanaja commented Oct 9, 2018

Hi team,

This PR was started because I lacked the "Edit Permission" capability for Active Directory users. I tried to find some kind of "minimal" changeset to achieve this.

And I like the little encouragement cause Edit Permissions works so far :) (tested it 5 minutes, time needed to create and edit permission on one new user)

I'll continue digging in this direction, but I would like to get your opinion and advice (and probably help as well) on this. I suppose you would prefer the complete refactor of the page (Auth object, Device object, getting rid of all select/update DB, etc etc), but I don't feel comfortable at all starting this, so I started "step by step" for the moment.

Let me know,

Thx,

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.

@murrant
Copy link
Member

murrant commented Oct 9, 2018

Definitely something that is needed.

If you are up to it, you might as well refactor it to a Laravel page. Put an route in web.php, create a controller and make a blade template.

@PipoCanaja
Copy link
Contributor Author

That's more or less the answer I expected ;) But I have absolutely no clue on how to do that :) I'll check if I get an idea on how to do it and depending on that, we'll decide if I go for it or not.

@PipoCanaja
Copy link
Contributor Author

Hello

I'll remove the WIP for the moment and let this PR be merged. I'll try to get the Laravel logik and do some experiments "off" but I need this PR into master so I can use it to create my users ;)

Thanx

@PipoCanaja PipoCanaja changed the title WIP - WebUI: edituser page with new Auth system WebUI: edituser page with new Auth system Oct 16, 2018
laf
laf previously approved these changes Oct 16, 2018
Copy link
Member

@laf laf left a comment

Choose a reason for hiding this comment

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

I can only test on MySQL auth. Works there still.

@laf laf added this to the 1.45 milestone Oct 16, 2018
@laf laf added the WebUI label Oct 16, 2018
@murrant murrant mentioned this pull request Oct 19, 2018
1 task
include 'includes/error-no-perm.inc.php';
} else {
if ($vars['user_id'] && !$vars['edit']) {
$user_data = LegacyAuth::get()->getUser($vars['user_id']);
/** @var User $user */
$user = User::all()->where('user_id', $vars['user_id'])->first();
Copy link
Member

@murrant murrant Oct 23, 2018

Choose a reason for hiding this comment

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

FYI, this is the long way of writing User::find($vars['user_id']); and actually loads all users from the db, then filters them in php.

@@ -432,7 +436,7 @@
}//end if !empty($users_details)
}//end if
} else {
$user_list = LegacyAuth::get()->getUserlist();
$userlist = User::all();
Copy link
Member

Choose a reason for hiding this comment

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

You need to use User::thisAuth()->all(); to exclude users from inactive auths.

@murrant
Copy link
Member

murrant commented Oct 25, 2018

@PipoCanaja can you fix the queries I've pointed out and we'll merge this now and rewrite later :)

@@ -264,7 +268,7 @@
}
}

$users_details = LegacyAuth::get()->getUser($vars['user_id']);
$users_details = User::all()->where('user_id', $vars['user_id'])->first()->toArray();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$users_details = User::all()->where('user_id', $vars['user_id'])->first()->toArray();
$users_details = User::find($vars['user_id'])->toArray();

@laf laf modified the milestones: 1.45, 1.46 Oct 26, 2018
@laf laf merged commit 3a5d64e into librenms:master Oct 29, 2018
@PipoCanaja PipoCanaja deleted the editUserNewSyntax branch December 5, 2018 21:48
@lock lock bot locked as resolved and limited conversation to collaborators Feb 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants