fix GH #16 undef of client_id should be undef client

overriding verify_client to always return 0 still allows an access token
to be generated if jwt_secret it supplied because when we call the
verify_client method we were undef'ing the wrong variable when the
verify_client returned 0

add test cases to cover above issue, bump VERSION and Changes for CPAN
release

Changes

@@ -1,5 +1,10 @@
 Revision history for Mojolicious-Plugin-OAuth2-Server
 
+0.38     2017-06-01
+        - Fix combination of verify_client and jwt_secret causing tokens
+          to be generated when verify_client return 0 for client_credentials
+          grant
+
 0.37     2017-05-12
         - Add support for jwt_claims callback in config
           (see jwt_claims_cb in Net::OAuth2::AuthorizationServer)

MANIFEST

@@ -22,4 +22,5 @@ t/100_implicit_grant_basic.t
 t/110_implicit_grant_overrides.t
 t/120_client_credentials_grant_basic.t
 t/130_client_credentials_grant_overrides.t
+t/140_gh_16_client_credentials_force_bad_client.t
 t/AllTests.pm

README.md

@@ -8,7 +8,7 @@ Authorization Server / Resource Server with Mojolicious
 
 # VERSION
 
-0.37
+0.38
 
 # SYNOPSIS
 

lib/Mojolicious/Plugin/OAuth2/Server.pm

@@ -11,7 +11,7 @@ Authorization Server / Resource Server with Mojolicious
 
 =head1 VERSION
 
-0.37
+0.38
 
 =head1 SYNOPSIS
 
@@ -101,7 +101,7 @@ use Mojo::Util qw/ b64_decode /;
 use Net::OAuth2::AuthorizationServer;
 use Carp qw/ croak /;
 
-our $VERSION = '0.37';
+our $VERSION = '0.38';
 
 my ( $AuthCodeGrant,$PasswordGrant,$ImplicitGrant,$ClientCredentialsGrant,$Grant,$JWTCallback );
 
