Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(Authentication): correctly catch uniq constraint on token insert #45126

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Apr 30, 2024

TODO

  • Migrate from UniqueConstraintViolationException to OCP\DB\Exception in auth token manager

Checklist

@ShGKme ShGKme self-assigned this Apr 30, 2024
@ShGKme ShGKme force-pushed the fix/auth-token-uniq-constraint-violation-handling branch from d7dbbbe to 4c8662e Compare April 30, 2024 16:35
@ShGKme ShGKme force-pushed the fix/auth-token-uniq-constraint-violation-handling branch from 4c8662e to 131dbaf Compare April 30, 2024 20:30
@@ -77,18 +77,22 @@ public function generateToken(string $token,
$type,
$remember
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add an @throws OCP\DB\Exception on generateToken because it’s not documented currently I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

* @throws Exception

should also work here with use above, right?

Copy link
Member

Choose a reason for hiding this comment

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

It is not part of the expected API behaviour, so from my POV it should not be documented

Copy link
Contributor

Choose a reason for hiding this comment

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

I was talking about the other generateToken, the one that is called, so

public function generateToken(string $token,

But it inherits from IProvider, so not sure if easy to document.

@come-nc
Copy link
Contributor

come-nc commented May 2, 2024

I though executeStatement was a drop-in replacement for execute but it does change the exception thrown so I will try to be careful about that in the future.

@nickvergessen
Copy link
Member

I though executeStatement was a drop-in replacement for execute but it does change the exception thrown so I will try to be careful about that in the future.

Is there something like a @never-throws X and then psalm complains when you catch X ?

@@ -55,6 +55,7 @@ public function __construct(PublicKeyTokenProvider $publicKeyTokenProvider) {
* @param int $type token type
* @param int $remember whether the session token should be used for remember-me
* @return OCPIToken
* @throws Exception
Copy link
Member

Choose a reason for hiding this comment

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

This is not part of the expected API behaviour, so from my POV it should not be documented

@solracsf solracsf added this to the Nextcloud 30 milestone Jun 18, 2024
@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jul 27, 2024
@skjnldsv skjnldsv modified the milestones: Nextcloud 30, Nextcloud 31 Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants