Skip to content
This repository was archived by the owner on Jul 9, 2025. It is now read-only.

Conversation

@a-b-r-o-w-n
Copy link
Contributor

@a-b-r-o-w-n a-b-r-o-w-n commented Mar 24, 2021

Description

If a user is a member of more than 1 tenant, a new tenant selection UI is shown before they can configure a new publish profile.

Breaking change: New publish profiles will encode the tenant that was selected to be used for fetching arm tokens. Old profiles will continue to work ONLY if a user is a member of a single tenant. Users that are in multiple tenants must either add the correct tenant id to the existing profile or create a new profile.

Old profiles for users in  a single tenant will continue to work. If a user is in multiple tenants, their publish will fail and they will need to recreate their publishing profile or add tenantId to the publish configuration.
Copy link
Contributor

@tonyanziano tonyanziano left a comment

Choose a reason for hiding this comment

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

A few minor comments. Otherwise looks good 👍

@cwhitten
Copy link
Member

@a-b-r-o-w-n

regarding:

Breaking change: New publish profiles will encode the tenant that was selected to be used for fetching arm tokens. Old profiles will continue to work ONLY if a user is a member of a single tenant. Users that are in multiple tenants must either add the correct tenant id to the existing profile or create a new profile.

How is this presented to the user?

tonyanziano
tonyanziano previously approved these changes Mar 24, 2021
@a-b-r-o-w-n
Copy link
Contributor Author

@cwhitten
Copy link
Member

@a-b-r-o-w-n is this the behavior today without this change, or are you adding it as a constraint?

Old profiles will continue to work ONLY if a user is a member of a single tenant

For example, I have access to more than one tenant, but I can also successfully publish on existing profiles. Will I be broken?

@coveralls
Copy link

coveralls commented Mar 24, 2021

Coverage Status

Coverage decreased (-0.005%) to 51.996% when pulling 4f16a8f on abrown/select-tenant into 5417fb1 on main.

@a-b-r-o-w-n
Copy link
Contributor Author

@cwhitten I don't fully understand all the scenarios and why some were working while others were failing, but I suspect that the tenant being used implicitly was the correct one. This change forces you to be explicit about which tenant to publish to. Not sure if that clears anything up.

I could revert back to the old method (just choosing the first tenant) and not show the notification which would be in parity with what we do today. What do you think?

@cwhitten
Copy link
Member

cwhitten commented Mar 24, 2021

Makes sense. I'm of the mind to preserve existing behavior so we don't break people, and when we see an error about tenant mismatch we render the notification as you have and suggest they modify their profile or create a new one.

@mewa1024 @emivers8 we could use a UX assist here.

We need to bring this functionality in so users with access to multiple tenants can publish deterministically. Without it there are potential mismatches with other information in the provisioning profile resulting in publishing failures. We also don't want to break people where this is working as expected. It's likely due to the "default" tenant that they have set up happens to be the right one with all the services they are provisioning and publishing to.

Attached is an example mismatch

Screen Shot 2021-03-24 at 3 56 29 PM

@mewa1024
Copy link
Contributor

@cwhitten sorry for the delay in reply. Just want to make sure I understand the solution:

  • For existing profiles, if the user had only one tenant when they created it and continues to have only one tenant, there is no error.
  • For existing profiles, if the user had only one tenant when they created it and now has more than one tenant, an error appears telling them to edit their profile and specify the tenant.
  • For new profiles, for users with one tenant or with more tenants, they have to choose the tenant.

So in this screen (from Composer 1.4 nightly), we would have an additional dropdown for "Azure directory" above "Subscription" for both new profiles and existing profiles that a user edits:

image

Is that summary right?

So when a user is signed in to Azure, we don't sign them into a specific tenant? In other words, unlike the Azure portal or LUIS.ai, where when you sign in you choose a tenant that you use for the entire session. There is a control for switching tenants separate from the UI where you create/choose resources:

image

If we need an Azure sign in control, it could go in the in the main menu above the gear, similar to this from VS Code:
image

@cwhitten
Copy link
Member

@mewa1024 see below:

For existing profiles, if the user had only one tenant when they created it and continues to have only one tenant, there is no error.

