Skip to content

Commit 458ba27

Browse files
quaarkquaark
authored
[Auth] Fix Service Account Configuration Validation Logic (mlrun#9250)
### 📝 Description The logic was written incorrectly, so it missed some edge cases (The parenthesis of a compound `if` statement were in the wrong place). This PR separates this check from the main method to enhance testability and maintainability while fixing the logic. --- ### 🛠️ Changes Made - Moved the service account configuration validation logic to a separate method - Fixed the logic in the separate method - Added extensive unit testing for all edge cases --- ### ✅ Checklist - [x] I have tested the changes in this PR --- ### 🧪 Testing - Added extensive unit testing to the logic --- ### 🔗 References - Ticket link: https://iguazio.atlassian.net/browse/IG4-1258 - Design docs links: - External links: --- ### 🚨 Breaking Changes? - [ ] Yes (explain below) - [x] No <!-- If yes, describe what needs to be changed downstream: -->
1 parent 63611fb commit 458ba27

File tree

2 files changed

+218
-17
lines changed

2 files changed

+218
-17
lines changed

‎server/py/framework/api/utils.py‎

Lines changed: 35 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -836,23 +836,9 @@ def resolve_project_service_account_details(
836836
default_service_account or mlrun.mlconf.function.spec.service_account.default
837837
)
838838

839-
# Sanity check on project configuration
840-
if (
841-
default_service_account
842-
and (
843-
allowed_service_accounts
844-
and default_service_account not in allowed_service_accounts
845-
)
846-
or (
847-
forbidden_service_accounts
848-
and default_service_account in forbidden_service_accounts
849-
)
850-
):
851-
raise mlrun.errors.MLRunInvalidArgumentError(
852-
f"Default service account {default_service_account} is not in list of allowed "
853-
+ f"service accounts {allowed_service_accounts} or is in the list of forbidden service accounts "
854-
+ f"{forbidden_service_accounts}"
855-
)
839+
_validate_service_account_details(
840+
default_service_account, allowed_service_accounts, forbidden_service_accounts
841+
)
856842

857843
return allowed_service_accounts, forbidden_service_accounts, default_service_account
858844

@@ -1392,3 +1378,35 @@ async def update_function(function):
13921378

13931379
tasks = [update_function(function) for function in functions]
13941380
await asyncio.gather(*tasks)
1381+
1382+
1383+
def _validate_service_account_details(
1384+
default_service_account: str,
1385+
allowed_service_accounts: typing.Optional[list[str]],
1386+
forbidden_service_accounts: typing.Optional[list[str]],
1387+
):
1388+
"""
1389+
Sanity check on project configuration.
1390+
Make sure the default service account is in the allowed list and not in the forbidden list if such lists exist.
1391+
1392+
:param default_service_account: The default service account name.
1393+
:param allowed_service_accounts: List of allowed service accounts.
1394+
:param forbidden_service_accounts: List of forbidden service accounts.
1395+
1396+
:raises MLRunInvalidArgumentError: In case of misconfiguration.
1397+
"""
1398+
if default_service_account and (
1399+
(
1400+
allowed_service_accounts
1401+
and default_service_account not in allowed_service_accounts
1402+
)
1403+
or (
1404+
forbidden_service_accounts
1405+
and default_service_account in forbidden_service_accounts
1406+
)
1407+
):
1408+
raise mlrun.errors.MLRunInvalidArgumentError(
1409+
f"Default service account {default_service_account} is not in list of allowed "
1410+
+ f"service accounts {allowed_service_accounts} or is in the list of forbidden service accounts "
1411+
+ f"{forbidden_service_accounts}"
1412+
)

‎server/py/services/api/tests/unit/api/test_utils.py‎

Lines changed: 183 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,9 @@
1515
import base64
1616
import json
1717
import pathlib
18+
import typing
1819
import unittest.mock
20+
from contextlib import nullcontext as does_not_raise
1921
from http import HTTPStatus
2022
from unittest.mock import AsyncMock, MagicMock, patch
2123

@@ -2098,3 +2100,184 @@ def test_resolve_auth_secret_name(
20982100
user_id="test-user",
20992101
token_name=expected_token_name,
21002102
)
2103+
2104+
2105+
@pytest.mark.parametrize(
2106+
"allowed_secret, expected_allowed",
2107+
[
2108+
(None, None),
2109+
("sa-allowed-1,sa-allowed-2", ["sa-allowed-1", "sa-allowed-2"]),
2110+
],
2111+
)
2112+
@pytest.mark.parametrize(
2113+
"forbidden_secret, forbidden_conf, auth_info, expected_forbidden",
2114+
[
2115+
# No forbidden service accounts
2116+
(None, "", mlrun.common.schemas.AuthInfo(), []),
2117+
# Forbidden from conf only
2118+
(
2119+
[],
2120+
"sa-forbidden-1,sa-forbidden-2",
2121+
mlrun.common.schemas.AuthInfo(),
2122+
["sa-forbidden-1", "sa-forbidden-2"],
2123+
),
2124+
# Forbidden from secret only
2125+
(
2126+
"sa-forbidden-1,sa-forbidden-2",
2127+
"",
2128+
mlrun.common.schemas.AuthInfo(),
2129+
["sa-forbidden-1", "sa-forbidden-2"],
2130+
),
2131+
# Secret extends conf
2132+
(
2133+
"sa-forbidden-3,sa-forbidden-4",
2134+
"sa-forbidden-1,sa-forbidden-2",
2135+
mlrun.common.schemas.AuthInfo(),
2136+
["sa-forbidden-1", "sa-forbidden-2", "sa-forbidden-3", "sa-forbidden-4"],
2137+
),
2138+
# filter out auth info service accounts
2139+
(
2140+
"sa-forbidden-1,sa-forbidden-2",
2141+
"",
2142+
mlrun.common.schemas.AuthInfo(
2143+
username="sa-forbidden-1",
2144+
kind=mlrun.common.schemas.AuthInfoKind.service_account,
2145+
),
2146+
["sa-forbidden-2"],
2147+
),
2148+
],
2149+
)
2150+
@pytest.mark.parametrize(
2151+
"default_secret, default_conf, expected_default",
2152+
[
2153+
# Default from conf
2154+
(
2155+
None,
2156+
"sa-default-conf",
2157+
"sa-default-conf",
2158+
),
2159+
# Default from secret
2160+
(
2161+
"sa-default-secret",
2162+
"",
2163+
"sa-default-secret",
2164+
),
2165+
# Secret overrides conf
2166+
(
2167+
"sa-default-secret",
2168+
"sa-default-conf",
2169+
"sa-default-secret",
2170+
),
2171+
# No default
2172+
(
2173+
None,
2174+
"",
2175+
"",
2176+
),
2177+
],
2178+
)
2179+
def test_resolve_project_service_account_details(
2180+
allowed_secret: list[str],
2181+
expected_allowed: list[str],
2182+
forbidden_secret: list[str],
2183+
forbidden_conf: str,
2184+
auth_info: mlrun.common.schemas.AuthInfo,
2185+
expected_forbidden: list[str],
2186+
default_secret: str,
2187+
default_conf: str,
2188+
expected_default: str,
2189+
):
2190+
mlrun.mlconf.function.spec.service_account.forbidden_service_accounts = (
2191+
forbidden_conf
2192+
)
2193+
mlrun.mlconf.function.spec.service_account.default = default_conf
2194+
2195+
def _mock_generate_client_project_secret_key(_, name):
2196+
return name
2197+
2198+
def _mock_get_project_secret(
2199+
project: str,
2200+
provider: mlrun.common.schemas.SecretProviderName,
2201+
secret_key: str,
2202+
token: typing.Optional[str] = None,
2203+
allow_secrets_from_k8s: bool = False,
2204+
allow_internal_secrets: bool = False,
2205+
key_map_secret_key: typing.Optional[str] = None,
2206+
):
2207+
return {
2208+
"allowed": allowed_secret,
2209+
"forbidden": forbidden_secret,
2210+
"default": default_secret,
2211+
}[secret_key]
2212+
2213+
mock_secrets_crud = unittest.mock.Mock()
2214+
mock_secrets_crud.generate_client_project_secret_key = (
2215+
_mock_generate_client_project_secret_key
2216+
)
2217+
mock_secrets_crud.get_project_secret = _mock_get_project_secret
2218+
2219+
# logic from _validate_service_account_details, to determine if we expect an exception,
2220+
# this is tested in a separate test below: test_test_resolve_project_service_account_details_validity
2221+
expectation = does_not_raise()
2222+
if expected_default and (
2223+
(expected_allowed and expected_default not in expected_allowed)
2224+
or (expected_forbidden and expected_default in expected_forbidden)
2225+
):
2226+
expectation = pytest.raises(mlrun.errors.MLRunInvalidArgumentError)
2227+
2228+
with patch(
2229+
"services.api.crud.secrets.Secrets",
2230+
return_value=mock_secrets_crud,
2231+
):
2232+
with expectation:
2233+
(
2234+
resolved_allowed,
2235+
resolved_forbidden,
2236+
resolved_default,
2237+
) = framework.api.utils.resolve_project_service_account_details(
2238+
project_name="test-project",
2239+
auth_info=auth_info,
2240+
)
2241+
2242+
assert resolved_allowed == expected_allowed
2243+
assert resolved_forbidden == expected_forbidden
2244+
assert resolved_default == expected_default
2245+
2246+
2247+
@pytest.mark.parametrize(
2248+
"allowed, forbidden, default, expectation",
2249+
[
2250+
# Empty values (should pass)
2251+
([], [], "", does_not_raise()),
2252+
([], [], None, does_not_raise()),
2253+
# Empty lists, default set (should pass)
2254+
([], [], "sa-default", does_not_raise()),
2255+
# Default in allowed (should pass)
2256+
(["sa-1", "sa-default"], [], "sa-default", does_not_raise()),
2257+
# Default not in forbidden (should pass)
2258+
([], ["sa-2", "sa-3"], "sa-default", does_not_raise()),
2259+
# Default not in allowed (should raise)
2260+
(
2261+
["sa-1", "sa-2"],
2262+
[],
2263+
"sa-default",
2264+
pytest.raises(mlrun.errors.MLRunInvalidArgumentError),
2265+
),
2266+
# Default in forbidden (should raise)
2267+
(
2268+
[],
2269+
["sa-default", "sa-2"],
2270+
"sa-default",
2271+
pytest.raises(mlrun.errors.MLRunInvalidArgumentError),
2272+
),
2273+
],
2274+
)
2275+
def test_test_resolve_project_service_account_details_validity(
2276+
allowed, forbidden, default, expectation
2277+
):
2278+
with expectation:
2279+
framework.api.utils._validate_service_account_details(
2280+
allowed_service_accounts=allowed,
2281+
forbidden_service_accounts=forbidden,
2282+
default_service_account=default,
2283+
)

0 commit comments

Comments
 (0)