Problem/Motivation

Currently when a Resource Owner/Password flow fails due to an invalid username/password, a 400 status code is returned with invalid_credentials set as the error type. To conform to the OAuth2 spec this should result in a invalid_grant error.

Simple oauth raises this error here: https://git.drupalcode.org/project/simple_oauth/-/blob/5.x/src/Repositor...

The league/oauth2-server library returned this same error until it was changed in v8 as a breaking change. There is lots more info on the change in this PR: https://github.com/thephpleague/oauth2-server/pull/967 For some reason they continued to provide the InvalidCredentials error with the library which explains why simple_oauth has continued using it.

Unfortunately this may be considered a breaking change to simple_oauth, too, and need to wait for a major release?

Steps to reproduce

Perform a Password grant with invalid user credentials and inspect the error response.

Proposed resolution

Throw OAuthServerException::invalidGrant(); instead.

Remaining tasks

Update tests for correct error.

User interface changes

N/A

API changes

N/A

Data model changes

N/A

CommentFileSizeAuthor
#12 3193609-12.patch390 bytesm.stenta
#11 3193609-11.patch2.23 KBpaul121

Issue fork simple_oauth-3193609

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

paul121 created an issue. See original summary.

paul121 opened merge request !8

paul121’s picture

paul121 commented

I pushed another commit that just returns NULL from the UserRepository if the user was not authenticated. Oauth2-server handles that accordingly and throws OAuthServerException::invalidGrant().

ptmkenny’s picture

ptmkenny commented
Status: Active » Needs review
bradjones1’s picture

bradjones1
Primary language English
commented
Parent issue: » #3217957: Roadmap for Updated Dependencies
bradjones1’s picture

bradjones1
Primary language English
commented
Issue tags: +Spec Compliance
bradjones1’s picture

bradjones1
Primary language English
commented
Status: Needs review » Active

This would change in #3230707: 5.x broken on php 8 due to incompatibility with lcobucci/jwt v4 via league/oauth2-server ^8.2 at https://git.drupalcode.org/project/simple_oauth/-/merge_requests/17/diff...

I am inclined (though, not strongly) that the benefit of getting us up to date on dependencies and making this match the spec outweigh considerations for BC break on implementations. The error is still 4xx series. We can and will include this in a change record for 5.1, but I don't think it requires rolling a new major. Thoughts?

Marking back to Active as we need discussion of this specific point, but the patch is more or less wrapped up into the other issue. I will credit the contributors to this issue in that issue on commit.

paul121’s picture

paul121 commented

We can and will include this in a change record for 5.1, but I don't think it requires rolling a new major. Thoughts?

Great - and yes I tend to agree as well.

Considering that existing OAuth client libraries are expecting the correct 400 error code I agree this "outweighs" the BC considerations. But to be fair, the BC wouldn't effect me. I mostly use existing OAuth client libraries so getting things correct is more important for my use case.

(Just realizing we never actually included this patch for farmOS - we are still "alpha", but it would be nice to have this included before our "beta" release, so it isn't a BC to our users.)

paul121’s picture

paul121 commented

Thank you for your work on this @bradjones1!

bradjones1’s picture

bradjones1
Primary language English
commented

You're welcome. farmOS looks really neat; it's cool to think about how this software is used in different ways.

paul121’s picture

paul121 commented
StatusFileSize
new 3193609-11.patch2.23 KB

Uploading a patch file that can be used for now.

m.stenta’s picture

m.stenta
he/him
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new 3193609-12.patch390 bytes

Patch from #11 does not apply to 5.1.0-beta1.

The changes to tests/src/Functional/PasswordFunctionalTest.php were incorporated via #3230707: 5.x broken on php 8 due to incompatibility with lcobucci/jwt v4 via league/oauth2-server ^8.2: https://git.drupalcode.org/project/simple_oauth/-/commit/ca09275#cda504b...

However, the changes to src/Repositories/UserRepository.php were not:

--- a/src/Repositories/UserRepository.php
+++ b/src/Repositories/UserRepository.php
@@ -36,7 +36,7 @@ class UserRepository implements UserRepositoryInterface {
 
       return $user;
     }
-    throw OAuthServerException::invalidCredentials();
+    throw OAuthServerException::invalidGrant();
   }
 
 }

Re-rolled the patch to only include that change.

paul121’s picture

paul121 commented

Ah @m.stenta I'm just realizing this, but that patch file actually includes two patch files (one for each commit in my MR?) - the last commit simplified things by just returning NULL instead:

-    throw OAuthServerException::invalidGrant();
+    return NULL;

But still, this is a good catch. Looking at the recent commits to 5.1.0-beta1 I don't see where this change (or equivalent) was made, but since the tests are working, I guess I must be missing something. Hmm!

paul121’s picture

paul121 commented

Looking at the recent commits to 5.1.0-beta1 I don't see where this change (or equivalent) was made, but since the tests are working, I guess I must be missing something.

Welp, I ran the password functional tests locally on 5.1.0-beta1 and they are failing on this:

1) Drupal\Tests\simple_oauth\Functional\PasswordFunctionalTest::testInvalidPasswordGrant
Correct error code invalid_grant for username.
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'invalid_grant'
+'invalid_credentials'

  • bradjones1 committed 2779a20 on 5.x authored by m.stenta 
    Issue #3193609 by paul121, m.stenta: invalid_credentials error does not...
bradjones1’s picture

bradjones1
Primary language English
commented
Status: Needs review » Fixed

Yes, returning NULL is correct here. The league package updated this to be spec compliant, and we just pulled that in.

I can't recreate this test failure locally. I need to work on getting CI setup external from Drupal, but the requirement for ext-sodium means we cant use DrupalCI presently.

I will ensure to continue to run the test suite while rolling releases.

m.stenta’s picture

m.stenta
he/him
Primary language English
commented

Thanks @bradjones1!

bradjones1’s picture

bradjones1
Primary language English
commented

@paul121 I opened #3257003: Password grant tests are frail for the random test failures. I noticed this tonight on 5.1.x.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.