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

having both SPA and mobile and desktop redirect URIs leads to an error #212

Closed
ekdnam opened this issue Aug 18, 2023 · 3 comments · Fixed by #239
Closed

having both SPA and mobile and desktop redirect URIs leads to an error #212

ekdnam opened this issue Aug 18, 2023 · 3 comments · Fixed by #239
Assignees
Labels
deployment Issues related to deploying Chat-Copilot external dependency issue Issues related to external dependencies (e.g. Azure) question Further information is requested

Comments

@ekdnam
Copy link

ekdnam commented Aug 18, 2023

Describe the bug
A clear and concise description of what the bug is.

I followed both the Azure Active Directory app registration procedures as stated in ./README.md and ./tools/importdocument/README.md.

Specifically:
From ./README.md

Supported account types: "Accounts in any organizational directory (Any Azure AD directory - Multitenant) and personal Microsoft accounts (e.g. Skype, Xbox)"
Redirect URI (optional): Single-page application (SPA) and use http://localhost:3000/.

and ./tools/importdocument/README.md

A registered App in Azure Portal (https://learn.microsoft.com/azure/active-directory/develop/quickstart-register-app)
Select Mobile and desktop applications as platform type, and the Redirect URI will be http://localhost
Select Accounts in any organizational directory (Any Azure AD directory - Multitenant) and personal Microsoft accounts (e.g. Skype, Xbox) as the supported account type for this sample.
Note the Application (client) ID from your app registration.

If there are both, it leads to the error

invalid_request: 90023 - [2023-08-18 13:08:17Z]: AADSTS90023: Cross-origin token redemption is permitted only for the 'Single-Page Application' client-type or 'Native' client-type with origin registered in AllowedOriginForNativeAppCorsRequestInOAuthToken allow list.
Trace ID: ---
Correlation ID: ---
Timestamp: 2023-08-18 13:08:17Z - Correlation ID: --- - Trace ID: ---
AuthError@http://localhost:3000/static/js/bundle.js:34540:20
ServerError@http://localhost:3000/static/js/bundle.js:35580:24
./node_modules/@azure/msal-common/dist/response/ResponseHandler.js/ResponseHandler.prototype.validateTokenResponse@http://localhost:3000/static/js/bundle.js:36903:13
./node_modules/@azure/msal-common/dist/client/AuthorizationCodeClient.js/AuthorizationCodeClient.prototype.acquireToken/AuthorizationCodeClient</<@http://localhost:3000/static/js/bundle.js:33038:29
step@http://localhost:3000/static/js/bundle.js:28896:17
./node_modules/@azure/msal-common/dist/_virtual/_tslib.js/__generator/verb/<@http://localhost:3000/static/js/bundle.js:28845:14
fulfilled@http://localhost:3000/static/js/bundle.js:28804:24

However if I remove the Mobile and desktop applications redirect URI and just keep SPA, the app works fine.

How to resolve?

To Reproduce
Steps to reproduce the behavior:

  1. Described above

Expected behavior
A clear and concise description of what you expected to happen.

Screenshots
If applicable, add screenshots to help explain your problem.

Platform

  • OS: MacOS
  • IDE: VS Code
  • Language: C#
  • Source: main branch of chat-copilot

Additional context
Add any other context about the problem here.

@crickman crickman self-assigned this Aug 18, 2023
@crickman crickman added question Further information is requested deployment Issues related to deploying Chat-Copilot external dependency issue Issues related to external dependencies (e.g. Azure) labels Aug 18, 2023
@crickman
Copy link
Contributor

Thank you for this question. We are always looking to improve our deployment documentation and process. I will follow-up after looking into this.

@ekdnam
Copy link
Author

ekdnam commented Aug 18, 2023

Yes please let me know if you need anything from my side

@gitri-ms
Copy link
Collaborator

Hi @ekdnam, I believe the solution here is that you need two separate app registrations: one for Chat Copilot that uses the SPA platform type, and another for the import document tool that uses the Mobile/Desktop platform type. (As you've seen, trying to combine the two causes problems with token redemption in Chat Copilot.) I will update the README to make this clearer.

github-merge-queue bot pushed a commit that referenced this issue Aug 23, 2023
### Motivation and Context
The recent changes to the authentication model in Chat Copilot
effiectively broke the import document tool. This change fixes it, by
enabling users to:
- Use the tool with an unauthenticated local instance of Chat Copilot
without signing in
- Use the tool with an Azure-deployed instance of Chat Copilot with
Azure AD authentication enabled

Fixes #212, fixes #231

### Description

<!-- Describe your changes, the overall approach, the underlying design.
These notes will help understanding how your code works. Thanks! -->

- Adds code paths to authenticate user (or not) depending on
configuration
- Removes user name/info from document import form
- Updates README with instructions for setting up the app registration
for import document tool and configuring appsettings.json

### Contribution Checklist

<!-- Before submitting this PR, please make sure: -->

- [x] The code builds clean without any errors or warnings
- [x] The PR follows the [Contribution
Guidelines](https://github.com/microsoft/copilot-chat/blob/main/CONTRIBUTING.md)
and the [pre-submission formatting
script](https://github.com/microsoft/copilot-chat/blob/main/CONTRIBUTING.md#development-scripts)
raises no violations
- [x] All unit tests pass, and I have added new tests where possible -
N/A, but both supported scenarios have been tested
- [x] I didn't break anyone 😄
teamleader-dev pushed a commit to vlink-group/chat-copilot that referenced this issue Oct 7, 2024
### Motivation and Context
The recent changes to the authentication model in Chat Copilot
effiectively broke the import document tool. This change fixes it, by
enabling users to:
- Use the tool with an unauthenticated local instance of Chat Copilot
without signing in
- Use the tool with an Azure-deployed instance of Chat Copilot with
Azure AD authentication enabled

Fixes microsoft#212, fixes microsoft#231

### Description

<!-- Describe your changes, the overall approach, the underlying design.
These notes will help understanding how your code works. Thanks! -->

- Adds code paths to authenticate user (or not) depending on
configuration
- Removes user name/info from document import form
- Updates README with instructions for setting up the app registration
for import document tool and configuring appsettings.json

### Contribution Checklist

<!-- Before submitting this PR, please make sure: -->

- [x] The code builds clean without any errors or warnings
- [x] The PR follows the [Contribution
Guidelines](https://github.com/microsoft/copilot-chat/blob/main/CONTRIBUTING.md)
and the [pre-submission formatting
script](https://github.com/microsoft/copilot-chat/blob/main/CONTRIBUTING.md#development-scripts)
raises no violations
- [x] All unit tests pass, and I have added new tests where possible -
N/A, but both supported scenarios have been tested
- [x] I didn't break anyone 😄
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployment Issues related to deploying Chat-Copilot external dependency issue Issues related to external dependencies (e.g. Azure) question Further information is requested
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

3 participants