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

acquire_token_interactive() allows customizing the redirect URI's path #593

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

rayluo
Copy link
Collaborator

@rayluo rayluo commented Sep 16, 2023

Traditionally, most parts of the redirect URI for a desktop app is hardcoded in this library as http://localhost, and only the port is customizable (by a port optional parameter).

Sometimes, an app targets to multiple "platforms" (in this context, "platform" means web, native/desktop, spa, etc.), and they may all run on localhost during development. Those multiple redirect URIs differentiate with each other only by path, as described in the second last bullet point of this "localhost exceptions" documentation.

In this PR, the path will become customizable, but the remaining content (schema and host) remains constant. Therefore, the new optional parameter is path, rather than redirect_uri.

This will resolve #574 .

@rayluo rayluo changed the title Placeholder acquire_token_interactive() allows customizing the redirect URI's path Sep 16, 2023
@rayluo rayluo marked this pull request as ready for review September 17, 2023 16:40
Copy link
Member

@bgavrilMS bgavrilMS left a comment

Choose a reason for hiding this comment

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

  1. I don't think http://localhost:port/path is a valid redirect uri for a desktop app using system browser. The HTTP listener built in MSAL can't tell between http://localhost:port/path1 and http://localhost:port/path2. I am concerned about potential vulnerability this can introduce.

  2. Please do not change the public API of MSAL with an OK from @localden and a discussion with other MSAL owners

  3. For Public Client, please loop in @ashok672 and @iulico-1 as they need to maintain this.

@rayluo
Copy link
Collaborator Author

rayluo commented Oct 10, 2023

Agreed with your suggestion 2 & 3. Will wait for the input from those pinged folks.

With regard to suggestion 1, the http://localhost:port/path is a valid redirect uri for a desktop app using system browser, per the specs. So, it is a matter of whether/how it will be implemented.

The HTTP listener built in MSAL can't tell between http://localhost:port/path1 and http://localhost:port/path2. I am concerned about potential vulnerability this can introduce.

That is an implementation detail that can be discussed offline. Traditionally, MSAL uses ephemeral port, state validation and PKCE for security. Path is merely a short well-known string which does not provide any security cryptographically anyway.

@bgavrilMS
Copy link
Member

bgavrilMS commented Oct 11, 2023

Agreed with your suggestion 2 & 3. Will wait for the input from those pinged folks.

With regard to suggestion 1, the http://localhost:port/path is a valid redirect uri for a desktop app using system browser, per the specs. So, it is a matter of whether/how it will be implemented.

The HTTP listener built in MSAL can't tell between http://localhost:port/path1 and http://localhost:port/path2. I am concerned about potential vulnerability this can introduce.

That is an implementation detail that can be discussed offline. Traditionally, MSAL uses ephemeral port, state validation and PKCE for security.

That's an excellent document you found. So I guess the user scenario is to use 1 app registration for both web and desktop? Smth like:

image

@rayluo
Copy link
Collaborator Author

rayluo commented Oct 11, 2023

That's an excellent document you found. So I guess the user scenario is to use 1 app registration for both web and desktop? Smth like: ...

That is correct. That is the scenario that app developers run into, every time they need to start their app locally for development or debug purpose.

msal/application.py Outdated Show resolved Hide resolved
msal/application.py Outdated Show resolved Hide resolved
msal/application.py Outdated Show resolved Hide resolved
msal/application.py Outdated Show resolved Hide resolved
msal/application.py Outdated Show resolved Hide resolved
msal/application.py Outdated Show resolved Hide resolved
@localden
Copy link
Collaborator

Minor feedback on documentation, @rayluo - other than that, looking good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Specifying a custom redirect URI in the PublicClientApplication
3 participants