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

Connect My Computer: Do not omit checking all permissions when the agent is configured #31927

Merged
merged 3 commits into from
Sep 18, 2023

Conversation

gzdunek
Copy link
Contributor

@gzdunek gzdunek commented Sep 15, 2023

Contributes to #27881

When I added showing CMC when the user has permissions or the agent has been configured, I took too simple approach.
Some permissions should be always checked, like a feature flag or OS, otherwise it can lead to bugs:

  • When a user configures the agent in version 14.1.0 and then downgrades to version 14.0.0, they will see a feature that is not fully completed.
  • When the user creates a configuration file on Windows, they will see the feature that will not work.

Copy link
Member

Choose a reason for hiding this comment

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

  • When a user configures the agent in version 14.1.0 and then downgrades to version 14.0.0, they will see a feature that is not fully completed.

This is a valid concern.

  • When the user creates a configuration file on Windows, they will see the feature that will not work.

I wouldn't worry about this, the internals of ~/Library/Application Support/Teleport Connect should be considered private.


We don't have a "formalized" permission system in Connect and IMHO the requirements in this PR serve as a good example as to why arbitrary functions are bad for this.

If anything, the requirement behind the feature flag always gating access to the feature (no matter if you have permissions or if the agent was configured) suggests to me that the feature flag check should be done outside of permissions. So we should really think of it in three categories:

isFeatureFlagEnabled && (hasPermissions || hasConfiguredAgent)

This also illustrates better the intent behind the checks. What we described in the RFD is that we want the user to always have access to the agent after the user sets up the agent. So even if the admin changes permissions so that technically the user is not able to set up a new agent, we still want the user to have the ability to manage the agent that was already set up.

But here instead of making the distinction between permissions, feature flag and feature state more clear, we add them app together.

I think about it like this: if we wanted to make a more formalized API for permissions, we'd likely want to consider the permissions based on the user state and the runtime settings, so that the API can be uniform for all different kinds of permissions. If each one require some additional data specific for a single use case, the API would get a bit unwieldy.


I think your previous solution was good. If we skip the second point about creating a config file on Windows and are left just with the one about the feature flag, as I said before, in my eye it's an argument to the feature flag check out of canUseConnectMyComputer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you are right, putting another parameters into canUseConnectMyComputer makes its API quite bad.
Thanks for writing down the condition (isFeatureFlagEnabled && (hasPermissions || hasConfiguredAgent)), it opened my eyes on what exactly we want to check.

Pushed a commit that refactors it.

@gzdunek gzdunek requested review from ravicious and removed request for ryanclark and ibeckermayer September 15, 2023 10:01
mainProcessClient.getRuntimeSettings()
);

return isFeatureFlagEnabled && (hasPermissions || isAgentConfigured);
Copy link
Member

Choose a reason for hiding this comment

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

We could note here what we said in the RFD, that the user should always have access to the agent after configuring it and link to relevant section of the RFD.

@gzdunek gzdunek enabled auto-merge September 15, 2023 15:07
@gzdunek gzdunek added this pull request to the merge queue Sep 18, 2023
Merged via the queue into master with commit 7143414 Sep 18, 2023
21 checks passed
@gzdunek gzdunek deleted the gzdunek/do-not-omit-checking-permissions branch September 18, 2023 05:28
@public-teleport-github-review-bot

@gzdunek See the table below for backport results.

Branch Result
branch/v14 Create PR

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