search

osuosl / timesync

A time tracking API
2 stars 1 forks source link

LDAP authentication docs unclear #80

Closed mathuin closed 7 years ago

mathuin commented 8 years ago

https://timesync.readthedocs.io/en/latest/auth.html#ldap-authentication

The following text is in the documentation:

Instead of comparing the username/password combination to values in a local database, however, it provides it to a configured LDAP provider for authentication.

"it provides it" is a big confusing.

There's no pointer to how to configure an LDAP provider, either. If that's an implementation issue and not something set in the API, then it might not hurt to mention that. Same goes for the password authentication.

MorganEPatch commented 8 years ago

I think the wording stems from a combination of not entirely understanding how LDAP works to begin with and trying to remain vague and open to how it's implemented. What it more or less is trying to say is "for password authentication, it compares the password provided to the one associated with the user's account", and for LDAP "it compares the password provided to the one associated with the LDAP account with the same username via whatever LDAP server the timesync instance is set up to communicate with". How those passwords are stored, and how it communicates with the LDAP server/what LDAP server it communicates with are up to the implementation. E.g. in timesync-node, it's just set by environment variables.

mathuin commented 8 years ago

I understand that the LDAP stuff is an implementation issue. In that case, why do we mention LDAP at all? It's just a username and a password, regardless of the backend, right?

Somewhere in the server's implementation, it's going to take a username and password and pass it off to something whether it's an LDAP library or an SQL database or maybe even HTTP basic authentication. From the API's perspective, the only reason to have authentication types is if it is expected that any API implementation support multiple authentication times. The only reason to hardcode those types is to limit implementers to those types. If that's the goal, that's good to know. If it's not, you can rip out a whole layer of complexity and reduce your breakable code by a non-trivial fraction here.

This might have to wait for /v1. It should be considered very seriously before that release.