Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Api fixes #17

Merged
merged 3 commits into from

2 participants

@karenc
Owner

Could you have a look at the tests to see if it's what you expect? Especially the api constraints tests. It won't work if you don't have the version in the header? And the tests when default is defined are also a bit strange...

@karenc
Owner

@jpslav Updated routes.rb:

   namespace :api, defaults: {format: 'json'} do
-    scope module: :v1, constraints: ApiConstraints.new(version: 1) do
+    # ApiConstraints.default should be set to true for the latest version,
+    # all other versions should not have default set to true
+    scope module: :v1, constraints: ApiConstraints.new(version: 1, default: true) do
       get '/me' => 'credentials#me'
app/models/api_user.rb
@@ -35,6 +35,10 @@ def initialize(doorkeeper_token, non_doorkeeper_user_proc)
end
end
+ def is_anonymous?
@jpslav Owner
jpslav added a note

At first I put this method in myself, but then I decided against it. An ApiUser is a wrapper of an application "user" and a human user, where the human user may not be set in the case of app-to-app communication. In this latter case, an ApiUser isn't really "known" (signed in) or anonymous, but rather just unspecified. This line of thought led me to write an UnspecifiedUser that was a peer to AnonymousUser, but that didn't end up being that useful.

In the end, I just decided to have two methods in ApiUser: is_human? and is_application?. These tell us if the underlying user is a human or an application. Once we know an ApiUser is a human, we can ask the question is_anonymous? (a.k.a. is_not_signed_in?).

This branching based on is_human? is reflected in the newer AccessPolicy stuff that I'm about to PR.

You added this is_anonymous? method here to handle CredentialsController#me's authorization check. If we keep this controller around, we can replace that call with

AccessPolicy.require_action_allowed!(:me, current_user, current_user)

The first current_user is the requestor of the action, the second is the resource to which access is being requested. Just funny in this case because this is the "me" method.

@jpslav Owner
jpslav added a note

Ok, I'm going slightly crazy. There are no is_human? or is_application? methods. I just check to see if those things are nil.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jpslav jpslav merged commit 70c28ef into openstax:master

1 check passed

Details default The Travis CI build passed
@karenc karenc deleted the karenc:api-fixes branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
9 app/controllers/api/v1/credentials_controller.rb
@@ -1,9 +0,0 @@
-class Api::V1::CredentialsController < Api::V1::OauthBasedApiController
-
- doorkeeper_for :all
-
- def me
- raise SecurityTransgression if current_user.is_anonymous?
- end
-
-end
View
2  app/models/api_user.rb
@@ -75,4 +75,4 @@ def can_sort?(resource)
can_do?(:sort, resource)
end
-end
+end
View
4 app/views/api/v1/credentials/me.json.jbuilder
@@ -1,4 +0,0 @@
-json.uid current_user.id
-json.username current_user.username
-json.first_name current_user.first_name
-json.last_name current_user.last_name
View
4 config/routes.rb
@@ -33,7 +33,9 @@
end
namespace :api, defaults: {format: 'json'} do
- scope module: :v1, constraints: ApiConstraints.new(version: 1) do
+ # ApiConstraints.default should be set to true for the latest version,
+ # all other versions should not have default set to true
+ scope module: :v1, constraints: ApiConstraints.new(version: 1, default: true) do
get '/me' => 'credentials#me'
resources :users, only: [:show, :update] do
View
4 lib/api_constraints.rb
@@ -5,6 +5,6 @@ def initialize(options)
end
def matches?(req)
- @default || req.headers['Accept'].include?("application/vnd.exercises.openstax.v#{@version}")
+ @default || req.headers['Accept'].try(:include?, "application/vnd.exercises.openstax.v#{@version}")
end
-end
+end
View
2  spec/features/add_application_spec.rb
@@ -12,7 +12,7 @@
login_as 'admin'
expect(page).to have_content('Welcome, admin')
visit '/oauth/applications'
- expect(page).to have_content('OAuth2 Provider')
+ expect(page).to have_content('OAuth Applications')
create_new_application
expect(page).to have_content('Application created.')
expect(page).to have_content('Application: example')
View
69 spec/lib/api_constraints_spec.rb
@@ -0,0 +1,69 @@
+require 'spec_helper'
+
+describe ApiConstraints do
+ context 'default is not defined' do
+ let!(:constraints) { ApiConstraints.new(version: 1) }
+ let(:req) { double('Request') }
+ it 'matches if version is correct in the accept headers' do
+ req.stub(:headers).and_return({
+ 'Accept' => 'application/vnd.exercises.openstax.v1'
+ })
+ expect(constraints.matches? req).to be_true
+ end
+
+ it 'does not match if version is incorrect in the accept headers' do
+ req.stub(:headers).and_return({
+ 'Accept' => 'application/vnd.exercises.openstax.v2'
+ })
+ expect(constraints.matches? req).to be_false
+ end
+
+ it 'does not match if version is not defined in the accept headers' do
+ req.stub(:headers).and_return({
+ 'Accept' => '*/*',
+ })
+ expect(constraints.matches? req).to be_false
+ end
+
+ it 'does not match if accept is not in headers' do
+ req.stub(:headers).and_return({
+ 'Host' => 'localhost'
+ })
+ expect(constraints.matches? req).to be_nil
+ end
+ end
+
+ context 'default is defined' do
+ let!(:default) { double('default') }
+ let!(:constraints) { ApiConstraints.new(version: 1, default: default) }
+ let(:req) { double('Request') }
+
+ it 'matches if version is correct in the accept headers' do
+ req.stub(:headers).and_return({
+ 'Accept' => 'application/vnd.exercises.openstax.v1'
+ })
+ expect(constraints.matches? req).to be_true
+ end
+
+ it 'returns default if version is incorrect in the accept headers' do
+ req.stub(:headers).and_return({
+ 'Accept' => 'application/vnd.exercises.openstax.v2'
+ })
+ expect(constraints.matches? req).to eq(default)
+ end
+
+ it 'returns default if version is not defined in the accept headers' do
+ req.stub(:headers).and_return({
+ 'Accept' => '*/*',
+ })
+ expect(constraints.matches? req).to eq(default)
+ end
+
+ it 'returns default if accept is not in headers' do
+ req.stub(:headers).and_return({
+ 'Host' => 'localhost'
+ })
+ expect(constraints.matches? req).to eq(default)
+ end
+ end
+end
Something went wrong with that request. Please try again.