Skip to content

Conversation

@come-nc
Copy link
Contributor

@come-nc come-nc commented Jan 4, 2024

Fix regression of authentication workflow because class hierarchy changed when moving Exception to OCP

Summary

27:

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

28:

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

This PR:

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

It is a bit sad to reference OC from OCP but I am not sure we have a better solution here to restore backward compatibility of the API. We should still move all code to using the OCP namespaced exceptions and remove the OC ones when possible.

Checklist

Fix regression of authentication workflow because class hierarchy
changed when moving Exception to OCP

Signed-off-by: Côme Chilliet <[email protected]>
@come-nc come-nc added the 2. developing Work in progress label Jan 4, 2024
@come-nc come-nc self-assigned this Jan 4, 2024
And always throw OC versions for BC

Signed-off-by: Côme Chilliet <[email protected]>
if ($token->getUID() !== $this->uid) {
throw new InvalidTokenException('This token does not belong to you!');
/* We have to throw the OC version so both OC and OCP catches catch it */
throw new OcInvalidTokenException('This token does not belong to you!');

Check notice

Code scanning / Psalm

DeprecatedClass

OC\Authentication\Exceptions\InvalidTokenException is marked deprecated
$token = $this->tokenProvider->getToken($password);
if ($token->getLoginName() !== $user) {
throw new InvalidTokenException('login name does not match');
throw new OcInvalidTokenException('login name does not match');

Check notice

Code scanning / Psalm

DeprecatedClass

OC\Authentication\Exceptions\InvalidTokenException is marked deprecated
$token = \OC::$server->get(\OC\Authentication\Token\IProvider::class)->getToken($password);
if ($token->getLoginName() !== $user) {
throw new InvalidTokenException('login name does not match');
throw new OcInvalidTokenException('login name does not match');

Check notice

Code scanning / Psalm

DeprecatedClass

OC\Authentication\Exceptions\InvalidTokenException is marked deprecated
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking this as I prefer #42640

@come-nc
Copy link
Contributor Author

come-nc commented Feb 1, 2024

Replaced by #42640

@come-nc come-nc closed this Feb 1, 2024
@nickvergessen nickvergessen deleted the fix/fix-authentication-exceptions branch February 1, 2024 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2. developing Work in progress

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

3 participants