Skip to content
This repository has been archived by the owner on Apr 22, 2021. It is now read-only.

With ldap auth #10

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open

With ldap auth #10

wants to merge 23 commits into from

Conversation

JannikZed
Copy link
Contributor

this pull request adds full LDAP support for the Kimai Dashboard. All user settings get scoped

@JannikZed JannikZed requested a review from xoxys July 6, 2020 16:57
@JannikZed
Copy link
Contributor Author

node-cron is still missing - needs to send Emails to all users

@JannikZed JannikZed self-assigned this Jul 9, 2020
@JannikZed JannikZed requested a review from tilman July 9, 2020 20:05
README.md Outdated Show resolved Hide resolved
pages/api/auth/[...nextauth].js Outdated Show resolved Hide resolved
@JannikZed JannikZed marked this pull request as ready for review July 22, 2020 12:24
pages/api/auth/[...nextauth].js Outdated Show resolved Hide resolved
Copy link
Contributor

@xoxys xoxys left a comment

Choose a reason for hiding this comment

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

The implementation now looks better. I will take a closer look at it tomorrow and test it. Thanks

package.json Outdated Show resolved Hide resolved
@xoxys
Copy link
Contributor

xoxys commented Sep 16, 2020

@JannikZed I know this would need another round of refactoring but could you take a look at passport and the corresponding LDAP strategy http://www.passportjs.org/packages/passport-ldapauth/ please? Passport seems to have a broader community and is also used in some bigger projects like codimd. User groups search/filter should work as well see vesse/passport-ldapauth#10 and https://github.com/vesse/node-ldapauth-fork#ldapauth-config-options

@JannikZed
Copy link
Contributor Author

I'm using right now our own fork of https://github.com/shaozi/ldap-authentication - this is just temporary, as I'm just waiting for the newest version to be published to npm. Will replace it with the official library afterwards. This library is actually just a little wrapper around the official LDAP.js library https://github.com/ldapjs/node-ldapjs - it is the same with most other libaries like passport-ldapauth (the pure communication with the ldap server is always done using node-ldapjs or forks from it). The library I use has just way less overhead, as we don't need the passport features (we are using next-auth, the official next.js authentication library).
I have also group searching and filtering in place now. Our used ldap library does a group lookup and gives us all groups that a user is part of.
I think the version now is the best setup for us with all needed features and support from the main framework next.js.

@xoxys
Copy link
Contributor

xoxys commented Sep 17, 2020

Thanks for the explanation. I'll give it a test 👍

@xoxys
Copy link
Contributor

xoxys commented Sep 17, 2020

