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

[Feature request] return AuthenticationResult object instead of user object #38

Open
fabiang opened this issue Oct 10, 2022 · 5 comments
Labels
enhancement New feature or request

Comments

@fabiang
Copy link
Collaborator

fabiang commented Oct 10, 2022

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.

@shaozi shaozi added the enhancement New feature or request label Oct 18, 2022
@shaozi
Copy link
Owner

shaozi commented Oct 18, 2022

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})

@fabiang
Copy link
Collaborator Author

fabiang commented Oct 28, 2022

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?

@shaozi
Copy link
Owner

shaozi commented Nov 12, 2022

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.

@fabiang
Copy link
Collaborator Author

fabiang commented Nov 14, 2022

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

@shaozi
Copy link
Owner

shaozi commented Nov 14, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants