[Bug]: NextCloud 28 seems to be handling the Oauth authentication wrongly.

[colonelm]

⚠️ This issue respects the following points: ⚠️

• This is a bug, not a question or a configuration/webserver/proxy issue.
• This issue is not already reported on Github OR Nextcloud Community Forum (I've searched it).
• Nextcloud Server is up to date. See Maintenance and Release Schedule for supported versions.
• I agree to follow Nextcloud's Code of Conduct.

Bug description

The token renewal is working properly with NextCloud version 27 and OpenProject (multiple versions including 13.0,13.1) etc. With NC 28, the OAuth token refresh is not working properly. Even when the token is expired the server return http response code 200 rather than 401. Behavior was proper with NC 27.

Steps to reproduce

NC 28 (when logged into OpenProject 13.1 for which session was expired) (NC OP Integration app version - 2.5.1)

1xx.5x.2xx.1xx - - [20/Dec/2023:08:10:56 +0530] "POST /ocs/v1.php/apps/integration_openproject/filesinfo HTTP/1.1" 200 174 "-" "Ruby"

NC 27 (when logged into OpenProject 13.1 for which session was expired) (NC OP Integration app version - 2.4.6/2.5.1)

1xx.5x.2xx.1xx - - [20/Dec/2023:09:28:57 +0530] "POST /ocs/v1.php/apps/integration_openproject/filesinfo HTTP/1.1" 401 140 "-" "Ruby"
1xx.5x.2xx.1xx - - [20/Dec/2023:09:28:57 +0530] "POST /index.php/apps/oauth2/api/v1/token HTTP/1.1" 200 269 "-" "Rack::OAuth2 (2.2.0)"
1xx.5x.2xx.1xx - - [20/Dec/2023:09:28:58 +0530] "POST /ocs/v1.php/apps/integration_openproject/filesinfo HTTP/1.1" 200 251 "-" "Ruby"
1xx.5x.2xx.1xx - - [20/Dec/2023:09:29:01 +0530] "GET /ocs/v1.php/cloud/user HTTP/1.1" 200 519 "-" "Ruby"

Expected behavior

When the Oauth Token is expired the API response should give 401 error code.

Installation method

Other Community project

Nextcloud Server version

28

Operating system

Debian/Ubuntu

PHP engine version

PHP 8.1

Web server

None

Database engine version

None

Is this bug present after an update or on a fresh install?

None

Are you using the Nextcloud Server Encryption module?

None

What user-backends are you using?

• Default user-backend (database)
• LDAP/ Active Directory
• SSO - SAML
• Other

Configuration report

No response

List of activated Apps

No response

Nextcloud Signing status

No response

Nextcloud Logs

No response

Additional info

No response

[colonelm]

Bug seems to have have come from the updation of ExpiredTokenException of OC package - https://github.com/nextcloud/server/blame/e15fcecaf0d382cebff924ec2b1f5319e130c0e8/lib/private/Authentication/Exceptions/ExpiredTokenException.php#L33 seems to have removed the inheritance model assumed between ExpiredTokenException and InvalidTokenException on this code here -

server/lib/private/User/Session.php

Line 636 in e15fcec

} catch (InvalidTokenException $ex) {

hence the ExpiredTokenExpection is not being caught. The change may have impact on other areas.

[come-nc]

Before:

graph BT;
     \OC\ExpiredTokenException --> \OC\InvalidTokenException
     \OC\WipeTokenException --> \OC\InvalidTokenException
     \OC\InvalidTokenException --> \Exception

Loading

After:

graph BT;
     \OC\ExpiredTokenException --> \OCP\ExpiredTokenException
     \OC\WipeTokenException --> \OCP\WipeTokenException
     \OC\InvalidTokenException --> \OCP\InvalidTokenException
     \OCP\ExpiredTokenException --> \OCP\InvalidTokenException
     \OCP\WipeTokenException --> \OCP\InvalidTokenException
     \OCP\InvalidTokenException --> \Exception

Loading

So indeed the relation between \OC\ExpiredTokenException and \OC\InvalidTokenException was broken.
Not sure how to fix that while still having a clear path forward.

[nickvergessen]

My suggestion with the least impact would be:

graph BT;
     \OC\ExpiredTokenException --> \OC\InvalidTokenException
     \OC\ExpiredTokenException --> \OCP\ExpiredTokenException
     \OC\WipeTokenException --> \OCP\WipeTokenException
     \OC\InvalidTokenException --> \OCP\InvalidTokenException
     \OCP\ExpiredTokenException --> \OCP\InvalidTokenException
     \OCP\WipeTokenException --> \OCP\InvalidTokenException
     \OCP\InvalidTokenException --> \Exception

Loading

But PHP does not allow extending 2 classes :(

So I would say we simply add a check for \OC\ExpiredTokenException everywhere we check for \OC\InvalidTokenException already, because that is not breaking OCP.

[come-nc]

So I would say we simply add a check for \OC\ExpiredTokenException everywhere we check for \OC\InvalidTokenException already, because that is not breaking OCP.

In this case we can simply change our uses of \OC\InvalidTokenException to use \OCP\InvalidTokenException and it will work fine.
But I was afraid applications may be using this as well, but since it’s in OC maybe it’s as easy as that, I’ll draft a third PR with that.