BP-2217: custom claims for OIDC (and other improvements) #133
Conversation
WalkthroughDocumentation reorganizes OIDC and SAML auth pages and shared auth snippets: imports and snippet usage reordered, procedural steps converted to structured Steps blocks, Okta-specific OIDC guidance and claim-mapping added, IDPIntro component introduced, and navigation JSON grouped. No public API or runtime changes. Changes
Sequence Diagram(s)(omitted — changes are documentation and component reuse; no new runtime control flow requiring sequence diagrams) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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 |
mistahj67
left a comment
There was a problem hiding this comment.
The claims are very specific unfortunately in order to be consumed by Bloodhound, otherwise the rest seems okay to me.
Co-authored-by: mistahj67 <[email protected]>
|
Thanks for the review @specterkris & @mistahj67! I pushed changes to address your comments and suggestions. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/manage-bloodhound/auth/oidc.mdx (1)
91-114: Minor formatting inconsistency in table rows.The "Last name" and "Role" tabs have extra blank lines after the Name row (lines 96 and 109), while the "First name" tab does not. This creates visual inconsistency in the rendered tables.
Apply this diff to remove the extra blank lines:
| Field | Setting | |-----------|---------| | **Name** | last_name | - | **Include in token type** | ID Token → Always | | **Value type** | Expression | | **Value** | `user.lastName` | | **Include in** | Any scope (or Profile) | </Tab> <Tab title="Role"> | Field | Setting | |-----------|---------| | **Name** | roles | - | **Include in token type** | ID Token → Always | | **Value type** | Groups | | **Filter** | Starts with --> `bh-` | | **Include in** | Any scope (or Profile) | </Tab>
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/manage-bloodhound/auth/oidc.mdx(1 hunks)
🔇 Additional comments (6)
docs/manage-bloodhound/auth/oidc.mdx (6)
1-14: LGTM!The frontmatter, imports, and edition tag are well-organized. The description clearly communicates the purpose of the page.
15-21: LGTM!The introductory content clearly sets expectations. The note about Entra ID not being supported with a link to the SAML alternative is helpful for users who might otherwise spend time trying to configure it.
23-43: LGTM!The Order of Operations section provides a clear high-level workflow. The emphasis on matching IDP names between the provider and BloodHound is important guidance that could prevent configuration errors.
45-74: LGTM!The Configure Okta section is well-structured with clear steps. The note about setting the application type to "Native" addresses a key requirement. The tip highlighting the Client ID and Issuer URL values needed later is good UX.
75-116: Custom claims configuration addresses previous feedback.The case-sensitive field names (
first_name,last_name,roles) are correctly configured as specified in the past review comments. The step provides clear guidance with the external Okta documentation link and organized tabs for each claim type.
119-123: LGTM!The snippet integrations at the end properly pass the
auth_mode="OIDC"andendpoint="callback"parameters, ensuring the shared content renders correctly for the OIDC context.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/manage-bloodhound/auth/oidc.mdx (1)
110-110: Clarify the Filter field notation.The arrow notation
-->in "Starts with -->bh-" is inconsistent with the documentation style used for other fields. Consider using clearer notation that matches the Okta UI exactly, such as "Starts with:bh-" or documenting it as a selection from a dropdown menu.Apply this diff to use consistent notation:
- | **Filter** | Starts with --> `bh-` | + | **Filter** | Starts with: `bh-` |
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/manage-bloodhound/auth/oidc.mdx(1 hunks)
🔇 Additional comments (3)
docs/manage-bloodhound/auth/oidc.mdx (3)
117-121: LGTM!The snippet includes properly pass the
endpoint="callback"parameter, which aligns with the callback URL configuration described earlier in the document. The consistent use ofauth_mode="OIDC"across all snippets ensures proper context for the shared documentation components.
75-114: No changes required—the Okta custom claims configuration is accurate and matches current Okta documentation for authorization server custom claims setup.
52-52: External Okta documentation links should be verified to ensure they remain accessible and point to the correct resources. These links are external to the repository and outside the developer's control, so consider whether this verification should occur during PR review or through a separate documentation maintenance process.
mistahj67
left a comment
There was a problem hiding this comment.
My concerns are addressed. Thank you!
| - [SAML: Active Directory Federation Services (ADFS) Configuration](/manage-bloodhound/auth/saml-adfs) | ||
| - [SAML: Auth0 Configuration](/manage-bloodhound/auth/saml-auth0) | ||
| - [SAML: Entra ID Configuration](/manage-bloodhound/auth/saml-entra-id) | ||
| - [SAML: Google IDP Configuration](/manage-bloodhound/auth/saml-google) |
There was a problem hiding this comment.
You left out Okta here - perhaps it would be better to nest all those and point to some kind of TOC-like page? Definitely a nit, but trying to think about how to scale this without having to remember to add a new provider every time.
There was a problem hiding this comment.
Good catch! I'll have to think on this one a bit because I think either way we'll need to manually maintain a list (whether here, in the docs.json file, or in a separate TOC page). I may be able to create a component that loops over the files, but maybe I'm over-thinking it 🤔
There was a problem hiding this comment.
I started playing around with a script + custom component to autogenerate the list from the docs.json file, but it started to feel over-engineered. For now, I recommend proceeding with the manual update. If we add more items to the list, we can revisit maintenance overhead.
StephenHinck
left a comment
There was a problem hiding this comment.
I left two comments for consideration, but don't have a strong immediate preference on how you want to handle those and wouldn't block improvements on it if we'd rather punt for now.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/snippets/auth/idp_intro.jsx (1)
1-17: LGTM: Clean, focused component implementation.The IDPIntro component is well-structured with clear logic:
- Properly handles missing
auth_modeprop with a fallback- Uses straightforward conditionals for OIDC vs. SAML routing
- Returns appropriate guidance with a dynamic link
- Follows Mintlify's arrow function requirement
Optional: Consider adding PropTypes for type safety
While not essential for this simple component, adding PropTypes can improve developer experience and catch issues early:
+import PropTypes from 'prop-types'; + // Must use arrow function syntax in Mintlify snippets export const IDPIntro = ({ auth_mode }) => { const mode = (auth_mode || '').toUpperCase(); const isOIDC = mode === 'OIDC'; const href = isOIDC ? '/manage-bloodhound/auth/oidc' : '/manage-bloodhound/auth/saml'; const label = isOIDC ? 'OIDC' : 'SAML'; return ( <Tip> See <a href={href}>{label} in BloodHound</a> for order of operations, general {label} setup, and user configuration in BloodHound. </Tip> ); }; +IDPIntro.propTypes = { + auth_mode: PropTypes.oneOf(['OIDC', 'SAML', 'oidc', 'saml']).isRequired, +}; + export default IDPIntro;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
docs/docs.json(1 hunks)docs/manage-bloodhound/auth/oidc-okta.mdx(1 hunks)docs/manage-bloodhound/auth/oidc.mdx(1 hunks)docs/manage-bloodhound/auth/saml-adfs.mdx(1 hunks)docs/manage-bloodhound/auth/saml-auth0.mdx(1 hunks)docs/manage-bloodhound/auth/saml-entra-id.mdx(1 hunks)docs/manage-bloodhound/auth/saml-google.mdx(1 hunks)docs/manage-bloodhound/auth/saml-okta.mdx(1 hunks)docs/manage-bloodhound/auth/saml.mdx(3 hunks)docs/snippets/auth/auth_config_steps_1.mdx(1 hunks)docs/snippets/auth/auth_role_mapping.mdx(2 hunks)docs/snippets/auth/idp_intro.jsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/manage-bloodhound/auth/oidc-okta.mdx
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/snippets/auth/auth_role_mapping.mdx
- docs/manage-bloodhound/auth/oidc.mdx
- docs/manage-bloodhound/auth/saml.mdx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-02T18:01:39.059Z
Learnt from: StephenHinck
Repo: SpecterOps/bloodhound-docs PR: 67
File: docs/collect-data/enterprise-collection/privileged-collection.mdx:7-7
Timestamp: 2025-10-02T18:01:39.059Z
Learning: In the BloodHound documentation repository, "BloodHound" as a standalone name refers to the entire product family and is appropriate to use when content applies to all products in the family (Enterprise and Community Edition). "BloodHound Enterprise" should be used only when referring specifically to Enterprise-only features or capabilities.
Applied to files:
docs/manage-bloodhound/auth/saml-auth0.mdxdocs/manage-bloodhound/auth/saml-entra-id.mdx
🔇 Additional comments (7)
docs/snippets/auth/auth_config_steps_1.mdx (1)
5-39: LGTM: Well-structured Steps component with clear guidance.The conversion to a structured Steps block improves readability and user experience. The Administrator login Note is appropriately positioned, the image includes descriptive alt text for accessibility, and the field descriptions table clearly distinguishes between OIDC-only, SAML-only, and shared configuration fields.
docs/manage-bloodhound/auth/saml-auth0.mdx (1)
6-10: LGTM: Clean adoption of the IDPIntro component.The import and usage of the IDPIntro component is correct and consistent with the pattern applied across other SAML documentation files in this PR. This improves maintainability by centralizing common introductory guidance.
docs/manage-bloodhound/auth/saml-google.mdx (1)
6-10: LGTM: Consistent IDPIntro component integration.The component integration follows the established pattern correctly, maintaining consistency across SAML documentation pages.
docs/manage-bloodhound/auth/saml-entra-id.mdx (1)
6-10: LGTM: Proper IDPIntro component usage.The integration is correct and consistent with the standardization approach across SAML documentation.
docs/manage-bloodhound/auth/saml-okta.mdx (1)
6-10: LGTM: Correct IDPIntro component implementation.The component is properly imported and rendered with the correct prop, maintaining consistency with other SAML configuration pages.
docs/manage-bloodhound/auth/saml-adfs.mdx (1)
6-10: LGTM: Consistent IDPIntro component integration.The component is correctly imported and used, completing the consistent pattern across all SAML configuration documentation.
docs/docs.json (1)
175-192: LGTM: Improved navigation structure with logical grouping.The restructuring of the Authentication and Authorization section into OIDC and SAML groups significantly improves discoverability and user navigation. The grouping logically separates the two authentication methods while preserving all existing page references.
Pushed commit to address requested changes
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/manage-bloodhound/auth/oidc.mdx (1)
37-47: Consider reordering snippets to match the Order of Operations flow.The current snippet ordering doesn't align with the step sequence defined in "Order of Operations":
- Step 3 (line 33): "Create the OIDC configuration in BloodHound"
- Step 4 (lines 35-36): "Create new users or modify existing users"
However, the implementation details appear as:
- Role mapping (line 39)
- Icons (line 41)
- Config steps (lines 43, 45) ← relates to Step 3
- User configuration (line 47) ← relates to Step 4
Users following the document sequentially might expect configuration details immediately after Step 3, not after supplementary information. Consider reordering to place
snip_auth_config_1andsnip_auth_config_2beforesnip_role_mappingandsnip_icons, or restructuring to make the relationship between steps and snippets more explicit.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/manage-bloodhound/auth/oidc.mdx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-02T18:01:39.059Z
Learnt from: StephenHinck
Repo: SpecterOps/bloodhound-docs PR: 67
File: docs/collect-data/enterprise-collection/privileged-collection.mdx:7-7
Timestamp: 2025-10-02T18:01:39.059Z
Learning: In the BloodHound documentation repository, "BloodHound" as a standalone name refers to the entire product family and is appropriate to use when content applies to all products in the family (Enterprise and Community Edition). "BloodHound Enterprise" should be used only when referring specifically to Enterprise-only features or capabilities.
Applied to files:
docs/manage-bloodhound/auth/oidc.mdx
🔇 Additional comments (2)
docs/manage-bloodhound/auth/oidc.mdx (2)
17-19: Helpful guidance for Entra ID users.This note effectively redirects users to the appropriate SAML configuration, preventing wasted effort on an unsupported OIDC setup.
29-31: The Okta guide reference is properly configured. The file exists at the referenced location and the step-by-step structure with separated Okta guidance is sound.
Purpose
This pull request (PR) clarifies requirements for configuring an OICD provider to properly display users' first and last names when signing into BloodHound. It leans heavily on Okta as an example, but should work for other IDPs.
I've also done some formatting to normalize the docs and align the style with some of the other work I've been doing across the repo. This includes removing screenshots that don't seem necessary. The goal is to improve readability and user experience.
The main OIDC and SAML doc pages share ~80% of content via snippets, which led to some challenging decisions. Specifically, the Enter provider details step in the Configure BloodHound section contains both SAML and OICD examples that may give readers pause.
Staging
https://specterops-bp-2217-oidc-updates.mintlify.app/manage-bloodhound/auth/oidc
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.