Still not working for me as expected:

  • error messages shown in URL bar (http://localhost:3000/api/auth/error?error=TypeError%3A%20Cannot%20convert%20undefined%20or%20null%20to%20object) that should never be the case and needs to be handled different
  • please remove the defaults for ldapAdmin* and ldapGroup* variables because they are useless
  • ldapGroup* should be completely optional, currently it doesn't work even if I set both vars to an empty string
  • normal users are not allowed to search the LDAP tree, but currently is seems to be done by the user who tries to log in and that's why it results in a "Not found" error (the same search works with an admin user to the search query in general is fine)
openldap     | 5f633253 conn=1002 fd=12 ACCEPT from IP=192.168.16.1:58280 (IP=0.0.0.0:389)
openldap     | 5f633253 conn=1002 op=0 BIND dn="uid=einstein,ou=people,dc=demoinc,dc=com" method=128
openldap     | 5f633253 conn=1002 op=0 BIND dn="uid=einstein,ou=people,dc=demoinc,dc=com" mech=SIMPLE ssf=0
openldap     | 5f633253 conn=1002 op=0 RESULT tag=97 err=0 text=
openldap     | 5f633253 conn=1002 op=1 SRCH base="ou=groups,dc=demoinc,dc=com" scope=2 deref=0 filter="(&(objectClass=groupOfNames)(member=uid=einstein,ou=people,dc=demoinc,dc=com))"
openldap     | 5f633253 conn=1002 op=1 SEARCH RESULT tag=101 err=32 nentries=0 text=
dashboard    | error: No Such Object
openldap     | 5f633253 conn=1002 op=2 UNBIND
openldap     | 5f633253 conn=1002 fd=12 closed
dashboard    | LDAPError [NoSuchObjectError]: No Such Object

@JannikZed
Copy link
Contributor Author

what is the expected behaviour when no groups lookup is used? The frontend will not allow any view on the frontend when a user is not part of the needed groups.

@xoxys
Copy link
Contributor

xoxys commented Sep 17, 2020

Without groups lookup every valid LDAP user is able to use the full app. That's how it should work. A common workflow is to configure an LDAP group filter as an admin e.g. (&(objectCategory=Group)(|(cn=kimai-users)(cn=kimai-admin))) and the auth framework will use this filter and check if the user who wants to log in is part of the result set or not. The used node LDAP framework seems to be working different and will give you just a full list of all groups, right? In this case the app will check if one of the groups from e.g. AUTH_GROUPS_DASHBOARD is in the users groups list, but only if groups lookup is configured.

@JannikZed
Copy link
Contributor Author

alright. Will implement it like this.

@JannikZed
Copy link
Contributor Author

using LDAP without Admin user is now no longer possible. So searching as normal user will not happen again. Not defining the groups make the dashboard just allow all groups.

@xoxys
Copy link
Contributor

xoxys commented Sep 17, 2020

Thanks a lot. I'll test it. You removed a bit too many defaults now :) Could you add back:

const ldapIdField = process.env.LDAP_MAPPING_UID || 'uid'
const ldapNameField = process.env.LDAP_MAPPING_NAME || 'displayName'
const ldapEmailField = process.env.LDAP_MAPPING_MAIL || 'mail'

@xoxys
Copy link
Contributor

xoxys commented Sep 17, 2020

Edit: Found it, it's already there.

Maybe it was not really clear from my site but now just AUTH_GROUPS_DASHBOARD is optional right? AUTH_GROUPS_BOOKING should of course work the same way.

@xoxys
Copy link
Contributor

xoxys commented Sep 17, 2020

It's still not working. With disabled groups lookup:

In this case the error is also still in the url... http://localhost:3000/api/auth/error?error=Error%3A%20There%20was%20an%20error%20in%20the%20application.%20Check%20the%20server%20logs

openldap     | 5f6373be conn=1001 fd=12 ACCEPT from IP=192.168.16.1:36324 (IP=0.0.0.0:389)
openldap     | 5f6373be conn=1001 op=0 BIND dn="uid=einstein,ou=people,dc=demoinc,dc=com" method=128
openldap     | 5f6373be conn=1001 op=0 BIND dn="uid=einstein,ou=people,dc=demoinc,dc=com" mech=SIMPLE ssf=0
openldap     | 5f6373be conn=1001 op=0 RESULT tag=97 err=0 text=
openldap     | 5f6373be conn=1001 op=1 UNBIND
openldap     | 5f6373be conn=1001 fd=12 closed
dashboard    | TypeError: Cannot read property 'map' of undefined
dashboard    |     at Object.authorize (/opt/app/.next/server/pages/api/auth/[...nextauth].js:206:48)
dashboard    |     at processTicksAndRejections (internal/process/task_queues.js:97:5)

With enabled groups lookup:

openldap     | 5f637324 conn=1002 fd=12 ACCEPT from IP=192.168.16.1:36258 (IP=0.0.0.0:389)
openldap     | 5f637324 conn=1002 op=0 BIND dn="uid=einstein,ou=people,dc=demoinc,dc=com" method=128
openldap     | 5f637324 conn=1002 op=0 BIND dn="uid=einstein,ou=people,dc=demoinc,dc=com" mech=SIMPLE ssf=0
openldap     | 5f637324 conn=1002 op=0 RESULT tag=97 err=0 text=
openldap     | 5f637324 conn=1002 op=1 SRCH base="ou=groups,dc=demoinc,dc=com" scope=2 deref=0 filter="(&(objectClass=groupOfNames)(member=uid=einstein,ou=people,dc=demoinc,dc=com))"
openldap     | 5f637324 conn=1002 op=1 SEARCH RESULT tag=101 err=32 nentries=0 text=
dashboard    | error: No Such Object
openldap     | 5f637324 conn=1002 op=2 UNBIND
openldap     | 5f637324 conn=1002 fd=12 closed
dashboard    | LDAPError [NoSuchObjectError]: No Such Object
dashboard    |     at messageCallback (/opt/app/node_modules/ldapjs/lib/client/client.js:1199:45)
dashboard    |     at Parser.onMessage (/opt/app/node_modules/ldapjs/lib/client/client.js:877:14)
dashboard    |     at Parser.emit (events.js:315:20)
dashboard    |     at Parser.write (/opt/app/node_modules/ldapjs/lib/messages/parser.js:107:8)
dashboard    |     at Socket.onData (/opt/app/node_modules/ldapjs/lib/client/client.js:864:22)
dashboard    |     at Socket.emit (events.js:315:20)
dashboard    |     at addChunk (_stream_readable.js:295:12)
dashboard    |     at readableAddChunk (_stream_readable.js:271:9)
dashboard    |     at Socket.Readable.push (_stream_readable.js:212:10)
dashboard    |     at TCP.onStreamRead (internal/stream_base_commons.js:186:23) {
dashboard    |   lde_message: 'No Such Object',
dashboard    |   lde_dn: null
dashboard    | }

@xoxys
Copy link
Contributor

xoxys commented Sep 17, 2020

For me it looks like the issue is related to the library https://github.com/shaozi/ldap-authentication/blob/master/index.js#L186 Even in authenticateWithAdmin the group search is done with the ldapUserClient instead of the ldapAdminClient

@JannikZed
Copy link
Contributor Author

JannikZed commented Sep 17, 2020 via email

@xoxys
Copy link
Contributor

xoxys commented Sep 17, 2020

Sure, just write me a mail. We're getting closer 🤞

@JannikZed
Copy link
Contributor Author

@xoxys please test it again - LDAP group lookup should be done with admin user now.

@xoxys
Copy link
Contributor

xoxys commented Oct 2, 2020

Thanks, I'll look into this next week.

@xoxys
Copy link
Contributor

xoxys commented Oct 6, 2020

@JannikZed still not working.... See below. Btw. in such case I still got error messages in the address bar: http://localhost:3000/api/auth/error?error=Error%3A%20There%20was%20an%20error%20in%20the%20application.%20Check%20the%20server%20logs please fix this globally.

openldap     | 5f7c3c2f conn=1001 fd=12 ACCEPT from IP=172.19.0.1:50206 (IP=0.0.0.0:389)
openldap     | 5f7c3c2f conn=1001 op=0 BIND dn="cn=admin,dc=demoinc,dc=com" method=128
openldap     | 5f7c3c2f conn=1001 op=0 BIND dn="cn=admin,dc=demoinc,dc=com" mech=SIMPLE ssf=0
openldap     | 5f7c3c2f conn=1001 op=0 RESULT tag=97 err=0 text=
openldap     | 5f7c3c2f conn=1001 op=1 SRCH base="ou=people,dc=demoinc,dc=com" scope=2 deref=0 filter="(uid=einstein)"
openldap     | 5f7c3c2f conn=1001 op=1 SEARCH RESULT tag=101 err=0 nentries=1 text=
openldap     | 5f7c3c2f conn=1001 op=2 UNBIND
openldap     | 5f7c3c2f conn=1001 fd=12 closed
openldap     | 5f7c3c2f conn=1002 fd=12 ACCEPT from IP=172.19.0.1:50208 (IP=0.0.0.0:389)
openldap     | 5f7c3c2f conn=1002 op=0 BIND dn="uid=einstein,ou=people,dc=demoinc,dc=com" method=128
openldap     | 5f7c3c2f conn=1002 op=0 BIND dn="uid=einstein,ou=people,dc=demoinc,dc=com" mech=SIMPLE ssf=0
openldap     | 5f7c3c2f conn=1002 op=0 RESULT tag=97 err=0 text=
openldap     | 5f7c3c2f conn=1002 op=1 UNBIND
openldap     | 5f7c3c2f conn=1002 fd=12 closed
dashboard    | TypeError: Cannot read property 'map' of undefined
dashboard    |     at Object.authorize (/opt/app/.next/server/pages/api/auth/[...nextauth].js:206:48)
dashboard    |     at processTicksAndRejections (internal/process/task_queues.js:97:5)

@xoxys
Copy link
Contributor

xoxys commented Oct 6, 2020

Looks like this error only occurs if LDAP_GROUPS_SEARCH_BASE and/or LDAP_GROUPS_OBJECT_CLASS is not set.

@xoxys
Copy link
Contributor

xoxys commented Oct 6, 2020

I've also noticed that the /import route is not group filtered right now, that should be added too.

@JannikZed
Copy link
Contributor Author

JannikZed commented Oct 6, 2020 via email

@xoxys
Copy link
Contributor

xoxys commented Oct 6, 2020

Group lookup and limiting access seems to be working

@JannikZed
Copy link
Contributor Author

updated everything as requested. Switched also back to the official LDAP package now, as my pull request got merged.

@JannikZed
Copy link
Contributor Author

any updates ? :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants