Skip to content

automatically assign DEP enrolled devices to a team if set via config#9135

Merged
roperzh merged 13 commits into
mainfrom
9068-dep-team-assignment
Jan 16, 2023
Merged

automatically assign DEP enrolled devices to a team if set via config#9135
roperzh merged 13 commits into
mainfrom
9068-dep-team-assignment

Conversation

@roperzh
Copy link
Copy Markdown
Contributor

@roperzh roperzh commented Dec 29, 2022

Related to #9068 and #8733, this builds on the work done in #9062 to tie the loose ends and assign the configured team to the device that's enrolling

Checklist for submitter

If some of the following don't apply, delete the relevant line.

  • Input data is properly validated, SELECT * is avoided, SQL injection is prevented (using placeholders for values in statements)
  • Added/updated tests
  • Manual QA for all new/changed functionality

Frank Sievertsen and others added 9 commits December 22, 2022 12:02
Co-authored-by: Chris McGillicuddy <108031970+chris-mcgillicuddy@users.noreply.github.com>
Co-authored-by: Chris McGillicuddy <108031970+chris-mcgillicuddy@users.noreply.github.com>
@roperzh roperzh marked this pull request as ready for review January 4, 2023 11:10
@roperzh roperzh requested a review from a team as a code owner January 4, 2023 11:10
@roperzh roperzh temporarily deployed to Docker Hub January 4, 2023 11:10 — with GitHub Actions Inactive
Comment thread server/datastore/mysql/apple_mdm.go

args := []interface{}{nil}
if name := appCfg.MDM.AppleBMDefaultTeam; name != "" {
team, err := ds.TeamByName(ctx, name)
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.

What happens if the team name changes after the app config setting? Do we flow team name updates throughout the datastore? It seems like it would be easy for this to get lost inside the app config json.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this is a good point, I will figure out what happens with the other configs that also require a team name (I think we have others?)

but seems something that should be enforced (if we want to) when the config is set, not at this stage

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.

but seems something that should be enforced (if we want to) when the config is set, not at this stage

Agreed. And I think we ought to have something to enforce this at the app config level. Is it possible for MySQL to have something like foreign key constraints inside json? If not, maybe we'd need something like we have for hostRefs?

As a precaution, I wonder if it would perhaps be a good idea as a general design policy that to save primary keys like team id inside json blobs in the datastore rather than values like name which might change.

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.

THis is something I mention here #8733 (comment), the team might exist when the config is set, but can be deleted/renamed afterwards. In fact, nothing prevents a different team from being created later on with that name, and it would then be used for enrollment.

seems something that should be enforced (if we want to) when the config is set

It is validated when the config is set IIRC, but the problem is that the team can be renamed/deleted soon after.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think this is an important issue we need to solve, I created #9231 as my gut is that this will involve some product chat.

gillespi314
gillespi314 previously approved these changes Jan 4, 2023
Comment thread server/datastore/mysql/apple_mdm_test.go
mna
mna previously approved these changes Jan 9, 2023
Copy link
Copy Markdown
Contributor

@mna mna left a comment

Choose a reason for hiding this comment

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

PR looks good to me, just left some questions about how to handle failure modes related to the configured default team. I don't think it needs to block the PR if it needs to be merged quickly, it can be something we address in a subsequent one if needed.

@roperzh roperzh dismissed stale reviews from mna and gillespi314 via eb685b9 January 10, 2023 12:47
@roperzh roperzh temporarily deployed to Docker Hub January 10, 2023 12:47 — with GitHub Actions Inactive
@roperzh roperzh requested review from gillespi314 and mna January 10, 2023 12:57
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jan 10, 2023

Codecov Report

❌ Patch coverage is 91.30435% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.91%. Comparing base (2447a37) to head (5fed017).
⚠️ Report is 16617 commits behind head on main.

Files with missing lines Patch % Lines
server/datastore/mysql/apple_mdm.go 91.30% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9135      +/-   ##
==========================================
+ Coverage   59.89%   59.91%   +0.02%     
==========================================
  Files         482      482              
  Lines       47946    47966      +20     
==========================================
+ Hits        28717    28740      +23     
+ Misses      16516    16514       -2     
+ Partials     2713     2712       -1     

☔ 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.

mna
mna previously approved these changes Jan 10, 2023
gillespi314
gillespi314 previously approved these changes Jan 10, 2023
@rfairburn rfairburn dismissed stale reviews from gillespi314 and mna via 5fed017 January 16, 2023 20:09
@roperzh roperzh temporarily deployed to Docker Hub January 16, 2023 20:09 — with GitHub Actions Inactive
@roperzh roperzh merged commit f87c28e into main Jan 16, 2023
@roperzh roperzh deleted the 9068-dep-team-assignment branch January 16, 2023 20:38
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.

5 participants