You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by "quantranhong1999 (via GitHub)" <gi...@apache.org> on 2023/03/03 10:39:58 UTC

[GitHub] [james-project] quantranhong1999 opened a new pull request, #1473: JAMES-3885 Delegation should be supported when using LDAP

quantranhong1999 opened a new pull request, #1473:
URL: https://github.com/apache/james-project/pull/1473

   Complement fix for: https://github.com/apache/james-project/pull/1446/commits/38c2031e75e30f06ab6837667a8ae127aefeaae7


-- 
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: notifications-unsubscribe@james.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org


[GitHub] [james-project] quantranhong1999 commented on a diff in pull request #1473: JAMES-3885 Delegation should be supported when using LDAP

Posted by "quantranhong1999 (via GitHub)" <gi...@apache.org>.
quantranhong1999 commented on code in PR #1473:
URL: https://github.com/apache/james-project/pull/1473#discussion_r1125947189


##########
server/data/data-cassandra/src/main/java/org/apache/james/user/cassandra/CassandraUsersDAO.java:
##########
@@ -212,11 +210,7 @@ public Mono<Void> addAuthorizedUsers(Username baseUser, Username userWithAccess)
             .setString(NAME, userWithAccess.asString())
             .setSet(DELEGATED_USERS, ImmutableSet.of(baseUser.asString()), String.class));
 
-        return getUserByNameReactive(userWithAccess).hasElement()
-            .filter(FunctionalUtils.identityPredicate())
-            .map(existAuthorizedUser -> (Statement) batchBuilder.build())
-            .switchIfEmpty(Mono.just(addAuthorizedStatement))
-            .flatMap(executor::executeVoid);
+        return executor.executeVoid(batchBuilder.build());

Review Comment:
   I explained it in the commit message:
   
   GIVEN there are no users in user table
   Behavior before: only delegatees source is modified, leaving the delegator source is not consistent with delegatees source
   
   We have LDAP user repository edge case for delegation where LDAP does not depend on `user` table (therefore empty table), so we need to be more lenient managing delegations for this case.



-- 
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: notifications-unsubscribe@james.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org


[GitHub] [james-project] quantranhong1999 commented on pull request #1473: JAMES-3885 Delegation should be supported when using LDAP

Posted by "quantranhong1999 (via GitHub)" <gi...@apache.org>.
quantranhong1999 commented on PR #1473:
URL: https://github.com/apache/james-project/pull/1473#issuecomment-1457871355

   > If you want but a NoClassDefFoundError usually smells pretty bad
   
   Local run seems fine. TBH I am kinda sure saw them in other builds in the past.


-- 
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: notifications-unsubscribe@james.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org


[GitHub] [james-project] Arsnael merged pull request #1473: JAMES-3885 Delegation should be supported when using LDAP

Posted by "Arsnael (via GitHub)" <gi...@apache.org>.
Arsnael merged PR #1473:
URL: https://github.com/apache/james-project/pull/1473


-- 
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: notifications-unsubscribe@james.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org


[GitHub] [james-project] Arsnael commented on pull request #1473: JAMES-3885 Delegation should be supported when using LDAP

Posted by "Arsnael (via GitHub)" <gi...@apache.org>.
Arsnael commented on PR #1473:
URL: https://github.com/apache/james-project/pull/1473#issuecomment-1457887575

   There seems to be some issues with the apache jenkins INFRA atm so plase be patient :)


-- 
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: notifications-unsubscribe@james.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org


[GitHub] [james-project] quantranhong1999 commented on pull request #1473: JAMES-3885 Delegation should be supported when using LDAP

Posted by "quantranhong1999 (via GitHub)" <gi...@apache.org>.
quantranhong1999 commented on PR #1473:
URL: https://github.com/apache/james-project/pull/1473#issuecomment-1457843387

   > Could not initialize class org.apache.james.backends.opensearch.DockerOpenSearchSingleton
   
   Could you rerun CI? Seem not related to me.


-- 
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: notifications-unsubscribe@james.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org


[GitHub] [james-project] Arsnael commented on a diff in pull request #1473: JAMES-3885 Delegation should be supported when using LDAP

Posted by "Arsnael (via GitHub)" <gi...@apache.org>.
Arsnael commented on code in PR #1473:
URL: https://github.com/apache/james-project/pull/1473#discussion_r1125936684


