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

Explicitly declare which values can be trusted #184

Open
7 tasks
edward-shen opened this issue Feb 14, 2024 · 0 comments
Open
7 tasks

Explicitly declare which values can be trusted #184

edward-shen opened this issue Feb 14, 2024 · 0 comments
Labels
improvement Improve existing implementation

Comments

@edward-shen
Copy link

Project Improvement

Hey! I've been going through implementing my own (async) implementation with this, and I've been collecting thoughts that I think are actionable to better the crate.

Notably, I'm very thankful for the primitives: once you understand what most pieces do it makes it a lot easier to implement things correctly. I've certainly

However, there's a bunch of things that I felt I was lost on, and had to read source code (and play around with what was being provided) to various traits:

By far the biggest concern is the documentation on the individual traits, specifically on Registrar, Authorizer, and Issuer. The documentation on all these traits currently do not inform a trait implementation whether or not an input can be trusted or not. For example:

  • In Registrar::negotiate, is the Option<Scope> raw user input? Can we directly trust BoundClient to have a server validated redirect uri, or must we validate it before generating a PreGrant?
  • In Issuer::issue, can we trust the Grant provided to us? Where is the Grant.until value being populated from?
  • In Issuer::refresh, can we trust the Grant provided to us?

This probably expands to all traits, but I think these are the most important ones when it comes to getting up and running in a secure manner.

There's also a couple clarifying questions I can't seem to figure out either that I think documentation could be extremely helpful on:

  • In Issuer::recover_refresh for an Issuer that supports refresh tokens, are we to return a Grant with an until value of the refresh token expiration or the session token expiration?
  • In the Issuer's recover flows, do we need to populate all fields of the Grant? As far as I can tell I was able to populate dummy values in the redirect_uri and extensions fields without any issues, though I suspect that's only because I currently am only doing the authorization grant flow.
  • As far as I can tell, a "solicitor" isn't part of the OAuth spec. You can figure it out as the "render-the-oauth-grant/deny-page", but it took a while for me to digest everything.

Finally, I think it would be really, really helpful embedding some of the expectations of the Flow structs. It took me quite a bit of time to figure out that you need to use HTTP Basic authorization to exchange an authorization code into bearer tokens. Suggestions on how long those bearer tokens should last would also be appreciated

Other context

I'm very appreciative of the work done already. It's definitely helped clear things up with OAuth on the server implementation side.

Tracking pull request

  • A pull request does not yet exist
@edward-shen edward-shen added the improvement Improve existing implementation label Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improve existing implementation
Projects
None yet
Development

No branches or pull requests

1 participant