jelhub / scimgateway

Using SCIM protocol as a gateway for user provisioning to other endpoints
MIT License
176 stars 57 forks source link

New suggetions to make code better #135

Closed niranjanvk closed 1 month ago

niranjanvk commented 1 month ago

Hi @jelhub, I am using scimgateway for quite a few months (with plugin-ldap) and think the following changes can make the framework more useful.

is it possible to parameterised log file size and max file https://github.com/jelhub/scimgateway/blob/a8c871fd97a70cd9ece5cf3818ddbfe3f8c11c78/lib/logger.js#L119

If a user is already member of group and a PATCH request try to add it again, scimgateway should return 204 (https://datatracker.ietf.org/doc/html/rfc7644#section-3.5.2.1) https://github.com/jelhub/scimgateway/blob/a8c871fd97a70cd9ece5cf3818ddbfe3f8c11c78/lib/scimgateway.js#L1594

I am still seeing reverse string in outbound response when inboundtype is array and type is string in mapping and when entra check source and target value it finds a difference. for ex- entra has 3 values for any attribute xyz, abc, def now I am mapping it using an expression "Join(",", Split([otherMails], ","))" so the string value in entra will be xyz,abc,def but the string value in response from scimgateway is def, abc,xyz. https://github.com/jelhub/scimgateway/blob/a8c871fd97a70cd9ece5cf3818ddbfe3f8c11c78/lib/scimgateway.js#L2743

Is it possible to use only info log level for all required logs throughout the code and debug only where troubleshooting is required and give an option to enable disable debug logs from config file and mostly debug logs are disabled in prod.

is it possible to append baseEntity in scimgateway.js logs (This will be very helpful while leveraging multitenancy). For multi tenant, is it possible to have different log files for each tenant with name something like plugin-ldap-${baseEntity}.log

Is it possible to add a list of attributes which should be in the ldapsearch (GET request result) in config file? ex- when a group have let say 20000 members and Entra sends patch request to add each groups and Entra sends almost 2 GET requests with each patch so everytime ldapjs query return value of all attributes and its time coslty and size of log file becomes very big so I want to exclude the member attribute from group search.

If you need any other information, please let me know.

jelhub commented 1 month ago

Thank you for your inputs.

Q1) - Logfile details config: I'm aware of this one and might be implemented in future release

Q2) - plugin-ldap, patch add exiting group member returns 500: This plugin-ldap issue will be fixed in next release

Q3) - plugin-ldap using inboundtype mapping: Working OK on Active Directory, none OpenLDAP (using reverse order) Fails on OpenLDAP servers (fails using reverse order) Will be fixed in next release. Introducing a new config typeOutboundReverse=true/false that will for plugin-ldap be automatically set by the isOpenLdap config.

Q4) - log level: All log levels are available both for file and console, see readme log.loglevel.file - off, error, info, or debug Setting off will turn off logging, example having read-only disk like Google Cloud App Engine Standard

Q5) - include baseEntity in all logs Will be included in next release

Q6) - logfiles for each baseEntity Probably possible, but will not as of now be prioritized

Q7) - exclude members from GET request: This is normally handled by request parameters like attributes for what to be included and excludeAttributes for what to exclude E.g. GET http://localhost:8880/Groups/Admins?excludedAttributes=members will return all group attributes but not members, see readme for some general filtering examples I understand this will not solve your issue since Entra ID sends requests that should include members. I don't think it's a good idea to make such customized config in the general plugin-ldap. Suggest you make your own customized version for your needs.

niranjanvk commented 1 month ago

Thanks @jelhub

jelhub commented 1 month ago

@niranjanvk, The new release v4.5.11 have now been published