oauth-wg / oauth-browser-based-apps

Best practices for OAuth in Browser-Based Apps
https://datatracker.ietf.org/doc/html/draft-ietf-oauth-browser-based-apps
Other
22 stars 12 forks source link

Restructured document #23

Closed philippederyck closed 10 months ago

philippederyck commented 11 months ago

Restructured the document and added a few TODOs following the discussion with Aaron Parecki at OSW2023.

Note that this restructuring is just a first step, leaving the detailed text to be reworded for consistency

aaronpk commented 10 months ago

Thanks, I like this for the most part.

I am not sure I agree with moving the Implicit Flow section into the "Alternative Architecture Patterns" section, since that makes it look like a legitimate choice. The idea of including it in the "Security Considerations" section was to provide the historical context for it without making it look like a current recommendation. If you are thinking that it isn't appropriate in the Security Considerations section, then maybe we can include it as a new top-level section.

aaronpk commented 10 months ago

FYI I pushed this to a branch to trigger the auto-build to review it in the final form, you can view it here:

https://drafts.oauth.net/oauth-browser-based-apps/restructuring/draft-ietf-oauth-browser-based-apps.html

philippederyck commented 10 months ago

My bad for the delay, I glanced this thread before and assumed that your last comment implied it was being merged. Otherwise I would have given feedback sooner.

I agree with your comment on the Implicit flow being a bad fit. How about the following changes:

aaronpk commented 10 months ago

Yes that sounds good to me. I think we also need to rename the header "First-Party Applications" to "Resource Owner Password Grant" in that case.

aaronpk commented 10 months ago

I actually think we can keep the "same-domain" pattern in the "discouraged" section, since the point of that section was to demonstrate why you might want to use OAuth even in the same-domain situation.

I just pushed a commit to this PR with the reworked headers, so I'll go ahead and merge this now!