pipacs / o2

OAuth 2.0 for Qt
BSD 2-Clause "Simplified" License
317 stars 147 forks source link

O2 Monsanto Company features #81

Closed dakcarto closed 7 years ago

dakcarto commented 7 years ago

Adds three features to o2:

Please let me know if splitting these features up into separate PRs is necessary. While the first two features are pretty straightforward, the third (implicit grant flow enhancements) might need implementation details discussion (works in our testing, though).

Work produced by Boundless Spatial and sponsored by Monsanto Company, in support of an OAuth2 authentication method plugin for the QGIS project.

pipacs commented 7 years ago

Indeed, please separate out the JavaScript changes from the rest of the request. The preferred way of interaction is by using the apps own web view, not a desktop browser, which is next to impossible to control.

dakcarto commented 7 years ago

@pipacs, thanks for the quick reply.

Indeed, please separate out the JavaScript changes from the rest of the request. The preferred way of interaction is by using the apps own web view, not a desktop browser, which is next to impossible to control.

Hmm. That is the opposite of what is recommended in the OAuth specification, where reliance upon a non-standard browser window is discouraged.

See Section 9. Native Applications, in its entirety and specifically:

      An external user-agent may improve completion rate, as the
      resource owner may already have an active session with the
      authorization server, removing the need to re-authenticate.  It
      provides a familiar end-user experience and functionality.  The
      resource owner may also rely on user-agent features or extensions
      to assist with authentication (e.g., password manager, 2-factor
      device reader).

and

      An embedded user-agent poses a security challenge because resource
      owners are authenticating in an unidentified window without access
      to the visual protections found in most external user-agents.  An
      embedded user-agent educates end-users to trust unidentified
      requests for authentication (making phishing attacks easier to
      execute).

As well as the followup Section 10.11. Phishing Attacks, which references the above.

See also Section 10.13. Clickjacking, specifically:

   To prevent this form of attack, native applications SHOULD use
   external browsers instead of embedding browsers within the
   application when requesting end-user authorization.

You do make a good point that many desktop applications developers do not integrate with the user's external browser when implementing OAuth authorization, but instead use an embedded user-agent, regardless of the spec and for the obvious reason it is easier to maintain control.

However, in light of security concerns, I was not able to take that approach with the QGIS authentication plugin I created, and needed the external browser support.

Before I delve into rebasing commits and separating out changesets, any suggestion on how the Add support for implicit grant flow completed by Javascript might be part of o2? I would really not like to resort to supporting a separate fork of o2 just for an enhancement that meets a specification recommendation, if possible.

pipacs commented 7 years ago

I think that part of the RFC is archaic, and, in our context (embedded Qt WebView), plain wrong. One can easily restrict network access within a Qt app, but it's next to impossible to do with an external monstrosity like IE.

On the other hand, you also have a point: the common sense argument is hard to defend against a security review, thanks to the RFC... So let's get this change in, assuming it doesn't break embedded use cases, like examples/sialis.

pipacs commented 7 years ago

... and there is no need to separate out changesets. Maybe you could explain in the README, in a future change, how to use implicit grant flow and implicit_redirect.html, or even supply a complete example. Thanks!

dakcarto commented 7 years ago

@pipacs thanks for the timely review and merge of these features!

I think that part of the RFC is archaic, and, in our context (embedded Qt WebView), plain wrong. One can easily restrict network access within a Qt app, but it's next to impossible to do with an external monstrosity like IE.

Agreed. The QGIS authentication plugin I've created may indeed run into issues with external browser support when eventually used across the myriad of platforms on which QGIS runs. No doubt down the road I will need to look at an embedded web view of some type, probably QWebEngineView, since being based upon Chromium it may offer a suitable standalone experience to make those concerned with any security implications more comfortable.

On the other hand, you also have a point: the common sense argument is hard to defend against a security review, thanks to the RFC... So let's get this change in, assuming it doesn't break embedded use cases, like examples/sialis.

Exactly the rock and hard place I found myself between, when the QGIS plugin work's requirements were drafted.

... and there is no need to separate out changesets. Maybe you could explain in the README, in a future change, how to use implicit grant flow and implicit_redirect.html, or even supply a complete example.

Yes, a good example would help anyone looking to implement this type of implicit/external browser integration. It's a little too complicated to just explain in prose.