-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: add MDM authentication notification when navigation is blocked #7291
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1553,7 +1553,8 @@ export class ClineProvider | |
| this.postMessageToWebview({ type: "state", state }) | ||
|
|
||
| // Check MDM compliance and send user to account tab if not compliant | ||
| if (!this.checkMdmCompliance()) { | ||
| // Only redirect if there's an actual MDM policy requiring authentication | ||
| if (this.mdmService?.requiresCloudAuth() && !this.checkMdmCompliance()) { | ||
| await this.postMessageToWebview({ type: "action", action: "accountButtonClicked" }) | ||
| } | ||
| } | ||
|
|
@@ -1872,7 +1873,9 @@ export class ClineProvider | |
| codebaseIndexSearchMaxResults: codebaseIndexConfig?.codebaseIndexSearchMaxResults, | ||
| codebaseIndexSearchMinScore: codebaseIndexConfig?.codebaseIndexSearchMinScore, | ||
| }, | ||
| mdmCompliant: this.checkMdmCompliance(), | ||
| // Only set mdmCompliant if there's an actual MDM policy | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good consistency with the MDM compliance check. The comment clearly explains the three states, which is helpful. |
||
| // undefined means no MDM policy, true means compliant, false means non-compliant | ||
| mdmCompliant: this.mdmService?.requiresCloudAuth() ? this.checkMdmCompliance() : undefined, | ||
| profileThresholds: profileThresholds ?? {}, | ||
| cloudApiUrl: getRooCodeApiUrl(), | ||
| hasOpenedModeSelector: this.getGlobalState("hasOpenedModeSelector") ?? false, | ||
|
|
@@ -2172,7 +2175,7 @@ export class ClineProvider | |
|
|
||
| /** | ||
| * Check if the current state is compliant with MDM policy | ||
| * @returns true if compliant, false if blocked | ||
| * @returns true if compliant or no MDM policy exists, false if MDM policy exists and user is non-compliant | ||
| */ | ||
| public checkMdmCompliance(): boolean { | ||
| if (!this.mdmService) { | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -105,8 +105,11 @@ const App = () => { | |
|
|
||
| const switchTab = useCallback( | ||
| (newTab: Tab) => { | ||
| // Check MDM compliance before allowing tab switching | ||
| // Only check MDM compliance if mdmCompliant is explicitly false (meaning there's an MDM policy and user is non-compliant) | ||
| // If mdmCompliant is undefined or true, allow tab switching | ||
| if (mdmCompliant === false && newTab !== "account") { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider extracting this MDM check logic into a custom hook or utility function for better reusability and testability. For example: This would make the logic easier to test and reuse if needed elsewhere. |
||
| // Notify the user that authentication is required by their organization | ||
| vscode.postMessage({ type: "showMdmAuthRequiredNotification" }) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider adding test coverage for this new notification feature. It would be valuable to verify:
Also, have you considered the UX for repeated attempts? Currently, the notification will show every time the user clicks a different tab. Would it make sense to throttle this or show it only once per session with a more persistent indicator?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider adding a telemetry event here to track when users encounter MDM blocks. This could help understand usage patterns and potential friction points: |
||
| return | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great improvement to only check MDM compliance when there's actually an MDM policy! Consider adding JSDoc comments to document the three states of
mdmCompliant:undefined: No MDM policy existstrue: MDM policy exists and user is compliantfalse: MDM policy exists and user is non-compliantThis would help future developers understand the distinction.