fix: re-enable PKCE by default #956
Conversation
|
Hmm, all of the integration tests on the NC master branch are failing. I can't really tell from the logs where exactly the problem is coming from, though. |
julien-nc
force-pushed
the
fix/pkce-default
branch
from
73d606d
to
34d7e5a
Compare
|
I fixed the CI in another PR that was not merged yet. It's now fixed. I rebased your branch on main and pushed again. Should be fine now. |
There was a problem hiding this comment.
Since it's now enabled by default, we could check if the provider supports our code challenge method (for safety). This information can be found in the discovery. S256 (the method we use) should be included in the code_challenge_methods_supported array in the provider's discovery.
Here is what's inside a classic Keycloak's discovery:
"code_challenge_methods_supported": [
"plain",
"S256"
],So I think we should use PKCE only if it is enabled in NC's config AND the provider supports S256.
Wdyt?
|
Makes sense to me, will take a look when I have the chance to get to it. |
Signed-off-by: Edward Ly <[email protected]>
Signed-off-by: Edward Ly <[email protected]>
edward-ly
force-pushed
the
fix/pkce-default
branch
from
90ac02e
to
f06bfc6
Compare
Ah, good idea, this should do it, I think. |
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! |
Resolves #807.