Skip to content
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

Ensure Connect shows Connect My Computer only to local users #29804

Merged
merged 6 commits into from
Jul 31, 2023

Conversation

ravicious
Copy link
Member

@ravicious ravicious commented Jul 31, 2023

Connect My Computer won't work in its current form for SSO users. That's because the implementation depends on updating the role list of a user which isn't as straightforward for SSO users as it is for local users.

The RPC in tshd already returns an error if it detects that the user is an SSO user, however the Electron app was still showing the UI to everybody.

Checking for an SSO user is done by grabbing GetUserType() from a user resource we get from the cluster on calls to GetCluster and then returning it with LoggedInUser, next to ACL.

UserTypeFromString was based on ResourceDeviceEnrollStatusFromString.

@github-actions github-actions bot requested review from kimlisa and ryanclark July 31, 2023 10:05
@ravicious ravicious requested review from gzdunek and removed request for kimlisa July 31, 2023 10:05
@ravicious ravicious enabled auto-merge July 31, 2023 10:07
@@ -333,5 +333,7 @@ export type CreateConnectMyComputerRoleResponse =
export type CreateConnectMyComputerNodeTokenResponse =
apiService.CreateConnectMyComputerNodeTokenResponse.AsObject;

export const UserType = apiCluster.LoggedInUser.UserType;
Copy link
Contributor

Choose a reason for hiding this comment

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

We can export value and type at the same time (we may need the type in the future).

Suggested change
export const UserType = apiCluster.LoggedInUser.UserType;
export { UserType }

I did it for export { FileTransferDirection } above.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to do this but I didn't know how to reexport something that's defined in a namespace. I think I found a solution, let me know what you think.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I didn't notice that it is defined in a namespace. Your solution is fine, I was afraid that clicking on UserType somewhere in the code will open the types.ts file, not the actual definition. But it work as it should!

@@ -333,5 +333,7 @@ export type CreateConnectMyComputerRoleResponse =
export type CreateConnectMyComputerNodeTokenResponse =
apiService.CreateConnectMyComputerNodeTokenResponse.AsObject;

export const UserType = apiCluster.LoggedInUser.UserType;
Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to do this but I didn't know how to reexport something that's defined in a namespace. I think I found a solution, let me know what you think.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know if those tests add enough value to warrant having them. It'd probably be better to test it indirectly through some more integrated tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably they don't, but I would keep them for now, until we do something better.

@ravicious ravicious requested a review from gzdunek July 31, 2023 14:21
@ravicious ravicious added this pull request to the merge queue Jul 31, 2023
@ravicious ravicious removed this pull request from the merge queue due to a manual request Jul 31, 2023
@ravicious ravicious added this pull request to the merge queue Jul 31, 2023
Merged via the queue into master with commit cd000ac Jul 31, 2023
@ravicious ravicious deleted the ravicious/cmc-sso-check branch July 31, 2023 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants