[PM-13008] Add ldap integration tests #637

eliykat · 2024-10-02T00:25:57Z

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-13008

See the parent ticket https://bitwarden.atlassian.net/browse/PM-13007 for additional context.

📔 Objective

Add integration tests for LdapDirectoryService.

We don't have any integration tests for our directory services, so this was a bit of trial and error, and is open to feedback about the best way to structure these.

The tests use jest because I still needed a test runner and I wanted to isolate LdapDirectoryService from the rest of the app and from the Bitwarden server. The integration is purely between our directory service implementation and the external directory service, which to me is the "here be dragons" part of our code at the moment.

This PR uses the OpenLdap docker image that we already recommend for local development. That's ideal because we can spin it up with seed data on demand. Additional integration tests will probably involve actual third party cloud services or Azure VMs which will require more investigation and setup.

Once this is merged I can remove the OpenLdap configuration from the server repository and update the contributing docs. It really belongs with this repo.

📸 Screenshots

⏰ Reminders before review

Contributor guidelines followed
All formatters and local linters executed and passed
Written new unit and / or integration tests where applicable
Used internationalization (i18n) for all UI strings
CI builds passed
Communicated to DevOps any deployment requirements
Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

👍 (:+1:) or similar for great changes
📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
❓ (:question:) for questions
🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
🎨 (:art:) for suggestions / improvements
❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
⛏ (:pick:) for minor or nitpick changes

codecov · 2024-10-02T00:27:41Z

Codecov Report

Attention: Patch coverage is 22.22222% with 63 lines in your changes missing coverage. Please review.

Project coverage is 8.62%. Comparing base (cf106b1) to head (bda1104).
Report is 17 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...ervices/ldap-directory.service.integration.spec.ts	0.00%	58 Missing ⚠️
src/models/groupEntry.ts	73.33%	4 Missing ⚠️
src/models/userEntry.ts	87.50%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##            main    #637      +/-   ##
========================================
+ Coverage   2.29%   8.62%   +6.33%     
========================================
  Files         59      60       +1     
  Lines       2573    2655      +82     
  Branches     467     475       +8     
========================================
+ Hits          59     229     +170     
+ Misses      2511    2403     -108     
- Partials       3      23      +20

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2024-10-02T00:29:46Z

Checkmarx One – Scan Summary & Details – 84abe62a-f346-4e99-8c51-dceab5071bd8

New Issues

Issue	Source File / Package	Checkmarx Insight
Container Capabilities Unrestricted	/docker-compose.yml: 2	Some capabilities are not needed in certain (or any) containers. Make sure that you only add capabilities that your container needs. Drop unnecessa...
Container Capabilities Unrestricted	/docker-compose.yml: 19	Some capabilities are not needed in certain (or any) containers. Make sure that you only add capabilities that your container needs. Drop unnecessa...
Healthcheck Not Set	/docker-compose.yml: 2	Check containers periodically to see if they are running properly.
Healthcheck Not Set	/docker-compose.yml: 19	Check containers periodically to see if they are running properly.
Privileged Ports Mapped In Container	/docker-compose.yml: 12	Privileged ports (1 to 1023) should not be mapped. Also you should drop net_bind_service linux capability from the container unless you absolutely ...
Privileged Ports Mapped In Container	/docker-compose.yml: 26	Privileged ports (1 to 1023) should not be mapped. Also you should drop net_bind_service linux capability from the container unless you absolutely ...
Security Opt Not Set	/docker-compose.yml: 19	Attribute 'security_opt' should be defined.
Security Opt Not Set	/docker-compose.yml: 2	Attribute 'security_opt' should be defined.
Use_Of_Hardcoded_Password	/src/services/ldap-directory.service.integration.spec.ts: 159	Attack Vector

Testing based on last modified date is probably too difficult to scale once we start using cloud directory services

eliykat · 2024-10-02T02:00:56Z

.github/workflows/integration-test.yml

I'm not sure whether this should be separate to the existing test.yml file. As we add more integration tests it seems useful to keep it separate, but I'm not sure how this affects code coverage reporting.

As it stands this is mostly a copy/paste of test.yml.

We may want to use https://github.com/dorny/paths-filter down the line to selectively run different tests based on which directory service was changed. (The standard path filter only applies to the whole workflow, not steps within it.)

eliykat · 2024-10-02T02:05:18Z

src/models/groupEntry.ts

The toJSON/fromJSON methods were added as helpers to instantiate the fixture data. It's maybe a bit weird to add this to runtime code, but the Jsonify type is based on the toJSON return type, so it was the tidiest and least verbose way to add type safety and factory methods to the fixture data.

eliykat · 2024-10-02T02:07:01Z

src/services/ldap-directory.service.integration.spec.ts

