You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by "pat-lego (via GitHub)" <gi...@apache.org> on 2024/08/08 14:28:26 UTC
[PR] Adding ability to review selected auth handler [sling-org-apache-sling-auth-core]
pat-lego opened a new pull request, #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-core/pull/15
This PR allows us to review the auth checker that was selected SLING-12403
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding ability to review selected auth handler [sling-org-apache-sling-auth-core]
Posted by "rombert (via GitHub)" <gi...@apache.org>.
rombert commented on PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-core/pull/15#issuecomment-2286400523
That is strange, https://github.com/apache/sling-org-apache-sling-auth-core/pull/15/files#diff-0575ce5bb14589a0898261fe91c2eaea14da53087a8b1b3145273d6f936f100fL121-L124 seems to delegate toString to the wrapper handler.
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding ability to review selected auth handler [sling-org-apache-sling-auth-core]
Posted by "pat-lego (via GitHub)" <gi...@apache.org>.
pat-lego commented on PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-core/pull/15#issuecomment-2326797887
@cziegeler fixed the code to use toString. Note that some classes override the toString and use generic naming conventions which makes it more complex to determine the class that represents AuthenticationHandler.
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding ability to review selected auth handler [sling-org-apache-sling-auth-core]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-core/pull/15#issuecomment-2297334186
## [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-core&pullRequest=15) **Quality Gate failed**
Failed conditions
 [50.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-auth-core&pullRequest=15&metric=new_coverage&view=list) (required ≥ 80%)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-core&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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding ability to review selected auth handler [sling-org-apache-sling-auth-core]
Posted by "pat-lego (via GitHub)" <gi...@apache.org>.
pat-lego commented on PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-core/pull/15#issuecomment-2297329454
@cziegeler I have removed the getHandler function and added logging in the respective class.
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding ability to review selected auth handler [sling-org-apache-sling-auth-core]
Posted by "pat-lego (via GitHub)" <gi...@apache.org>.
pat-lego commented on PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-core/pull/15#issuecomment-2286322869
@rombert we did but it just returns the address of the holder class we need the implementation of the handler which is not accessible without exposing it via the abstract class. Without this it is impossible to know which handler was called.
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding ability to review selected auth handler [sling-org-apache-sling-auth-core]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-core/pull/15#issuecomment-2326782854
## [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-core&pullRequest=15) **Quality Gate failed**
Failed conditions
 [50.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-auth-core&pullRequest=15&metric=new_coverage&view=list) (required ≥ 80%)
 [B Maintainability Rating on New Code](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-core&pullRequest=15) (required ≥ A)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-core&pullRequest=15)