Correct

For existing profiles, if the user had only one tenant when they created it and now has more than one tenant, an error appears telling them to edit their profile and specify the tenant.

Not always. If the information in the profile is associated with the "default" tenant, it works successfully. If the default tenant was changed, they would see an error.

For new profiles, for users with one tenant or with more tenants, they have to choose the tenant.

Correct

The tenant/directory selection currently happens when you sign-in to azure, not when you are selecting resources. @a-b-r-o-w-n are you able to share a gif or screenshot of this for @mewa1024 ?

@a-b-r-o-w-n
Copy link
Contributor Author

@cwhitten

The tenant/directory selection currently happens when you sign-in to azure, not when you are selecting resources.

This is sort of true. There is a new page in between selecting resources and the initial sign-in. The flow works like this:

  1. User clicks on "Add new profile" and selects an Azure profile.
  2. The user will be prompted to sign in.
  3. If the user is part of multiple tenants, a new screen appears with a tenant selection UI.
  4. After the user chooses the tenant, they have to log in again to that tenant. (I don't know of a way around this, neither did @tonyanziano).
  5. The provisioning / publishing flow works as normal.
Screen.Recording.2021-03-25.at.9.30.03.AM.mov

@cwhitten
Copy link
Member

Ahh, I got it. Following up w/ you on Teams

@cwhitten cwhitten changed the base branch from main to wenyluo/kv March 25, 2021 20:46
@mewa1024
Copy link
Contributor

fyi, Andy, Jas, and I talked offline. Andy figured out how to make the sign in work without the second approval step, so we can move the Azure tenant selector to the same screen where you pick the subscription etc should work. Jas updated screen here:
https://www.figma.com/file/J3oIdXDmeBQbAe4a4PYbQc/Jaskeerat-s-files?node-id=5319%3A87048

(Zooming out in the Figma file shows the latest provisioning flow.)

@cwhitten
Copy link
Member

fyi, Andy, Jas, and I talked offline. Andy figured out how to make the sign in work without the second approval step, so we can move the Azure tenant selector to the same screen where you pick the subscription etc should work. Jas updated screen here:
https://www.figma.com/file/J3oIdXDmeBQbAe4a4PYbQc/Jaskeerat-s-files?node-id=5319%3A87048

(Zooming out in the Figma file shows the latest provisioning flow.)

Nice!

Base automatically changed from wenyluo/kv to main March 26, 2021 16:41
@GeoffCoxMSFT
Copy link
Member

@a-b-r-o-w-n @cwhitten Can we get this checked in soon? We have a whole set of work in these files for publishing and one involves deferring sign in until later in the process so we can't really work on that until this is checked in. :)

@cwhitten
Copy link
Member

@a-b-r-o-w-n @cwhitten Can we get this checked in soon? We have a whole set of work in these files for publishing and one involves deferring sign in until later in the process so we can't really work on that until this is checked in. :)

Yep, planning to get it in this morning

@cwhitten cwhitten merged commit da1ac55 into main Mar 29, 2021
@cwhitten cwhitten deleted the abrown/select-tenant branch March 29, 2021 19:06
@cwhitten cwhitten mentioned this pull request May 20, 2021
lei9444 pushed a commit to lei9444/BotFramework-Composer-1 that referenced this pull request Jun 15, 2021
…es (microsoft#6542)

* feat: allow users to select their tenant

* clear login error after successful login

* show sign in copy before configure resources

* require tenantId on new publishing profiles

Old profiles for users in  a single tenant will continue to work. If a user is in multiple tenants, their publish will fail and they will need to recreate their publishing profile or add tenantId to the publish configuration.

* update error message when publish profile is no longer supported

* use formatMessage for strings

* maintain existing flow but catch mutliple tenants error

* open dev tools when running electron

* do not take focus away for electron debug task dependencies

* show tenant selection ui when configuring resources

* use internal logic to persist tenant id to publish profile

* remove unused import

Co-authored-by: Chris Whitten <christopher.whitten@microsoft.com>
Co-authored-by: Geoff Cox (Microsoft) <gcox@microsoft.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants