You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@sling.apache.org by "nscendoni (via GitHub)" <gi...@apache.org> on 2025/05/09 12:13:35 UTC
[PR] Support for PKCE and nonce in OidcAuthenticationHandler + other bug fixes [sling-org-apache-sling-auth-oauth-client]
nscendoni opened a new pull request, #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/15
- Added nonce validation
- Added support for Authorization Code Flow with PKCE
- Refactoring of IT Tests
- Improved README.md
- Added multiple paths in OidcAuthenticationHandler configuration
- Added redirect to original url in OidcAuthenticationHandler
- Renamed UserInfoProcessot to SlingUserInfoProcessor
- Added configuration to store access and refresh with encryption to SlingUserInfoProcessor
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for PKCE and nonce in OidcAuthenticationHandler + other bug fixes [sling-org-apache-sling-auth-oauth-client]
Posted by "nscendoni (via GitHub)" <gi...@apache.org>.
nscendoni commented on PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/15#issuecomment-2873025013
> 75 commits seems a bit excessive, can you please split into logical commits?
Yes I did it. I didn't realized that I pushed from an old branch we already merged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-12714 - Impementation of OidcAuthenticationHandler (Support for PKCE, nonce in OidcAuthenticationHandler, bug fixes) [sling-org-apache-sling-auth-oauth-client]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/15#issuecomment-2913214808
## [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15) **Quality Gate passed**
Issues
 [16 New issues](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [85.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-12714 - Impementation of OidcAuthenticationHandler (Support for PKCE, nonce in OidcAuthenticationHandler, bug fixes) [sling-org-apache-sling-auth-oauth-client]
Posted by "rombert (via GitHub)" <gi...@apache.org>.
rombert commented on code in PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/15#discussion_r2109590620
##########
src/main/java/org/apache/sling/auth/oauth_client/impl/OidcAuthenticationHandler.java:
##########
@@ -187,32 +198,40 @@ public AuthenticationInfo extractCredentials(@NotNull HttpServletRequest request
return authInfo;
}
- //The request is not authenticated.
- // 1. Check if the State cookie match with the state in the request received from the idp
+ //The request is not authenticated.
+ // 1. Extract nonce cookie and state cookie from the request
StringBuffer requestURL = request.getRequestURL();
if ( request.getQueryString() != null )
requestURL.append('?').append(request.getQueryString());
Optional<OAuthState> clientState; //state returned by the idp in the redirect request
String authCode; //authorization code returned by the idp in the redirect request
Cookie stateCookie;
+ Cookie nonceCookie;
Review Comment:
If we are sure we can fit within 4096 byes I would suggest that we keep one cookie, this should make the payload smaller and the code easier to understand.
I am not sure if it makes sense to have different coookie names for the OAuth servlets vs the Authentication Handler, to simplify the code. Just an idea, not a suggestion :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-12714 - Impementation of OidcAuthenticationHandler (Support for PKCE, nonce in OidcAuthenticationHandler, bug fixes) [sling-org-apache-sling-auth-oauth-client]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/15#issuecomment-2910108353
## [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15) **Quality Gate failed**
Failed conditions
 [B Reliability Rating on New Code](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15) (required ≥ A)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15)
##
 Catch issues before they fail your Quality Gate with our IDE extension  [SonarQube for IDE](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=pull-request)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-12714 - Impementation of OidcAuthenticationHandler (Support for PKCE, nonce in OidcAuthenticationHandler, bug fixes) [sling-org-apache-sling-auth-oauth-client]
Posted by "rombert (via GitHub)" <gi...@apache.org>.
rombert commented on code in PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/15#discussion_r2098032369
##########
src/main/java/org/apache/sling/auth/oauth_client/ClientConnection.java:
##########
@@ -34,4 +35,10 @@ public interface ClientConnection {
* @return the name of the connection
*/
@NotNull String name();
+ String clientId();
Review Comment:
@nscendoni - the `ClientConnection` was designed to not leak implementation details to the client. What was the reason to add all these field to it?
BTW, it does not seem related to the 'move to Java 11' commit :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-12714 - Impementation of OidcAuthenticationHandler (Support for PKCE, nonce in OidcAuthenticationHandler, bug fixes) [sling-org-apache-sling-auth-oauth-client]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/15#issuecomment-2911622742
## [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15) **Quality Gate passed**
Issues
 [16 New issues](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [85.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for PKCE and nonce in OidcAuthenticationHandler + other bug fixes [sling-org-apache-sling-auth-oauth-client]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/15#issuecomment-2872794014
## [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15) **Quality Gate failed**
Failed conditions
 [C Reliability Rating on New Code](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15) (required ≥ A)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15)
##
 Catch issues before they fail your Quality Gate with our IDE extension  [SonarQube for IDE](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=pull-request)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-12714 - Impementation of OidcAuthenticationHandler (Support for PKCE, nonce in OidcAuthenticationHandler, bug fixes) [sling-org-apache-sling-auth-oauth-client]
Posted by "nscendoni (via GitHub)" <gi...@apache.org>.
nscendoni commented on code in PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/15#discussion_r2108347045
##########
src/main/java/org/apache/sling/auth/oauth_client/impl/SlingUserInfoProcessorImpl.java:
##########
@@ -0,0 +1,154 @@
+/*
+ * Licensed to the Sakai Foundation (SF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The SF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations under the License.
+ */
+package org.apache.sling.auth.oauth_client.impl;
+
+import com.nimbusds.oauth2.sdk.ParseException;
+import com.nimbusds.oauth2.sdk.TokenResponse;
+import com.nimbusds.openid.connect.sdk.claims.UserInfo;
+import net.minidev.json.JSONObject;
+import net.minidev.json.JSONValue;
+import net.minidev.json.JSONArray;
+import org.apache.sling.auth.oauth_client.spi.OidcAuthCredentials;
+import org.apache.sling.auth.oauth_client.spi.UserInfoProcessor;
+import org.apache.sling.commons.crypto.CryptoService;
+import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
+
+import org.osgi.service.component.annotations.Activate;
+import org.osgi.service.component.annotations.Component;
+import org.osgi.service.component.annotations.Reference;
+import org.osgi.service.component.annotations.ReferencePolicyOption;
+import org.osgi.service.metatype.annotations.AttributeDefinition;
+import org.osgi.service.metatype.annotations.Designate;
+import org.osgi.service.metatype.annotations.ObjectClassDefinition;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+@Component(
+ service = UserInfoProcessor.class,
+ immediate = true,
Review Comment:
I removed it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for PKCE and nonce in OidcAuthenticationHandler + other bug fixes [sling-org-apache-sling-auth-oauth-client]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/15#issuecomment-2866624852
## [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15) **Quality Gate failed**
Failed conditions
 [C Reliability Rating on New Code](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15) (required ≥ A)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15)
##
 Catch issues before they fail your Quality Gate with our IDE extension  [SonarQube for IDE](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=pull-request)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-12714 - Impementation of OidcAuthenticationHandler (Support for PKCE, nonce in OidcAuthenticationHandler, bug fixes) [sling-org-apache-sling-auth-oauth-client]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/15#issuecomment-2901526055
## [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15) **Quality Gate passed**
Issues
 [11 New issues](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [85.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15&metric=new_coverage&view=list)
 [1.4% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-12714 - Impementation of OidcAuthenticationHandler (Support for PKCE, nonce in OidcAuthenticationHandler, bug fixes) [sling-org-apache-sling-auth-oauth-client]
Posted by "nscendoni (via GitHub)" <gi...@apache.org>.
nscendoni commented on PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/15#issuecomment-2934835544
> I think this is very close to being done. From my point of view there are only three issues to solve:
>
> * the cookie topic (important)
I've pushed a change that works with only one encrypted cookie. I also refactored the OAuth Code to work with that.
There are not anymore information passed in the state parameter.
> * removal of unrelated formatting changes, e.g. adding empty lines, cleaning up whitespace,etc. This makes history much harder to track. Any formatting changes must come as a separate PR and we should do it by upgrading to the latest parent pom which enforces a code formatting standard. But let's do that after we merge this PR :-)
I fixed the setting in my ide :-)
> * the ITs leave untracked files in the project's base dir, please review the persistence code. Also the file name seems wrong (no extension)
>
> ```
> cookie-tokens14644194543422998595bin
> cookie-tokens17544281903593273246bin
> cookie-tokens401491827215654835bin
> cookie-tokens5223146744397278699bin
> cookie-tokens5292745495627831483bin
> cookie-tokens5503793274250268725bin
> cookie-tokens6559516317127871037bin
> ```
I fixed it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-12714 - Impementation of OidcAuthenticationHandler (Support for PKCE, nonce in OidcAuthenticationHandler, bug fixes) [sling-org-apache-sling-auth-oauth-client]
Posted by "rombert (via GitHub)" <gi...@apache.org>.
rombert commented on code in PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/15#discussion_r2104523356
##########
src/main/java/org/apache/sling/auth/oauth_client/impl/OidcAuthenticationHandler.java:
##########
@@ -159,11 +168,13 @@ public OidcAuthenticationHandler(@Reference(policyOption = ReferencePolicyOption
this.stateManager = stateManager;
this.idp = config.idp();
this.callbackUri = config.callbackUri();
- this.defaultRedirect = config.defaultRedirect();
this.loginCookieManager = loginCookieManager;
this.defaultConnectionName = config.defaultConnectionName();
this.userInfoProcessor = userInfoProcessor;
this.userInfoEnabled = config.userInfoEnabled();
+ this.checkNonce = config.checkNonce();
Review Comment:
So you are concerned about sending a nonce in the authentication request to an authorization server that does not support nonces?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for PKCE and nonce in OidcAuthenticationHandler + other bug fixes [sling-org-apache-sling-auth-oauth-client]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/15#issuecomment-2866360021
## [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15) **Quality Gate failed**
Failed conditions
 [78.2% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15&metric=new_coverage&view=list) (required ≥ 80%)
 [C Reliability Rating on New Code](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15) (required ≥ A)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15)
##
 Catch issues before they fail your Quality Gate with our IDE extension  [SonarQube for IDE](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=pull-request)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-12714 - Impementation of OidcAuthenticationHandler (Support for PKCE, nonce in OidcAuthenticationHandler, bug fixes) [sling-org-apache-sling-auth-oauth-client]
Posted by "nscendoni (via GitHub)" <gi...@apache.org>.
nscendoni commented on code in PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/15#discussion_r2106862728
##########
src/main/java/org/apache/sling/auth/oauth_client/impl/OidcAuthenticationHandler.java:
##########
@@ -159,11 +168,13 @@ public OidcAuthenticationHandler(@Reference(policyOption = ReferencePolicyOption
this.stateManager = stateManager;
this.idp = config.idp();
this.callbackUri = config.callbackUri();
- this.defaultRedirect = config.defaultRedirect();
Review Comment:
I removed it because now the AuthenticationHandler implements the standard DefaultAuthenticationFeedbackHandler pattern and redirect the user to the url he initially requested.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-12714 - Impementation of OidcAuthenticationHandler (Support for PKCE, nonce in OidcAuthenticationHandler, bug fixes) [sling-org-apache-sling-auth-oauth-client]
Posted by "nscendoni (via GitHub)" <gi...@apache.org>.
nscendoni commented on code in PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/15#discussion_r2108347579
##########
src/main/java/org/apache/sling/auth/oauth_client/spi/UserInfoProcessor.java:
##########
@@ -17,15 +17,13 @@
*/
package org.apache.sling.auth.oauth_client.spi;
-import com.nimbusds.oauth2.sdk.TokenResponse;
-import com.nimbusds.openid.connect.sdk.claims.UserInfo;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
/**
* Process the user info received from the identity provider and return the credentials that will be returned by the authentication handler.
*/
public interface UserInfoProcessor {
- @NotNull OidcAuthCredentials process(@Nullable UserInfo userInfo, @NotNull TokenResponse tokenResponse,
+ @NotNull OidcAuthCredentials process(@Nullable String userInfo, @NotNull String tokenResponse,
Review Comment:
Added documentation
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-12714 - Impementation of OidcAuthenticationHandler (Support for PKCE, nonce in OidcAuthenticationHandler, bug fixes) [sling-org-apache-sling-auth-oauth-client]
Posted by "rombert (via GitHub)" <gi...@apache.org>.
rombert merged PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/15
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for PKCE and nonce in OidcAuthenticationHandler + other bug fixes [sling-org-apache-sling-auth-oauth-client]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/15#issuecomment-2875605401
## [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15) **Quality Gate passed**
Issues
 [7 New issues](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [85.4% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-12714 - Impementation of OidcAuthenticationHandler (Support for PKCE, nonce in OidcAuthenticationHandler, bug fixes) [sling-org-apache-sling-auth-oauth-client]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/15#issuecomment-2934141123
## [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15) **Quality Gate failed**
Failed conditions
 [C Reliability Rating on New Code](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15) (required ≥ A)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15)
##
 Catch issues before they fail your Quality Gate with our IDE extension  [SonarQube for IDE](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=pull-request)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for PKCE and nonce in OidcAuthenticationHandler + other bug fixes [sling-org-apache-sling-auth-oauth-client]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/15#issuecomment-2873402892
## [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15) **Quality Gate failed**
Failed conditions
 [C Reliability Rating on New Code](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15) (required ≥ A)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15)
##
 Catch issues before they fail your Quality Gate with our IDE extension  [SonarQube for IDE](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=pull-request)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-12714 - Impementation of OidcAuthenticationHandler (Support for PKCE, nonce in OidcAuthenticationHandler, bug fixes) [sling-org-apache-sling-auth-oauth-client]
Posted by "rombert (via GitHub)" <gi...@apache.org>.
rombert commented on PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/15#issuecomment-2894502064
@nscendoni - I think it would be useful to separate 3d4330474a55b236270a30560ed37722094ed36e into a different Jira issue and pull request. Agreed we should downgrade to Java 11 (for now) to target a wider deployment base.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-12714 - Impementation of OidcAuthenticationHandler (Support for PKCE, nonce in OidcAuthenticationHandler, bug fixes) [sling-org-apache-sling-auth-oauth-client]
Posted by "nscendoni (via GitHub)" <gi...@apache.org>.
nscendoni commented on code in PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/15#discussion_r2104535220
##########
src/main/java/org/apache/sling/auth/oauth_client/impl/OidcAuthenticationHandler.java:
##########
@@ -159,11 +168,13 @@ public OidcAuthenticationHandler(@Reference(policyOption = ReferencePolicyOption
this.stateManager = stateManager;
this.idp = config.idp();
this.callbackUri = config.callbackUri();
- this.defaultRedirect = config.defaultRedirect();
this.loginCookieManager = loginCookieManager;
this.defaultConnectionName = config.defaultConnectionName();
this.userInfoProcessor = userInfoProcessor;
this.userInfoEnabled = config.userInfoEnabled();
+ this.checkNonce = config.checkNonce();
Review Comment:
Yes. If the server would not support it, we would not validate the token.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for PKCE and nonce in OidcAuthenticationHandler + other bug fixes [sling-org-apache-sling-auth-oauth-client]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/15#issuecomment-2873053557
## [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15) **Quality Gate failed**
Failed conditions
 [C Reliability Rating on New Code](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15) (required ≥ A)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15)
##
 Catch issues before they fail your Quality Gate with our IDE extension  [SonarQube for IDE](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=pull-request)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-12714 - Impementation of OidcAuthenticationHandler (Support for PKCE, nonce in OidcAuthenticationHandler, bug fixes) [sling-org-apache-sling-auth-oauth-client]
Posted by "nscendoni (via GitHub)" <gi...@apache.org>.
nscendoni commented on code in PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/15#discussion_r2097160275
##########
src/main/java/org/apache/sling/auth/oauth_client/impl/OidcAuthenticationHandler.java:
##########
@@ -120,7 +126,7 @@ public class OidcAuthenticationHandler extends DefaultAuthenticationFeedbackHand
@AttributeDefinition(name = "Path",
description = "Repository path for which this authentication handler should be used by Sling. If this is " +
"empty, the authentication handler will be disabled. By default this is set to \"/\".")
- String path() default "/";
+ String[] path() default {"/"};
Review Comment:
Actually it is from: https://github.com/apache/sling-org-apache-sling-auth-core/blob/47eee11b0a05bdd3d20fcbfba81b2077f503f058/src/main/java/org/apache/sling/auth/core/spi/AuthenticationHandler.java#L44-L57
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-12714 - Impementation of OidcAuthenticationHandler (Support for PKCE, nonce in OidcAuthenticationHandler, bug fixes) [sling-org-apache-sling-auth-oauth-client]
Posted by "rombert (via GitHub)" <gi...@apache.org>.
rombert commented on code in PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/15#discussion_r2097851701
##########
src/main/java/org/apache/sling/auth/oauth_client/impl/OidcAuthenticationHandler.java:
##########
@@ -120,7 +126,7 @@ public class OidcAuthenticationHandler extends DefaultAuthenticationFeedbackHand
@AttributeDefinition(name = "Path",
description = "Repository path for which this authentication handler should be used by Sling. If this is " +
"empty, the authentication handler will be disabled. By default this is set to \"/\".")
- String path() default "/";
+ String[] path() default {"/"};
Review Comment:
Make sense, thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-12714 - Impementation of OidcAuthenticationHandler (Support for PKCE, nonce in OidcAuthenticationHandler, bug fixes) [sling-org-apache-sling-auth-oauth-client]
Posted by "nscendoni (via GitHub)" <gi...@apache.org>.
nscendoni commented on code in PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/15#discussion_r2108344797
##########
src/main/java/org/apache/sling/auth/oauth_client/impl/OidcAuthenticationHandler.java:
##########
@@ -159,11 +168,13 @@ public OidcAuthenticationHandler(@Reference(policyOption = ReferencePolicyOption
this.stateManager = stateManager;
this.idp = config.idp();
this.callbackUri = config.callbackUri();
- this.defaultRedirect = config.defaultRedirect();
this.loginCookieManager = loginCookieManager;
this.defaultConnectionName = config.defaultConnectionName();
this.userInfoProcessor = userInfoProcessor;
this.userInfoEnabled = config.userInfoEnabled();
+ this.checkNonce = config.checkNonce();
Review Comment:
I removed that configuration.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-12714 - Impementation of OidcAuthenticationHandler (Support for PKCE, nonce in OidcAuthenticationHandler, bug fixes) [sling-org-apache-sling-auth-oauth-client]
Posted by "nscendoni (via GitHub)" <gi...@apache.org>.
nscendoni commented on code in PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/15#discussion_r2108527000
##########
src/main/java/org/apache/sling/auth/oauth_client/impl/OidcAuthenticationHandler.java:
##########
@@ -187,32 +198,40 @@ public AuthenticationInfo extractCredentials(@NotNull HttpServletRequest request
return authInfo;
}
- //The request is not authenticated.
- // 1. Check if the State cookie match with the state in the request received from the idp
+ //The request is not authenticated.
+ // 1. Extract nonce cookie and state cookie from the request
StringBuffer requestURL = request.getRequestURL();
if ( request.getQueryString() != null )
requestURL.append('?').append(request.getQueryString());
Optional<OAuthState> clientState; //state returned by the idp in the redirect request
String authCode; //authorization code returned by the idp in the redirect request
Cookie stateCookie;
+ Cookie nonceCookie;
+ Cookie codeVerifierCookie = null;
+ AuthorizationResponse authResponse;
try {
- AuthorizationResponse authResponse = AuthorizationResponse.parse(new URI(requestURL.toString()));
+ authResponse = AuthorizationResponse.parse(new URI(requestURL.toString()));
clientState = extractClientState(authResponse);
- authCode = extractAuthCode(authResponse);
- stateCookie = extractStateCookie(request);
- } catch (ParseException | URISyntaxException e) {
- logger.debug("Failed to parse authorization response");
+ } catch (ParseException | URISyntaxException | IllegalStateException e) {
+ // If we fail parsing the response, we consider the request not for this authentication handler
Review Comment:
With this code ParseException is thrown when there are request without parameter in the request. I t was generating some noise.
I modified it to not execute this code if there are not parameters in the request, and I restored the log
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-12714 - Impementation of OidcAuthenticationHandler (Support for PKCE, nonce in OidcAuthenticationHandler, bug fixes) [sling-org-apache-sling-auth-oauth-client]
Posted by "nscendoni (via GitHub)" <gi...@apache.org>.
nscendoni commented on code in PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/15#discussion_r2102790255
##########
src/main/java/org/apache/sling/auth/oauth_client/impl/OidcAuthenticationHandler.java:
##########
@@ -187,32 +198,40 @@ public AuthenticationInfo extractCredentials(@NotNull HttpServletRequest request
return authInfo;
}
- //The request is not authenticated.
- // 1. Check if the State cookie match with the state in the request received from the idp
+ //The request is not authenticated.
+ // 1. Extract nonce cookie and state cookie from the request
StringBuffer requestURL = request.getRequestURL();
if ( request.getQueryString() != null )
requestURL.append('?').append(request.getQueryString());
Optional<OAuthState> clientState; //state returned by the idp in the redirect request
String authCode; //authorization code returned by the idp in the redirect request
Cookie stateCookie;
+ Cookie nonceCookie;
Review Comment:
I'm currently working on this. I'll push a PR with a refactoring to work with single cookie.
##########
src/main/java/org/apache/sling/auth/oauth_client/impl/OidcAuthenticationHandler.java:
##########
@@ -187,32 +198,40 @@ public AuthenticationInfo extractCredentials(@NotNull HttpServletRequest request
return authInfo;
}
- //The request is not authenticated.
- // 1. Check if the State cookie match with the state in the request received from the idp
+ //The request is not authenticated.
+ // 1. Extract nonce cookie and state cookie from the request
StringBuffer requestURL = request.getRequestURL();
if ( request.getQueryString() != null )
requestURL.append('?').append(request.getQueryString());
Optional<OAuthState> clientState; //state returned by the idp in the redirect request
String authCode; //authorization code returned by the idp in the redirect request
Cookie stateCookie;
+ Cookie nonceCookie;
Review Comment:
@rombert @anchela
Currently in oauth flow we send several information in the state:
- perRequestKey (random value to be verified)
- connectionName
- redirect
The perRequestKey is also sent in one encrypted cookie to be verified for CSRF protection.
During OIDC authentication flow, additional information (nonce and code verifier) are sent into separated cookies.
If you agree we may send a single json encrypted cookie with all the information required by both the flows, and send only the perRequestKey in the state.
##########
src/main/java/org/apache/sling/auth/oauth_client/ClientConnection.java:
##########
@@ -34,4 +35,10 @@ public interface ClientConnection {
* @return the name of the connection
*/
@NotNull String name();
+ String clientId();
Review Comment:
The issue was that after refactoring for java 11, the pipeline failed because there was too much code duplicated in `OAuthConnectionImpl` and `OidcConnectionImpl`. I had to refactor these class to inherit some code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-12714 - Impementation of OidcAuthenticationHandler (Support for PKCE, nonce in OidcAuthenticationHandler, bug fixes) [sling-org-apache-sling-auth-oauth-client]
Posted by "nscendoni (via GitHub)" <gi...@apache.org>.
nscendoni commented on code in PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/15#discussion_r2104625354
##########
src/main/java/org/apache/sling/auth/oauth_client/impl/OidcAuthenticationHandler.java:
##########
@@ -187,32 +198,40 @@ public AuthenticationInfo extractCredentials(@NotNull HttpServletRequest request
return authInfo;
}
- //The request is not authenticated.
- // 1. Check if the State cookie match with the state in the request received from the idp
+ //The request is not authenticated.
+ // 1. Extract nonce cookie and state cookie from the request
StringBuffer requestURL = request.getRequestURL();
if ( request.getQueryString() != null )
requestURL.append('?').append(request.getQueryString());
Optional<OAuthState> clientState; //state returned by the idp in the redirect request
String authCode; //authorization code returned by the idp in the redirect request
Cookie stateCookie;
+ Cookie nonceCookie;
Review Comment:
In [rfc6265](https://datatracker.ietf.org/doc/html/rfc6265?#section-6.1) read that these are the limits that browsers SHOULD support:
```
o At least 4096 bytes per cookie (as measured by the sum of the
length of the cookie's name, value, and attributes).
o At least 50 cookies per domain.
o At least 3000 cookies total.
```
If we want to be in a safer side and limit the number of cookies as well we may:
- keep the current setup for OAuth
- Modify the current code to have only one new cookie for nonce and code verifier
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-12714 - Impementation of OidcAuthenticationHandler (Support for PKCE, nonce in OidcAuthenticationHandler, bug fixes) [sling-org-apache-sling-auth-oauth-client]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/15#issuecomment-2934200312
## [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15) **Quality Gate passed**
Issues
 [24 New issues](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [82.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for PKCE and nonce in OidcAuthenticationHandler + other bug fixes [sling-org-apache-sling-auth-oauth-client]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/15#issuecomment-2875484460
## [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15) **Quality Gate failed**
Failed conditions
 [C Reliability Rating on New Code](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15) (required ≥ A)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15)
##
 Catch issues before they fail your Quality Gate with our IDE extension  [SonarQube for IDE](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=pull-request)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support for PKCE and nonce in OidcAuthenticationHandler + other bug fixes [sling-org-apache-sling-auth-oauth-client]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/15#issuecomment-2871355338
## [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15) **Quality Gate failed**
Failed conditions
 [C Reliability Rating on New Code](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15) (required ≥ A)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15)
##
 Catch issues before they fail your Quality Gate with our IDE extension  [SonarQube for IDE](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=pull-request)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-12714 - Impementation of OidcAuthenticationHandler (Support for PKCE, nonce in OidcAuthenticationHandler, bug fixes) [sling-org-apache-sling-auth-oauth-client]
Posted by "rombert (via GitHub)" <gi...@apache.org>.
rombert commented on code in PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/15#discussion_r2104546978
##########
src/main/java/org/apache/sling/auth/oauth_client/impl/OidcAuthenticationHandler.java:
##########
@@ -187,32 +198,40 @@ public AuthenticationInfo extractCredentials(@NotNull HttpServletRequest request
return authInfo;
}
- //The request is not authenticated.
- // 1. Check if the State cookie match with the state in the request received from the idp
+ //The request is not authenticated.
+ // 1. Extract nonce cookie and state cookie from the request
StringBuffer requestURL = request.getRequestURL();
if ( request.getQueryString() != null )
requestURL.append('?').append(request.getQueryString());
Optional<OAuthState> clientState; //state returned by the idp in the redirect request
String authCode; //authorization code returned by the idp in the redirect request
Cookie stateCookie;
+ Cookie nonceCookie;
Review Comment:
So your proposal is:
- state parameter: perRequestKey
- encrypted cookie: perRequestKey, connectionName, redirect, nonce, code verifier
?
I don't see a problem with it but also I can't say I can figure out all the security implications on my own. One issue I would raise is that we make sure we don't exceed some sort of limits related to Cookie/HTTP Header size. That is of course true for the state parameter in the redirect as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-12714 - Impementation of OidcAuthenticationHandler (Support for PKCE, nonce in OidcAuthenticationHandler, bug fixes) [sling-org-apache-sling-auth-oauth-client]
Posted by "rombert (via GitHub)" <gi...@apache.org>.
rombert commented on code in PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/15#discussion_r2104581605
##########
src/main/java/org/apache/sling/auth/oauth_client/spi/UserInfoProcessor.java:
##########
@@ -17,15 +17,13 @@
*/
package org.apache.sling.auth.oauth_client.spi;
-import com.nimbusds.oauth2.sdk.TokenResponse;
-import com.nimbusds.openid.connect.sdk.claims.UserInfo;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
/**
* Process the user info received from the identity provider and return the credentials that will be returned by the authentication handler.
*/
public interface UserInfoProcessor {
- @NotNull OidcAuthCredentials process(@Nullable UserInfo userInfo, @NotNull TokenResponse tokenResponse,
+ @NotNull OidcAuthCredentials process(@Nullable String userInfo, @NotNull String tokenResponse,
Review Comment:
I think it would be good to document the String values here; links to the RFC would IMO be enough.
##########
src/main/java/org/apache/sling/auth/oauth_client/impl/OidcAuthenticationHandler.java:
##########
@@ -159,11 +168,13 @@ public OidcAuthenticationHandler(@Reference(policyOption = ReferencePolicyOption
this.stateManager = stateManager;
this.idp = config.idp();
this.callbackUri = config.callbackUri();
- this.defaultRedirect = config.defaultRedirect();
Review Comment:
Why did you remove this config setting?
##########
src/main/java/org/apache/sling/auth/oauth_client/impl/SlingUserInfoProcessorImpl.java:
##########
@@ -0,0 +1,154 @@
+/*
+ * Licensed to the Sakai Foundation (SF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The SF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations under the License.
+ */
+package org.apache.sling.auth.oauth_client.impl;
+
+import com.nimbusds.oauth2.sdk.ParseException;
+import com.nimbusds.oauth2.sdk.TokenResponse;
+import com.nimbusds.openid.connect.sdk.claims.UserInfo;
+import net.minidev.json.JSONObject;
+import net.minidev.json.JSONValue;
+import net.minidev.json.JSONArray;
+import org.apache.sling.auth.oauth_client.spi.OidcAuthCredentials;
+import org.apache.sling.auth.oauth_client.spi.UserInfoProcessor;
+import org.apache.sling.commons.crypto.CryptoService;
+import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
+
+import org.osgi.service.component.annotations.Activate;
+import org.osgi.service.component.annotations.Component;
+import org.osgi.service.component.annotations.Reference;
+import org.osgi.service.component.annotations.ReferencePolicyOption;
+import org.osgi.service.metatype.annotations.AttributeDefinition;
+import org.osgi.service.metatype.annotations.Designate;
+import org.osgi.service.metatype.annotations.ObjectClassDefinition;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+@Component(
+ service = UserInfoProcessor.class,
+ immediate = true,
Review Comment:
Do you need immediate here? It will be activated as soon as a reference is requested. You maybe also want to set the configuration policy to required.
##########
src/main/java/org/apache/sling/auth/oauth_client/ClientConnection.java:
##########
@@ -34,4 +36,10 @@ public interface ClientConnection {
* @return the name of the connection
*/
@NotNull String name();
+ @NotNull String clientId();
Review Comment:
Same comment as in https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/17#discussion_r2099767014 , please don't expose this kind of information in `ClientConnection`
##########
src/main/java/org/apache/sling/auth/oauth_client/impl/OidcAuthenticationHandler.java:
##########
@@ -159,11 +168,13 @@ public OidcAuthenticationHandler(@Reference(policyOption = ReferencePolicyOption
this.stateManager = stateManager;
this.idp = config.idp();
this.callbackUri = config.callbackUri();
- this.defaultRedirect = config.defaultRedirect();
this.loginCookieManager = loginCookieManager;
this.defaultConnectionName = config.defaultConnectionName();
this.userInfoProcessor = userInfoProcessor;
this.userInfoEnabled = config.userInfoEnabled();
+ this.checkNonce = config.checkNonce();
Review Comment:
I realise I'm being pedantic, but I don't like seeing config options that should not be there :-) You don't have to remove if you're really sure that there are going to be non-compliant OIDC server implementations out there we need to support.
Otherwise I would always keep it enabled because it enhances security and disabling it would bring no benefit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-12714 - Impementation of OidcAuthenticationHandler (Support for PKCE, nonce in OidcAuthenticationHandler, bug fixes) [sling-org-apache-sling-auth-oauth-client]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/15#issuecomment-2906863024
## [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15) **Quality Gate passed**
Issues
 [11 New issues](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [85.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15&metric=new_coverage&view=list)
 [1.4% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-12714 - Impementation of OidcAuthenticationHandler (Support for PKCE, nonce in OidcAuthenticationHandler, bug fixes) [sling-org-apache-sling-auth-oauth-client]
Posted by "rombert (via GitHub)" <gi...@apache.org>.
rombert commented on code in PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/15#discussion_r2109632757
##########
src/test/resources/keycloak-import/sling.json:
##########
@@ -1237,7 +1285,7 @@
"internationalizationEnabled" : false,
"supportedLocales" : [ ],
"authenticationFlows" : [ {
- "id" : "bd07af7f-436a-4c7d-a2ae-78e20a897cb3",
+ "id" : "aff611f8-41d9-49ad-ba2b-7a24ae030bc8",
Review Comment:
Ok, I agree with those changes. But there are many changes that are not necessary. I reverted the changes that I think are unnecessary with [096eeb7](https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/15/commits/096eeb726c53a993d8afef253d50bbbd46b79abc) and the tests still pass, so this should be fine.
This way we only have the minimal changes in the sling.json file and we can track history easier.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-12714 - Impementation of OidcAuthenticationHandler (Support for PKCE, nonce in OidcAuthenticationHandler, bug fixes) [sling-org-apache-sling-auth-oauth-client]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/15#issuecomment-2910156761
## [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15) **Quality Gate passed**
Issues
 [15 New issues](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [84.7% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-oauth-client&pullRequest=15)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-12714 - Impementation of OidcAuthenticationHandler (Support for PKCE, nonce in OidcAuthenticationHandler, bug fixes) [sling-org-apache-sling-auth-oauth-client]
Posted by "nscendoni (via GitHub)" <gi...@apache.org>.
nscendoni commented on code in PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/15#discussion_r2108345943
##########
src/main/java/org/apache/sling/auth/oauth_client/ClientConnection.java:
##########
@@ -34,4 +36,10 @@ public interface ClientConnection {
* @return the name of the connection
*/
@NotNull String name();
+ @NotNull String clientId();
Review Comment:
fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org