197g / 197g/oxide-auth

Explicitly declare which values can be trusted

Offen
#184 0 Kommentare 1 Reaktion 0 zugewiesene Personen Auf GitHub ansehen
improvement
Vorherrschende Sprache
Rust
Sterne
783
Forks
102
PR-Merge-Kennzahlen
Keine gemergten PRs in 30 T.

Beschreibung

# 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` 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

Beitragsleitfaden

Beitragsleitfaden öffnen

Bewertung

Dieses Issue wurde noch nicht bewertet.

Neue Issues direkt in Ihr Postfach

Eine kurze Übersicht über anfängerfreundliche GitHub-Issues.