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

[Plugin generation] Kiota should warn users when api contains unsupported auth flows #5403

Closed
Tracked by #5021
SLdragon opened this issue Sep 12, 2024 · 18 comments · Fixed by #5768
Closed
Tracked by #5021

[Plugin generation] Kiota should warn users when api contains unsupported auth flows #5403

SLdragon opened this issue Sep 12, 2024 · 18 comments · Fixed by #5768
Assignees
Labels
type:bug A broken experience vscode-extension Work related to the vscode-extension WIP

Comments

@SLdragon
Copy link

SLdragon commented Sep 12, 2024

What are you generating using Kiota, clients or plugins?

Kiota plugin

In what context or format are you using Kiota?

Windows executable

Client library/SDK language

None

Describe the bug

Copilot not support OAuth with implicit flow, currently it only support OAuth with authcode flow. Kiota should warn if user selected API contains unsupported auth flow like OAuth with implicit flow.

components:
  securitySchemes:
    oAuth2AuthImplicit:  
      type: oauth2
      description: This API uses OAuth 2 with the implicit grant flow. [More info](https://api.example.com/docs/auth)
      flows:
        implicit:   # <---- OAuth flow(authorizationCode, implicit, password or clientCredentials)
          authorizationUrl: https://api.example.com/oauth2/authorize
          scopes:
            read_pets: read your pets
            write_pets: modify pets in your account

Sample schema:
https://raw.githubusercontent.com/SLdragon/example-openapi-spec/main/oauth-implicit.yaml

Related bug:
#5288
#5070
#5162

Expected behavior

Kiota should generate API Plugins and neither warn nor error when the OpenAPI description has no security definition.
Kiota should generate API Plugins and warn users when the specified auth flow is different from OAuth code flow and HTTP bearer token (current supported auth).
Kiota should generate API Plugins and warn users when there is a combination of two auth methods for an endpoint (AND scenario, e.g., API Key + OAuth for calling a #GET operation)."

How to reproduce

Open sample schema: https://raw.githubusercontent.com/SLdragon/example-openapi-spec/main/oauth-implicit.yaml -> click generate plugin

Open API description file

No response

Kiota Version

v1.18.100000002

Latest Kiota version known to work for scenario above?(Not required)

No response

Known Workarounds

No response

Configuration

No response

Debug output

Click to expand log ```
</details>


### Other information

_No response_
@SLdragon SLdragon added status:waiting-for-triage An issue that is yet to be reviewed or assigned type:bug A broken experience labels Sep 12, 2024
@github-project-automation github-project-automation bot moved this to Needs Triage 🔍 in Kiota Sep 12, 2024
@thewahome thewahome self-assigned this Sep 12, 2024
@thewahome thewahome moved this from Needs Triage 🔍 to Todo 📃 in Kiota Sep 12, 2024
@thewahome thewahome added the vscode-extension Work related to the vscode-extension label Sep 12, 2024
@petrhollayms petrhollayms added this to the Kiota v1.18.1 milestone Sep 12, 2024
@thewahome thewahome moved this from Todo 📃 to In Progress 🚧 in Kiota Sep 12, 2024
@petrhollayms
Copy link
Contributor

The original issue for checking the authentication type #5070 was underspecified, TTK really only supports oauth2 with authcode flow when looking into https://github.com/OfficeDev/teams-toolkit/blob/7422a1dffb7d54ca5e926e025fcdc72ae0380e17/packages/spec-parser/src/manifestUpdater.ts#L115

@sebastienlevert , @darrelmiller Please confirm Sydney does not support OAuth with implicit flow for GA.

@petrhollayms petrhollayms added pm-input-needed and removed status:waiting-for-triage An issue that is yet to be reviewed or assigned labels Sep 12, 2024
@darrelmiller
Copy link
Member

darrelmiller commented Sep 12, 2024

Implicit OAuth2 flow is not supported by Copilot. However, just because an OpenAPI does not say the API supports OAuth2 AuthCode does not mean the API doesn't support it. We should not fail to generate a Plugin because of this.

There are many OpenAPI descriptions that do not describe the security capabilities of an API. In fact only 25% of OpenAPI descriptions contain security information. Security information is often conveyed out-of-band.

@petrhollayms
Copy link
Contributor

Thanks @darrelmiller , so closing as WON'T DO for GA now. We can come back later and revisit if needed.

@petrhollayms petrhollayms moved this from In Progress 🚧 to Closed ⛔ in Kiota Sep 12, 2024
@github-project-automation github-project-automation bot moved this from Closed ⛔ to Done ✔️ in Kiota Sep 12, 2024
@petrhollayms petrhollayms closed this as not planned Won't fix, can't repro, duplicate, stale Sep 12, 2024
@petrhollayms petrhollayms moved this from Done ✔️ to Closed ⛔ in Kiota Sep 12, 2024
@petrhollayms petrhollayms removed this from the Kiota v1.18.1 milestone Sep 12, 2024
@SLdragon
Copy link
Author

There are many OpenAPI descriptions that do not describe the security capabilities of an API. In fact only 25% of OpenAPI descriptions contain security information. Security information is often conveyed out-of-band.

@darrelmiller , I think you're right If a user doesn't specify security in their API spec, it doesn't imply the API has no authentication. However, if the API spec includes authentication, we should assume it only supports the specified auth schemes. We could either throw an error or warn the user if an unsupported auth scheme is used, to prevent errors when their plugin runs in Copilot.

Currently, Kiota generates an auth code flow auth property in the ai-plugin.json file when encountering an implicit OAuth flow in OpenAPI spec without throw error or warning messages, potentially causing unexpected behavior when running in Copilot.

If we want to avoid blocking the user from generating the plugin file, it's advisable to display a warning message and generate the ai-plugin file without authentication. This allows the user to add the authentication details themselves later.

@maisarissi maisarissi reopened this Oct 2, 2024
@github-project-automation github-project-automation bot moved this from Closed ⛔ to In Progress 🚧 in Kiota Oct 2, 2024
@maisarissi
Copy link
Contributor

maisarissi commented Oct 2, 2024

I agree with @SLdragon . We shouldn't fail on OpenAPI descriptions that doesn't specify security, but it the security is specified, we should check and just create plugins when the authentication flow is supported.
@petrhollayms could you please prioritize this work?

@baywet baywet moved this from In Progress 🚧 to Todo 📃 in Kiota Oct 2, 2024
@thewahome thewahome self-assigned this Oct 28, 2024
@darrelmiller
Copy link
Member

@SLdragon You could, but you would be violating the spirit of the OpenAPI specification. consider a different example. If an operation declares that it returns 200 and a 400, should we assume that it will never return a 500? Making that assumption would be a mistake.

@darrelmiller
Copy link
Member

Also there seem to be two different issues being discussed here:

  1. Should we fail if implicit flow is supported?
  2. Should we fail if some security schemes are described but Copilot does not support any of them?

The first is obviously not something we should do.

The second depends on whether you believe that when an author provides OpenAPI security information in an OpenAPI description that it is required to be exhaustive. The fact is, it is not. I have no issue emitting a warning that there appear to be no compatible security schemes but to emit and error would be wrong.

@thewahome
Copy link
Contributor

@maisarissi @darrelmiller is implicit flow the only one that's not supported?
How do I get the list of supported / unsupported security schemes?

@maisarissi
Copy link
Contributor

This epic has the rules and supported auth flows: #5162

@sebastienlevert
Copy link
Contributor

I don't think we should throw an error here. I think we should warn, but not error out. Thoughts on this @maisarissi?

@maisarissi
Copy link
Contributor

maisarissi commented Oct 31, 2024

After some thought, yes, I do agree we should use warnings but let the user create the plugin. We shouldn't fail generation for "unsupported" auth, including OAuth implicit flow.

So, to summarize:

  • We should generate API Plugins and neither warn nor error when the OpenAPI description has no security definition.
  • We should generate API Plugins and warn users when the specified auth flow is different from OAuth code flow and HTTP bearer token (current supported auth).
  • We should generate API Plugins and warn users when there is a combination of two auth methods for an endpoint (AND scenario, e.g., API Key + OAuth for calling a #GET operation)."

The warning should be emitted for each selected endpoint that does not fill the requirements: <selected_endpoint>: "There appear to be no compatible security schemes for Copilot usage."

This is the same approach used by the manifest validator library.

cc @thewahome

I've also updated the issue title to reflect the ask cc @SLdragon

@maisarissi maisarissi changed the title [Plugin generation] Kiota should throw error when api contains OAuth with implicit flow [Plugin generation] Kiota should warn users when api contains unsupported auth flows Oct 31, 2024
@SLdragon
Copy link
Author

SLdragon commented Nov 1, 2024

Thank you for all your responses. Recently, we've been redesigning the validation flow in TTK to prevent users from being blocked when the OpenAPI specification includes authentication methods not supported by Copilot.

@thewahome
Copy link
Contributor

@maisarissi is the set of warnings also expected when using the CLI?

@maisarissi
Copy link
Contributor

@maisarissi is the set of warnings also expected when using the CLI?

Yes

@thewahome
Copy link
Contributor

Hello @maisarissi
We have been throwing errors and failing plugin generation when the specified auth is unsupported / we have multiple schemes defined. But from the requirements, they are going to change to warnings

We should generate API Plugins and warn users when the specified auth flow is different from OAuth code flow and HTTP bearer token (current supported auth).
We should generate API Plugins and warn users when there is a combination of two auth methods for an endpoint (AND scenario, e.g., API Key + OAuth for calling a #GET operation)."

Introducing these changes means that we go ahead and generate the plugins but it's unclear to me what authorization we are going to set for the path once we give the warning. What should happen to the path? Should we skip it if unsupported?

@maisarissi
Copy link
Contributor

Hey Charles,

if the auth is supported, continue to do what we are doing right now, if it's not supported, warn the user and create with auth none and let the dev handle that.

@thewahome
Copy link
Contributor

thewahome commented Nov 18, 2024

@maisarissi a plugin could have many paths with the wrong authentication set, I would propose that we log the warnings in the output instead. Since showing the warning would be very intrusive to the user.

Also to make sure that we are only showing warnings from authentication errors at this point, I propose that each error starts with "Authentication warning" so that it ends up being: Authentication warning. <selected_endpoint> - There appear to be no compatible security schemes for Copilot usage.. Using anonymous auth. This will allow us to filter because different warnings are currently being emitted. A sample is Github which has upto 10,000 openapi warnings emitted

@maisarissi
Copy link
Contributor

We absolutely should log the warning so the user can filter and look at them. I like to idea of having the "Authentication warning". Not sure about the "Using anonymous auth" for each endpoint though. We would be repeating that every endpoint with unsupported auth.

@thewahome could we do a single warn with the message: "Incompatible security schemes for Copilot usage detected in the selected endpoints." and maybe add a button "Logs" where the user can click to load the filtered log?

@github-project-automation github-project-automation bot moved this from Todo 📃 to Done ✔️ in Kiota Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug A broken experience vscode-extension Work related to the vscode-extension WIP
Projects
Status: Done ✔️
Development

Successfully merging a pull request may close this issue.

6 participants