API access when using LDAP authentication #3178

Merged
merged 3 commits into from Mar 13, 2016

Projects

None yet

4 participants

@Ultra2D
Contributor
Ultra2D commented Mar 7, 2016

Partly fixes #3069, but might break API access for authentication mechanisms that do store username and user_id in the database, but don't have the get_user function. Do those exist?

The get_user function I added is not pretty, but should do for now.

@Ultra2D Ultra2D API access when using LDAP authentication
322053c
@laf
Member
laf commented Mar 8, 2016

We do have authentication modules without get_user() function.

Probably better to do a if (!function_exists('get_user')) { and if it doesn't exist then fall back to the old check?

@laf laf added the API label Mar 8, 2016
@Ultra2D
Contributor
Ultra2D commented Mar 9, 2016

@laf are you sure about that? Active directory, HTTP, MySQL and RADIUS all have a similar (but not identical) line. I am assuming two factor and LDAP authorization still require one of the other types, but that might be wrong.

@Ultra2D
Contributor
Ultra2D commented Mar 9, 2016

In case I wasn't clear, adding a fall back to the old behaviour is no problem of course. Imho it should only be done if necessary though. As far as I can tell it is not, but please correct me otherwise.

More broadly speaking, isn't this the wrong place to do it (as I final solution I mean)? Shouldn't this function exist for every authentication type to allow for some abstraction? Otherwise you could end up with the same fallback code in multiple places.

@laf
Member
laf commented Mar 9, 2016

sorry I'm not sure what you mean anymore? What I was inferring was to check get_user function existed and if it does use it as you planned in this PR. If it doesn't, just fall back to the old method as people using auth methods that weren't originally supported by this can purely add entries in the DB to 'fake' users and auth tokens.

@Ultra2D Ultra2D Only use get_user if it exists
d63744c
@Ultra2D
Contributor
Ultra2D commented Mar 10, 2016

It's just that I could not find any other auth methods that don't have the get_user function. Anyway, the current commit will work.

As to my second point, this seems like creating a work-around (for auth methods missing a function). That same work-around could then be needed on multiple locations in the code (because you cannot depend on get_user), which could be prevented by adding get_user to auth methods missing it.

@laf
Member
laf commented Mar 10, 2016

ldap-authorization.inc.php doesn't have it so anyone using that the intial pr would have caused a 500 error in the API.

get_user is used only in one other place, edituser.inc.php which checks first with can_update_user() which ldap-authorization supports and returns false for meaning get_user never gets called.

@Ultra2D
Contributor
Ultra2D commented Mar 10, 2016

Ok, my assumption ("I am assuming two factor and LDAP authorization still require one of the other types, but that might be wrong.") was wrong.

ldap-authorization looks similar to ldap. Should I update it too (though I can not test it!) ? Otherwise, this version can be merged.

@Ultra2D
Contributor
Ultra2D commented Mar 10, 2016

Hang on, don't merge yet, the function does exist for ldap-authorization.

function get_user ($user_id) {
// Not supported
return 0;
}

I will also modify ldap-authorization. Can the function_exists check be removed in that case, or should it be left just to be sure?

@Ultra2D Ultra2D API access when using LDAP authorization
12a3b30
@Ultra2D
Contributor
Ultra2D commented Mar 10, 2016

@BarbarossaTM you added ldap-authorization. Could you perhaps test if API access works with these changes?

@laf laf merged commit 240ffe5 into librenms:master Mar 13, 2016

3 checks passed

Auto-Deploy Build finished. No test results found.
Details
Scrutinizer 4 new issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@laf
Member
laf commented Mar 13, 2016

Thanks for the work on this @Ultra2D

@Ultra2D Ultra2D deleted the Ultra2D:api-ldap branch Mar 14, 2016
@compuguy

@Ultra2D I have LDAP authentication, but I'm unable to make api access tokens with the latest release, so I think this is still broken.

@Ultra2D
Contributor
Ultra2D commented May 17, 2016

@compuguy the front-end has never worked. You need to add the token to database manually, see #3069 (comment) .

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