+  beforeEach(() => {
+    logService = mock();
+    i18nService = mock();
+    stateService = mock();


Future refactor: it would be good to remove stateService from the directory service implementations and pass in their config as arguments instead.

eliykat · 2024-10-02T02:19:16Z

src/services/ldap-directory.service.integration.spec.ts

+      getLdapConfiguration({
+        ssl: true,
+        startTls: true,
+        sslAllowUnauthorized: true, // TODO: this could be more robust if we configured certs correctly
+      }),


This is a bit of a half baked SSL config at the moment, I just wanted something in place. Ideally we'd specify a certificate and not use sslAllowUnauthorized.

…/directory-connector into ldap-integration-tests

.github/workflows/integration-test.yml

addisonbeck · 2024-10-04T10:19:40Z

.github/workflows/integration-test.yml

+      # Tests in apps/ are typechecked when their app is built, so we just do it here for libs/
+      # See https://bitwarden.atlassian.net/browse/EC-497
+      - name: Run typechecking
+        run: npm run test:types --coverage


You probably don't need this, it's not required to run tests and is already done in the main test job.

Fixed in bda1104.

addisonbeck · 2024-10-04T10:20:01Z

utils/integration-tests/.env

@@ -0,0 +1 @@
+COMPOSE_PROJECT_NAME=directory-connector


What's this for?

It gives a nice name to the grouping in docker desktop:

Does it make more sense to set this in docker-compose.yml? Docker's documentation says it will take the top level name value here.

Also, if you're planning to use this as the scaffolding location for local development containers it doesn't make sense to put it in the integration-tests folder. I do think it's reasonable to put docker-compose.yml (and .env if you keep it) in project root.

eliykat · 2024-10-07T04:01:45Z

I realised belatedly that the OpenLDAP Docker image is unmaintained since ~3 years ago: https://github.com/osixia/docker-openldap.

I'll look at changing it out for https://hub.docker.com/r/bitnami/openldap, which appears maintained and published by Vmware. Other suggestions are welcome.

eliykat added 16 commits October 1, 2024 07:57

Add basic ldap user sync integration test

3369f83

Add docker config and npm run script

47d6b09

Refactor test setup

5cdeef2

add phpldapadmin to docker compose

262c7a3

Tweak default config

e093091

Use full 20 users

b6952c9

Add groups test

b4f1090

Fix json implementation

ccce372

Add user filter test

9776b41

Add groups filter test

edf4795

Add path tests

c54b126

Group related tests

8b5f6ba

Stub out TODO tests

3be14fc

Reorder directories and fix jest config

4ec99ff

Initial attempt at Github Workflow

16c6ad2

Use separate workflow

8613d53

eliykat added 3 commits October 2, 2024 11:06

basic TLS test

88d3f40

Remove cert

fc29b00

Remove TODO test

31ca7a2

Testing based on last modified date is probably too difficult to scale once we start using cloud directory services

eliykat requested a review from addisonbeck October 2, 2024 01:43

eliykat added 3 commits October 2, 2024 11:48

Final tweaks

08f8f7b

Final tweaks

897fa64

Add missing type

d8f4039

eliykat commented Oct 2, 2024

View reviewed changes

eliykat marked this pull request as ready for review October 2, 2024 02:07

eliykat requested a review from a team as a code owner October 2, 2024 02:07

Merge branch 'main' into ldap-integration-tests

fc57f4e

eliykat commented Oct 2, 2024

View reviewed changes

eliykat added 4 commits October 2, 2024 12:34

Address security feedback

c909c55

Merge branch 'ldap-integration-tests' of https://github.com/bitwarden…

5b5d7dd

…/directory-connector into ldap-integration-tests

Attempt to fix CI

35fecbc

Add host interface specification

5d2616a

addisonbeck reviewed Oct 2, 2024

View reviewed changes

.github/workflows/integration-test.yml Show resolved Hide resolved

addisonbeck reviewed Oct 4, 2024

View reviewed changes

Remove unnecessary typechecking

bda1104

eliykat requested a review from addisonbeck October 7, 2024 03:33

eliykat added the hold do not merge, do not approve yet label Oct 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PM-13008] Add ldap integration tests #637

[PM-13008] Add ldap integration tests #637

eliykat commented Oct 2, 2024 •

edited

Loading

codecov bot commented Oct 2, 2024 •

edited

Loading

github-actions bot commented Oct 2, 2024 •

edited

Loading

eliykat Oct 2, 2024

eliykat Oct 2, 2024

eliykat Oct 2, 2024

eliykat Oct 2, 2024

addisonbeck Oct 4, 2024

eliykat Oct 7, 2024

addisonbeck Oct 4, 2024

eliykat Oct 7, 2024

addisonbeck Oct 7, 2024 •

edited

Loading

eliykat commented Oct 7, 2024 •

edited

Loading

		@@ -0,0 +1 @@
		COMPOSE_PROJECT_NAME=directory-connector

[PM-13008] Add ldap integration tests #637

Are you sure you want to change the base?

[PM-13008] Add ldap integration tests #637

Conversation

eliykat commented Oct 2, 2024 • edited Loading

🎟️ Tracking

📔 Objective

📸 Screenshots

⏰ Reminders before review

🦮 Reviewer guidelines

codecov bot commented Oct 2, 2024 • edited Loading

Codecov Report

github-actions bot commented Oct 2, 2024 • edited Loading

New Issues

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addisonbeck Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

eliykat commented Oct 7, 2024 • edited Loading

eliykat commented Oct 2, 2024 •

edited

Loading

codecov bot commented Oct 2, 2024 •

edited

Loading

github-actions bot commented Oct 2, 2024 •

edited

Loading

addisonbeck Oct 7, 2024 •

edited

Loading

eliykat commented Oct 7, 2024 •

edited

Loading