payprop / net-oauth2-authorizationserver

Module to implement core functions of an OAuth2 authorization server
7 stars 10 forks source link

Return normalised auth details from default implementation of verify_access_token #16

Closed sillitoe closed 6 years ago

sillitoe commented 6 years ago

This makes an effort to ensure that the default implementation of _verify_access_token returns a normalised data structure on success (see #20 on Mojolicious::Plugin::OAuth2::Server).

Note: it tries to take into account some apparent differences in keys (eg client => client_id, scope => scopes). It might be neater to try to avoid these key clashes at source (or enforce this in an object rather than hash?). However that seems like a more substantial change - this has the advantage that it does the job with minimal disruption to existing code.

I'm only adding tests here and all tests pass for me.

HOWEVER, this does make 051_jwt_refresh.t fail on Mojolicious::Plugin::OAuth2::Server.

The returned data structure is now different ('client' is now a HashRef rather than Str) - I don't know whether this constitutes a breaking change.

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.05%) to 97.954% when pulling fe9c50196c7385b5a64684047c7b7b7ccdee21da on sillitoe:default_returns_auth_details into fd68982fd7ee835daebe763b09a60916337d2e4a on Humanstate:master.

leejo commented 6 years ago

I really need to get my head back into this code as it's been almost a year since I touched it. The curious thing about the test failure here is if I dump out the, not different, structure the client appears to contain a structure the same as the parent structure but with some differences:

{
  'aud' => undef,
  'client' => {
    'aud' => undef,
    'client' => 'frey',
    'iat' => 1523718346,
    'jti' => 'We8OXobLgGXr87QZd5otaxkC5zH8tIb4',
    'scopes' => [
      'betrayal'
    ],
    'type' => 'refresh',
    'user_id' => undef
  },
  'exp' => 1523721946,
  'iat' => 1523718346,
  'iss' => 'some iss',
  'jti' => 'Grz9H42bU43sojliUWQHlWxNHKsw1Pmj',
  'scopes' => [
    'betrayal'
  ],
  'sub' => 'not the passed user_id',
  'type' => 'access',
  'user_id' => undef
}
sillitoe commented 6 years ago

Ah yes, I noticed this and couldn't immediately find a good reason for it (which was why I flagged it up). I was hoping this was something to do with an aspect of the data flow that I wasn't familiar with - though it doesn't sound like it is.

Let me know if I can help - if you're okay with the general idea of this PR then I can dig around more.