@@ -509,7 +509,7 @@ sub _client_credentials_from_header {
 
   if ( my $auth_header = $self->req->headers->header( 'Authorization' ) ) {
     if ( my ( $encoded_details ) = ( split( 'Basic ',$auth_header ) )[1] ) {
-      my $decoded_details = b64_decode( $encoded_details );
+      my $decoded_details = b64_decode( $encoded_details // '' );
       my ( $client_id,$client_secret ) = split( ':',$decoded_details );
       return ( $client_id,$client_secret );
     }
@@ -558,7 +558,7 @@ sub _verify_credentials {
       scopes          => $scope,
     );
 
-    undef( $client_id ) if ! $res;
+    undef( $client ) if ! $res;
 
   } else {
     ( $client,$error,$scope,$user_id ) = $Grant->verify_auth_code(

t/140_gh_16_client_credentials_force_bad_client.t

@@ -0,0 +1,78 @@
+#!perl
+
+use strict;
+use warnings;
+
+use Mojolicious::Lite;
+use Test::More;
+use FindBin qw/ $Bin /;
+use Mojo::Util qw/ b64_encode /;
+use lib $Bin;
+use AllTests;
+
+my $verify_client_sub = sub {
+  return ( 0,'unauthorized_client' );
+};
+
+my $token_route = '/o/token';
+
+MOJO_APP: {
+  # plugin configuration
+  plugin 'OAuth2::Server' => {
+    args_as_hash        => 0,
+    access_token_route  => $token_route,
+    verify_client       => $verify_client_sub,
+    jwt_secret          => 'eio',
+    jwt_claims          => sub { return ( iss => "https://localhost:5001" ) },
+  };
+
+  group {
+    # /api - must be authorized
+    under '/api' => sub {
+      my ( $c ) = @_;
+      return 1 if $c->oauth;
+      $c->render( status => 401, text => 'Unauthorized' );
+      return undef;
+    };
+
+    get '/eat' => sub { shift->render( text => "food"); };
+  };
+
+  # /sleep - must be authorized and have sleep scope
+  get '/api/sleep' => sub {
+    my ( $c ) = @_;
+    $c->oauth( 'sleep' )
+      || $c->render( status => 401, text => 'You cannot sleep' );
+
+    $c->render( text => "bed" );
+  };
+};
+
+my $t = AllTests::run({
+  access_token_route => $token_route,
+  grant_type         => 'client_credentials',
+  skip_revoke_tests  => 1, # there is no auth code
+  no_200_responses   => 1,
+});
+
+
+# posting 
+$t->post_ok(
+	$token_route => {
+		Authorization => ( "Basic " . b64_encode( join( ':',2,'wrong' ),'' ) )
+	} => form => {
+		grant_type => 'client_credentials',
+	}
+)
+	->status_isnt( 200 )
+;
+
+is(
+	$t->tx->res->json->{error},
+	'invalid_request',
+	'trying to get token gives error'
+);
+
+done_testing();
+
+# vim: ts=2:sw=2:et

t/AllTests.pm

@@ -126,7 +126,7 @@ sub run {
       $tx->req->headers->header( 'Authorization' => "Bearer $access_token" );
     });
 
-    $t->get_ok('/api/eat')->status_is( 200 );
+    $t->get_ok('/api/eat')->status_is( $args->{no_200_responses} ? 401 : 200 );
     $t->get_ok('/api/sleep')->status_is( 401 );
 
     return;
@@ -203,27 +203,39 @@ sub run {
   }
 
   $t->post_ok( @post_args )
-    ->status_is( 200 )
+    ->status_is( $args->{no_200_responses} ? 400 : 200 )
     ->header_is( 'Cache-Control' => 'no-store' )
     ->header_is( 'Pragma'        => 'no-cache' )
   ;
 
-  cmp_deeply(
-    $t->tx->res->json,
-    {
-      access_token  => re( '^.+$' ),
-      token_type    => 'Bearer',
-      expires_in    => '3600',
-      ( $grant_type eq 'client_credentials'
-        ? ()
-        : ( refresh_token => re( '^.+$' ) )
-      ),
-    },
-    'json_is_deeply'
-  );
+  if ( $args->{no_200_responses} ) {
 
-  my $access_token  = $t->tx->res->json->{access_token};
-  my $refresh_token = $t->tx->res->json->{refresh_token};
+    cmp_deeply(
+      $t->tx->res->json,
+      {
+        'error' => 'unauthorized_client',
+      },
+      'json_is_deeply'
+    );
+
+  } else {
+    cmp_deeply(
+      $t->tx->res->json,
+      {
+        access_token  => re( '^.+$' ),
+        token_type    => 'Bearer',
+        expires_in    => '3600',
+        ( $grant_type eq 'client_credentials'
+          ? ()
+          : ( refresh_token => re( '^.+$' ) )
+        ),
+      },
+      'json_is_deeply'
+    );
+  }
+
+  my $access_token  = $t->tx->res->json->{access_token} // '';
+  my $refresh_token = $t->tx->res->json->{refresh_token} // '';
 
   note( "don't use access token to access route" );
   $t->get_ok('/api/eat')->status_is( 401 );
@@ -236,7 +248,7 @@ sub run {
     $tx->req->headers->header( 'Authorization' => "Bearer $access_token" );
   });
 
-  $t->get_ok('/api/eat')->status_is( 200 );
+  $t->get_ok('/api/eat')->status_is( $args->{no_200_responses} ? 401 : 200 );
   $t->get_ok('/api/sleep')->status_is( 401 );
 
   if ( $grant_type ne 'client_credentials' ) {
@@ -279,7 +291,7 @@ sub run {
     isnt( $t->tx->res->json->{refresh_token},$refresh_token,'new refresh_token' );
   }
 
-  return if $args->{skip_revoke_tests};
+  return $t if $args->{skip_revoke_tests};
 
   my $new_access_token  = $t->tx->res->json->{access_token};
   my $new_refresh_token = $t->tx->res->json->{refresh_token};
@@ -309,6 +321,7 @@ sub run {
   $t->get_ok('/api/eat')->status_is( 401 );
   $t->get_ok('/api/sleep')->status_is( 401 );
 
+  return $t;
 }
 
 1;