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/09 10:49:30 UTC
[GitHub] [james-project] vttranlina opened a new pull request, #1382: JAMES-3756 JMAP endpoint (upload/download) should support being called with accountIds of delegated accounts
vttranlina opened a new pull request, #1382:
URL: https://github.com/apache/james-project/pull/1382
resolve https://github.com/linagora/james-project/issues/4679
--
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] chibenwa commented on a diff in pull request #1382: JAMES-3756 JMAP endpoint (upload/download) should support being called with accountIds of delegated accounts
Posted by GitBox <gi...@apache.org>.
chibenwa commented on code in PR #1382:
URL: https://github.com/apache/james-project/pull/1382#discussion_r1064750069
##########
server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/routes/UploadRoutes.scala:
##########
@@ -113,27 +115,30 @@ class UploadRoutes @Inject()(@Named(InjectionKeys.RFC_8621) val authenticator: A
}
}
- def post(request: HttpServerRequest, response: HttpServerResponse, contentType: ContentType, session: MailboxSession): SMono[Void] = {
+ def post(request: HttpServerRequest, response: HttpServerResponse, contentType: ContentType, mailboxSession: MailboxSession): SMono[Void] = {
Id.validate(request.param(accountIdParam)) match {
case Right(id: Id) =>
val targetAccountId: AccountId = AccountId(id)
- AccountId.from(session.getUser).map(accountId => accountId.equals(targetAccountId))
- .fold[SMono[Void]](
- e => SMono.error(e),
- value => if (value) {
- SMono.fromCallable(() => ReactorUtils.toInputStream(request.receive
- // Unwrapping to byte array needed to solve data races and buffer reordering when using .asByteBuffer()
- .asByteArray()
- .map(array => ByteBuffer.wrap(array))))
- .flatMap(content => handle(targetAccountId, contentType, content, session, response))
- } else {
- SMono.error(ForbiddenException())
- })
+
+ sessionTranslator.delegateIfNeeded(mailboxSession, targetAccountId)
+ .flatMap(session => AccountId.from(session.getUser)
+ .fold(e => SMono.error(e),
+ accountId => SMono.just(accountId.equals(targetAccountId))
+ .filter(containAccountId => containAccountId)
+ .flatMap(_ => handle(request, response, contentType, session, targetAccountId))
+ .switchIfEmpty(SMono.error(ForbiddenException()))))
Review Comment:
Idem there's some unused logic to be simplified in here, similar to download.
--
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] chibenwa merged pull request #1382: JAMES-3756 JMAP endpoint (upload/download) should support being called with accountIds of delegated accounts
Posted by GitBox <gi...@apache.org>.
chibenwa merged PR #1382:
URL: https://github.com/apache/james-project/pull/1382
--
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] chibenwa commented on a diff in pull request #1382: JAMES-3756 JMAP endpoint (upload/download) should support being called with accountIds of delegated accounts
Posted by GitBox <gi...@apache.org>.
chibenwa commented on code in PR #1382:
URL: https://github.com/apache/james-project/pull/1382#discussion_r1064748692
##########
server/protocols/jmap-rfc-8621-integration-tests/memory-jmap-rfc-8621-integration-tests/src/test/java/org/apache/james/jmap/rfc8621/memory/MemoryUploadTest.java:
##########
@@ -38,6 +42,10 @@ public class MemoryUploadTest implements UploadContract {
.usersRepository(DEFAULT)
.build())
.server(configuration -> MemoryJamesServerMain.createServer(configuration)
- .overrideWith(new TestJMAPServerModule()))
+ .overrideWith(new TestJMAPServerModule())
+ .overrideWith(binder -> {
+ binder.bind(DelegationStoreAuthorizator.class).in(Scopes.SINGLETON);
+ binder.bind(Authorizator.class).to(DelegationStoreAuthorizator.class);
+ }))
Review Comment:
With https://github.com/apache/james-project/pull/1380/commits/42bad47a6381046a135a08e27be264c4857c0fc9 this should not be 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: 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] vttranlina commented on pull request #1382: JAMES-3756 JMAP endpoint (upload/download) should support being called with accountIds of delegated accounts
Posted by GitBox <gi...@apache.org>.
vttranlina commented on PR #1382:
URL: https://github.com/apache/james-project/pull/1382#issuecomment-1379948717
green
--
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] chibenwa commented on pull request #1382: JAMES-3756 JMAP endpoint (upload/download) should support being called with accountIds of delegated accounts
Posted by GitBox <gi...@apache.org>.
chibenwa commented on PR #1382:
URL: https://github.com/apache/james-project/pull/1382#issuecomment-1378194960
Please rebase @vttranlina
--
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] chibenwa commented on a diff in pull request #1382: JAMES-3756 JMAP endpoint (upload/download) should support being called with accountIds of delegated accounts
Posted by GitBox <gi...@apache.org>.
chibenwa commented on code in PR #1382:
URL: https://github.com/apache/james-project/pull/1382#discussion_r1064742796
##########
server/protocols/jmap-rfc-8621-integration-tests/memory-jmap-rfc-8621-integration-tests/src/test/java/org/apache/james/jmap/rfc8621/memory/MemoryDownloadTest.java:
##########
@@ -42,7 +46,11 @@ public class MemoryDownloadTest implements DownloadContract {
.usersRepository(DEFAULT)
.build())
.server(configuration -> MemoryJamesServerMain.createServer(configuration)
- .overrideWith(new TestJMAPServerModule()))
+ .overrideWith(new TestJMAPServerModule())
+ .overrideWith(binder -> {
+ binder.bind(DelegationStoreAuthorizator.class).in(Scopes.SINGLETON);
+ binder.bind(Authorizator.class).to(DelegationStoreAuthorizator.class);
+ }))
Review Comment:
Cherry pick https://github.com/apache/james-project/pull/1380/commits/42bad47a6381046a135a08e27be264c4857c0fc9 instead
--
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] chibenwa commented on a diff in pull request #1382: JAMES-3756 JMAP endpoint (upload/download) should support being called with accountIds of delegated accounts
Posted by GitBox <gi...@apache.org>.
chibenwa commented on code in PR #1382:
URL: https://github.com/apache/james-project/pull/1382#discussion_r1064747554
##########
server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/routes/DownloadRoutes.scala:
##########
@@ -297,14 +298,14 @@ class DownloadRoutes @Inject()(@Named(InjectionKeys.RFC_8621) val authenticator:
Id.validate(request.param(accountIdParam)) match {
case Right(id: Id) =>
val targetAccountId: AccountId = AccountId(id)
- AccountId.from(mailboxSession.getUser).map(accountId => accountId.equals(targetAccountId))
- .fold[SMono[Unit]](
- e => SMono.error(e),
- value => if (value) {
- get(request, response, mailboxSession)
- } else {
- SMono.error(ForbiddenException())
- })
+
+ sessionTranslator.delegateIfNeeded(mailboxSession, targetAccountId)
+ .flatMap(session => AccountId.from(session.getUser)
+ .fold(e => SMono.error(e),
+ accountId => SMono.just(accountId.equals(targetAccountId))
+ .filter(containAccountId => containAccountId)
+ .flatMap(_ => get(request, response, session))
+ .switchIfEmpty(SMono.error(ForbiddenException()))))
Review Comment:
This code is hard to read I am pretty sure it can be refactored to be more readable...
I am pretty sure the `(accountId.equals(targetAccountId)` check is now useless and can be removed:
```
sessionTranslator.delegateIfNeeded(mailboxSession, targetAccountId)
.flatMap(session => get(request, response, session))
```
--
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] vttranlina commented on a diff in pull request #1382: JAMES-3756 JMAP endpoint (upload/download) should support being called with accountIds of delegated accounts
Posted by GitBox <gi...@apache.org>.
vttranlina commented on code in PR #1382:
URL: https://github.com/apache/james-project/pull/1382#discussion_r1065408878
##########
server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/routes/DownloadRoutes.scala:
##########
@@ -297,14 +298,14 @@ class DownloadRoutes @Inject()(@Named(InjectionKeys.RFC_8621) val authenticator:
Id.validate(request.param(accountIdParam)) match {
case Right(id: Id) =>
val targetAccountId: AccountId = AccountId(id)
- AccountId.from(mailboxSession.getUser).map(accountId => accountId.equals(targetAccountId))
- .fold[SMono[Unit]](
- e => SMono.error(e),
- value => if (value) {
- get(request, response, mailboxSession)
- } else {
- SMono.error(ForbiddenException())
- })
+
+ sessionTranslator.delegateIfNeeded(mailboxSession, targetAccountId)
+ .flatMap(session => AccountId.from(session.getUser)
+ .fold(e => SMono.error(e),
+ accountId => SMono.just(accountId.equals(targetAccountId))
+ .filter(containAccountId => containAccountId)
+ .flatMap(_ => get(request, response, session))
+ .switchIfEmpty(SMono.error(ForbiddenException()))))
Review Comment:
It really pretty
--
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] vttranlina commented on pull request #1382: JAMES-3756 JMAP endpoint (upload/download) should support being called with accountIds of delegated accounts
Posted by GitBox <gi...@apache.org>.
vttranlina commented on PR #1382:
URL: https://github.com/apache/james-project/pull/1382#issuecomment-1378246667
rebased
--
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