Connect to Workspace Supports Tenant ID Resource Identifier#3303
Connect to Workspace Supports Tenant ID Resource Identifier#3303ScottCarda-MS wants to merge 6 commits into
Conversation
ScottCarda-MS
changed the title
ScottCarda-MS
requested review from
billti,
idavis and
minestarks
as code owners
amcasey
left a comment
amcasey
left a comment
There was a problem hiding this comment.
Seems sensible. Minor suggestions
|
|
||
| const workspaceAuth = await getAuthSession( | ||
| [scopes.quantum, `VSCODE_TENANT:${workspace.tenantId}`], | ||
| [scopes.quantum, `VSCODE_TENANT:${workspace.tenantId ?? ""}`], |
There was a problem hiding this comment.
This doesn't look right to me. Did you test this flow when this value is blank?
The VSCODE_TENANT scope, if provided here, has a specific value required for authenticating to the right tenant (see https://github.com/microsoft/vscode/blob/1.126.0/extensions/microsoft-authentication/src/common/scopeData.ts#L41 ). I would expect passing an empty string here would break it, so I'd be interested in how this was tested and the behavior your saw.
There was a problem hiding this comment.
Yes this is the scenario we've had up until now, where tenant id is derived from subscription id. If a tenant id is provided, we use that, otherwise we use the subscription id to get the tenant id.
| // needed for Entra ID auth and for building portal deep links. If it wasn't | ||
| // supplied in the connection string, derive it from the subscription id. | ||
| // This must happen before validation because the Entra ID auth path below | ||
| // (used whenever there is no API key) requires a tenant id. |
There was a problem hiding this comment.
Exactly (see other comment I just left). It should never just default to an empty string if missing. That would be invalid. It should throw an error if the tenantId is missing when trying to do Entra authentication.
|
|
||
| return ( | ||
| `tenantId=${workspace.tenantId}` + | ||
| `tenantId=${workspace.tenantId ?? ""}` + |
There was a problem hiding this comment.
Same question here. Is it ever valid for a workspace portal link to have tenantId=""? If not, this should error, not build an invalid link.
There was a problem hiding this comment.
The tenant id is optional for now from the URI's we accept, but you are correct that by the time we reach this part of the code we should always have a tenant id. It is possible that something went wrong with getting the tenant id from the subscription id. I've added a check here that throws an error if that happens.
After the next sprint, we shouldn't have this complexity, as the portal will have made the switch to just using URIs with tenant id in them, so it can be fully mandatory then.
3 participants
This expands the ability of the Connect to Workspace feature to be able to consume resource identification strings containing tenant ids, in addition to being able to consume connection strings with API keys. When connecting to a resource with a resource identification string, the user will be authenticated with Entra ID.