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

Refactored authorizers to classes #7497

Merged
merged 10 commits into from Nov 18, 2017

Conversation

Projects
None yet
6 participants
@mcq8
Contributor

mcq8 commented Oct 16, 2017

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

The idea of this PR is to refactor all the authentication to classes and have less global methods.
It should now be more clear what to implement if you want to add an authoriser.

@CLAassistant

This comment has been minimized.

Show comment
Hide comment
@CLAassistant

CLAassistant Oct 16, 2017

CLA assistant check
All committers have signed the CLA.

CLAassistant commented Oct 16, 2017

CLA assistant check
All committers have signed the CLA.

@mcq8

This comment has been minimized.

Show comment
Hide comment
@mcq8

mcq8 Oct 16, 2017

Contributor

I forgot about php 5.3, I'm just to used to shortened array syntax

Contributor

mcq8 commented Oct 16, 2017

I forgot about php 5.3, I'm just to used to shortened array syntax

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Oct 17, 2017

Member

Thanks for working on this. Didn't have time to give this a full look yet, but here are some initial thoughts.

You can avoid the global variable $authorizer by making a proxy with static methods for the singleton authorizer. You could also put your factory in that class and use the autoloader instead of sourcing a php file.

Please use Config::get() instead of direct access to $config, we are trying to remove global variables :)

Can you move the interface to LibreNMS\Interfaces\?

Member

murrant commented Oct 17, 2017

Thanks for working on this. Didn't have time to give this a full look yet, but here are some initial thoughts.

You can avoid the global variable $authorizer by making a proxy with static methods for the singleton authorizer. You could also put your factory in that class and use the autoloader instead of sourcing a php file.

Please use Config::get() instead of direct access to $config, we are trying to remove global variables :)

Can you move the interface to LibreNMS\Interfaces\?

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Oct 20, 2017

Member

Looks pretty good, just needs testing. Too bad we can't use dependency injection eh?

What auth methods did you test?

Member

murrant commented Oct 20, 2017

Looks pretty good, just needs testing. Too bad we can't use dependency injection eh?

What auth methods did you test?

@mcq8

This comment has been minimized.

Show comment
Hide comment
@mcq8

mcq8 Oct 20, 2017

Contributor

Still doing the Config.
Only the mysql so far I don't have an AD.

Contributor

mcq8 commented Oct 20, 2017

Still doing the Config.
Only the mysql so far I don't have an AD.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Oct 21, 2017

Member

Good, I can test AD

Member

murrant commented Oct 21, 2017

Good, I can test AD

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Oct 30, 2017

Member

@mcq8 Sorry needs a rebase now due to another merged PR.

Member

laf commented Oct 30, 2017

@mcq8 Sorry needs a rebase now due to another merged PR.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Oct 30, 2017

Member

@mcq8 we merged an update to ldap authentication. Are you still planning on updating this?

Member

murrant commented Oct 30, 2017

@mcq8 we merged an update to ldap authentication. Are you still planning on updating this?

@mcq8

This comment has been minimized.

Show comment
Hide comment
@mcq8

mcq8 Nov 1, 2017

Contributor

yes I'll rebase

Contributor

mcq8 commented Nov 1, 2017

yes I'll rebase

@laf

Works for me. Tested Alerts, API and Webui for http auth.

No access to other auth modules.

@laf

laf approved these changes Nov 15, 2017

@laf laf referenced this pull request Nov 15, 2017

Merged

Single Sign-On Authentication Mechanism #7601

1 of 1 task complete

murrant added some commits Nov 16, 2017

@murrant

Tested AD webui and API.
Two small fixes.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Nov 17, 2017

Member

AD alerting tested.

Member

murrant commented Nov 17, 2017

AD alerting tested.

murrant added some commits Nov 17, 2017

Re-work auth_test.php AD bind tests to work properly with the new class.
Reflection is not the nicest tool, but I think it is appropriate here.
Handle exceptions more nicely in auth_test.php
Restore AD getUseList fix
Not sure how it got removed
@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier Nov 17, 2017

The inspection completed: 65 new issues, 141 updated code elements

scrutinizer-notifier commented Nov 17, 2017

The inspection completed: 65 new issues, 141 updated code elements

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Nov 17, 2017

Member

Looks good to me. Would be good to have ldap and radius tests. But they looked ok to me.

Member

murrant commented Nov 17, 2017

Looks good to me. Would be good to have ldap and radius tests. But they looked ok to me.

@laf laf merged commit c9728a1 into librenms:master Nov 18, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@papatango71

This comment has been minimized.

Show comment
Hide comment
@papatango71

papatango71 Nov 19, 2017

Contributor

The update to LibreNMS/Authentication/LdapAuthorizationAuthorizer.php breaks my /overview pages. I am able to log in and and see everything else except my main overview dashboard. Below is the error that I got when running /overview/debug=yes

Array ( [0] => 1 [1] => Uncaught Error: Call to undefined method LibreNMS\Authentication\LdapAuthorizer::ldap_compare() in /opt/librenms/LibreNMS/Authentication/LdapAuthorizer.php:152 Stack trace: #0 /opt/librenms/LibreNMS/Authentication/LdapAuthorizer.php(178): LibreNMS\Authentication\LdapAuthorizer->getUserlist() #1 /opt/librenms/html/includes/functions.inc.php(1606): LibreNMS\Authentication\LdapAuthorizer->getUser('4') #2 /opt/librenms/html/pages/front/tiles.php(23): get_dashboards() #3 /opt/librenms/html/index.php(218): require('/opt/librenms/h...') #4 {main} thrown [2] => /opt/librenms/LibreNMS/Authentication/LdapAuthorizer.php [3] => 152 )

Contributor

papatango71 commented Nov 19, 2017

The update to LibreNMS/Authentication/LdapAuthorizationAuthorizer.php breaks my /overview pages. I am able to log in and and see everything else except my main overview dashboard. Below is the error that I got when running /overview/debug=yes

Array ( [0] => 1 [1] => Uncaught Error: Call to undefined method LibreNMS\Authentication\LdapAuthorizer::ldap_compare() in /opt/librenms/LibreNMS/Authentication/LdapAuthorizer.php:152 Stack trace: #0 /opt/librenms/LibreNMS/Authentication/LdapAuthorizer.php(178): LibreNMS\Authentication\LdapAuthorizer->getUserlist() #1 /opt/librenms/html/includes/functions.inc.php(1606): LibreNMS\Authentication\LdapAuthorizer->getUser('4') #2 /opt/librenms/html/pages/front/tiles.php(23): get_dashboards() #3 /opt/librenms/html/index.php(218): require('/opt/librenms/h...') #4 {main} thrown [2] => /opt/librenms/LibreNMS/Authentication/LdapAuthorizer.php [3] => 152 )

@lock

This comment has been minimized.

Show comment
Hide comment
@lock

lock bot May 16, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

lock bot commented May 16, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

@lock lock bot locked as resolved and limited conversation to collaborators May 16, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.