Skip to content

UI: Conditional access - Microsoft Entra#27982

Merged
jacobshandling merged 42 commits into
mainfrom
27043-MS-compliance-frontend
Apr 15, 2025
Merged

UI: Conditional access - Microsoft Entra#27982
jacobshandling merged 42 commits into
mainfrom
27043-MS-compliance-frontend

Conversation

@jacobshandling
Copy link
Copy Markdown
Contributor

@jacobshandling jacobshandling commented Apr 8, 2025

Note - currently feature flagged. Build frontend with ALLOW_CONDITIONAL_ACCESS=true NODE_ENV=development yarn run webpack --progress --watch to enable this feature. Also, all of this functionality depends on the new config.license.managed_cloud being true, so you'll need to mock that data somehow. This branch has the appropriate fake data for testing

For #27043, #27864

Build front end for Fleet's integration with Microsoft Entra, allowing conditional prevention of single sign-on for hosts failing any policies on a team

Trigger the integration

trigger

Triggered, but configuration still not verified

√ not-verified-return-to-prefilled-form

Verified, short and long tenant ids:

ezgif-75f82492180d28

Verified –> Deleted

√ verified -  delete -  deleted

Enable for policies of a team

√ enable-for-team

Activities

√ activities

Unavailable for self-hosted Fleet instances:

no-access-self-hosted

Premium only

√ premium-only

  • Changes file added for user-visible changes in changes/
  • 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

@jacobshandling jacobshandling force-pushed the 27043-MS-compliance-frontend branch from cdb7ce2 to 63f9239 Compare April 8, 2025 17:09
@jacobshandling jacobshandling marked this pull request as ready for review April 8, 2025 22:54
@jacobshandling jacobshandling requested a review from a team as a code owner April 8, 2025 22:54
@jacobshandling jacobshandling marked this pull request as draft April 9, 2025 19:11
@jacobshandling
Copy link
Copy Markdown
Contributor Author

@sgress454 setting as draft per updated decision to merge with feature flag, will mark ready for review when that's implemented

@jacobshandling jacobshandling marked this pull request as ready for review April 9, 2025 20:29
Comment on lines +184 to +201
export const createMockLocation = (
overrides?: Partial<IRouterLocation>
): IRouterLocation => {
// Default values for the location object
const defaultLocation: IRouterLocation = {
pathname: "/",
host: "localhost:8080",
hostname: "localhost",
port: "8080",
protocol: "http:",
};

return {
...defaultLocation,
...overrides,
};
};

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.

Did you end up using this or IRouterLocation?

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.

Did not, but figured it might be useful to leave there for the future

Comment on lines +785 to +788
// These fields will never actually be changed here. See comment above
// IGlobalIntegrations definition.
zendesk: teamConfig?.integrations.zendesk || [],
jira: teamConfig?.integrations.jira || [],
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.

why not ...teamConfig.integrations like you have above for globalConfig?

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.

Not sure, will update

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.

Oh, perhaps related to patch semantics? This way there will always be an [] in each of these
fields, whereas above they can be null

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.

Yea...looks like if one of this is null but the other is set, this logic will wipe out the
null field even if it already exists on the db. Definitely room for improvement on that PATCH
endpoint but that seems to be why, same as above.

},
};
await teamsAPI.update(payload, teamIdForApi);
refetchGlobalConfig();
Copy link
Copy Markdown
Contributor

@sgress454 sgress454 Apr 10, 2025

Choose a reason for hiding this comment

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

why refetching global config here, and team config above? seems backwards.

edit -- the one above looks like it's for no team, not all teams... so if we set this up for No Team then we add it to the global config?

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.

Yep thanks for the catch, switching...

so if we set this up for No Team then we add it to the global config

That's right! Not the clearest system...

Copy link
Copy Markdown
Contributor

@sgress454 sgress454 left a comment

Choose a reason for hiding this comment

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

A couple of questions but overall code looks really good, nice job on the different phases of the setup flow. Testing now 👍

Comment on lines +30 to +33
if (
(!isManagedCloud && section?.includes("conditional-access")) ||
featureFlags.allowConditionalAccess !== "true"
) {
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.

Suggested change
if (
(!isManagedCloud && section?.includes("conditional-access")) ||
featureFlags.allowConditionalAccess !== "true"
) {
if (
section?.includes("conditional-access") &&
(!isManagedCloud || featureFlags.allowConditionalAccess !== "true")
) {

This is currently blocking the Integrations page completely if the feature flag is off -- you want it just for the conditional access section.

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.

Err, nothing to see here...😅

);
setIsUpdating(false);
toggleDeleteConditionalAccessModal();
setPhase(Phase.Form);
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.

Nit, we probably should be re-fetching the config here via the API, rather than trusting that this worked (like we do with GitOps mode). It's pretty low risk here since we should be able to trust our own backend when it says "this action was successful", but always better to be visualizing real data from the backend when we can.

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.

Great call. The omission may have been to allow testing with the fake data before the backend
actually works, since if we refetch the backend when it's not actually doing anything it won't lead
to the right phase. Will include this in the "fake data" commit.

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.

The goal here is for the page state to be driven by the state of the backend as much as possible. I realize that for certain parts of the process (like when the user is doing something on another site) we can't easily reach that goal. But for this case, I think we can by removing the setPhase(Phase.Form) here and moving it to the onSuccess of the config fetcher (contingent on the new config having microsoft_entra_connection_configured be false). Does that make sense?

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.

Maybe move the isUpdating(false) to there too, just to avoid an edge case of "config load takes a moment and user can do weird things in the meantime"

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.

Yes this makes sense

@jacobshandling jacobshandling merged commit f585199 into main Apr 15, 2025
14 checks passed
@jacobshandling jacobshandling deleted the 27043-MS-compliance-frontend branch April 15, 2025 20:55
sgress454 added a commit that referenced this pull request Apr 16, 2025
Support for this property was removed in
#28245, which merged just before
#27982. It's now causing linting
errors. I confirmed that the Save button looks the same without it.

<img width="793" alt="image"
src="https://github.com/user-attachments/assets/f3357cd2-1c4f-4c91-8c2e-e56797fb86e9"
/>
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