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.

Sign up for GitHub

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

Jump to bottom

[Feature Request] Region auto enable on env variable #7328

Open
bgavrilMS opened this issue Sep 20, 2024 · 4 comments · May be fixed by #7354
Open

[Feature Request] Region auto enable on env variable #7328

bgavrilMS opened this issue Sep 20, 2024 · 4 comments · May be fixed by #7354
Assignees
@Robbie-Microsoft
Labels
confidential-client Issues regarding ConfidentialClientApplications feature Feature requests. msal-node Related to msal-node package

Comments

@bgavrilMS
Copy link
Member

Region auto-enable

  1. On creation of a ConfidentialClientApplication, MSAL shall detect env variable MSAL_FORCE_REGION, which will be set to a specific region (e.g. westus1)
  2. If this env variable is set, MSAL shall opt-in to ESTS-R with the value of this variable.

Disabling the behavior

If region is explicitly set in MSAL to the string "DisableMsalForceRegion`, then the above no longer applies

Conflicts with WithAzureRegion(xyz)

Use of the api WithRegion(xyz) takes precedence over the env variable.

Acceptance tests

For all of the tests, assume env variable MSAL_FORCE_REGION=eastus

  1. No other config is used. ACTUAL region used: eastus
  2. App developer configures region "westus" in MSAL. ACTUAL region used: westus
  3. App developer configures region "DisableMsalForceRegion" in MSAL. ACTUAL region used: none

Original issue:

AzureAD/microsoft-authentication-library-for-dotnet#4930
@bgavrilMS bgavrilMS added the confidential-client Issues regarding ConfidentialClientApplications label Sep 20, 2024
@github-actions github-actions bot added the more-information-needed Use this label when you are waiting on information from the issue creator label Sep 20, 2024
@bgavrilMS bgavrilMS added feature Feature requests. msal-node Related to msal-node package and removed more-information-needed Use this label when you are waiting on information from the issue creator labels Sep 20, 2024
@bgavrilMS bgavrilMS reopened this Sep 20, 2024
@AzureAD AzureAD deleted a comment from github-actions bot Sep 20, 2024
@Robbie-Microsoft
Copy link
Collaborator

MSALJS does not use the WithRegion(xyz) api like .net does.

@bgavrilMS
Copy link
Member Author

bgavrilMS commented Sep 27, 2024 •
edited
Loading

MSALJS does not use the WithRegion(xyz) api like .net does.

Can you please adapt the feature request to MSAL JS API. It's should not be that different ? Happy to review,.

@Robbie-Microsoft Robbie-Microsoft linked a pull request Oct 1, 2024 that will close this issue
Open
@Robbie-Microsoft Robbie-Microsoft linked a pull request Oct 1, 2024 that will close this issue
Region auto enable on env variable #7354
Open
@Robbie-Microsoft
Copy link
Collaborator

Robbie-Microsoft commented Oct 3, 2024 •
edited
Loading

@bgavrilMS According to your acceptance tests, I think I don't even need to check for the value of the passed in region. If it exists at all, then it overrides the environment variable. Is that correct? It doesn't matter if it's passed in with a value of "DisableMsalForceRegion" or "alskdjh". However if it is "DisableMsalForceRegion", then I'll need to set it to undefined instead of leaving it as that string.

I was able to pass the three acceptance tests listed above with the code

const region: AzureRegion | undefined =
            ENV_MSAL_FORCE_REGION && !validRequest.azureRegion
                ? ENV_MSAL_FORCE_REGION
                : validRequest.azureRegion === "DisableMsalForceRegion"
                ? undefined
                : validRequest.azureRegion;

but I don't think this is the functionality you're looking for.

@bgavrilMS
Copy link
Member Author

@bgavrilMS According to your acceptance tests, I think I don't even need to check for the value of the passed in region. If it exists at all, then it overrides the environment variable. Is that correct? It doesn't matter if it's passed in with a value of "DisableMsalForceRegion" or "alskdjh". However if it is "DisableMsalForceRegion", then I'll need to set it to undefined instead of leaving it as that string.

I was able to pass the three acceptance tests listed above with the code

const region: AzureRegion | undefined =
            ENV_MSAL_FORCE_REGION && !validRequest.azureRegion
                ? ENV_MSAL_FORCE_REGION
                : validRequest.azureRegion === "DisableMsalForceRegion"
                ? undefined
                : validRequest.azureRegion;

but I don't think this is the functionality you're looking for.

Using the ternary operator : there makes the code hard to read. Consider refactoring with if clauses.

If the region API is set to DisableMsalForceRegion, then the request goes to ESTS, not to ESTS-R.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Robbie-Microsoft Robbie-Microsoft

Labels
confidential-client Issues regarding ConfidentialClientApplications feature Feature requests. msal-node Related to msal-node package
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Region auto enable on env variable AzureAD/microsoft-authentication-library-for-js
2 participants
@bgavrilMS @Robbie-Microsoft