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(files_sharing): Do not wrap password policy exception into a generic one #49366

Merged
merged 3 commits into from
Dec 12, 2024

Conversation

come-nc
Copy link
Contributor

@come-nc come-nc commented Nov 19, 2024

Follow-up of #49361

Summary

Let the controller access the HintException and show the error to the user.

Checklist

@come-nc come-nc added the 3. to review Waiting for reviews label Nov 19, 2024
@come-nc come-nc self-assigned this Nov 19, 2024
@come-nc come-nc added this to the Nextcloud 31 milestone Nov 19, 2024
} catch (HintException $e) {
throw new \Exception($e->getHint());
}
$this->dispatcher->dispatchTyped(new ValidatePasswordPolicyEvent($password));
Copy link
Contributor

Choose a reason for hiding this comment

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

Still disappointed that this is done using exceptions rather than setting property on the even like isValid...

Copy link
Member

@tobiasKaminsky tobiasKaminsky left a comment

Choose a reason for hiding this comment

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

Unfortunately this does not fix it

@come-nc
Copy link
Contributor Author

come-nc commented Nov 19, 2024

Unfortunately this does not fix it

I’m out of ideas

@tobiasKaminsky
Copy link
Member

Do you need more info how to reproduce it?
Me as Android dev cannot really help, expect of steps to reproduce…

@come-nc
Copy link
Contributor Author

come-nc commented Nov 25, 2024

Do you need more info how to reproduce it? Me as Android dev cannot really help, expect of steps to reproduce…

Do you have a full stacktrace for the Exception? (with this PR applied on server side)

@come-nc come-nc force-pushed the fix/remove-share-hint-exception-wrapping branch from d0beeed to 900007d Compare December 10, 2024 10:06
@come-nc
Copy link
Contributor Author

come-nc commented Dec 10, 2024

@tobiasKaminsky Can you test again? It should be good now, message should be back and error code should be 400.

@tobiasKaminsky
Copy link
Member

This works now without any modification on client side 🎉

@tobiasKaminsky tobiasKaminsky self-requested a review December 11, 2024 06:07
@tobiasKaminsky
Copy link
Member

@come-nc can we merge this? 💙

@come-nc come-nc added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Dec 12, 2024
…ric one

Let the controller access the HintException and show the error to the user.

Signed-off-by: Côme Chilliet <[email protected]>
This fixes a regression that bad password returned 403 instead of 400
 because of previous changes.

Signed-off-by: Côme Chilliet <[email protected]>
@come-nc come-nc force-pushed the fix/remove-share-hint-exception-wrapping branch from cca3072 to 365ff40 Compare December 12, 2024 14:39
@come-nc come-nc merged commit 7cc8a1b into master Dec 12, 2024
189 checks passed
@come-nc come-nc deleted the fix/remove-share-hint-exception-wrapping branch December 12, 2024 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants