Skip to content
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

handle and show ldap errors #35

Merged
merged 10 commits into from
Aug 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions featureflags/graph/actions.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,11 @@ async def sign_in(
conn: SAConnection,
session: UserSession,
ldap: BaseLDAP,
) -> bool:
) -> tuple[bool, str | None]:
assert username and password, "Username and password are required"
if not await ldap.check_credentials(username, password):
return False
is_success, error_msg = await ldap.check_credentials(username, password)
if not is_success:
return False, error_msg

user_id = await get_auth_user(username, conn=conn)

Expand All @@ -68,7 +69,7 @@ async def sign_in(
)

session.sign_in(user_id, expiration_time)
return True
return True, None


@track
Expand Down
5 changes: 2 additions & 3 deletions featureflags/graph/graph.py
Original file line number Diff line number Diff line change
Expand Up @@ -505,16 +505,15 @@ async def sing_in(ctx: dict, options: dict) -> AuthResult:
return AuthResult("Password is required")

async with ctx[GraphContext.DB_ENGINE].acquire() as conn:
success = await actions.sign_in(
is_success, error_msg = await actions.sign_in(
username,
password,
conn=conn,
session=ctx[GraphContext.USER_SESSION],
ldap=ctx[GraphContext.LDAP_SERVICE],
)

error = "Invalid username or password" if not success else None
return AuthResult(error)
return AuthResult(error_msg)


@pass_context
Expand Down
27 changes: 19 additions & 8 deletions featureflags/services/ldap.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@
from string import Template

import ldap3
from ldap3.core.exceptions import LDAPBindError
from ldap3.core.exceptions import (
LDAPException,
LDAPInvalidCredentialsResult,
)

from featureflags.utils import escape_dn_chars

Expand All @@ -19,7 +22,7 @@ async def check_credentials(
*,
connect_timeout: int = 5,
receive_timeout: int = 5,
) -> bool:
) -> tuple[bool, str | None]:
raise NotImplementedError()


Expand All @@ -34,8 +37,8 @@ async def check_credentials(
*,
connect_timeout: int = 5,
receive_timeout: int = 5,
) -> bool:
return self.user_is_bound
) -> tuple[bool, str | None]:
return self.user_is_bound, None


class LDAP(BaseLDAP):
Expand All @@ -50,12 +53,13 @@ async def check_credentials(
*,
connect_timeout: int = 5,
receive_timeout: int = 5,
) -> bool:
) -> tuple[bool, str | None]:
server = ldap3.Server(self._host, connect_timeout=connect_timeout)

dn_tpl = Template(self._base_dn)
dn = dn_tpl.safe_substitute(user=escape_dn_chars(user))

error_msg = None
try:
with ldap3.Connection(
server=server,
Expand All @@ -68,8 +72,15 @@ async def check_credentials(
"LDAP -> Who am I: %s",
connection.extend.standard.who_am_i(),
)
except LDAPBindError:
except LDAPException as e:
user_is_bound = False
log.info("LDAP -> Bind error")
if type(e) is LDAPInvalidCredentialsResult:
error_msg = "Invalid username or password"
else:
try:
error_msg = getattr(e, "message")
except AttributeError:
error_msg = str(e)
log.error(f"LDAP -> Bind error: {error_msg}")

return user_is_bound
return user_is_bound, error_msg
4 changes: 2 additions & 2 deletions lets.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -82,15 +82,15 @@ commands:
- build-test
- postgres-test
cmd: |
docker-compose run --rm test
docker compose run --rm test

postgres:
description: Run postgres
cmd: docker-compose up postgres

postgres-test:
description: Run postgres test db
cmd: docker-compose up -d postgres-test
cmd: docker compose up -d postgres-test

apply-migrations-dev:
description: Apply migrations to local postgres
Expand Down
Loading