Skip to content
This repository has been archived by the owner on May 14, 2024. It is now read-only.

Add PasswordPolicyControl #4

Merged
merged 1 commit into from
Nov 2, 2023
Merged

Add PasswordPolicyControl #4

merged 1 commit into from
Nov 2, 2023

Conversation

its-sami
Copy link
Contributor

I've done the steps outlined in ldapjs/node-ldapjs#946 (comment) but haven't linked to the issue as it's closed. Let me know if there's anything else I should do!

Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

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

Good work. Thank you for following the defined organization.

I have one question, do you think it would be appropriate to add a mechanism for error code lookup? We do it in:

const validCodeNames = [
'SUCCESS',
'OPERATIONS_ERROR',
'TIME_LIMIT_EXCEEDED',
'STRONGER_AUTH_REQUIRED',
'ADMIN_LIMIT_EXCEEDED',
'NO_SUCH_ATTRIBUTE',
'INAPPROPRIATE_MATCHING',
'INSUFFICIENT_ACCESS_RIGHTS',
'BUSY',
'UNWILLING_TO_PERFORM',
'OTHER'
]
const filteredCodes = Object.entries(RESULT_CODES).filter(([k, v]) => validCodeNames.includes(k))
const VALID_CODES = new Map([
...filteredCodes,
...filteredCodes.map(([k, v]) => { return [v, k] })
])

/**
* A map of possible response codes. Includes `CODE => VALUE` and
* `VALUE => CODE`. For example, `RESPONSE_CODES.get(0)` returns
* `LDAP_SUCCESS`, and `RESPONSE_CODES.get('LDAP_SUCCESS')` returns `0`.
*/
static RESPONSE_CODES = Object.freeze(VALID_CODES)

@its-sami
Copy link
Contributor Author

Thanks, I had avoided a lookup like that as the error codes in the control don't match the result codes defined in the protocol. Also no mapping is defined in the similar case:

/**
* @typedef {object} EntryChangeNotificationControlValue
* @property {number} changeType One of 1 (add), 2 (delete), 4 (modify),
* or 8 (modifyDN).
* @property {string} previousDN Only set when operation is a modifyDN op.
* @property {number} changeNumber
*/

I'd be happy to create some kind of mapping though.

@jsumners
Copy link
Member

jsumners commented Nov 2, 2023

That's fair.

Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@jsumners jsumners merged commit e55985d into ldapjs:master Nov 2, 2023
5 checks passed
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