Skip to content

Fixes and optimizations for Entra ID#28938

Merged
getvictor merged 21 commits intomainfrom
victor/28196-scim-entra-id-tweaks
May 8, 2025
Merged

Fixes and optimizations for Entra ID#28938
getvictor merged 21 commits intomainfrom
victor/28196-scim-entra-id-tweaks

Conversation

@getvictor
Copy link
Copy Markdown
Member

@getvictor getvictor commented May 7, 2025

For #28196

This PR depends on PR #28832

This PR adds support for excludeAttributes=members, which is being used by Microsoft Entra ID.

This PR modifies the primary key of host_scim_user table to be host_id. This should have been done initially and has added accidental complexity and maintainability challenges, so we are doing it now. This means a host can have a maximum of 1 SCIM user associated with it. A SCIM user, on the other hand, can be associated with multiple hosts.

Checklist for submitter

  • Changes file added for user-visible changes in changes/, orbit/changes/ or ee/fleetd-chrome/changes.
  • If database migrations are included, checked table schema to confirm autoupdate
  • For database migrations:
    • Checked schema for all modified table for columns that will auto-update timestamps during migration.
    • Confirmed that updating the timestamps is acceptable, and will not cause unwanted side effects.
    • Ensured the correct collation is explicitly set for character columns (COLLATE utf8mb4_unicode_ci).
  • Added/updated automated tests
  • A detailed QA plan exists on the associated ticket (if it isn't there, work with the product group's QA engineer to add it)
  • Manual QA for all new/changed functionality

@codecov
Copy link
Copy Markdown

codecov Bot commented May 7, 2025

Codecov Report

Attention: Patch coverage is 52.98507% with 63 lines in your changes missing coverage. Please review.

Project coverage is 64.17%. Comparing base (bc0a214) to head (9cd83d2).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
...ns/tables/20250507170845_HostSCIMUserPrimaryKey.go 38.09% 46 Missing and 6 partials ⚠️
server/datastore/mysql/scim.go 68.75% 7 Missing and 3 partials ⚠️
ee/server/scim/groups.go 94.44% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #28938      +/-   ##
==========================================
- Coverage   64.19%   64.17%   -0.02%     
==========================================
  Files        1818     1819       +1     
  Lines      176325   176425     +100     
  Branches     5126     5126              
==========================================
+ Hits       113188   113228      +40     
- Misses      54244    54296      +52     
- Partials     8893     8901       +8     
Flag Coverage Δ
backend 65.17% <52.98%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@getvictor getvictor marked this pull request as ready for review May 7, 2025 23:05
@getvictor getvictor requested a review from a team as a code owner May 7, 2025 23:05
gillespi314
gillespi314 previously approved these changes May 8, 2025
Copy link
Copy Markdown
Contributor

@gillespi314 gillespi314 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a few clarifying questions/comments

Comment thread ee/server/scim/users.go Outdated
return concreteType, nil
}

func getConcreteBoolType(u *UserHandler, v interface{}, name string) (bool, error) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there another code comment somewhere to explain how/why this is being used (i.e. the MS boolean-as-string encoding issue that you mentioned in standup)? If not, would you mind adding something with a link to any supporting documentation for future reference?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was actually an attempt to fix the Microsoft issue that didn't work. It didn't work since there is additional validation in the base SCIM library we are using. So, I'll back out this specific change for readability/maintainability.

Comment on lines +13 to +14
// Step 1: Create a temporary table to store the rows we want to keep
// (for each host_id, keep only the row with the smallest scim_user_id)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any particular reason to prefer the earlier (smaller) user id over others or is it just an arbitrary selection?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arbitrary selection, based on what Martin did in his recent IdP variables story.

Base automatically changed from victor/28196-scim-entra-id to main May 8, 2025 18:02
@getvictor getvictor dismissed gillespi314’s stale review May 8, 2025 18:02

The base branch was changed.

@getvictor getvictor requested a review from lukeheath as a code owner May 8, 2025 18:02
getvictor added 2 commits May 8, 2025 13:13
…ra-id-tweaks

# Conflicts:
#	ee/server/integrationtest/scim/scim_test.go
#	ee/server/scim/groups.go
#	ee/server/scim/users.go
#	server/datastore/mysql/scim.go
#	server/datastore/mysql/scim_test.go
#	server/fleet/scim.go
@getvictor getvictor merged commit 0ae58c0 into main May 8, 2025
33 checks passed
@getvictor getvictor deleted the victor/28196-scim-entra-id-tweaks branch May 8, 2025 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants