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

feat: Adds a prototype for token based auth #34

Merged
merged 3 commits into from
Nov 9, 2023

Conversation

nickfloyd
Copy link
Collaborator

@nickfloyd nickfloyd commented Nov 8, 2023

What

This is a quick take on implementing a token auth provider. Other providers will need to be implemented. It does 3 fundamental things to help the generated SDK call the API

  1. Creates a provider to store auth info in as requests are being made
  2. Adds a version header - should be abstracted or at least configured outside of the provider
  3. Adds the auth header for each request thought the instantiation of the HttpClientRequestAdapter.

Why

The provider pattern a standard approach to providing extended functionality for things like this, it is also what is recommended by our Kiota friends.

Implementation details

  • The sample app requires that you have a token set in your environment named GITHUB_TOKEN
  • There is still an issue with WebhookConfig.cs post generation where the using statement using Microsoft.Kiota.Abstractions; should be qualified with using Microsoft.Kiota.Abstractions.Serialization; - we need a middleware for this and to investigate why this is happening with that model only.

cc: @baywet | if you have time look just for a gut check on if we're headed the right direction.

@nickfloyd nickfloyd requested a review from kfcampbell November 8, 2023 15:05
@nickfloyd nickfloyd self-assigned this Nov 8, 2023
Copy link
Member

@kfcampbell kfcampbell left a comment

Choose a reason for hiding this comment

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

This looks great, thank you!

@kfcampbell kfcampbell merged commit ee8039d into main Nov 9, 2023
1 check passed
@kfcampbell kfcampbell deleted the add-basic-auth-prototype branch November 9, 2023 00:20
@baywet
Copy link

baywet commented Nov 9, 2023

Didn't get to this on time, but I'll provide my feedback anyway.
I think this is a great start, the only thing is it mixes concerns.

http headers

It'd be better if the user agent/API version work was in its own middleware handler like the retry handler for instance, you could then build a "GitHub client factory", which would rely on the kiota client factory to add this middleware. And derive from the request adapter to call this factory instead of the default one.
This would allow for reusability of the version/user agent behavior, regardless of the authentication method.

other authentication schemes

Looking at the TODO comments:

  • Kiota abstractions already provides an anonymous authentication provider.
  • Kiota abstractions provides a base bearer authentication provider, you just need to derive from it and implement IAccesTokenProvider, this saves a little bit of code.
  • @andreaTP (from Red Hat) already implemented a basic authentication provider in Java, maybe we could mutualize efforts here.
  • I have already implemented a device code flow provider as part of my demo work, maybe you could clear it up and use that if you want https://github.com/baywet/GitHubTodoDemo/tree/main/dotnet/GitHubAuthentication

All in all I'm not sure what the goal of this repository is, if it's to demo the feasibility, I think the code is good enough as is. If it's to provide production level libraries, I think you should seriously consider the http headers recommendations.

@nickfloyd
Copy link
Collaborator Author

@baywet This is so so good! Thanks for the heads up on all of this.... it seems like we have several spring boards and approaches that I need to try.

RE: The header work, I totally agree, I was thinking about it last night and knew that it needs to be broken out. I'm going to go round 2 on this and see what I can hammer out. Thank you for the orientation on this!

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

Successfully merging this pull request may close these issues.

3 participants