-
Notifications
You must be signed in to change notification settings - Fork 39
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
Experimental Authorization-only mode #269
base: master
Are you sure you want to change the base?
Conversation
Add documentation for AuthZ-only scenario Second pass at heiglandreas#258 - this time updating 3.0.x
As a note during evaluation of this PR - as things currently stand, the authLdap_authorize_only function/filter will process even if authLDAP is not enabled. This is intentional, and allows scenarios where only "external" authentication is allowed as well as "external" and "local" auth are both allowed, while LDAP interaction is reserved solely for authorization purposes. Obviously this can be further refined, but I wanted to get the existing code out there for review and discussion rather than making unilateral decisions on how that could be implemented. |
Thanks for the great work you did in the PullRequest! There are several things I noticed though that I think make it more complicated that it needs to be. The WordPress process of authentication is as far as I checked that the So
This means that when a filter-function with a higher priority (lower Right now we do not check whether the first parameter is already an authenticated users in that function. But actually we would not need to do authentication when we already have an authenitcated user. So thinking about it, we would not need any new UI element at all to use the LDAP group-matching when a different authentication mechanism (SAML, OIDC, OAuth2...) was used. The plugins handling that login also needs to hook into the The only reason we might need a UI-switch would be to hide some of the configuration parts. But right now I would skip that. |
@@ -184,6 +190,8 @@ | |||
$authLDAPGroupOverUser = authLdap_get_option('GroupOverUser'); | |||
$authLDAPDoNotOverwriteNonLdapUsers = authLdap_get_option('DoNotOverwriteNonLdapUsers'); | |||
$authLDAPUseUserAccount = authLdap_get_option('UserRead'); | |||
$authLDAPExternalUsers = authLdap_get_option('ExternalUsers'); |
Check warning
Code scanning / Phpmd (reported by Codacy)
Detects when a field, local, or parameter has a very long name. Warning
@@ -193,6 +201,8 @@ | |||
$tStartTLSChecked = ($authLDAPStartTLS) ? ' checked="checked"' : ''; | |||
$tDoNotOverwriteNonLdapUsers = ($authLDAPDoNotOverwriteNonLdapUsers) ? ' checked="checked"' : ''; | |||
$tUserRead = ($authLDAPUseUserAccount) ? ' checked="checked"' : ''; | |||
$tExtChecked = ($authLDAPExternalUsers) ? ' checked="checked"' : ''; |
Check notice
Code scanning / Phpmd (reported by Codacy)
Prohibit the definition or assignment of unused local variables Note
@@ -193,6 +201,8 @@ | |||
$tStartTLSChecked = ($authLDAPStartTLS) ? ' checked="checked"' : ''; | |||
$tDoNotOverwriteNonLdapUsers = ($authLDAPDoNotOverwriteNonLdapUsers) ? ' checked="checked"' : ''; | |||
$tUserRead = ($authLDAPUseUserAccount) ? ' checked="checked"' : ''; | |||
$tExtChecked = ($authLDAPExternalUsers) ? ' checked="checked"' : ''; | |||
$tLocalWExtChecked = ($authLDAPLocalWithExternal) ? ' checked="checked"' : ''; |
Check notice
Code scanning / Phpmd (reported by Codacy)
Prohibit the definition or assignment of unused local variables Note
* @conf boolean authLDAPDebug true, if debugging should be turned on | ||
* @conf string authLDAPURI LDAP server URI | ||
* | ||
* @return Org_Heigl\AuthLdap\LdapList LDAP server object | ||
*/ | ||
function authLdap_get_server() | ||
function authLdap_get_server($reset = false) |
Check warning
Code scanning / Phpmd (reported by Codacy)
The method authLdap_get_server has a boolean flag argument $reset, which is a certain sign of a Single Responsibility Principle violation. Warning
To address your comments more generally, here's the logic behind the changes as they currently stand, and why I implemented certain things the way that I did. I'll state up front that I am 100% on board with simplifying things where possible. I just don't see the places to remove complexity at this stage (with one possible exception I'll get to at the end). The authLdap authenticate filter triggers at priority 10, default WP logins at priority 20. At least one of the plugins I tested with triggers at priority 21. So I set up the The presence of the UI flags driving the options was, in part, to allow me to test different authenticate/authorization flows without having to change the code repeatedly, as well as to allow me to revert and recover settings back to the "release" state. They're also there to ensure that, should this functionality get integrated, it doesn't step on anyone's existing configurations. For example, if someone was to have an existing WP instance that uses SAML for authentication, and there happens to be an LDAP-based user match for that uid that isn't present in the groups as defined, forcing the authorization flow on that user would revert them to the Default Role as defined by authLdap rather than allowing them to keep their current roles. In short, I wanted to make sure that anyone who decides to experiment with this new functionality is doing so intentionally, rather than stumbling into an unexpected problem. As another note, I also kept things separate in case there's a desired use case for a true "authorization only" mode for a WP site - authLdap installed, but with the "Enabled" flag off so that the username/password is never looked up against LDAP, but the group assignments in LDAP are used for authentication. For simplification, I could see taking the duplicate authorization code (the block beginning As a final note - I'm currently ignoring the various automated checks until we've rolled in any necessary changes. Chasing down linter errors or other recommendations isn't useful if that stuff might end up going away anyway. 😉 |
Moving authLdap to priority 50 would also be an option. Or adding a fumction to check on i stallation of any plugin whether they use the "authenticate" filter and set the authldap filter above by +1 of the max value? The 10 is just an arbitrary number I set some 15 years ago 😁 An alternative would be to add a setting that lists the different authentication plugins and allows to inject the plugin somewhere in between or to the end or beginning.... |
Did some quick local tests in my sandbox. As it stands, if I disable the new function and the settings, and bump the Moving things to a later priority (with no other changes) would also introduce some weird issues if the CheckPW setting is enabled. If the user changes their LDAP password, their old password would still work (because local password storage would be evaluated first), and if that failed, the new (LDAP-checked) password would also work for logins. But that means if someone has a saved password in their browser, and changes things in LDAP due to a compromise, their account may still function as compromised in the WP context until they log in to the site and the database updates. If, on the other hand, authLdap_login was to check for the an instance of WP_User again and kick them out of the login loop, then anything after that check would no longer be applied - including all of the authorization pieces of logic. As to checking for other instances of an authenticate filter, I think that's more complex than it sounds. From what I've seen just over the last few weeks, different plugins define that very differently, and finding all instances where it might be used could lead to a lot of complicated logic that is evaluating code which is outside of your control. I don't know about you, but it sounds like a maintenance nightmare to me. |
That indeed sounds like a maintenance nightmare! So: I think the best would be to actually let authldap call the authenticate filter twice. Once for the authentication with priority 10 as it already is. And then with the authorization part with priority 100. That way the authentication will.continue to work as right now. And other authentications can also work as they do and then in the end we do the group mapping after all other authentications ran. Requires to be able to enable or disable the two parts. So we need three parts in the settings:
Does that sound sensible? |
Sounds sensible to me, and what you scoped is close (although not identical) to what the PR is doing.
This is essentially what's happening at priority 50, although I have it split into two distinct functions - the login, which only operates when the user is logging in via authLdap, and then the authorization happening at a later stage.
In the PR, the code treats the existing "Enable" flag as the on for enabling authLDAP authentication, and the new "ExternalUsers" flag for enabling authorization. The other new flag ("LocalWithExternal") is used for enabling/disabling existing 'local-only' accounts (those without any findable object in LDAP) entirely, as different scenarios might call for that functionality.
This is more or less how the settings are currently broken out, although I'm sure with the new functionality, you'll probably want to do some rearranging and possible variable renaming. I'm going to take a crack at simplifying to code some more based on the above. Shouldn't be too long, provided I don't run into any big issues. |
I think I've trimmed things down as much as I'm able to. +243 lines / -11 lines on the PR, but of the 243 added lines:
That's 136 lines that aren't really functional changes in the back-end logic. A bunch of the other +107/-11 is comments, whitespace, basic additions (importing settings in the Authorization loop, for example), and minor modifications to clean various things up/provide compatibility, so the overall change set is relatively minimal with regards to the code itself. I put a bit of a spin on your recommendations, resulting in the following
It would absolutely be possible to embed an authLdap_authorization inside of authLdap_login and trigger it that way (with some attendant changes to the authorization function to prevent double-processing at priority 100), but I don't know if it makes much sense to do that since the current flow works as well. How do you think things look after this latest set of deltas? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still have some headaches with some of the code but the general idea is great! Thank you!
And the still working behat-tests show that your changes do not interrupt the currently existing flows (at least the main ones 🙈) which is pretty awesome!
authLdap.php
Outdated
{ | ||
static $_ldapserver = null; | ||
if (is_null($_ldapserver)) { | ||
if (is_null($_ldapserver)|| $reset === "reset") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When $reset
is a boolean value this comparison will always be false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This actually isn't a boolean (I neglected to update the comment), but rather an explicit string of "reset". I should actually set a default value of null
, or change it to a boolean - whichever will make the code behave better. But this should only trigger when explicitly called by line 363 $ldapServerList = authLdap_get_server("reset");
.
authLdap.php
Outdated
@@ -254,7 +265,7 @@ function authLdap_get_server() | |||
* @param string $username | |||
* @param string $password | |||
* @param boolean $already_md5 | |||
* @return boolean true, if login was successfull or false, if it wasn't | |||
* @return WP_User, if login was successfull or false, if it wasn't |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks 😂
This should be WP_Uer|WP_Error|false
to match the actual results, shouldn't it? ANd yes! It was definitely worse before! So: Thanks 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose it should, shouldn't it? 😂
authLdap.php
Outdated
@@ -276,6 +285,14 @@ function authLdap_login($user, $username, $password, $already_md5 = false) | |||
return $user; | |||
} | |||
|
|||
// don't do anything when the password is not defined - assume an alternate method of authentication | |||
if ($password == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to keep login-logic out of this method. Can we somehow handle this case in the Authenticate
class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure I can just remove that block altogether - I might have left that one in for testing by mistake.
global $authLDAPisLdapLogin; | ||
$authLDAPisLdapLogin = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need this? After all the process right now consists of 2 completely separate checks: Authenticate and Authorize. Why does the Authorize need to know where the authentication was done?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but it's a bit hard to articulate why. Basically, we have 3 potential sources of user authentication (generally speaking):
- LDAP users
- Internal WP users
- "Other"/External users
Before we invoke Authorize, we need to know if we can recycle the existing LDAP connection - which will only work if we successfully authenticated via LDAP in the authLdap_login function. If we blindly reset the LDAP connection regardless of the authentication method (which we could do, of course), it overwrites the functionality of the UserRead flag.
So this global is there to ensure that a successful LDAP authentication continues to use the existing LDAP connection, and in all other cases, force it to reset because without that reset, the LDAP connection doesn't function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. Let me check why I introduced that flag. There was a reason. I'll get back to you on that later
authLdap.php
Outdated
|
||
// If this isn't already an LDAP user, force a reset in the LDAP server list, or LDAP lookups will fail | ||
if (!($authLDAPisLdapLogin === true)) { | ||
$ldapServerList = authLdap_get_server("reset"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a boolean true
, shouldn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That or I need to adjust the default value, as mentioned above. Will ponder this a bit.
authLdap.php
Outdated
global $authLDAPisLdapLogin; | ||
// Don't trigger if this isn't an LDAP login AND External Users aren't enabled | ||
if (!($authLDAPisLdapLogin === true) && !authLdap_get_option('ExternalUsers')) { | ||
authLdap_debug('User ineligible for LDAP Authorization '); | ||
return $loggedInUser; | ||
} | ||
|
||
// If this isn't already an LDAP user, force a reset in the LDAP server list, or LDAP lookups will fail | ||
if (!($authLDAPisLdapLogin === true)) { | ||
$ldapServerList = authLdap_get_server("reset"); | ||
$ldapServerList->bind(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd ratehr move the logic into the Authorize
class to have everything deciding whether the class should do something actually within that class. That makes future maintenance easier as there is only one place to modify when conditions should change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a matter of principle, I agree with you here. I was (and still am) struggling with how to make that happen without building in two-way dependency loops between Authorize
and the core plugin script (which provides authLdap_get_server
and builds the LDAP connections). Frankly, I'm not thrilled with the idea of using a global either, so I want to take some time to think about that piece. But I'm definitely open to suggestions on both fronts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those dependencies are a completely separate topic that I will address in a separate PR later. That will then also get rid of the WordPress functions scattered throughout the code and use "propper" dependency injection etc.
So for now: Don't worry about that.
$userInfoLdap[0][(string) $this->uidAttribute][0], | ||
$userInfoLdap[0]['dn'] | ||
); | ||
// Suppress PHP errors when no entries are returned |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Great thinking! Thanks!
// if the DN is not present in LDAP, and we both allow External users and local users | ||
// alongside them, then just return the user object untouched so that roles are not changed | ||
if ( | ||
( !isset($userInfoLdap[0]['dn']) || $userInfoLdap[0]['dn'] === null ) && | ||
$this->externalUsers->isEnabled() === true && | ||
$this->localWithExternal->isEnabled() === true | ||
) { | ||
$this->logger->log('user not found in LDAP, but permitted due to LocalWithExternal setting'); | ||
return $user; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can skip that step.
There are several things happening right now:
- The user has no role assigned in WordPress and we don'T have the user in LDAP: The user gets the default role assigned. (Which is necessary, otherwise they won't be able to do anything)
- The user has a role assigned in WordPress and we don't have the user in LDAP: Depending on the setting "Overwrite WP-Roles" the existing roles get overwritten with the default role or the existing roles will stay as they are.
- The user has no role assigned in WordPress and we have the user in LDAP: The user gets the LDAP-roles (or the default role as fallback) assigned.
- The user has roles assigned in WordPress (or from a previous authentication step) and we have the user in LDAP: Depending on the "Overwrite WP-Roles" setting, the user gets the LDAP-roles (or the default role as fallback) assigned or keeps their current ones.
So this should all already be handled. Or am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The user has a role assigned in WordPress and we don't have the user in LDAP: Depending on the setting "Overwrite WP-Roles" the existing roles get overwritten with the default role or the existing roles will stay as they are.
In this scenario, what happens without "Overwrite WP-Roles" set is that the existing roles get overwritten (or the user is denied access if that's the default). I added this clause specifically to handle the case where local users already existed with role assignments (we have a local "break glass" admin account on our WP instances that meets these conditions) and was getting locked out.
That's also the purpose of the "LocalWithExternal" flag - it basically says that, regardless of what the other LDAP settings are, if this is a true non-LDAP user, let them use their existing roles regardless of the Overwrite flag.
src/Authorize.php
Outdated
// Return an empty array if dn is not set or is null | ||
if (!isset($dn) || $dn === null) { | ||
return []; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we not move this to the top of the function? That way we can immediately return an empty array without having to invoke any (expensive) WordPress functions.
And we should be able to remove the isset
here as the variable is definitely set as it is a function parameter.
But we might need to adapt the signature to allow null
besides string
. Or we pass an empty string instead of the null and then check for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we not move this to the top of the function? That way we can immediately return an empty array without having to invoke any (expensive) WordPress functions.
Oh, for sure. I just shoved that in to resolve an error and didn't pay much mind to what was before it - I just needed it prior to the try/catch to resolve an error.
And we should be able to remove the
isset
here as the variable is definitely set as it is a function parameter.
You're likely correct on the isset
- I was going fast out of frustration when I added this bit. 😅
But we might need to adapt the signature to allow
null
besidesstring
. Or we pass an empty string instead of the null and then check for that.
I think a $dn === null
should be sufficient without any other tweaking? But I can definitely run through some tests when I make the next set of changes.
Shall we try to find the user in LDAP even if they logged in through a different mechanism (SAML, OIDC, social logins, local accounts, etc)? CAUTION: This has the potential to break access for users, | ||
depending on the configuration of other settings. | ||
</p> | ||
<p class="description">This should only be checked if you know what you are doing! THIS WILL TRIGGER EVEN IF LDAP AUTHENTICATION IS DISABLED.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "Enable AuthLDAP" should enable the whole plugin! Not only the Authentication. It is a safety catch to disable all LDAP-interaction in case that becomes necessaary.
To disable the authentication part of authLdap we need a separate flag to "Disable Authentication via LDAP"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm 100% fine with this. Wasn't sure how you wanted to approach things, so I didn't add that in, but you're absolutely right - that's definitely needed as a safety switch.
Will respond to the comments above now, and then take a look at your replies and any necessary code changes tomorrow, as it's late for me. I just didn't want to leave this unanswered before I went to bed. 😅 |
My day job got a little crazy this week, so updates were slow going to say the least. I'm working on an additional commit that addresses the following based on your feedback and stuff from the code scans. Everything has been tested on my local sandbox instance before I even wrote it on the list here, so these aren't blind changes.
Things that I haven't touched yet:
|
authLdap_debug( | ||
'LDAP disabled in AuthLDAP plugin options (use the first option in the AuthLDAP options to enable it)' | ||
); | ||
return $user; |
Check notice
Code scanning / Phpmd (reported by Codacy)
Prohibit the definition or assignment of unused local variables Note
Just as a note here - latest commit above was a standard merge of the 3.1.0 release into the PR's branch - no other changes have been introduced at this time. It's been a couple of weeks since we've talked about anything regarding this PR, so I'm not sure if we should resume the conversation or basically "start over" in terms of discussing the change set. But I'm happy to answer any questions, clarify what's happening, or make additional alterations as needed. Thanks again for entertaining this entire effort. |
I've got "going through the changes" on my agenda for the weekend! |
Add support for Authorization-Only mode:
Other changes made in support of this PR:
Happy to discuss any of the reasoning used here, make further modifications as necessary, or have this torn apart for further reuse if that's what's needed.