-
Notifications
You must be signed in to change notification settings - Fork 82
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
[PM-13008] Add ldap integration tests #637
base: main
Are you sure you want to change the base?
Changes from all commits
3369f83
47d6b09
5cdeef2
262c7a3
e093091
b6952c9
b4f1090
ccce372
9776b41
edf4795
c54b126
8b5f6ba
3be14fc
4ec99ff
16c6ad2
8613d53
88d3f40
fc29b00
31ca7a2
08f8f7b
897fa64
d8f4039
fc57f4e
c909c55
5b5d7dd
35fecbc
5d2616a
bda1104
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,92 @@ | ||
name: Integration Testing | ||
|
||
on: | ||
workflow_dispatch: | ||
push: | ||
branches: | ||
- "main" | ||
paths: | ||
- ".github/workflows/integration-test.yml" # this file | ||
- "src/services/ldap-directory.service*" # we only have integration for LDAP testing at the moment | ||
pull_request: | ||
paths: | ||
- ".github/workflows/integration-test.yml" # this file | ||
addisonbeck marked this conversation as resolved.
Show resolved
Hide resolved
|
||
- "src/services/ldap-directory.service*" # we only have integration for LDAP testing at the moment | ||
|
||
jobs: | ||
check-test-secrets: | ||
name: Check for test secrets | ||
runs-on: ubuntu-22.04 | ||
outputs: | ||
available: ${{ steps.check-test-secrets.outputs.available }} | ||
permissions: | ||
contents: read | ||
|
||
steps: | ||
- name: Check | ||
id: check-test-secrets | ||
run: | | ||
if [ "${{ secrets.CODECOV_TOKEN }}" != '' ]; then | ||
echo "available=true" >> $GITHUB_OUTPUT; | ||
else | ||
echo "available=false" >> $GITHUB_OUTPUT; | ||
fi | ||
|
||
testing: | ||
name: Run tests | ||
if: ${{ startsWith(github.head_ref, 'version_bump_') == false }} | ||
runs-on: ubuntu-22.04 | ||
needs: check-test-secrets | ||
permissions: | ||
checks: write | ||
contents: read | ||
pull-requests: write | ||
|
||
steps: | ||
- name: Check out repo | ||
uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7 | ||
|
||
- name: Get Node version | ||
id: retrieve-node-version | ||
run: | | ||
NODE_NVMRC=$(cat .nvmrc) | ||
NODE_VERSION=${NODE_NVMRC/v/''} | ||
echo "node_version=$NODE_VERSION" >> $GITHUB_OUTPUT | ||
|
||
- name: Set up Node | ||
uses: actions/setup-node@60edb5dd545a775178f52524783378180af0d1f8 # v4.0.2 | ||
with: | ||
cache: 'npm' | ||
cache-dependency-path: '**/package-lock.json' | ||
node-version: ${{ steps.retrieve-node-version.outputs.node_version }} | ||
|
||
- name: Install Node dependencies | ||
run: npm ci | ||
|
||
- name: Start OpenLDAP docker container | ||
run: docker compose --project-directory utils/integration-tests --profile server up -d | ||
|
||
- name: Run integration tests | ||
run: | | ||
npm run test:integration --coverage | ||
|
||
- name: Report test results | ||
uses: dorny/test-reporter@31a54ee7ebcacc03a09ea97a7e5465a47b84aea5 # v1.9.1 | ||
if: ${{ needs.check-test-secrets.outputs.available == 'true' && !cancelled() }} | ||
with: | ||
name: Test Results | ||
path: "junit.xml" | ||
reporter: jest-junit | ||
fail-on-error: true | ||
|
||
- name: Upload coverage to codecov.io | ||
uses: codecov/codecov-action@e28ff129e5465c2c0dcc6f003fc735cb6ae0c673 # v4.5.0 | ||
if: ${{ needs.check-test-secrets.outputs.available == 'true' }} | ||
env: | ||
CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} | ||
|
||
- name: Upload results to codecov.io | ||
uses: codecov/test-results-action@1b5b448b98e58ba90d1a1a1d9fcb72ca2263be46 # v1.0.0 | ||
if: ${{ needs.check-test-secrets.outputs.available == 'true' }} | ||
env: | ||
CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,191 @@ | ||
import { mock, MockProxy } from "jest-mock-extended"; | ||
|
||
import { I18nService } from "../../jslib/common/src/abstractions/i18n.service"; | ||
import { LogService } from "../../jslib/common/src/abstractions/log.service"; | ||
import { groupFixtures } from "../../utils/integration-tests/group-fixtures"; | ||
import { userFixtures } from "../../utils/integration-tests/user-fixtures"; | ||
import { DirectoryType } from "../enums/directoryType"; | ||
import { LdapConfiguration } from "../models/ldapConfiguration"; | ||
import { SyncConfiguration } from "../models/syncConfiguration"; | ||
|
||
import { LdapDirectoryService } from "./ldap-directory.service"; | ||
import { StateService } from "./state.service"; | ||
|
||
// These tests integrate with the OpenLDAP docker image and seed data located in utils/integration-tests. | ||
// To start the docker container for local testing: | ||
// cd utils/integration-tests | ||
// docker compose --profile server up -d | ||
// Once the docker container is running, these tests can be run using: | ||
// npm run test:integration:watch | ||
// The docker compose config also includes a PhpLDAPAdmin container for inspecting the LDAP directory contents. | ||
|
||
describe("ldapDirectoryService", () => { | ||
let logService: MockProxy<LogService>; | ||
let i18nService: MockProxy<I18nService>; | ||
let stateService: MockProxy<StateService>; | ||
|
||
let directoryService: LdapDirectoryService; | ||
|
||
beforeEach(() => { | ||
logService = mock(); | ||
i18nService = mock(); | ||
stateService = mock(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Future refactor: it would be good to remove |
||
|
||
stateService.getDirectoryType.mockResolvedValue(DirectoryType.Ldap); | ||
stateService.getLastUserSync.mockResolvedValue(null); // do not filter results by last modified date | ||
i18nService.t.mockImplementation((id) => id); // passthrough implementation for any error messages | ||
|
||
directoryService = new LdapDirectoryService(logService, i18nService, stateService); | ||
}); | ||
|
||
it("gets users and groups without TLS", async () => { | ||
stateService.getDirectory | ||
.calledWith(DirectoryType.Ldap) | ||
.mockResolvedValue(getLdapConfiguration()); | ||
stateService.getSync.mockResolvedValue(getSyncConfiguration({ groups: true, users: true })); | ||
|
||
const result = await directoryService.getEntries(true, true); | ||
expect(result).toEqual([groupFixtures, userFixtures]); | ||
}); | ||
|
||
it("gets users and groups with TLS", async () => { | ||
stateService.getDirectory.calledWith(DirectoryType.Ldap).mockResolvedValue( | ||
getLdapConfiguration({ | ||
ssl: true, | ||
startTls: true, | ||
sslAllowUnauthorized: true, // TODO: this could be more robust if we configured certs correctly | ||
}), | ||
Comment on lines
+53
to
+57
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
); | ||
stateService.getSync.mockResolvedValue(getSyncConfiguration({ groups: true, users: true })); | ||
|
||
const result = await directoryService.getEntries(true, true); | ||
expect(result).toEqual([groupFixtures, userFixtures]); | ||
}); | ||
|
||
describe("users", () => { | ||
it("respects the users path", async () => { | ||
stateService.getDirectory | ||
.calledWith(DirectoryType.Ldap) | ||
.mockResolvedValue(getLdapConfiguration()); | ||
stateService.getSync.mockResolvedValue( | ||
getSyncConfiguration({ | ||
users: true, | ||
userPath: "ou=Human Resources", | ||
}), | ||
); | ||
|
||
// These users are in the Human Resources ou | ||
const hrUsers = userFixtures.filter( | ||
(u) => | ||
u.referenceId === "cn=Roland Dyke,ou=Human Resources,dc=bitwarden,dc=com" || | ||
u.referenceId === "cn=Charin Goulfine,ou=Human Resources,dc=bitwarden,dc=com" || | ||
u.referenceId === "cn=Angelle Guarino,ou=Human Resources,dc=bitwarden,dc=com", | ||
); | ||
|
||
const result = await directoryService.getEntries(true, true); | ||
expect(result[1]).toEqual(expect.arrayContaining(hrUsers)); | ||
expect(result[1].length).toEqual(hrUsers.length); | ||
}); | ||
|
||
it("filters users", async () => { | ||
stateService.getDirectory | ||
.calledWith(DirectoryType.Ldap) | ||
.mockResolvedValue(getLdapConfiguration()); | ||
stateService.getSync.mockResolvedValue( | ||
getSyncConfiguration({ users: true, userFilter: "(cn=Roland Dyke)" }), | ||
); | ||
|
||
const roland = userFixtures.find( | ||
(u) => u.referenceId === "cn=Roland Dyke,ou=Human Resources,dc=bitwarden,dc=com", | ||
); | ||
const result = await directoryService.getEntries(true, true); | ||
expect(result).toEqual([undefined, [roland]]); | ||
}); | ||
}); | ||
|
||
describe("groups", () => { | ||
it("respects the groups path", async () => { | ||
stateService.getDirectory | ||
.calledWith(DirectoryType.Ldap) | ||
.mockResolvedValue(getLdapConfiguration()); | ||
stateService.getSync.mockResolvedValue( | ||
getSyncConfiguration({ | ||
groups: true, | ||
groupPath: "ou=Janitorial", | ||
}), | ||
); | ||
|
||
// These groups are in the Janitorial ou | ||
const janitorialGroups = groupFixtures.filter((g) => g.name === "Cleaners"); | ||
|
||
const result = await directoryService.getEntries(true, true); | ||
expect(result).toEqual([janitorialGroups, undefined]); | ||
}); | ||
|
||
it("filters groups", async () => { | ||
stateService.getDirectory | ||
.calledWith(DirectoryType.Ldap) | ||
.mockResolvedValue(getLdapConfiguration()); | ||
stateService.getSync.mockResolvedValue( | ||
getSyncConfiguration({ groups: true, groupFilter: "(cn=Red Team)" }), | ||
); | ||
|
||
const redTeam = groupFixtures.find( | ||
(u) => u.referenceId === "cn=Red Team,dc=bitwarden,dc=com", | ||
); | ||
const result = await directoryService.getEntries(true, true); | ||
expect(result).toEqual([[redTeam], undefined]); | ||
}); | ||
}); | ||
}); | ||
|
||
/** | ||
* @returns a basic ldap configuration without TLS/SSL enabled. Can be overridden by passing in a partial configuration. | ||
*/ | ||
const getLdapConfiguration = (config?: Partial<LdapConfiguration>): LdapConfiguration => ({ | ||
ssl: false, | ||
startTls: false, | ||
tlsCaPath: null, | ||
sslAllowUnauthorized: false, | ||
sslCertPath: null, | ||
sslKeyPath: null, | ||
sslCaPath: null, | ||
hostname: "127.0.0.1", | ||
port: 1389, | ||
domain: null, | ||
rootPath: "dc=bitwarden,dc=com", | ||
currentUser: false, | ||
username: "cn=admin,dc=bitwarden,dc=com", | ||
password: "admin", | ||
ad: false, | ||
pagedSearch: false, | ||
...(config ?? {}), | ||
}); | ||
|
||
/** | ||
* @returns a basic sync configuration. Can be overridden by passing in a partial configuration. | ||
*/ | ||
const getSyncConfiguration = (config?: Partial<SyncConfiguration>): SyncConfiguration => ({ | ||
users: false, | ||
groups: false, | ||
interval: 5, | ||
userFilter: null, | ||
groupFilter: null, | ||
removeDisabled: false, | ||
overwriteExisting: false, | ||
largeImport: false, | ||
// Ldap properties | ||
groupObjectClass: "posixGroup", | ||
userObjectClass: "person", | ||
groupPath: null, | ||
userPath: null, | ||
groupNameAttribute: "cn", | ||
userEmailAttribute: "mail", | ||
memberAttribute: "memberUid", | ||
useEmailPrefixSuffix: false, | ||
emailPrefixAttribute: "sAMAccountName", | ||
emailSuffix: null, | ||
creationDateAttribute: "whenCreated", | ||
revisionDateAttribute: "whenChanged", | ||
...(config ?? {}), | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
COMPOSE_PROJECT_NAME=directory-connector | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's this for? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it make more sense to set this in 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)