You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by GitBox <gi...@apache.org> on 2023/01/04 11:01:28 UTC
[GitHub] [james-project] quantranhong1999 commented on a diff in pull request #1372: JAMES-3756 Delegation store should allow listing accounts delegated to me
quantranhong1999 commented on code in PR #1372:
URL: https://github.com/apache/james-project/pull/1372#discussion_r1061365640
##########
server/data/data-cassandra/src/main/java/org/apache/james/user/cassandra/CassandraDelegationStore.java:
##########
@@ -40,16 +46,31 @@ public Publisher<Username> authorizedUsers(Username baseUser) {
@Override
public Publisher<Void> clear(Username baseUser) {
- return cassandraUsersDAO.removeAllAuthorizedUsers(baseUser);
+ return cassandraUsersDAO.getAuthorizedUsers(baseUser)
+ .flatMap(authorizedUser -> cassandraUsersDAO.removeDelegatedToUser(authorizedUser, baseUser))
+ .then(cassandraUsersDAO.removeAllAuthorizedUsers(baseUser));
}
@Override
public Publisher<Void> addAuthorizedUser(Username baseUser, Username userWithAccess) {
- return cassandraUsersDAO.addAuthorizedUsers(baseUser, userWithAccess);
+ return cassandraUsersDAO.addAuthorizedUsers(baseUser, userWithAccess)
+ .then(Mono.from(cassandraUsersDAO.containsReactive(userWithAccess))
+ .filter(FunctionalUtils.identityPredicate())
+ .flatMap(authorizedUser -> cassandraUsersDAO.addDelegatedToUsers(userWithAccess, baseUser))
Review Comment:
Please try to use (logged) BATCH in Cassandra to archive atomicity IMO.
##########
server/data/data-api/src/main/java/org/apache/james/user/api/DelegationStore.java:
##########
@@ -45,4 +45,6 @@ default Fluent addAuthorizedUser(Username userWithAccess) {
default Fluent removeAuthorizedUser(Username userWithAccess) {
return baseUser -> removeAuthorizedUser(baseUser, userWithAccess);
}
+
+ Publisher<Username> delegatedUsers(Username baseUser);
}
Review Comment:
TODO:
`Publisher<Void> removeDelegatedAccount(Username baseUser, Username delegatedToUser);`
and test for deletion?
##########
server/data/data-cassandra/src/main/java/org/apache/james/user/cassandra/CassandraDelegationStore.java:
##########
@@ -40,16 +46,31 @@ public Publisher<Username> authorizedUsers(Username baseUser) {
@Override
public Publisher<Void> clear(Username baseUser) {
- return cassandraUsersDAO.removeAllAuthorizedUsers(baseUser);
+ return cassandraUsersDAO.getAuthorizedUsers(baseUser)
+ .flatMap(authorizedUser -> cassandraUsersDAO.removeDelegatedToUser(authorizedUser, baseUser))
+ .then(cassandraUsersDAO.removeAllAuthorizedUsers(baseUser));
}
@Override
public Publisher<Void> addAuthorizedUser(Username baseUser, Username userWithAccess) {
- return cassandraUsersDAO.addAuthorizedUsers(baseUser, userWithAccess);
+ return cassandraUsersDAO.addAuthorizedUsers(baseUser, userWithAccess)
+ .then(Mono.from(cassandraUsersDAO.containsReactive(userWithAccess))
+ .filter(FunctionalUtils.identityPredicate())
+ .flatMap(authorizedUser -> cassandraUsersDAO.addDelegatedToUsers(userWithAccess, baseUser))
+ .onErrorResume(error -> {
+ LOGGER.warn("Can not add delegated user: {} to user: {}", userWithAccess, baseUser);
+ return Mono.empty();
+ }));
}
@Override
public Publisher<Void> removeAuthorizedUser(Username baseUser, Username userWithAccess) {
- return cassandraUsersDAO.removeAuthorizedUser(baseUser, userWithAccess);
+ return cassandraUsersDAO.removeAuthorizedUser(baseUser, userWithAccess)
+ .then(cassandraUsersDAO.removeDelegatedToUser(userWithAccess, baseUser));
Review Comment:
Please try to use (logged) BATCH in Cassandra to archive atomicity IMO.
##########
server/data/data-cassandra/src/main/java/org/apache/james/user/cassandra/CassandraUsersRepositoryModule.java:
##########
@@ -32,6 +32,7 @@ public interface CassandraUsersRepositoryModule {
.withColumn(CassandraUserTable.REALNAME, DataTypes.TEXT)
.withColumn(CassandraUserTable.PASSWORD, DataTypes.TEXT)
.withColumn(CassandraUserTable.ALGORITHM, DataTypes.TEXT)
- .withColumn(CassandraUserTable.AUTHORIZED_USERS, DataTypes.setOf(DataTypes.TEXT)))
+ .withColumn(CassandraUserTable.AUTHORIZED_USERS, DataTypes.setOf(DataTypes.TEXT))
+ .withColumn(CassandraUserTable.DELEGATED_USERS, DataTypes.setOf(DataTypes.TEXT)))
Review Comment:
Maybe modify `upgrade-instruction` in this PR also so we would not forget?
##########
server/data/data-cassandra/src/main/java/org/apache/james/user/cassandra/CassandraDelegationStore.java:
##########
@@ -40,16 +46,31 @@ public Publisher<Username> authorizedUsers(Username baseUser) {
@Override
public Publisher<Void> clear(Username baseUser) {
- return cassandraUsersDAO.removeAllAuthorizedUsers(baseUser);
+ return cassandraUsersDAO.getAuthorizedUsers(baseUser)
+ .flatMap(authorizedUser -> cassandraUsersDAO.removeDelegatedToUser(authorizedUser, baseUser))
+ .then(cassandraUsersDAO.removeAllAuthorizedUsers(baseUser));
Review Comment:
Please try to use (logged) BATCH in Cassandra to archive atomicity IMO.
--
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