LDAP Improvements and E2E testing #2199
Conversation
cmintey
force-pushed
the
fix/ldap-starttls
branch
from
8da400d to
9773f06
Compare
cmintey
changed the title
cmintey
force-pushed
the
fix/ldap-starttls
branch
from
9773f06 to
6b5815b
Compare
cmintey
changed the title
| @@ -82,5 +88,17 @@ jobs: | |||
| env: | |||
| DB_ENGINE: ${{ matrix.Database }} | |||
| POSTGRES_SERVER: localhost | |||
| LDAP_AUTH_ENABLED: True | |||
| LDAP_SERVER_URL: ldap://localhost:10389 | |||
There was a problem hiding this comment.
| LDAP_SERVER_URL: ldap://localhost:10389 | |
| LDAP_SERVER_URL: ldap://ldap-service:10389 |
I think you can set it to the name of the service.
There was a problem hiding this comment.
The reason I used localhost instead of the service name is mainly to keep it consistent with how the Postgres service is being used above
| @@ -27,6 +27,12 @@ jobs: | |||
| options: --health-cmd pg_isready --health-interval 10s --health-timeout 5s --health-retries 5 | |||
| ports: | |||
| - 5432:5432 | |||
| ldap: | |||
There was a problem hiding this comment.
| ldap: | |
| ldap-service: |
Just suggesting that the name match my suggestion on line 92.
| ports: | ||
| - 10389:10389 | ||
| - 10636:10636 |
There was a problem hiding this comment.
| ports: | |
| - 10389:10389 | |
| - 10636:10636 |
You shouldn't have to expose any ports if you use the service name (similar to how you can do in docker-compose).
| @@ -57,3 +58,192 @@ def test_user_lockout_after_bad_attemps(api_client: TestClient, unique_user: Tes | |||
| user_service = UserService(database) | |||
| user = database.users.get_one(unique_user.user_id) | |||
| user_service.unlock_user(user) | |||
|
|
|||
|
|
|||
| @pytest.mark.skipif(not os.environ.get("GITHUB_ACTIONS", False), reason="requires ldap service in github actions") | |||
There was a problem hiding this comment.
This makes it impossible to test locally. Is there a reason you can't set it to skip unless LDAP_SERVER_URL is set in the environment?
There was a problem hiding this comment.
The reason it's like that and not just checking for the LDAP url variable is because the tests rely on the specific docker image that is spun up during the tests. So just setting your LDAP url, the tests would likely fail since it wouldn't be the correct LDAP url and wouldn't have the correct users the tests expect.
You can still test it locally by setting GITHUB_ACTIONS=true in your environment. Then you'd need to spin up the docker image and point your LDAP environment variables to that docker image. Same process as in the GitHub workflow.
Amazing. I really really really really appreciate all the work you've been putting into the LDAP integration lately. It's super impressive! |
What type of PR is this?
What this PR does / why we need it:
LDAP_QUERY_BINDandLDAP_QUERY_PASSWORDenv variables toNoneid_attributeandmail_attributebe implied inLDAP_USER_FILTER. This now makesLDAP_USER_FILTERoptional. This change maintains compatibility with previous changes.Which issue(s) this PR fixes:
Fixes #2185
Fixes #2218
Fixes #2228
Special notes for your reviewer:
This PR also introduces end-to-end tests for LDAP as discussed here. Instead of GLAuth, I found this awesome docker image that comes pre-baked with users as well as support for STARTTLS (which GLAuth did not support).
Testing
Release Notes