Auto create SSO groups feature flag#8515
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds automatic SSO-driven account group provisioning and related configuration. Introduces 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can customize the tone of the review comments and chat replies.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
backend/tests/unit/test_sso_generate_groups.py (1)
21-62: Consider using pytest fixtures or monkeypatch for config cleanup.The try/finally pattern for restoring config values works but is verbose and error-prone if an exception occurs before the try block. Using
monkeypatch.setattrwould automatically restore values after each test.♻️ Example using monkeypatch
async def test_groups_not_created_when_disabled( self, db: InfrahubDatabase, default_branch: Branch, register_core_models_schema, existing_group, monkeypatch ) -> None: """Verify that groups are NOT created when sso_generate_groups is disabled (default)""" monkeypatch.setattr(config.SETTINGS.security, "sso_generate_groups", False) await signin_sso_account( db=db, account_name="test-user", sso_groups=["existing-group", "new-group-1", "new-group-2"] ) groups = await NodeManager.query(db=db, schema=CoreAccountGroup) group_names = {g.name.value for g in groups} assert "existing-group" in group_names assert "new-group-1" not in group_names assert "new-group-2" not in group_names🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/unit/test_sso_generate_groups.py` around lines 21 - 62, Replace the try/finally config toggling in tests test_groups_not_created_when_disabled and test_groups_created_when_enabled by using pytest's monkeypatch to set and auto-restore config.SETTINGS.security.sso_generate_groups; specifically, add the monkeypatch fixture to each test signature and call monkeypatch.setattr(config.SETTINGS.security, "sso_generate_groups", False) (or True) before calling signin_sso_account so the original value is restored automatically instead of manually saving/restoring in the test.backend/tests/functional/api/test_sso_groups.py (1)
19-25: Inconsistent schema usage in fixture.The fixture uses
CoreAccountGroup(a protocol) as the schema, butauth.pyand the unit tests useInfrahubKind.ACCOUNTGROUP. Consider usingInfrahubKind.ACCOUNTGROUPfor consistency.♻️ Proposed fix for consistency
+from infrahub.core.constants import InfrahubKind + class TestSSOAutoGroupCreation: `@pytest.fixture` async def existing_group(self, db: InfrahubDatabase) -> CoreAccountGroup: """Create an existing group for testing.""" - group = await Node.init(db=db, schema=CoreAccountGroup) + group = await Node.init(db=db, schema=InfrahubKind.ACCOUNTGROUP) await group.new(db=db, name="ExistingGroup") await group.save(db=db) return group🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/functional/api/test_sso_groups.py` around lines 19 - 25, The existing_group fixture uses the protocol type CoreAccountGroup as the Node.init schema which is inconsistent with other code; update the fixture to call Node.init with schema=InfrahubKind.ACCOUNTGROUP (and adjust any type hints/returns if necessary) so Node.init(db=db, schema=InfrahubKind.ACCOUNTGROUP) is used instead of schema=CoreAccountGroup, keeping the fixture name existing_group and the surrounding calls (new, save) unchanged.docs/docs/guides/sso.mdx (1)
447-447: Consider renaming to avoid step number confusion.The main SSO setup guide already has "Step 4: Validate the SSO configuration" at line 300. This nested "Step 4" within the "Group mapping" subsection could confuse readers. Consider renaming to something like "Enable automatic group creation (optional)" without the "Step 4" prefix, or use a different numbering scheme for the group mapping substeps.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/docs/guides/sso.mdx` at line 447, The heading "#### Step 4: Enable automatic group creation (optional)" conflicts with the main guide's "Step 4: Validate the SSO configuration"; update the group mapping subsection header to remove the "Step 4:" prefix or use a distinct substep label (e.g., "Enable automatic group creation (optional)" or "Group mapping — Step 1") so numbering doesn't collide, and ensure any related internal anchors or references that use the exact heading text are updated accordingly (search for the exact string "Step 4: Enable automatic group creation (optional)" and the main header "Step 4: Validate the SSO configuration" to confirm uniqueness).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/infrahub/auth.py`:
- Around line 166-171: Wrap the call to re.match(filter_pattern, group_name) in
a try/except that catches re.error, logs a warning including the offending
filter_pattern and exception, and then skips processing that group (continue);
update the block around filter_pattern / match / effective_name in auth.py (the
branch that computes effective_name from match.group(1)) to use this try/except
so an invalid sso_generate_groups_filter cannot raise and break SSO login.
- Around line 157-177: The code can create duplicate groups because
existing_group_names is built from raw SSO names but new groups are created from
the transformed effective_name; fix by resolving the effective_name before
deciding if a group exists and checking the DB for that effective_name as well:
for each group_name from sso_groups (inside the loop where
sso_generate_groups_filter and match logic live) compute effective_name, skip if
no match, then check both existing_group_names and created_group_names AND
perform an existence check against the Infrahub store (e.g., query for name ==
effective_name or use an existence/get method) before calling Node.init / new /
save for InfrahubKind.ACCOUNTGROUP to avoid creating a duplicate of an
already-present transformed name.
In `@backend/infrahub/config.py`:
- Around line 762-769: Add a Pydantic validator for the
sso_generate_groups_filter field that attempts to compile the provided string
with re.compile and raises a ValidationError with message "Invalid regex
pattern" on failure; locate the model that defines sso_generate_groups and
sso_generate_groups_filter and add a method decorated with
`@validator`("sso_generate_groups_filter", pre=True, always=False) (or the
equivalent pydantic v2 validator) to skip validation for None and run
re.compile(value) for non-empty strings, raising ValueError("Invalid regex
pattern") if compilation fails so the unit test in test_sso_generate_groups.py
passes.
In `@backend/tests/unit/test_sso_generate_groups.py`:
- Around line 149-159: Add a pydantic validator on the SecuritySettings model
for the sso_generate_groups_filter field that checks each provided pattern
(supporting str or list[str]) can compile via re.compile and on failure raise a
ValueError with the message "Invalid regex pattern" so that creating
SecuritySettings(sso_generate_groups_filter="[invalid(regex") triggers a
pydantic.ValidationError; update or add the validator function (e.g.,
`@validator`("sso_generate_groups_filter", pre=True, each_item=True) def
validate_sso_generate_groups_filter(...)) to perform the re.compile check and
return the original value when valid.
---
Nitpick comments:
In `@backend/tests/functional/api/test_sso_groups.py`:
- Around line 19-25: The existing_group fixture uses the protocol type
CoreAccountGroup as the Node.init schema which is inconsistent with other code;
update the fixture to call Node.init with schema=InfrahubKind.ACCOUNTGROUP (and
adjust any type hints/returns if necessary) so Node.init(db=db,
schema=InfrahubKind.ACCOUNTGROUP) is used instead of schema=CoreAccountGroup,
keeping the fixture name existing_group and the surrounding calls (new, save)
unchanged.
In `@backend/tests/unit/test_sso_generate_groups.py`:
- Around line 21-62: Replace the try/finally config toggling in tests
test_groups_not_created_when_disabled and test_groups_created_when_enabled by
using pytest's monkeypatch to set and auto-restore
config.SETTINGS.security.sso_generate_groups; specifically, add the monkeypatch
fixture to each test signature and call
monkeypatch.setattr(config.SETTINGS.security, "sso_generate_groups", False) (or
True) before calling signin_sso_account so the original value is restored
automatically instead of manually saving/restoring in the test.
In `@docs/docs/guides/sso.mdx`:
- Line 447: The heading "#### Step 4: Enable automatic group creation
(optional)" conflicts with the main guide's "Step 4: Validate the SSO
configuration"; update the group mapping subsection header to remove the "Step
4:" prefix or use a distinct substep label (e.g., "Enable automatic group
creation (optional)" or "Group mapping — Step 1") so numbering doesn't collide,
and ensure any related internal anchors or references that use the exact heading
text are updated accordingly (search for the exact string "Step 4: Enable
automatic group creation (optional)" and the main header "Step 4: Validate the
SSO configuration" to confirm uniqueness).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/infrahub/auth.pybackend/infrahub/config.pybackend/tests/functional/api/test_sso_groups.pybackend/tests/unit/test_sso_generate_groups.pydocs/docs/guides/sso.mdxdocs/docs/reference/sso.mdx
| class TestSSOGenerateGroupsFilterValidation: | ||
| def test_invalid_regex_pattern_raises_error(self): | ||
| """Verify that invalid regex patterns raise a validation error""" | ||
| from pydantic import ValidationError | ||
|
|
||
| from infrahub.config import SecuritySettings | ||
|
|
||
| with pytest.raises(ValidationError) as exc_info: | ||
| SecuritySettings(sso_generate_groups_filter="[invalid(regex") | ||
|
|
||
| assert "Invalid regex pattern" in str(exc_info.value) |
There was a problem hiding this comment.
Test will fail: missing config validation implementation.
This test expects SecuritySettings to raise a ValidationError with "Invalid regex pattern" when given an invalid regex, but the sso_generate_groups_filter field in config.py has no validator to enforce this. The test will fail until the validator is added.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/tests/unit/test_sso_generate_groups.py` around lines 149 - 159, Add a
pydantic validator on the SecuritySettings model for the
sso_generate_groups_filter field that checks each provided pattern (supporting
str or list[str]) can compile via re.compile and on failure raise a ValueError
with the message "Invalid regex pattern" so that creating
SecuritySettings(sso_generate_groups_filter="[invalid(regex") triggers a
pydantic.ValidationError; update or add the validator function (e.g.,
`@validator`("sso_generate_groups_filter", pre=True, each_item=True) def
validate_sso_generate_groups_filter(...)) to perform the re.compile check and
return the original value when valid.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
backend/infrahub/config.py (1)
771-785: Add a docstring to the new validator method.Please document accepted value shapes and failure behavior for this validator in Google-style format.
As per coding guidelines, "Always use triple quotes (""") for Python docstrings" and "Follow Google-style docstring format in Python."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/infrahub/config.py` around lines 771 - 785, Add a Google-style docstring (triple-quoted) to the validator method validate_sso_generate_groups_filter_regex (decorated with `@field_validator`) that documents accepted input shapes (str, list[str], or None), explains that each string is treated as a regular expression and will be compiled, and describes failure behavior (raises ValueError when any pattern fails to compile, original re.error chained). Keep the docstring brief and include Args and Raises sections following Google-style conventions.backend/tests/unit/test_sso_generate_groups.py (1)
22-326: Add explicit-> Nonereturn annotations to async test methods.Most new async tests omit return type annotations; please annotate them consistently for mypy/ruff alignment.
🧩 Example pattern to apply across the file
- async def test_groups_not_created_when_disabled( + async def test_groups_not_created_when_disabled( self, db: InfrahubDatabase, default_branch: Branch, register_core_models_schema, existing_group - ): + ) -> None:As per coding guidelines, "Use type hints for all function parameters and return values in Python."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/unit/test_sso_generate_groups.py` around lines 22 - 326, Several async test functions in this file (e.g., test_groups_not_created_when_disabled, test_groups_created_when_enabled, test_filter_allows_matching_groups, test_no_filter_allows_all_groups, test_filter_with_complex_pattern, test_filter_does_not_duplicate_existing_extracted_group, test_filter_extracts_group_name_from_capture_group, test_filter_without_capture_group_uses_original_name, test_filter_deduplicates_extracted_names, test_filter_skips_non_matching_groups) are missing explicit return type annotations; update each async def signature to include "-> None" (e.g., async def test_groups_not_created_when_disabled(...) -> None:) to satisfy type-checker/style rules, leaving bodies unchanged. Ensure you only modify the function signatures for the listed async test_* functions and preserve existing imports and try/finally blocks.backend/infrahub/auth.py (1)
142-174: Add a docstring for the new helper function.Please add a concise Google-style docstring describing filter semantics and return behavior.
As per coding guidelines, "Always use triple quotes (""") for Python docstrings" and "Follow Google-style docstring format in Python."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/infrahub/auth.py` around lines 142 - 174, Add a Google-style triple-quoted docstring to the _extract_effective_sso_group_names function that succinctly describes its purpose, input parameters, filter semantics (filter_pattern may be a string, list of strings, or None; patterns are compiled as regular expressions and matched with re.match semantics; if a regex contains a capture group the first group is used as the effective name, otherwise the whole group_name is used), return type (set[str] of effective group names), and behavior on invalid regex (logs a warning and returns an empty set). Keep it concise and use parameter/returns sections consistent with Google-style docstrings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/infrahub/auth.py`:
- Around line 216-235: The check-then-create loop for SSO groups is not
race-safe: concurrent requests can both see a missing name and create
duplicates; update the logic around Node.init/Node.new/Node.save so creation is
done in a DB-safe way—either perform the work inside a single transaction and
use a DB upsert/INSERT ... ON CONFLICT DO NOTHING (or SELECT ... FOR UPDATE) to
avoid duplicates, or keep the optimistic flow but catch
unique-constraint/IntegrityError from Node.save, then re-query
existing_group_names and, if the save failed due to duplicate, discard that
created name and continue; after handling conflicts always run the
NodeManager.query(CoreAccountGroup, filters={"name__values": ...}) to fetch the
canonical rows for infrahub_groups.
In `@docs/docs/guides/sso.mdx`:
- Around line 491-492: Replace the phrase "regex capture groups" with "regular
expression (regex) capture groups" on first use and remove the word "just" from
the sentence so it reads something like: "If your identity provider sends groups
with prefixes or paths (like `ldap/groups/network_automation`), you can use
regular expression (regex) capture groups to extract the relevant part as the
group name." Update the sentence where "regex capture groups" appears (the line
referencing `ldap/groups/network_automation`) to implement this wording and
maintain a professional, neutral tone.
---
Nitpick comments:
In `@backend/infrahub/auth.py`:
- Around line 142-174: Add a Google-style triple-quoted docstring to the
_extract_effective_sso_group_names function that succinctly describes its
purpose, input parameters, filter semantics (filter_pattern may be a string,
list of strings, or None; patterns are compiled as regular expressions and
matched with re.match semantics; if a regex contains a capture group the first
group is used as the effective name, otherwise the whole group_name is used),
return type (set[str] of effective group names), and behavior on invalid regex
(logs a warning and returns an empty set). Keep it concise and use
parameter/returns sections consistent with Google-style docstrings.
In `@backend/infrahub/config.py`:
- Around line 771-785: Add a Google-style docstring (triple-quoted) to the
validator method validate_sso_generate_groups_filter_regex (decorated with
`@field_validator`) that documents accepted input shapes (str, list[str], or
None), explains that each string is treated as a regular expression and will be
compiled, and describes failure behavior (raises ValueError when any pattern
fails to compile, original re.error chained). Keep the docstring brief and
include Args and Raises sections following Google-style conventions.
In `@backend/tests/unit/test_sso_generate_groups.py`:
- Around line 22-326: Several async test functions in this file (e.g.,
test_groups_not_created_when_disabled, test_groups_created_when_enabled,
test_filter_allows_matching_groups, test_no_filter_allows_all_groups,
test_filter_with_complex_pattern,
test_filter_does_not_duplicate_existing_extracted_group,
test_filter_extracts_group_name_from_capture_group,
test_filter_without_capture_group_uses_original_name,
test_filter_deduplicates_extracted_names, test_filter_skips_non_matching_groups)
are missing explicit return type annotations; update each async def signature to
include "-> None" (e.g., async def test_groups_not_created_when_disabled(...) ->
None:) to satisfy type-checker/style rules, leaving bodies unchanged. Ensure you
only modify the function signatures for the listed async test_* functions and
preserve existing imports and try/finally blocks.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/infrahub/auth.pybackend/infrahub/config.pybackend/tests/unit/test_sso_generate_groups.pydocs/docs/guides/sso.mdxdocs/docs/reference/sso.mdx
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/docs/reference/sso.mdx
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/tests/unit/test_sso_generate_groups.py (1)
31-47: Prefer scoped patching over mutating shared global settings in tests.Directly assigning to
config.SETTINGS.securityrelies on global mutable state. This is brittle for parallel or interleaved test execution. Usepatch.object(...)context managers for per-test isolation.♻️ Suggested pattern
+from unittest.mock import patch ... - original_value = config.SETTINGS.security.sso_generate_groups - config.SETTINGS.security.sso_generate_groups = False - - try: + with patch.object(config.SETTINGS.security, "sso_generate_groups", False): await signin_sso_account( db=db, account_name="test-user", sso_groups=["existing-group", "new-group-1", "new-group-2"] ) - - groups = await NodeManager.query(db=db, schema=CoreAccountGroup) - group_names = {g.name.value for g in groups} - - assert "existing-group" in group_names - assert "new-group-1" not in group_names - assert "new-group-2" not in group_names - finally: - config.SETTINGS.security.sso_generate_groups = original_value + groups = await NodeManager.query(db=db, schema=CoreAccountGroup) + group_names = {g.name.value for g in groups} + assert "existing-group" in group_names + assert "new-group-1" not in group_names + assert "new-group-2" not in group_names🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/unit/test_sso_generate_groups.py` around lines 31 - 47, The test currently mutates global state by assigning config.SETTINGS.security.sso_generate_groups directly; replace this with a scoped patch (e.g., use unittest.mock.patch.object or pytest.MonkeyPatch) to temporarily set config.SETTINGS.security.sso_generate_groups for the duration of the test that calls signin_sso_account and then queries NodeManager.query(CoreAccountGroup); patching ensures isolation and automatic restore instead of manual try/finally mutation of config.SETTINGS.security.sso_generate_groups.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/infrahub/auth.py`:
- Around line 166-167: The code currently uses "if match.lastindex:" before
doing effective_group_names.add(match.group(1)), which still allows None to be
added when group(1) is an optional capture; update both occurrences (the block
around match.lastindex leading to effective_group_names.add(...) and the similar
logic in the 217-224 region) to explicitly check the captured value is not None
(e.g., val = match.group(1); if match.lastindex and val is not None:
effective_group_names.add(val)) so only actual captured strings are added.
In `@backend/tests/unit/test_sso_generate_groups.py`:
- Around line 171-205: The test functions
test_invalid_regex_pattern_raises_error, test_valid_regex_pattern_accepted,
test_valid_regex_pattern_list_accepted, test_none_filter_accepted, and
test_invalid_regex_pattern_in_list_raises_error are missing explicit return type
annotations; update each function signature to include "-> None" (e.g., def
test_invalid_regex_pattern_raises_error() -> None:) to comply with the project's
typing standards and ensure all sync test methods declare return types.
---
Nitpick comments:
In `@backend/tests/unit/test_sso_generate_groups.py`:
- Around line 31-47: The test currently mutates global state by assigning
config.SETTINGS.security.sso_generate_groups directly; replace this with a
scoped patch (e.g., use unittest.mock.patch.object or pytest.MonkeyPatch) to
temporarily set config.SETTINGS.security.sso_generate_groups for the duration of
the test that calls signin_sso_account and then queries
NodeManager.query(CoreAccountGroup); patching ensures isolation and automatic
restore instead of manual try/finally mutation of
config.SETTINGS.security.sso_generate_groups.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fc750956-81a8-4de6-8051-0169f6707211
📒 Files selected for processing (4)
backend/infrahub/auth.pybackend/infrahub/config.pybackend/tests/unit/test_sso_generate_groups.pydocs/docs/guides/sso.mdx
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/infrahub/config.py
gmazoyer
left a comment
gmazoyer
left a comment
There was a problem hiding this comment.
The PR is a pretty good start, clean separation of _extract_effective_sso_group_names as a testable helper, pragmatic concurrency handling for group creation, and thorough documentation with good coverage across unit and functional tests 👍
Users are never removed from groups when the IdP stops sending a claim. With auto-generation this becomes more significant since groups proliferate automatically and stale memberships accumulate silently. Worth documenting as a known limitation or addressing in a follow-up PR.
| if match.lastindex: | ||
| effective_group_names.add(match.group(1)) | ||
| else: | ||
| effective_group_names.add(group_name) |
There was a problem hiding this comment.
I have not verified this entirely but it seems this could lead to unexpected results (which could probably be highlighted by running test_filter_with_complex_pattern). I think that as regexp such as ^(dev|prod)-.*-team$ would produce groups like dev and prod instead f groups like dev-foo-team and prod-bar-team.
There was a problem hiding this comment.
That makes sense right? If you want to capture a part of a group name, using regex groups is the way to go.
The reason I do this is because syncing LDAP groups to SSO causes groupnames to appear like ldap/groups/team_name_1. In that case, I want to capture only team_name_1 using a pattern like ^ldap\/groups\/(.*)
| effective_group_names: set[str] = set() | ||
| for group_name in sso_groups: | ||
| for compiled_filter in compiled_filters: | ||
| match = compiled_filter.match(group_name) |
There was a problem hiding this comment.
The code uses compiled_filter.match(group_name) which anchors at the start of the string. A filter like groups/(\w+) would* not* match ldap/groups/network because the pattern is expected to match from position 0. This should either use re.search and document that patterns are searched within the string or document explicitly that patterns are anchored to the start (like adding "Patterns are matched from the start of the group name" to the docs).
There was a problem hiding this comment.
import re
path = "ldap/groups/admins"
match = re.search(r"ldap/groups/(\w+)", path)
match.group(0) # "ldap/groups/admins" — the full match
match.group(1) # "admins" — the captured partAlso the current capture would match pattern containing - e.g. full-admin
There was a problem hiding this comment.
I think it makes sense to keep using match to prevent matching unwanted cases through search. I've updated the docs to match this.
What do you think?
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/infrahub/auth.py (1)
232-243:⚠️ Potential issue | 🟠 MajorCatch the HFID uniqueness conflict in this retry path.
Line 232 only retries
ValidationError, butcreate_node()can also surface duplicate-name conflicts asHFIDViolatedError. In that case concurrent SSO logins still fail here instead of reloading the group that won the race.🛠️ Minimal fix
- except ValidationError: + except (ValidationError, HFIDViolatedError): concurrent_groups = await NodeManager.query( db=db, schema=CoreAccountGroup, filters={"name__values": [effective_name]}, prefetch_relationships=True, )You'll also need to add
HFIDViolatedErrorto the exception imports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/infrahub/auth.py` around lines 232 - 243, The except block currently only handles ValidationError but create_node() can also raise HFIDViolatedError; update the exception handler to catch both ValidationError and HFIDViolatedError, perform the same retry logic that queries NodeManager.query(schema=CoreAccountGroup, filters={"name__values":[effective_name]}, prefetch_relationships=True), extend infrahub_groups with any concurrent_groups and update existing_group_names from group.name.value, and re-raise only if no concurrent_groups are found; also add HFIDViolatedError to the exception imports so the symbol is available.
🧹 Nitpick comments (1)
backend/tests/unit/test_sso_generate_groups.py (1)
55-89: This test never reaches the conflict-reload branch.Because
create_nodeon Line 77 always succeeds, the fallback query path for concurrent creation is never exercised here. Please add a companion case that makescreate_noderaise the uniqueness-conflict exception and asserts the already-created group is reloaded instead of failing the login.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/unit/test_sso_generate_groups.py` around lines 55 - 89, Add a companion unit case to simulate a uniqueness-conflict during safe creation: duplicate the test_signin_uses_safe_create_path_for_missing_groups setup but patch infrahub.auth.create_node to raise the uniqueness-conflict exception on its first await (then succeed or remain failing as appropriate), keep query side_effect such that the first two queries return [] and the third returns the already-created fake_group, call signin_sso_account the same way, and assert that after the create_node raises the uniqueness error the code falls back to re-querying and ultimately loads the existing group (verify create_node was awaited and that NodeManager.query / Node.init or the fake_db.schema.get_node_schema were called as expected). Ensure you reference the same symbols: signin_sso_account, create_node, NodeManager.query, Node.init, and the uniqueness-conflict exception you use in production.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/docs/guides/sso.mdx`:
- Around line 372-373: The in-page anchor in the paragraph uses the wrong slug:
update the link target in the sentence currently pointing to
"#step-4-enable-automatic-group-creation-optional" so it matches the generated
heading slug "#enable-automatic-group-creation-optional" (or alternatively
change the subsection heading to include "Step 4" so its slug becomes
"#step-4-enable-automatic-group-creation-optional"); locate the link text in
docs/guides/sso.mdx and adjust either the href or the heading text to make the
two slugs identical.
---
Duplicate comments:
In `@backend/infrahub/auth.py`:
- Around line 232-243: The except block currently only handles ValidationError
but create_node() can also raise HFIDViolatedError; update the exception handler
to catch both ValidationError and HFIDViolatedError, perform the same retry
logic that queries NodeManager.query(schema=CoreAccountGroup,
filters={"name__values":[effective_name]}, prefetch_relationships=True), extend
infrahub_groups with any concurrent_groups and update existing_group_names from
group.name.value, and re-raise only if no concurrent_groups are found; also add
HFIDViolatedError to the exception imports so the symbol is available.
---
Nitpick comments:
In `@backend/tests/unit/test_sso_generate_groups.py`:
- Around line 55-89: Add a companion unit case to simulate a uniqueness-conflict
during safe creation: duplicate the
test_signin_uses_safe_create_path_for_missing_groups setup but patch
infrahub.auth.create_node to raise the uniqueness-conflict exception on its
first await (then succeed or remain failing as appropriate), keep query
side_effect such that the first two queries return [] and the third returns the
already-created fake_group, call signin_sso_account the same way, and assert
that after the create_node raises the uniqueness error the code falls back to
re-querying and ultimately loads the existing group (verify create_node was
awaited and that NodeManager.query / Node.init or the
fake_db.schema.get_node_schema were called as expected). Ensure you reference
the same symbols: signin_sso_account, create_node, NodeManager.query, Node.init,
and the uniqueness-conflict exception you use in production.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c715a40b-cacf-47e3-b459-b2d120950d1e
📒 Files selected for processing (5)
backend/infrahub/auth.pybackend/infrahub/config.pybackend/tests/functional/api/test_sso_groups.pybackend/tests/unit/test_sso_generate_groups.pydocs/docs/guides/sso.mdx
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/infrahub/config.py
- backend/tests/functional/api/test_sso_groups.py
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
AlexanderGrooff
force-pushed
the
auto-create-sso-groups
branch
from
2b989fe to
1908d40
Compare
|
This PR has been inactive for 2 weeks and has been marked as stale. If you're still working on this, please:
The stale label will be removed automatically when there's new activity. |
|
@AlexanderGrooff thanks for the contribution. We have not forgotten about this. We like the overall direction this is going. But before we can decide on the final implementation, we need to get additional requirements. We'll get back to you ASAP. |
|
Closing this PR as the engineering team redid the feature as part of 1.10 delivery |
5 participants
Why
I'm missing the ability to automatically create groups coming from SSO. This PR adds feature flag
sso_generate_groupsto automatically create groups that are imported from SSO, and adds the ability to filter it based on a pattern defined insso_generate_groups_filter.What changed
Adds a new block in the authentication code to import groups from the SSO provider when the feature flag is enabled.
I've also changed the documentation to reflect these changes.
The current behaviour doesn't change. It only gets picked up when the feature flag is enabled.
How to review
How to test
I've added testcases, but you can also try importing SSO groups when the feature flag is enabled, that way you can see automatic groups being created.
Impact & rollout
INFRAHUB_SECURITY_SSO_GENERATE_GROUPSandINFRAHUB_SECURITY_SSO_GENERATE_GROUPS_FILTERChecklist
uv run towncrier create ...)Summary by CodeRabbit
New Features
Documentation
Tests