You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by GitBox <gi...@apache.org> on 2021/03/12 10:58:40 UTC
[GitHub] [sling-org-apache-sling-feature-cpconverter] anchela opened a new pull request #61: SLING-10210 : Option to enforce service-user-mapping with principal names
anchela opened a new pull request #61:
URL: https://github.com/apache/sling-org-apache-sling-feature-cpconverter/pull/61
initial draft. tests still missing.
@karlpauls , i would appreciate if you would could review it..... if you feel that such an additional option would be a reasonable thing to add, i can spend a bit more time writing additional tests. wdyt?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-feature-cpconverter] anchela commented on a change in pull request #61: SLING-10210 : Option to enforce service-user-mapping with principal names
Posted by GitBox <gi...@apache.org>.
anchela commented on a change in pull request #61:
URL: https://github.com/apache/sling-org-apache-sling-feature-cpconverter/pull/61#discussion_r594962920
##########
File path: src/main/java/org/apache/sling/feature/cpconverter/accesscontrol/Mapping.java
##########
@@ -60,6 +66,9 @@ public Mapping(@NotNull final String spec) {
if (s.charAt(0) == '[' && s.charAt(s.length()-1) == ']') {
this.userName = null;
this.principalNames = extractPrincipalNames(s);
+ } else if (enforceMappingByPrincipal) {
+ this.userName = null;
+ this.principalNames = Collections.singleton(s);
Review comment:
see my other comment. group principals are not valid in service user mapping and would anyway be ignored.... i am aware of the potential breaking nature but it's fine for those cases where mapping-by-userid was just chosen for sloppiness and not because group membership is 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-feature-cpconverter] anchela merged pull request #61: SLING-10210 : Option to enforce service-user-mapping with principal names
Posted by GitBox <gi...@apache.org>.
anchela merged pull request #61:
URL: https://github.com/apache/sling-org-apache-sling-feature-cpconverter/pull/61
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-feature-cpconverter] sonarcloud[bot] commented on pull request #61: SLING-10210 : Option to enforce service-user-mapping with principal names
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #61:
URL: https://github.com/apache/sling-org-apache-sling-feature-cpconverter/pull/61#issuecomment-800112010
Kudos, SonarCloud Quality Gate passed!
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=VULNERABILITY)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=SECURITY_HOTSPOT) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=SECURITY_HOTSPOT)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/60.png' alt='88.2%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&metric=new_coverage&view=list) [88.2% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&metric=new_coverage&view=list)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&metric=new_duplicated_lines_density&view=list)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-feature-cpconverter] kwin commented on a change in pull request #61: SLING-10210 : Option to enforce service-user-mapping with principal names
Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #61:
URL: https://github.com/apache/sling-org-apache-sling-feature-cpconverter/pull/61#discussion_r593101084
##########
File path: src/main/java/org/apache/sling/feature/cpconverter/cli/ContentPackage2FeatureModelConverterLauncher.java
##########
@@ -101,6 +101,9 @@
@Option(names = { "--enforce-principal-based-supported-path" }, description = "Converts service user access control entries to principal-based setup using the given supported path.", required = false)
private String enforcePrincipalBasedSupportedPath = null;
+ @Option(names = { "--enforce-servicemapping-by-principal" }, description = "Converts service user mappings with the form 'service:sub=userID' to 'service:sub=[principalname]'.", required = false)
Review comment:
It should be clarified how the mapping is done: To which degree are the user's group membership considered?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-feature-cpconverter] anchela commented on a change in pull request #61: SLING-10210 : Option to enforce service-user-mapping with principal names
Posted by GitBox <gi...@apache.org>.
anchela commented on a change in pull request #61:
URL: https://github.com/apache/sling-org-apache-sling-feature-cpconverter/pull/61#discussion_r594985396
##########
File path: src/main/java/org/apache/sling/feature/cpconverter/accesscontrol/Mapping.java
##########
@@ -60,6 +66,9 @@ public Mapping(@NotNull final String spec) {
if (s.charAt(0) == '[' && s.charAt(s.length()-1) == ']') {
this.userName = null;
this.principalNames = extractPrincipalNames(s);
+ } else if (enforceMappingByPrincipal) {
+ this.userName = null;
+ this.principalNames = Collections.singleton(s);
Review comment:
@karlpauls , right.... also if mapping-by-id is no longer valid in AEMaaCS those mappings by id will be rendered invalid and enabling the option is in the majority of cases that i have analyzed so far a non-breaking transformation.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-feature-cpconverter] sonarcloud[bot] commented on pull request #61: SLING-10210 : Option to enforce service-user-mapping with principal names
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #61:
URL: https://github.com/apache/sling-org-apache-sling-feature-cpconverter/pull/61#issuecomment-797420819
Kudos, SonarCloud Quality Gate passed!
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=VULNERABILITY)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=SECURITY_HOTSPOT) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=SECURITY_HOTSPOT)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/60.png' alt='83.7%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&metric=new_coverage&view=list) [83.7% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&metric=new_coverage&view=list)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&metric=new_duplicated_lines_density&view=list)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-feature-cpconverter] kwin commented on a change in pull request #61: SLING-10210 : Option to enforce service-user-mapping with principal names
Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #61:
URL: https://github.com/apache/sling-org-apache-sling-feature-cpconverter/pull/61#discussion_r594952299
##########
File path: src/main/java/org/apache/sling/feature/cpconverter/accesscontrol/Mapping.java
##########
@@ -60,6 +66,9 @@ public Mapping(@NotNull final String spec) {
if (s.charAt(0) == '[' && s.charAt(s.length()-1) == ']') {
this.userName = null;
this.principalNames = extractPrincipalNames(s);
+ } else if (enforceMappingByPrincipal) {
+ this.userName = null;
+ this.principalNames = Collections.singleton(s);
Review comment:
My comment was referring to the fact that you change user id to principal names which changes the semantics as long as you don't consider the transitive group memberships (see my other comment).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-feature-cpconverter] kwin commented on a change in pull request #61: SLING-10210 : Option to enforce service-user-mapping with principal names
Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #61:
URL: https://github.com/apache/sling-org-apache-sling-feature-cpconverter/pull/61#discussion_r593100511
##########
File path: src/main/java/org/apache/sling/feature/cpconverter/accesscontrol/Mapping.java
##########
@@ -60,6 +66,9 @@ public Mapping(@NotNull final String spec) {
if (s.charAt(0) == '[' && s.charAt(s.length()-1) == ']') {
this.userName = null;
this.principalNames = extractPrincipalNames(s);
+ } else if (enforceMappingByPrincipal) {
+ this.userName = null;
+ this.principalNames = Collections.singleton(s);
Review comment:
I think you need to recursively add all involved principal ids, otherwise the conversion may change privileges of the service user....
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-feature-cpconverter] anchela commented on a change in pull request #61: SLING-10210 : Option to enforce service-user-mapping with principal names
Posted by GitBox <gi...@apache.org>.
anchela commented on a change in pull request #61:
URL: https://github.com/apache/sling-org-apache-sling-feature-cpconverter/pull/61#discussion_r594932657
##########
File path: src/main/java/org/apache/sling/feature/cpconverter/accesscontrol/Mapping.java
##########
@@ -60,6 +66,9 @@ public Mapping(@NotNull final String spec) {
if (s.charAt(0) == '[' && s.charAt(s.length()-1) == ']') {
this.userName = null;
this.principalNames = extractPrincipalNames(s);
+ } else if (enforceMappingByPrincipal) {
+ this.userName = null;
+ this.principalNames = Collections.singleton(s);
Review comment:
@kwin , the mapping is by a single service-user-id and doesn't involve any additional principal ids. and group principals are not valid service user ids/prinicipals. they get resolved upon service-login if the mapping is done by id.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-feature-cpconverter] sonarcloud[bot] removed a comment on pull request #61: SLING-10210 : Option to enforce service-user-mapping with principal names
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] removed a comment on pull request #61:
URL: https://github.com/apache/sling-org-apache-sling-feature-cpconverter/pull/61#issuecomment-800065351
Kudos, SonarCloud Quality Gate passed!
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=VULNERABILITY)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=SECURITY_HOTSPOT) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=SECURITY_HOTSPOT)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/60.png' alt='87.8%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&metric=new_coverage&view=list) [87.8% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&metric=new_coverage&view=list)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&metric=new_duplicated_lines_density&view=list)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-feature-cpconverter] anchela commented on a change in pull request #61: SLING-10210 : Option to enforce service-user-mapping with principal names
Posted by GitBox <gi...@apache.org>.
anchela commented on a change in pull request #61:
URL: https://github.com/apache/sling-org-apache-sling-feature-cpconverter/pull/61#discussion_r594960592
##########
File path: src/main/java/org/apache/sling/feature/cpconverter/cli/ContentPackage2FeatureModelConverterLauncher.java
##########
@@ -101,6 +101,9 @@
@Option(names = { "--enforce-principal-based-supported-path" }, description = "Converts service user access control entries to principal-based setup using the given supported path.", required = false)
private String enforcePrincipalBasedSupportedPath = null;
+ @Option(names = { "--enforce-servicemapping-by-principal" }, description = "Converts service user mappings with the form 'service:sub=userID' to 'service:sub=[principalname]'.", required = false)
Review comment:
@kwin , group principals are not valid in service user mapping.... so yes this is on purpose.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-feature-cpconverter] kwin commented on a change in pull request #61: SLING-10210 : Option to enforce service-user-mapping with principal names
Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #61:
URL: https://github.com/apache/sling-org-apache-sling-feature-cpconverter/pull/61#discussion_r594939734
##########
File path: src/main/java/org/apache/sling/feature/cpconverter/cli/ContentPackage2FeatureModelConverterLauncher.java
##########
@@ -101,6 +101,9 @@
@Option(names = { "--enforce-principal-based-supported-path" }, description = "Converts service user access control entries to principal-based setup using the given supported path.", required = false)
private String enforcePrincipalBasedSupportedPath = null;
+ @Option(names = { "--enforce-servicemapping-by-principal" }, description = "Converts service user mappings with the form 'service:sub=userID' to 'service:sub=[principalname]'.", required = false)
Review comment:
Sling behaves the same as AEM: https://github.com/apache/sling-org-apache-sling-jcr-base/blob/66be360910c265473799635fcac0e23895898913/src/main/java/org/apache/sling/jcr/base/AbstractSlingRepository2.java#L240
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-feature-cpconverter] anchela commented on a change in pull request #61: SLING-10210 : Option to enforce service-user-mapping with principal names
Posted by GitBox <gi...@apache.org>.
anchela commented on a change in pull request #61:
URL: https://github.com/apache/sling-org-apache-sling-feature-cpconverter/pull/61#discussion_r594931133
##########
File path: src/main/java/org/apache/sling/feature/cpconverter/cli/ContentPackage2FeatureModelConverterLauncher.java
##########
@@ -101,6 +101,9 @@
@Option(names = { "--enforce-principal-based-supported-path" }, description = "Converts service user access control entries to principal-based setup using the given supported path.", required = false)
private String enforcePrincipalBasedSupportedPath = null;
+ @Option(names = { "--enforce-servicemapping-by-principal" }, description = "Converts service user mappings with the form 'service:sub=userID' to 'service:sub=[principalname]'.", required = false)
Review comment:
good point. i will expand the comment explaining that group membership will potentially no longer be resolved. the reason why i say "potentially": it depends on the implementation. i am not too familiar with the default in sling but with AEM group membership is not resolved as this was the primary reason for introducing the mapping by principal-names. group membership with service users should be considered a potential security risk because permission setup with service users is part of the application/service, while permissions for groups in particular everyone is not part of the application/service any may vary from installation to installation.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-feature-cpconverter] kwin commented on a change in pull request #61: SLING-10210 : Option to enforce service-user-mapping with principal names
Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #61:
URL: https://github.com/apache/sling-org-apache-sling-feature-cpconverter/pull/61#discussion_r594965830
##########
File path: src/main/java/org/apache/sling/feature/cpconverter/accesscontrol/Mapping.java
##########
@@ -60,6 +66,9 @@ public Mapping(@NotNull final String spec) {
if (s.charAt(0) == '[' && s.charAt(s.length()-1) == ']') {
this.userName = null;
this.principalNames = extractPrincipalNames(s);
+ } else if (enforceMappingByPrincipal) {
+ this.userName = null;
+ this.principalNames = Collections.singleton(s);
Review comment:
> i am aware of the potential breaking nature
This is very dangerous and therefore cp2fm should either be a lossless conversion or throw an exception. Debugging issues like these is very hard as most consumers will not be aware of the potentially lossy transformation here.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-feature-cpconverter] karlpauls commented on a change in pull request #61: SLING-10210 : Option to enforce service-user-mapping with principal names
Posted by GitBox <gi...@apache.org>.
karlpauls commented on a change in pull request #61:
URL: https://github.com/apache/sling-org-apache-sling-feature-cpconverter/pull/61#discussion_r594975284
##########
File path: src/main/java/org/apache/sling/feature/cpconverter/accesscontrol/Mapping.java
##########
@@ -60,6 +66,9 @@ public Mapping(@NotNull final String spec) {
if (s.charAt(0) == '[' && s.charAt(s.length()-1) == ']') {
this.userName = null;
this.principalNames = extractPrincipalNames(s);
+ } else if (enforceMappingByPrincipal) {
+ this.userName = null;
+ this.principalNames = Collections.singleton(s);
Review comment:
Right, but that is why it is off by default and needs to be enabled explicitly, no?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-feature-cpconverter] sonarcloud[bot] removed a comment on pull request #61: SLING-10210 : Option to enforce service-user-mapping with principal names
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] removed a comment on pull request #61:
URL: https://github.com/apache/sling-org-apache-sling-feature-cpconverter/pull/61#issuecomment-797420819
Kudos, SonarCloud Quality Gate passed!
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=VULNERABILITY)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=SECURITY_HOTSPOT) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=SECURITY_HOTSPOT)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/60.png' alt='83.7%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&metric=new_coverage&view=list) [83.7% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&metric=new_coverage&view=list)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&metric=new_duplicated_lines_density&view=list)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-feature-cpconverter] sonarcloud[bot] commented on pull request #61: SLING-10210 : Option to enforce service-user-mapping with principal names
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #61:
URL: https://github.com/apache/sling-org-apache-sling-feature-cpconverter/pull/61#issuecomment-800065351
Kudos, SonarCloud Quality Gate passed!
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=VULNERABILITY)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=SECURITY_HOTSPOT) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=SECURITY_HOTSPOT)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/60.png' alt='87.8%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&metric=new_coverage&view=list) [87.8% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&metric=new_coverage&view=list)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-feature-cpconverter&pullRequest=61&metric=new_duplicated_lines_density&view=list)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org