ldap-authentication icon indicating copy to clipboard operation
ldap-authentication copied to clipboard

[Feature request] return AuthenticationResult object instead of user object

Open fabiang opened this issue 1 year ago • 5 comments

Can I please have a flag (or another method), where a AuthenticationResult object is returned instead of the user object, which contains:

  • the user object,
  • the result message/error,
  • a flag where I can check against a constant (RESULT_SUCCESS, RESULT_INVALID_CREDENTIALS etc.),
  • and the ldapjs client object?

I'm guessing many other want to do some extra checks after the user was authenticated, e.g. check if user belongs to a group recursively.

If that's ok, I would be happy to provide a PR.

fabiang avatar Oct 10 '22 13:10 fabiang

This is a great idea. If you have time, please create a PR. I would suggest to add one more field to the option to the authenticate(option) function call. such as:


authResult = authenticate({ ..., returnAuthResult: true})

shaozi avatar Oct 18 '22 20:10 shaozi

Currently working on it and to make things a bit easier I supplied PR #39.

My suggestion for the authentication class:

const AUTH_RESULT_FAILURE = 0
const AUTH_RESULT_SUCCESS = 1
const AUTH_RESULT_FAILURE_IDENTITY_NOT_FOUND = -1
const AUTH_RESULT_FAILURE_IDENTITY_AMBIGUOUS = -2
const AUTH_RESULT_FAILURE_CREDENTIAL_INVALID = -3
const AUTH_RESULT_FAILURE_UNCATEGORIZED = -4

class AuthenticationResult {
    constructor(authCode, identity, user, messages, client) {
        this.authCode = authCode // one of the above constants
        this.identity = identity // identity supplied as string
        this.user     = user // user object found on ldap server OR null
        this.messages = messages // authentication messages array, which contains server messages
        this.client   = client // ldapClient instance
    }

    get code() {
        return this.authCode
    }

    get identity() {
        return this.identity
    }

    get messages() {
        return this.messages
    }

    get client() {
        return this.client
    }

    get user() {
        return this.user
    }
}

Does this looks good to you?

fabiang avatar Oct 28 '22 14:10 fabiang

That definitely is clean. The thing bothers me though is if we return this object, it will not be backward compatible while user is expecting the return of a user object.

shaozi avatar Nov 12 '22 02:11 shaozi

So instead of changing the existing method, should we add another which returns a result object and the old method just use it?

fabiang avatar Nov 14 '22 12:11 fabiang

This is a great idea. people can then use the new method if they want, while older version keep using old methods without breaking.

shaozi avatar Nov 14 '22 17:11 shaozi