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

Strange behavior occurring when using custom LDAP id option #1872

Closed
ssddanbrown opened this issue Feb 5, 2020 · 13 comments
Closed

Strange behavior occurring when using custom LDAP id option #1872

ssddanbrown opened this issue Feb 5, 2020 · 13 comments

Comments

@ssddanbrown
Copy link
Member

@ssddanbrown ssddanbrown commented Feb 5, 2020

@ssddanbrown It seems that it's not possible to login again after the first one. I checked and the external authentication ID is very strange, actually:

image

Obviously, this is not the objectGUID that this user have in AD.

Originally posted by @joaomezzari in #592 (comment)

@mflagler

This comment has been minimized.

Copy link

@mflagler mflagler commented Feb 10, 2020

I'm having the same issue. I tried using a couple different formats for the objectGUID such as the Hex value as well as the GUID string and neither worked, so I created a temp user in AD and registered that user in Bookstack. Initial login worked, but subsequent logins did not work. It seems that it's not storing the value correctly. In the database and on the display for that user for their External Authentication ID I have this value: ?K?E??"?S|??

@ssddanbrown

This comment has been minimized.

Copy link
Member Author

@ssddanbrown ssddanbrown commented Feb 10, 2020

Thanks for reporting @mflagler.

This is strange, I didn't think we handled this value any differently than other values we get from LDAP.
In the next bugfix, I'll sneak in an option to dump retrieved details upon fetch so we have an easier way to debug this, since I don't really have visibility of the raw values coming back from AD.

@kanlas-net

This comment has been minimized.

Copy link

@kanlas-net kanlas-net commented Feb 13, 2020

@ssddanbrown I used GUID instead of objectGUID an it worked!

@mflagler

This comment has been minimized.

Copy link

@mflagler mflagler commented Feb 13, 2020

@kanlas-net I just tried it and while it does work to register users and login, it's still just using the old distinguished name and not the unique (and non-changing) objectGUID. I also tried ObjectSid which gave similar results to using objectGUID with �������������?�?Vz*�?�??, �� as the external authentication ID.

@kanlas-net

This comment has been minimized.

Copy link

@kanlas-net kanlas-net commented Feb 13, 2020

@mflagler as mentioned here this is because guid is stored as hexadecimal byte arrays, so it needs some conversion to become a text

@kanlas-net

This comment has been minimized.

Copy link

@kanlas-net kanlas-net commented Feb 13, 2020

@mflagler, @ssddanbrown seems like I have fixed it.
FIle: ./app/Auth/Access/LdapService.php

    public function getUserDetails(string $userName): ?array
    {
        $idAttr = $this->config['id_attribute'];
        $emailAttr = $this->config['email_attribute'];
        $displayNameAttr = $this->config['display_name_attribute'];

        $user = $this->getUserWithAttributes($userName, ['cn', 'dn', $idAttr, $emailAttr, $displayNameAttr]);

        if ($user === null) {
            return null;
        }

        $userCn = $this->getUserResponseProperty($user, 'cn', null);
        return [
-           'uid'   => $this->getUserResponseProperty($user, $idAttr, $user['dn']),
+           'uid'   => bin2hex($this->getUserResponseProperty($user, $idAttr, $user['dn'])),
            'name'  => $this->getUserResponseProperty($user, $displayNameAttr, $userCn),
            'dn'    => $user['dn'],
            'email' => $this->getUserResponseProperty($user, $emailAttr, null),
        ];
    }

screenshot

@necouchman

This comment has been minimized.

Copy link

@necouchman necouchman commented Feb 14, 2020

I was seeing this issue, too, with the odd behavior that I could log in with my AD account once, but the next time I logged it would just take me back to the login page, and I'd get the following error in the storage/logs/laravel.log file:

[2020-02-14 02:05:23] production.ERROR: A user with the email nick@example.com already exists but with different credentials. 
{"exception":"[object] (BookStack\\Exceptions\\UserRegistrationException(code: 0): A user with the email nick@example.com already exists but with different credentials. at /var/www/bookstack/app/Auth/Access/RegistrationService.php:60)

The patch provided by @kanlas-net seems to have resolved the issue.

@mflagler

This comment has been minimized.

Copy link

@mflagler mflagler commented Feb 14, 2020

I was looking around and want to test it, but don't have a dev server to test it on right now, but would it be better to use a suggested function similar to what is shown here in the comments: https://www.php.net/manual/en/function.mssql-guid-string.php#119391

This is removed in PHP 7, but the top note on the link suggests a different function to change the formatting to a standard GUID string like is viewable in Attribute Editor in this format: 4a9209bd-9252-4914-01a3-24c283062394

Also, would the fix given above by @kanlas-net break the existing storage of a DN instead of objectGUID, so it would need to be conditioned to only perform the function if the result is returned in binary format or if we're using objectGUID or ObjectSid?

@ssddanbrown ssddanbrown self-assigned this Feb 15, 2020
ssddanbrown added a commit that referenced this issue Feb 15, 2020
…ode option

Related to #1872
@ssddanbrown

This comment has been minimized.

Copy link
Member Author

@ssddanbrown ssddanbrown commented Feb 15, 2020

Thank you very much @kanlas-net for figuring out what was going on here.

I've just deployed v0.28.1 and v0.28.2. In these I have added the ability to prefix any LDAP attribute options with BIN; to mark them as binary to be decoded to hex on fetch. Therefore you should be able to use the following in your .env:

LDAP_ID_ATTRIBUTE=BIN;objectGUID

If you have many any edits so far to the app/Auth/Access/LdapService.php you might want to revert and clear your changes before updating to avoid issues when running the update commands.

I have also added a LDAP_DUMP_USER_DETAILS=true option which will dump user details as JSON upon fetch to help with debugging where needed.

I don't have AD myself so have not been able to fully test the changes made. Please let me know the above attribute option works. If so I'll go ahead and update the docs to reflect these changes otherwise I'll remain hesitant for now.


@joaomezzari FYI

@joaomezzari

This comment has been minimized.

Copy link

@joaomezzari joaomezzari commented Feb 17, 2020

Strange, I just tested with the new patch but it still doesn't work. I used the binary value and hex for the user ID, no success.

EDIT: My mistake. It works!

@mflagler

This comment has been minimized.

Copy link

@mflagler mflagler commented Feb 17, 2020

Works perfectly! Took a little bit of work to convert my users over, but it's working great! Thank you!

@Windoze345

This comment has been minimized.

Copy link

@Windoze345 Windoze345 commented Feb 17, 2020

Works for me too! :)

ssddanbrown added a commit to BookStackApp/website that referenced this issue Feb 18, 2020
- Documented new 'LDAP_DUMP_USER_DETAILS'.
- Documented 'BIN;' option on LDAP attribute.
- Updated AD details with 'BIN;' option.

For BookStackApp/BookStack#1872
@ssddanbrown

This comment has been minimized.

Copy link
Member Author

@ssddanbrown ssddanbrown commented Feb 18, 2020

Thanks everyone for confirming. Docs have now been updated to reflect these changes so I'll close this off.

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

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.