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

Create farm_api_oauth module #207

Closed
wants to merge 93 commits into from
Closed

Conversation

paul121
Copy link
Member

@paul121 paul121 commented Oct 1, 2019

This module uses the oauth2_server and restws modules to create a farmOS Oauth Server with a client pre-configured for access to the farmOS API.

@paul121 paul121 mentioned this pull request Oct 1, 2019
9 tasks
@paul121
Copy link
Member Author

paul121 commented Oct 1, 2019

This also creates a callback page at /api/authorized for the API Client. Right now it is fairly basic and displays the attributes of the token returned from the server in a Drupal form. This is done by pulling the values from parameters/fragments the server redirects to. Doing this makes it easier to copy out the access_token and refresh_token when configuring more "custom" clients.

This page could be extended to support errors the server returns, when a token isn't returned. For example, not supplying a correct redirect_uri results in an error, as does not supplying valid client credentials. This would help users handle errors the might experience while interfacing with OAuth in farmOS

@paul121 paul121 force-pushed the farm-oauth branch 3 times, most recently from 36dc957 to d49de50 Compare January 23, 2020 17:27
@paul121 paul121 force-pushed the farm-oauth branch 2 times, most recently from 98bc0d2 to 3bc447d Compare January 24, 2020 15:12
@paul121 paul121 requested a review from mstenta January 24, 2020 15:27
@paul121
Copy link
Member Author

paul121 commented Jan 24, 2020

Okay, I've updated this PR to include everything required to enable farm_api_oauth on both new and existing farmOS instances. Summary:

  • oauth2-server-php library added to drupal-org.make
  • xautoload, oauth2_server and restws_oauth2_server contrib modules added to drupal-org.make
  • Fix the deletion of OAuth2 server entities when disabling the farm_api_oauth module - previously this caused errors when disabling and re-enabling the module.
  • Disable the client_credentials OAuth2 grant. The client_secret shouldn't be made public.
  • Disable the implicit OAuth flow as per OAuth2 recommendations. (Authorization code w/ PKCE should be used instead)

also

  • Updates farm_api_farm_info to use the farm_info scope

Additional considerations before we release this:

  • @mstenta and I chatted about enabling JWT with oauth2_server but decided this might make a migration more difficult. We may include this in the D8 OAuth implementation.
  • Should we review the default token lifetimes? Easier to change this now than later. access_token_lifetime = 3600 (6 min), refresh_token_lifetime = 1209600 (14 days)
  • I'd like to further review the issue regarding the redirect_uri in our implementation. EDIT: We've opted to create "managed" OAuth Clients via a drupal hook. This allows 3rd parties to register a redirect_uri with their OAuth Client. This solves the issues described below, as we can now use redirect for the Authorization Flow 🚀
    • Because it is difficult for 3rd parties to create an OAuth2 client on self-hosted farmOS servers, this makes it slightly more challenging to complete the authorization flow.
    • Normally, the 3rd party registers a client that defines a redirect_uri that OAuth2 will use to redirect back to their application. It's bad practice to accept any unregistered redirect_uri because someone who intercepts the request could modify the redirect_uri with malicious intent to steal the authorization_code that is returned (and then used to retrieve an authorization_token)
    • Currently in the Aggregator, I create a pop-up window that redirects to the callback defined by farm_api_oauth at http://hostname/api/authroized - this page displays the access_token parameters that are returned, and with some JS, uses window.opener.postMessage() to send the data back to the browser window that opened the pop-up.
      • Obviously this a bit a more complicated than using the redirect process. One concern is the requirement for 3rd parties to implement this "pop-up window callback process"
    • There is an oauth2 spec that enables Dynamic Client Registration but this is not supported with oauth2_server
      • Perhaps we could create a custom "managed" Dynamic Client Registration process? 3rd parties could POST a redirect_uri to farmOS, which a manager could "enable" via the UI. This redirect_ui would then be added to the farmos_api_client, and allow 3rd parties to authorize via redirect_uri, instead of a popup.

@alexadamsmith
Copy link

This is really exciting progress @paul121 !!

@paul121
Copy link
Member Author

paul121 commented Jan 28, 2020

Quick update:

  • Added hook_farm_oauth_client to allow creation of "managed" OAuth Clients
  • Added config page at /admin/config/farm/oauth to enable & disable "managed" clients
  • farm_api_oauth initializes the server with community-trusted oauth scopes (only farmos_api_client for now)
  • Refactor the module to manage the oauth2_server entity with Drupal Features

@paul121
Copy link
Member Author

paul121 commented Feb 18, 2020

I just rebased this branch off of @mstenta's farmOS-map work: https://github.com/mstenta/farmOS/tree/farmOS-map

Haven't tested the whole drush build process, but assuming that will all still be good.

@mstenta
Copy link
Member

mstenta commented Mar 25, 2020

Merged! See https://www.drupal.org/project/farm/issues/3034214#comment-13523455

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

Successfully merging this pull request may close these issues.

None yet

3 participants