Key issues for v1.3.0 #17

marohrdanz · 2024-03-08T23:28:51Z

This pull request addresses key features for v1.3.0 release:

Add option to bind to LDAP with the user attempting to authenticate (Bind to LDAP with user attempting to authenticate #12)
- Previously a service account was required
Address the unhandled ECONNRESET error from ldapauth-fork (Server crashes with unhandled ECONNRESET error #9)
Add a health check endpoint (Add a 'health check' endpoint #13)
Run as non-root user (Run as non-root user #14)
Convert test suite from python to node (Convert tests to node #10)
Set up continuous integration (Set up CI #11)

In addition I did quite a bit of refactoring to make the code easier to test, maintain, and read.

I find this easier to deal with than console logs when deploying to production environment. Also added a utility function so that JSON.stringify can be used w/o logging the bindCredentials or passwords.

I have been using winston logging...and I have not changed that (yet?). However the underlying ldapauth-fork and ldapjs use bunyan, and since I'm working on figuring out the connection errors I'm waning to use the logging already in those package. So adding that for the moment. Maybe later I'll swap out the current winston for bunyan? Addresses issue #6

Because the ldapauth-fork and ldapjs use bunyan, switching to that for general logging.

This is to address the ECONNRESET issue from ldapauth-fork. This solution is to bind and unbind from the ldap server for each authentication. Note: the /verify endpoint does NOT require any connection to ldap, only the /authenticate endpoint. Addresses issue #9

The unit tests for this project are written in python (I know..they should be in node since the rest of the project is in node). Because I had some python package version issues when running them today, I added info to create a virtual environment for running the tests.

A number of key events were logged at debug level, so promoted those to info or error level as appropriate. Also added some extra logging details regarding authorized groups that might be handy. These changes might be handy for analyzing security issues on the server (e.g. attack attempts).

One school of thought is 'ERROR' is an application error. With that in mind changing these to 'WARN'. This will also make it a little easier to grep the logs and find failed login attempts.

This saves a little space and makes the log easier to read.

The log object creates a circular reference and breaks JSON.stringify. So masking it in the replacer function for JSON.stringify.

Adding the option to configure the server to bind to LDAP using the authenticating user instead of a specific service account. Addresses #12

This is helpful in development, and will be used when DEBUG is set. Also removed the no-longer-used 'debug' setting.

This eliminates a number of vulnerabilities found by docker scout.

This is to avoid the `COPY . ./` in the Dockerfile.

Adding different build targets for development and production. This allows for development w/ handy things like vim and nodemon w/o adding bloat to the production build. Also adding a docker-compose file to simplify configuration (sometimes this is easier than `docker run`).

Build arguments with reasonable defaults used for non-root user. Addresses issue #14.

This should have been part of de49a5d.

This env is used by index.js to determine if it should use http or httpS. Use this to determine if server should use http or httpS.

This addresses issue #13.

This should have been part of de49a5d.

Moving these functions just to make the index.js file simpler.

I like to make those pretty websites of API documentation.

Adding this mock server to facilitate testing.

Moving the guts of the `/authenticate` and `/verify` routes to their own functions. This cleans up the logic in index.js and potentially makes things easier to test. Also did some refactoring within those handlers to make the code more clear, and added JSDoc.

This changes the hard-coded 'ldap-jwt' base path for the URL to be configurable. The default is 'ldap-jwt'.

If the user sends a garbage token, e.g. something that can't even be decoded, we want to send back a 401 (not a 500). Therefore adding this first try/catch in verifyHandler.

userInAuhorizedGroups and getGroupCN used to be exported from utils.js, and when used in index.js, were referenced with `ut.`. But that's no longer the case. This change should have been included a few commits ago.

Use supertest to directly test the application's main routes. This addresses issue #10. Removed the now-superseded python tests.

For running a GitHub Actions Worflow for CI testing, we don't need to run a docker container. We can just use a runner with node installed. With this in mind, giving setconfig the option to exit after writing the config file in a CI build, and for index.js to start the mock ldap server in a CI build.

I had this at the top level to start automatically in development...but moving it into __tests_ to make things cleaner. If a developer wants to use it for their testing, it's easy enough for them to add a few lines of code to do so.

Adding this to make it easier for people to run test suite.

Trying to debug the workflow dispatch.

I had been wondering why the workflow dispatch wasn't showing up in the browser. It's because it only shows up if the workflow is in the default branch of the project. So it should show up once this is merged into main.

This version adds non-breaking features.

This name just seems clearer

Tweaked code comments for clarity, added a variable or two to try and make the code easier to understand.

ChrisWakefield

I've looked at all the code changes, but anticipate the Github action to do its testing.
Some of these commits should be flattened but perhaps not all of them, so we may not want to worry about that.

Remind me about the "warn" loglevel.

marohrdanz · 2024-03-14T15:08:29Z

Thanks for your input @ChrisWakefield

Do you have a specific question about the warn log level?

Maybe you're wondering why some log messages are warn when they seem more like errors? That is because the system administrators said they prefer these as warn because when they see error they think it's an application error.

marohrdanz and others added 30 commits August 4, 2023 14:33

Add winston logging package

94fa1fd

I find this easier to deal with than console logs when deploying to production environment. Also added a utility function so that JSON.stringify can be used w/o logging the bindCredentials or passwords.

Update node packages.

23ab09f

Remove winston logging

9f931fd

Because the ldapauth-fork and ldapjs use bunyan, switching to that for general logging.

Log failed attempts as 'warn'

85a4e4d

One school of thought is 'ERROR' is an application error. With that in mind changing these to 'WARN'. This will also make it a little easier to grep the logs and find failed login attempts.

Only log CN of group

5055670

This saves a little space and makes the log easier to read.

Remove log object from JSON.stringify function

6317d06

The log object creates a circular reference and breaks JSON.stringify. So masking it in the replacer function for JSON.stringify.

Tweak JSON.stringify of output of log object

bfafad7

Allow bind to LDAP with authenticating user

de49a5d

Adding the option to configure the server to bind to LDAP using the authenticating user instead of a specific service account. Addresses #12

Add nodemon to devDependencies

c5491ba

This is helpful in development, and will be used when DEBUG is set. Also removed the no-longer-used 'debug' setting.

Update node base image tag 17.4.0 -> 20.11.1-slim

7e59fe4

This eliminates a number of vulnerabilities found by docker scout.

Move app to subdirectory

ecda815

This is to avoid the `COPY . ./` in the Dockerfile.

Run container as non-root user

8898299

Build arguments with reasonable defaults used for non-root user. Addresses issue #14.

Bug fix: add missing -v in setconfig

48750fa

This should have been part of de49a5d.

Add SSL env to docker-compose.yml w/ default true

f1aa609

This env is used by index.js to determine if it should use http or httpS. Use this to determine if server should use http or httpS.

Add health check GET endpoint

93478e5

This addresses issue #13.

Bug fix: make bind/unbind async

6dc1c71

This should have been part of de49a5d.

Fix typos in log messages

a19ef8f

Refactor to move functions to utils.js

50e5463

Moving these functions just to make the index.js file simpler.

Convert function documentation to jsdoc standard

3508580

I like to make those pretty websites of API documentation.

Add devDependency ldap-server-mock

5a5e5fe

Adding this mock server to facilitate testing.

Add jest and initial tests

56c17d0

Refactor for clarity

27d309b

Moving the guts of the `/authenticate` and `/verify` routes to their own functions. This cleans up the logic in index.js and potentially makes things easier to test. Also did some refactoring within those handlers to make the code more clear, and added JSDoc.

Make URL base path configurable

b9bc893

This changes the hard-coded 'ldap-jwt' base path for the URL to be configurable. The default is 'ldap-jwt'.

Use two try/catch in verifyHandler

559440d

If the user sends a garbage token, e.g. something that can't even be decoded, we want to send back a 401 (not a 500). Therefore adding this first try/catch in verifyHandler.

Bug fix: functions no longer exported from utils

d47c2c0

userInAuhorizedGroups and getGroupCN used to be exported from utils.js, and when used in index.js, were referenced with `ut.`. But that's no longer the case. This change should have been included a few commits ago.

marohrdanz added 13 commits March 8, 2024 09:25

Use supertest to test application routes

914ad82

Use supertest to directly test the application's main routes. This addresses issue #10. Removed the now-superseded python tests.

Add intial CI workflow

17a4b71

Move mock ldap server into __tests__

7fd61fc

I had this at the top level to start automatically in development...but moving it into __tests_ to make things cleaner. If a developer wants to use it for their testing, it's easy enough for them to add a few lines of code to do so.

Add ci build target

0678ed6

Adding this to make it easier for people to run test suite.

Remove unused variable

ea67f0f

Run test suite on pushes to main and manually

94fdc8b

Update README

c64bef5

Run action on v1.3.0_mary branch

088f5cd

Trying to debug the workflow dispatch.

Remove workflow run on v1.3._mary

eb7d5e0

I had been wondering why the workflow dispatch wasn't showing up in the browser. It's because it only shows up if the workflow is in the default branch of the project. So it should show up once this is merged into main.

Bump version number: 1.2.0 -> 1.3.0

e527009

This version adds non-breaking features.

Chagne name of github action

ab2a044

This name just seems clearer

Refactor gently for clarity

7a6bf38

Tweaked code comments for clarity, added a variable or two to try and make the code easier to understand.

marohrdanz added this to the Key features for v1.3.0 milestone Mar 8, 2024

marohrdanz requested review from bmbroom and ChrisWakefield March 8, 2024 23:29

ChrisWakefield approved these changes Mar 14, 2024

View reviewed changes

bmbroom approved these changes Mar 14, 2024

View reviewed changes

marohrdanz merged commit 302451d into main Mar 14, 2024

marohrdanz deleted the v1.3.0_mary branch April 2, 2024 17:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Key issues for v1.3.0 #17

Key issues for v1.3.0 #17

marohrdanz commented Mar 8, 2024

ChrisWakefield left a comment

marohrdanz commented Mar 14, 2024

Key issues for v1.3.0 #17

Key issues for v1.3.0 #17

Conversation

marohrdanz commented Mar 8, 2024

ChrisWakefield left a comment

Choose a reason for hiding this comment

marohrdanz commented Mar 14, 2024