##
 Catch issues before they fail your Quality Gate with our IDE extension  [SonarLint](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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding ability to review selected auth handler [sling-org-apache-sling-auth-core]
Posted by "klcodanr (via GitHub)" <gi...@apache.org>.
klcodanr commented on code in PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-core/pull/15#discussion_r1709679110
##########
src/main/java/org/apache/sling/auth/core/impl/SlingAuthenticator.java:
##########
@@ -663,7 +663,7 @@ private AuthenticationInfo getAuthenticationInfo(HttpServletRequest request, Htt
if (holder.isPathRequiresHandler(path)){
final AuthenticationInfo authInfo = holder.extractCredentials(
request, response);
-
+ log.debug("getAuthenticationInfo: Selected class {} to perform authentication", holder.getHandler().getClass().getName());
Review Comment:
Based on the EngineAuthenticationHandlerHolder there are cases where the handler could be null, which should be handled. How about:
```suggestion
log.debug("getAuthenticationInfo: Selected class {} to perform authentication", Optional.ofNullable(holder.getHandler()).map(h -> h.getClass().getName()).orElse("null"));
```
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding ability to review selected auth handler [sling-org-apache-sling-auth-core]
Posted by "cziegeler (via GitHub)" <gi...@apache.org>.
cziegeler commented on PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-core/pull/15#issuecomment-2286423934
I think that was exactly done for making it easy to log the handler :) no getHandler() method needed
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding ability to review selected auth handler [sling-org-apache-sling-auth-core]
Posted by "cziegeler (via GitHub)" <gi...@apache.org>.
cziegeler merged PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-core/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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding ability to review selected auth handler [sling-org-apache-sling-auth-core]
Posted by "cziegeler (via GitHub)" <gi...@apache.org>.
cziegeler commented on PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-core/pull/15#issuecomment-2323807272
@pat-lego Thanks for the updates, any reason why you want to explicitly log the class name instead of relying on toString() returning something (more) useful?
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding ability to review selected auth handler [sling-org-apache-sling-auth-core]
Posted by "pat-lego (via GitHub)" <gi...@apache.org>.
pat-lego commented on PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-core/pull/15#issuecomment-2324734509
The original purpose of this change was to expose the authentication handler class name. This was the necessary information needed to resolve a large escalation no other information was needed. This pull request provides that exact information, anything else is just extra and was not needed to resolve the issue.
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding ability to review selected auth handler [sling-org-apache-sling-auth-core]
Posted by "cziegeler (via GitHub)" <gi...@apache.org>.
cziegeler commented on PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-core/pull/15#issuecomment-2324746762
ok, simply relying on toString() will give you that - avoids all null checking/handling - and if the handler overrides toString you benefit from that 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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding ability to review selected auth handler [sling-org-apache-sling-auth-core]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-core/pull/15#issuecomment-2275992274
## [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-core&pullRequest=15) **Quality Gate passed**
Issues
 [3 New issues](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-auth-core&pullRequest=15&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-auth-core&pullRequest=15&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-auth-core&pullRequest=15&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [80.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-auth-core&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-core&pullRequest=15&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-core&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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding ability to review selected auth handler [sling-org-apache-sling-auth-core]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-core/pull/15#issuecomment-2276136700
## [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-core&pullRequest=15) **Quality Gate passed**
Issues
 [6 New issues](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-auth-core&pullRequest=15&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-auth-core&pullRequest=15&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-auth-core&pullRequest=15&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [80.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-auth-core&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-core&pullRequest=15&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-auth-core&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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding ability to review selected auth handler [sling-org-apache-sling-auth-core]
Posted by "klcodanr (via GitHub)" <gi...@apache.org>.
klcodanr commented on code in PR #15:
URL: https://github.com/apache/sling-org-apache-sling-auth-core/pull/15#discussion_r1709679110
##########
src/main/java/org/apache/sling/auth/core/impl/SlingAuthenticator.java:
##########
@@ -663,7 +663,7 @@ private AuthenticationInfo getAuthenticationInfo(HttpServletRequest request, Htt
if (holder.isPathRequiresHandler(path)){
final AuthenticationInfo authInfo = holder.extractCredentials(
request, response);
-
+ log.debug("getAuthenticationInfo: Selected class {} to perform authentication", holder.getHandler().getClass().getName());
Review Comment:
Based on the EngineAuthenticationHandlerHolder there are cases where this could be null, which should be handled. How about:
```suggestion
log.debug("getAuthenticationInfo: Selected class {} to perform authentication", Optional.ofNullable(holder.getHandler()).map(h -> h.getClass().getName()).orElse("null"));
```
##########
src/main/java/org/apache/sling/auth/core/impl/SlingAuthenticator.java:
##########
@@ -672,9 +672,10 @@ private AuthenticationInfo getAuthenticationInfo(HttpServletRequest request, Htt
authInfo.put(AUTH_INFO_PROP_FEEDBACK_HANDLER,
holder.getFeedbackHandler());
}
-
+ log.debug("getAuthenticationInfo: Selected class {} returned non null result returning as authenticated result {}", holder.getHandler().getClass().getName(), authInfo);
Review Comment:
Same issue as above.
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org