Rails API support #992

Borzik · 2017-09-27T08:08:50Z

This was influenced by #893, but I took a bit different approach and extended functionality to other areas. It is still WIP, but I just wanted to share what is done already, and I'll be glad to get some advices.

The main premise for this PR is that usually Rails APIs and frontend apps are running on different domains, so we cannot easily identity users by cookies or something like that (e.g. in our project we use devise_token_auth and authenticate users by tokens).

Switching on api_mode configuration will:

disable request forgery protection, since we assume user is not authenticated by cookies - if he is, there's no much sense in turning api_mode on
remove helpers since we don't rely on HTML views
render JSON with status: redirect and redirect_uri instead of directly redirecting user
disable applications and authorized_applications controller (maybe I will update it to support JSON as well and enable them back - not sure if it's needed though)

As a side effects, this PR:

makes Doorkeeper::Oauth::PreAuthorization serializable by as_json method which will provide all information needed to show preauthorization page
(I need more information on this one from someone more familiar with project) sets redirect_uri on Doorkeeper::Oauth::ErrorResponse - for some reason, it was not set before. If we render ErrorResponse with api_mode, it will render the same status: redirect and redirect_uri with error message

Anyone who will use api_mode, will also need some oauth page (which can be easily implemented as a small SPA and that will be running on the same domain as main frontend application). They can make main frontend app do this job, but I assume it is not worth loading the whole app just to show a single dialogue message with Allow/Deny buttons. It will have to be responsible for:

logging users in
making pre-auth requests which will return client name, and other information
sending POST oauth/authorize
redirecting user whenever they get status: redirect to redirect_uri provided in response

Please let me know your thoughts about it.

houndci-bot · 2017-09-27T08:09:01Z

spec/lib/oauth/pre_authorization_spec.rb

+        expect(json[:response_type]).to eq subject.response_type
+        expect(json[:scope]).to eq subject.scope
+        expect(json[:client_name]).to eq client_name
+        expect(json[:status]).to eq 'preauthorization'


Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2017-09-27T08:09:01Z

spec/lib/oauth/pre_authorization_spec.rb

+      end
+      let(:json){ subject.as_json({}) }
+      it { is_expected.to respond_to :as_json }
+      it 'returns correct values' do


Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2017-09-27T08:09:01Z

spec/lib/oauth/pre_authorization_spec.rb

+        allow(client).to receive(:uid).and_return client_id
+        allow(client).to receive(:name).and_return client_name
+      end
+      let(:json){ subject.as_json({}) }


Space missing to the left of {.

houndci-bot · 2017-09-27T08:09:01Z

spec/lib/oauth/pre_authorization_spec.rb

+
+    describe "as_json" do
+      let(:client_id){ 'client_uid_123' }
+      let(:client_name){ 'Acme Co.' }


Space missing to the left of {.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2017-09-27T08:09:01Z

spec/lib/oauth/pre_authorization_spec.rb

@@ -151,5 +151,25 @@ module Doorkeeper::OAuth
      subject.redirect_uri = nil
      expect(subject).not_to be_authorizable
    end
+
+    describe "as_json" do
+      let(:client_id){ 'client_uid_123' }


Space missing to the left of {.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2017-09-27T08:09:03Z

spec/controllers/authorizations_controller_spec.rb

+    it 'sets redirect_uri to correct value' do
+      redirect_uri = JSON.parse(response.body)['redirect_uri']
+      expect(redirect_uri).to_not be_nil
+      expect(redirect_uri.match(/token_type=(\w+)&?/)[1]).to eq 'bearer'


Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2017-09-27T08:09:03Z

spec/controllers/authorizations_controller_spec.rb

+    end
+
+    it 'sets redirect_uri to correct value' do
+      redirect_uri = JSON.parse(response.body)['redirect_uri']


Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2017-09-27T08:09:03Z

spec/controllers/authorizations_controller_spec.rb

+      expect(JSON.parse(response.body)['status']).to eq('redirect')
+    end
+
+    it 'sets redirect_uri to correct value' do


Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2017-09-27T08:09:03Z

spec/controllers/authorizations_controller_spec.rb

+    end
+
+    it 'sets status to redirect' do
+      expect(JSON.parse(response.body)['status']).to eq('redirect')


Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2017-09-27T08:09:03Z

spec/controllers/authorizations_controller_spec.rb

+      expect(Doorkeeper::AccessToken.count).to be 1
+    end
+
+    it 'sets status to redirect' do


Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2017-09-27T08:14:18Z

spec/lib/oauth/pre_authorization_spec.rb

@@ -151,5 +151,25 @@ module Doorkeeper::OAuth
      subject.redirect_uri = nil
      expect(subject).not_to be_authorizable
    end
+
+    describe 'as_json' do


Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2017-09-27T08:14:18Z

spec/lib/config_spec.rb

@@ -331,4 +331,18 @@
      it { expect(Doorkeeper.configuration.base_controller).to eq('ApplicationController') }
    end
  end
+
+  describe 'api_mode' do
+    it 'is false by default' do


Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2017-09-27T08:14:18Z

spec/lib/config_spec.rb

@@ -331,4 +331,18 @@
      it { expect(Doorkeeper.configuration.base_controller).to eq('ApplicationController') }
    end
  end
+


Trailing whitespace detected.

houndci-bot · 2017-09-27T08:14:18Z

spec/controllers/authorizations_controller_spec.rb

+      expect(Doorkeeper::AccessToken.first.application_id).to eq(client.id)
+    end
+
+    it 'issues the token for the current resource owner' do


Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

JoeWoodward · 2018-01-25T04:59:42Z

No movement on this PR?

nbulaj · 2018-01-27T09:54:27Z

Hi @Borzik . I like this approach more than original PR. Does it still WIP , maybe you need some help? I will look at your questions (aka side-effects) ASAP.

Borzik · 2018-01-27T10:12:15Z

Hi! When I submitted this PR I was working on a feature which was postponed, so I'm not working actively on this at the moment. Since you think this PR is worth attention, I'll look into it again next week, will rebase it on current master, and will try to get it out of WIP status (as I remember, it worked well for me on my app, but it still needs refactoring).

nbulaj · 2018-01-27T10:28:36Z

Great! Let me know it you will need some help.

JoeWoodward · 2018-02-02T07:46:57Z

Just found another issue with this configuration. I have an app that is api only but also one that is api and web. The api only works fine, however, the hybrid I want to remove protect_from forgery for authorization & token & revoke endpoints but would need to still have the routes defined for applications

I think removing the routes should be separately configurable

houndci-bot · 2018-02-02T12:23:47Z

spec/controllers/authorizations_controller_spec.rb

@@ -200,6 +272,44 @@ def translated_error_message(key)
    end
  end

+  describe 'GET #new in API mode with skip_authorization true' do


Block has too many lines. [29/25]

houndci-bot · 2018-02-02T12:23:47Z

spec/controllers/authorizations_controller_spec.rb

+    end
+
+    it 'includes correct redirect URI' do
+      expect(redirect_uri).to match(%r{^#{client.redirect_uri}})


Use // around regular expression.

houndci-bot · 2018-02-02T12:23:47Z

spec/controllers/authorizations_controller_spec.rb

+      post :create, client_id: client.uid, response_type: 'token', scope: 'invalid', redirect_uri: client.redirect_uri
+    end
+    let(:response_json_body){ JSON.parse(response.body) }
+    let(:redirect_uri){ response_json_body['redirect_uri'] }


Space missing to the left of {.

houndci-bot · 2018-02-02T12:23:47Z

spec/controllers/authorizations_controller_spec.rb

+      default_scopes_exist :public
+      post :create, client_id: client.uid, response_type: 'token', scope: 'invalid', redirect_uri: client.redirect_uri
+    end
+    let(:response_json_body){ JSON.parse(response.body) }


Space missing to the left of {.

houndci-bot · 2018-02-02T12:23:47Z

spec/controllers/authorizations_controller_spec.rb

+    end
+
+    it 'renders correct redirect uri' do
+      expect(redirect_uri).to match(%r{^#{client.redirect_uri}})


Use // around regular expression.

houndci-bot · 2018-02-02T12:23:48Z

lib/doorkeeper/oauth/error_response.rb

@@ -5,7 +5,8 @@ class ErrorResponse < BaseResponse

      def self.from_request(request, attributes = {})
        state = request.state if request.respond_to?(:state)
-        new(attributes.merge(name: request.error, state: state))
+        redirect_uri = request.redirect_uri if request.respond_to?(:redirect_uri)
+        new(attributes.merge(name: request.error, state: state, redirect_uri: redirect_uri))


Line is too long. [92/80]

houndci-bot · 2018-02-02T12:23:48Z

lib/doorkeeper/oauth/error_response.rb

@@ -5,7 +5,8 @@ class ErrorResponse < BaseResponse

      def self.from_request(request, attributes = {})
        state = request.state if request.respond_to?(:state)
-        new(attributes.merge(name: request.error, state: state))
+        redirect_uri = request.redirect_uri if request.respond_to?(:redirect_uri)


Line is too long. [81/80]

houndci-bot · 2018-02-02T12:23:48Z

app/controllers/doorkeeper/authorizations_controller.rb

-        redirect_to auth.redirect_uri
-      else
-        render json: auth.body, status: auth.status
+        return render(json: { status: :redirect, redirect_uri: auth.redirect_uri }, status: auth.status) if Doorkeeper.configuration.api_mode


Line is too long. [141/80]

houndci-bot · 2018-02-02T12:23:48Z

app/controllers/doorkeeper/authorizations_controller.rb

        end
      else
-        render :error
+        if Doorkeeper.configuration.api_mode
+          render json: @pre_auth.error_response.body[:error_description], status: :bad_request


Line is too long. [94/80]

houndci-bot · 2018-02-02T12:23:48Z

app/controllers/doorkeeper/authorizations_controller.rb

        end
      else
-        render :error
+        if Doorkeeper.configuration.api_mode


Convert if nested inside else to elsif.

houndci-bot · 2018-02-02T12:23:49Z

spec/controllers/authorizations_controller_spec.rb

@@ -200,6 +272,44 @@ def translated_error_message(key)
    end
  end

+  describe 'GET #new in API mode with skip_authorization true' do


Block has too many lines. [29/25]

houndci-bot · 2018-02-02T12:23:49Z

spec/controllers/authorizations_controller_spec.rb

+    end
+
+    it 'includes correct redirect URI' do
+      expect(redirect_uri).to match(%r{^#{client.redirect_uri}})


Use // around regular expression.

houndci-bot · 2018-02-02T12:23:49Z

spec/controllers/authorizations_controller_spec.rb

+      post :create, client_id: client.uid, response_type: 'token', scope: 'invalid', redirect_uri: client.redirect_uri
+    end
+    let(:response_json_body){ JSON.parse(response.body) }
+    let(:redirect_uri){ response_json_body['redirect_uri'] }


Space missing to the left of {.

houndci-bot · 2018-02-02T12:23:49Z

spec/controllers/authorizations_controller_spec.rb

+      default_scopes_exist :public
+      post :create, client_id: client.uid, response_type: 'token', scope: 'invalid', redirect_uri: client.redirect_uri
+    end
+    let(:response_json_body){ JSON.parse(response.body) }


Space missing to the left of {.

houndci-bot · 2018-02-02T12:23:50Z

spec/controllers/authorizations_controller_spec.rb

+    end
+
+    it 'renders correct redirect uri' do
+      expect(redirect_uri).to match(%r{^#{client.redirect_uri}})


Use // around regular expression.

houndci-bot · 2018-02-02T12:23:50Z

lib/doorkeeper/oauth/error_response.rb

@@ -5,7 +5,8 @@ class ErrorResponse < BaseResponse

      def self.from_request(request, attributes = {})
        state = request.state if request.respond_to?(:state)
-        new(attributes.merge(name: request.error, state: state))
+        redirect_uri = request.redirect_uri if request.respond_to?(:redirect_uri)
+        new(attributes.merge(name: request.error, state: state, redirect_uri: redirect_uri))


Line is too long. [92/80]

houndci-bot · 2018-02-02T12:23:50Z

lib/doorkeeper/oauth/error_response.rb

@@ -5,7 +5,8 @@ class ErrorResponse < BaseResponse

      def self.from_request(request, attributes = {})
        state = request.state if request.respond_to?(:state)
-        new(attributes.merge(name: request.error, state: state))
+        redirect_uri = request.redirect_uri if request.respond_to?(:redirect_uri)


Line is too long. [81/80]

houndci-bot · 2018-02-02T12:23:51Z

app/controllers/doorkeeper/authorizations_controller.rb

-        redirect_to auth.redirect_uri
-      else
-        render json: auth.body, status: auth.status
+        return render(json: { status: :redirect, redirect_uri: auth.redirect_uri }, status: auth.status) if Doorkeeper.configuration.api_mode


Line is too long. [141/80]

houndci-bot · 2018-02-02T12:23:51Z

app/controllers/doorkeeper/authorizations_controller.rb

        end
      else
-        render :error
+        if Doorkeeper.configuration.api_mode
+          render json: @pre_auth.error_response.body[:error_description], status: :bad_request


Line is too long. [94/80]

houndci-bot · 2018-02-02T12:23:51Z

app/controllers/doorkeeper/authorizations_controller.rb

        end
      else
-        render :error
+        if Doorkeeper.configuration.api_mode


Convert if nested inside else to elsif.

houndci-bot · 2018-02-02T12:44:12Z

spec/controllers/authorizations_controller_spec.rb

@@ -200,6 +272,46 @@ def translated_error_message(key)
    end
  end

+  describe 'GET #new in API mode with skip_authorization true' do


Block has too many lines. [31/25]

houndci-bot · 2018-02-02T12:44:12Z

lib/doorkeeper/oauth/error_response.rb

+          name: request.error,
+          state: state,
+          redirect_uri: redirect_uri
+        }))


Indent the right brace the same as the first position after the preceding left parenthesis.

houndci-bot · 2018-02-02T12:44:12Z

lib/doorkeeper/oauth/error_response.rb

+          redirect_uri = request.redirect_uri
+        end
+        new(attributes.merge({
+          name: request.error,


Use 2 spaces for indentation in a hash, relative to the first position after the preceding left parenthesis.

houndci-bot · 2018-02-02T12:44:13Z

lib/doorkeeper/oauth/error_response.rb

+        if request.respond_to?(:redirect_uri)
+          redirect_uri = request.redirect_uri
+        end
+        new(attributes.merge({


Redundant curly braces around a hash parameter.

houndci-bot · 2018-02-02T12:44:13Z

app/controllers/doorkeeper/authorizations_controller.rb

-        redirect_to auth.redirect_uri
-      else
-        render json: auth.body, status: auth.status
+        return render(


Favor a normal if-statement over a modifier clause in a multiline statement.

houndci-bot · 2018-02-02T12:44:13Z

app/controllers/doorkeeper/authorizations_controller.rb

        else
          render :new
        end
      else
-        render :error
+        if Doorkeeper.configuration.api_mode


Convert if nested inside else to elsif.

houndci-bot · 2018-02-02T13:00:50Z

lib/doorkeeper/oauth/error_response.rb

+          redirect_uri = request.redirect_uri
+        end
+        new(attributes.merge(
+          name: request.error,


Indent the first parameter one step more than attributes.merge(.

Borzik · 2018-02-02T13:26:12Z

@JoeWoodward
I think such hybrid app support is out of this PR's scope. This update is intended to support Rails applications generated with --api argument only, and to do it in the simplest possible way (by toggling one configuration line).

@nbulaj
I've done some cleanup and fixed most Hound comments.
I don't really like what's going on in app/controllers/doorkeeper/authorizations_controller.rb, specifically this structure:

if success
  if api_mode?
  else
  end
else
  if api_mode?
  else
  end
end

And redirectable? doesn't seem to convey correct meaning here:

if redirectable? and api_mode?
  render ...
elsif redirectable?
  redirect
else
  render
end

If you have any ideas how this can be improved, please let me know.

nbulaj · 2018-02-16T15:08:19Z

Hi @Borzik. Can you please resolve the conflicts?

What about some code issues, you can extract successful and error handling to separate private methods, like

def new
  if pre_auth.authorizable?
    render_successful
   else
     render_error
  end
end

private 

# ...

# Nothing wrong with if elsif else :)
def render_successful
  if skip_authorization? || matching_token?
    auth = authorization.authorize
    redirect_or_render auth
  elsif Doorkeeper.configuration.api_mode
    render json: @pre_auth
  else
    render :new
  end
end

def render_error
  if Doorkeeper.configuration.api_mode
    render json: @pre_auth.error_response.body[:error_description],
           status: :bad_request
  else
    render :error
  end
end

nbulaj · 2018-02-27T07:15:28Z

Hi @Borzik . Could you please rebase with the master to resolve the conflicts? I think it is ready to be merged

pacop · 2018-03-05T11:06:10Z

Interested in seeing this in the new version. Hope that @Borzik can rebase these files. Is sth that could be done to help?

houndci-bot · 2018-03-05T11:55:31Z

lib/doorkeeper/oauth/error_response.rb

@@ -4,7 +4,11 @@ class ErrorResponse < BaseResponse
      include OAuth::Helpers

      def self.from_request(request, attributes = {})
-        new(attributes.merge(name: request.error, state: request.try(:state)))
+        new(attributes.merge(
+            name: request.error,


Indent the first parameter one step more than attributes.merge(.

houndci-bot · 2018-03-05T12:04:47Z

app/controllers/doorkeeper/authorizations_controller.rb

+    def render_error
+      if Doorkeeper.configuration.api_mode
+        render json: @pre_auth.error_response.body[:error_description],
+                status: :bad_request


Align the elements of a hash literal if they span more than one line.

Borzik · 2018-03-05T12:07:48Z

@nbulaj @pacop Done

nbulaj · 2018-03-05T21:14:27Z

Thanks @Borzik ! It would be really great if somebody could test it in his app, maybe @pacop ? Try to use master branch to be sure we didn't break something

houndci-bot reviewed Sep 27, 2017

View reviewed changes

nbulaj mentioned this pull request Jan 27, 2018

Add api_only option #893

Closed

nbulaj added the needs work label Jan 27, 2018

nbulaj self-assigned this Jan 27, 2018

nbulaj force-pushed the master branch from d87618a to 342f887 Compare January 31, 2018 14:57

Borzik force-pushed the rails-api-support branch from c8e3f15 to df79d14 Compare February 2, 2018 12:23

houndci-bot reviewed Feb 2, 2018

View reviewed changes

Borzik force-pushed the rails-api-support branch from df79d14 to 03cdd92 Compare February 2, 2018 12:43

houndci-bot reviewed Feb 2, 2018

View reviewed changes

Borzik force-pushed the rails-api-support branch 3 times, most recently from a49d8aa to d5849e7 Compare February 2, 2018 13:00

houndci-bot reviewed Feb 2, 2018

View reviewed changes

Borzik force-pushed the rails-api-support branch 4 times, most recently from ca7debb to 2a3036e Compare February 2, 2018 13:08

nbulaj added the feature label Feb 4, 2018

nbulaj force-pushed the master branch from e122698 to 11b99da Compare February 7, 2018 09:19

nbulaj added this to the 5.0 milestone Feb 13, 2018

Borzik force-pushed the rails-api-support branch from 2a3036e to ca36a93 Compare March 5, 2018 11:55

houndci-bot reviewed Mar 5, 2018

View reviewed changes

Borzik force-pushed the rails-api-support branch from ca36a93 to 26d4543 Compare March 5, 2018 12:04

houndci-bot reviewed Mar 5, 2018

View reviewed changes

implement api_mode

00fc954

Borzik force-pushed the rails-api-support branch from 26d4543 to 00fc954 Compare March 5, 2018 12:05

Borzik changed the title ~~[WIP] Rails API support~~ Rails API support Mar 5, 2018

nbulaj merged commit ba374aa into doorkeeper-gem:master Mar 5, 2018

tannakartikey mentioned this pull request Mar 9, 2018

Doorkeeper does not initialise with api_only #1047

Closed

pjmartorell mentioned this pull request Oct 27, 2020

Why are applications and authorized_applications routes skipped in api_only mode? #1448

Closed

Rails API support #992

Rails API support #992

Conversation

Borzik commented Sep 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeWoodward commented Jan 25, 2018

nbulaj commented Jan 27, 2018

Borzik commented Jan 27, 2018

nbulaj commented Jan 27, 2018

JoeWoodward commented Feb 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Borzik commented Feb 2, 2018

nbulaj commented Feb 16, 2018 • edited Loading

nbulaj commented Feb 27, 2018 • edited Loading

pacop commented Mar 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Borzik commented Mar 5, 2018

nbulaj commented Mar 5, 2018

JoeWoodward commented Feb 2, 2018 •

edited

Loading

nbulaj commented Feb 16, 2018 •

edited

Loading

nbulaj commented Feb 27, 2018 •

edited

Loading

pacop commented Mar 5, 2018 •

edited

Loading