##########
server/data/data-cassandra/src/main/java/org/apache/james/user/cassandra/CassandraUsersDAO.java:
##########
@@ -212,11 +210,7 @@ public Mono<Void> addAuthorizedUsers(Username baseUser, Username userWithAccess)
             .setString(NAME, userWithAccess.asString())
             .setSet(DELEGATED_USERS, ImmutableSet.of(baseUser.asString()), String.class));
 
-        return getUserByNameReactive(userWithAccess).hasElement()
-            .filter(FunctionalUtils.identityPredicate())
-            .map(existAuthorizedUser -> (Statement) batchBuilder.build())
-            .switchIfEmpty(Mono.just(addAuthorizedStatement))
-            .flatMap(executor::executeVoid);
+        return executor.executeVoid(batchBuilder.build());

Review Comment:
   Why this change?



-- 
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: notifications-unsubscribe@james.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org


[GitHub] [james-project] Arsnael commented on pull request #1473: JAMES-3885 Delegation should be supported when using LDAP

Posted by "Arsnael (via GitHub)" <gi...@apache.org>.
Arsnael commented on PR #1473:
URL: https://github.com/apache/james-project/pull/1473#issuecomment-1457443071

   ```
   org.apache.james.JPAWithLDAPJamesServerTest.org.apache.james.JPAWithLDAPJamesServerTest
   
   Unable to create injector, see the following errors:
   
   1) [Guice/BindingAlreadySet]: Authorizator was bound multiple times.
   
   Bound at:
   1  : JPAMailboxModule.configure(JPAMailboxModule.java:112)
         \_ installed by: Modules$OverrideModule -> Modules$OverrideModule -> Modules$OverrideModule -> Modules$OverrideModule -> Modules$CombinedModule -> Modules$CombinedModule -> Modules$CombinedModule -> Modules$CombinedModule -> JPAMailboxModule
   2  : LdapUsersRepositoryModule.configure(LdapUsersRepositoryModule.java:45)
         \_ installed by: Modules$OverrideModule -> Modules$OverrideModule -> Modules$OverrideModule -> Modules$OverrideModule -> Modules$CombinedModule -> LdapUsersRepositoryModule
   
   Learn more:
     https://github.com/google/guice/wiki/BINDING_ALREADY_SET
   
   1 error
   
   ======================
   Full classname legend:
   ======================
   Authorizator:              "org.apache.james.mailbox.Authorizator"
   JPAMailboxModule:          "org.apache.james.modules.mailbox.JPAMailboxModule"
   LdapUsersRepositoryModule: "org.apache.james.data.LdapUsersRepositoryModule"
   Modules$CombinedModule:    "com.google.inject.util.Modules$CombinedModule"
   Modules$OverrideModule:    "com.google.inject.util.Modules$OverrideModule"
   ========================
   End of classname legend:
   ========================
   ```


-- 
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: notifications-unsubscribe@james.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org


[GitHub] [james-project] Arsnael commented on pull request #1473: JAMES-3885 Delegation should be supported when using LDAP

Posted by "Arsnael (via GitHub)" <gi...@apache.org>.
Arsnael commented on PR #1473:
URL: https://github.com/apache/james-project/pull/1473#issuecomment-1457829852

   https://ci-builds.apache.org/job/james/job/ApacheJames/job/PR-1473/3/testReport/
   
   =>
   ```
   org.apache.james.mailbox.opensearch.OpenSearchIntegrationTest.org.apache.james.mailbox.opensearch.OpenSearchIntegrationTest
   
   Error Message
   Could not initialize class org.apache.james.backends.opensearch.DockerOpenSearchSingleton
   Stacktrace
   java.lang.NoClassDefFoundError: Could not initialize class org.apache.james.backends.opensearch.DockerOpenSearchSingleton
   	at org.apache.james.mailbox.opensearch.OpenSearchIntegrationTest.<clinit>(OpenSearchIntegrationTest.java:89)
   ```


-- 
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: notifications-unsubscribe@james.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org


[GitHub] [james-project] Arsnael commented on pull request #1473: JAMES-3885 Delegation should be supported when using LDAP

Posted by "Arsnael (via GitHub)" <gi...@apache.org>.
Arsnael commented on PR #1473:
URL: https://github.com/apache/james-project/pull/1473#issuecomment-1457857788

   If you want but a NoClassDefFoundError usually smells pretty bad


-- 
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: notifications-unsubscribe@james.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org