Validate for client_secret when provided in PKCE flow

[vinayknl]

Summary: When using CAS as OIDC provider, and when we want to PKCE (Proof Key for Code Exchange) flow using client_secret skips code verification.

When we have a service registered as Confidential client giving it a client_id and client_secret, we should be providing both when using code_verifier in authorisation code flow.

i.e if the authorization request was made including a code_challenge (as defined in https://tools.ietf.org/html/rfc7636#section-4.3), oauth code will be issued associating with code challenge.

Client then makes an access token request with code and a code_verifier.
https://tools.ietf.org/html/rfc7636#section-4.5

Currently, CAS is validating for code_verifier only if client_secret is not supplied. But for confidential clients (for those client_secret was issued), we also need to validate for client_secret.

Fix in the PR includes skipping ClientCredential authentication in case of PKCE flow and validate for client secret in PKCE Authenticator.

• Brief description of changes applied
• Test cases for all modified changes, where applicable
• The same pull request targetted at the master branch, if applicable
• Any documentation on how to configure, test: NA
• Any possible limitations, side effects, etc: None
• Reference any other pull requests that might be related: None

[codecov]

Codecov Report

Merging #4262 into master will decrease coverage by 23.97%.
The diff coverage is 54.05%.

@@              Coverage Diff              @@
##             master    #4262       +/-   ##
=============================================
- Coverage     44.14%   20.16%   -23.98%     
+ Complexity     5620     2627     -2993     
=============================================
  Files          1665     1655       -10     
  Lines         36900    36507      -393     
  Branches       3404     3366       -38     
=============================================
- Hits          16288     7362     -8926     
- Misses        19079    28372     +9293     
+ Partials       1533      773      -760

Impacted Files	Coverage Δ	Complexity Δ
...nticator/OAuth20UsernamePasswordAuthenticator.java	2.32% <0%> (ø)	1 <0> (ø)	⬇️
...ator/OAuth20ProofKeyCodeExchangeAuthenticator.java	10.34% <22.22%> (-6.33%)	2 <1> (ø)
...ator/OAuth20ClientIdClientSecretAuthenticator.java	63.88% <66.66%> (+60.18%)	4 <1> (+3)	⬆️
.../adaptors/generic/GroovyAuthenticationHandler.java	0% <0%> (-100%)	0% <0%> (-5%)
...tication/principal/TokenWebApplicationService.java	0% <0%> (-100%)	0% <0%> (-1%)
...rg/apereo/cas/adaptors/generic/CasUserAccount.java	0% <0%> (-100%)	0% <0%> (-1%)
...n/principal/PrincipalBearingPrincipalResolver.java	0% <0%> (-100%)	0% <0%> (-3%)
...hentication/principal/OpenIdPrincipalResolver.java	0% <0%> (-100%)	0% <0%> (-3%)
...eb/support/InMemoryThrottledSubmissionCleaner.java	0% <0%> (-100%)	0% <0%> (-3%)
...rg/apereo/cas/adaptors/yubikey/YubiKeyAccount.java	0% <0%> (-100%)	0% <0%> (-1%)
... and 1070 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be41a77...9c903b4. Read the comment docs.

[mmoayyed]

Thanks for the reference. Let’s clean that up too!

[mmoayyed]

LGTM. Thank you 👍

Let's wait for CI to pass and then we should be G2G.

[mmoayyed]

Should be unnecessary; Constant is already available in OAuthConstants?

[vinayknl]

These are test constants not available in OAuthConstants.