[jira] [Commented] (SLING-8869) SimpleHttpDistributionTransport does not refresh the secret for token based implementations.
[
https://issues.apache.org/jira/browse/SLING-8869?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16989081#comment-16989081
]
Timothee Maret commented on SLING-8869:
---
Merged PR
[#28|https://github.com/apache/sling-org-apache-sling-distribution-core/pull/28]
thanks!
> SimpleHttpDistributionTransport does not refresh the secret for token based
> implementations.
>
>
> Key: SLING-8869
> URL: https://issues.apache.org/jira/browse/SLING-8869
> Project: Sling
> Issue Type: Bug
> Components: Content Distribution
>Reporter: Mohit Arora
>Assignee: Timothee Maret
>Priority: Critical
> Fix For: Content Distribution Core 0.4.2
>
> Attachments: SLING-8869-new.patch, SLING-8869.patch
>
> Time Spent: 2.5h
> Remaining Estimate: 0h
>
> While saving the {{contextKeyExecutor}} in {{DistributionTransportContext}}
> map, it is not expected that the secret associated with the executor could be
> expired. This can happen in case of access token based implementations where
> the token is expired after a certain period of time and has to be refreshed.
> The code to refresh the token is written in the secret provider but since the
> executor is [cached in the
> map|https://github.com/apache/sling-org-apache-sling-distribution-core/blob/master/src/main/java/org/apache/sling/distribution/transport/impl/SimpleHttpDistributionTransport.java#L208]
> the secrets are not refreshed. It works fine for credentials based secret
> provider but not for access token based.
> cc - [~marett]
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (SLING-8869) SimpleHttpDistributionTransport does not refresh the secret for token based implementations.
[
https://issues.apache.org/jira/browse/SLING-8869?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16988966#comment-16988966
]
Mohit Arora commented on SLING-8869:
[~marett] I have addressed the NPE. Can you please review the PR again?
> SimpleHttpDistributionTransport does not refresh the secret for token based
> implementations.
>
>
> Key: SLING-8869
> URL: https://issues.apache.org/jira/browse/SLING-8869
> Project: Sling
> Issue Type: Bug
> Components: Content Distribution
>Reporter: Mohit Arora
>Assignee: Timothee Maret
>Priority: Critical
> Fix For: Content Distribution Core 0.4.2
>
> Attachments: SLING-8869-new.patch, SLING-8869.patch
>
> Time Spent: 2h 10m
> Remaining Estimate: 0h
>
> While saving the {{contextKeyExecutor}} in {{DistributionTransportContext}}
> map, it is not expected that the secret associated with the executor could be
> expired. This can happen in case of access token based implementations where
> the token is expired after a certain period of time and has to be refreshed.
> The code to refresh the token is written in the secret provider but since the
> executor is [cached in the
> map|https://github.com/apache/sling-org-apache-sling-distribution-core/blob/master/src/main/java/org/apache/sling/distribution/transport/impl/SimpleHttpDistributionTransport.java#L208]
> the secrets are not refreshed. It works fine for credentials based secret
> provider but not for access token based.
> cc - [~marett]
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (SLING-8869) SimpleHttpDistributionTransport does not refresh the secret for token based implementations.
[
https://issues.apache.org/jira/browse/SLING-8869?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16988892#comment-16988892
]
Timothee Maret commented on SLING-8869:
---
Based on [~ashishc] PR
[#30|https://github.com/apache/sling-org-apache-sling-distribution-core/pull/30]
it seems we'd require a lot of changes (drop functional API) to be able to
refresh the headers only upon 401s and 403s.
Let's go with #28 once the NPE issue is addressed.
> SimpleHttpDistributionTransport does not refresh the secret for token based
> implementations.
>
>
> Key: SLING-8869
> URL: https://issues.apache.org/jira/browse/SLING-8869
> Project: Sling
> Issue Type: Bug
> Components: Content Distribution
>Reporter: Mohit Arora
>Assignee: Timothee Maret
>Priority: Critical
> Fix For: Content Distribution Core 0.4.2
>
> Attachments: SLING-8869-new.patch, SLING-8869.patch
>
> Time Spent: 1h 40m
> Remaining Estimate: 0h
>
> While saving the {{contextKeyExecutor}} in {{DistributionTransportContext}}
> map, it is not expected that the secret associated with the executor could be
> expired. This can happen in case of access token based implementations where
> the token is expired after a certain period of time and has to be refreshed.
> The code to refresh the token is written in the secret provider but since the
> executor is [cached in the
> map|https://github.com/apache/sling-org-apache-sling-distribution-core/blob/master/src/main/java/org/apache/sling/distribution/transport/impl/SimpleHttpDistributionTransport.java#L208]
> the secrets are not refreshed. It works fine for credentials based secret
> provider but not for access token based.
> cc - [~marett]
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (SLING-8869) SimpleHttpDistributionTransport does not refresh the secret for token based implementations.
[
https://issues.apache.org/jira/browse/SLING-8869?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16988792#comment-16988792
]
Timothee Maret commented on SLING-8869:
---
{quote}I'll try to update that with a commit applying the executor-eviction on
401 strategy.
{quote}
Thanks!
> SimpleHttpDistributionTransport does not refresh the secret for token based
> implementations.
>
>
> Key: SLING-8869
> URL: https://issues.apache.org/jira/browse/SLING-8869
> Project: Sling
> Issue Type: Bug
> Components: Content Distribution
>Reporter: Mohit Arora
>Assignee: Timothee Maret
>Priority: Critical
> Fix For: Content Distribution Core 0.4.2
>
> Attachments: SLING-8869-new.patch, SLING-8869.patch
>
> Time Spent: 20m
> Remaining Estimate: 0h
>
> While saving the {{contextKeyExecutor}} in {{DistributionTransportContext}}
> map, it is not expected that the secret associated with the executor could be
> expired. This can happen in case of access token based implementations where
> the token is expired after a certain period of time and has to be refreshed.
> The code to refresh the token is written in the secret provider but since the
> executor is [cached in the
> map|https://github.com/apache/sling-org-apache-sling-distribution-core/blob/master/src/main/java/org/apache/sling/distribution/transport/impl/SimpleHttpDistributionTransport.java#L208]
> the secrets are not refreshed. It works fine for credentials based secret
> provider but not for access token based.
> cc - [~marett]
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (SLING-8869) SimpleHttpDistributionTransport does not refresh the secret for token based implementations.
[
https://issues.apache.org/jira/browse/SLING-8869?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16988770#comment-16988770
]
Ashish Chopra commented on SLING-8869:
--
{quote}
bq. I still don't see in which case a {{DistributionTransportSecretProvider}}
can be created/updated without a corresponding creation of new instance of
{{RemoteDistributionPackageImporter}}
Any secret provider that changes secrets without deactivating/activating
itself. A concrete example, used in this ticket, is an access token provider.
{quote}
We are talking about different things - I meant changing of the
{{DistributionTransportSecretProvider}} Java implementation object itself,
whereas you meant _contents_ of the {{Map}} returned by
{{DistributionTransportSecretProvider.getSecret().asCredentialsMap()}} changing
(which is what I called "dynamic" secret provider earlier).
But I get your point - there can indeed be a
{{DistributionTransportSecretProvider}} impl which change the username/password
without changing the Java implementation object (reading them off a
secret-store via any API, perhaps).
{quote}
bq. None of the secret-providers can actually 'refresh' the token on a 401
because no such method exists in the API
The implementation parses the response status code and can evict the Executor
from the cache. There would be no need to extend the API.
{quote}
My intent was not to propose extending [the
API|https://github.com/apache/sling-org-apache-sling-distribution-api/blob/master/src/main/java/org/apache/sling/distribution/transport/DistributionTransportSecretProvider.java#L45]
- I wished to point out that _unless_ the
[API|https://github.com/apache/sling-org-apache-sling-distribution-api/blob/master/src/main/java/org/apache/sling/distribution/transport/DistributionTransportSecretProvider.java#L45]
has a 'refresh-secret' method, [evicting the executor upon a 401 v/s calling
{{DistributionTransportSecretProvider.getSecret().asCredentialsMap()}} before
issuing the transport every time will be
equivalent|https://issues.apache.org/jira/browse/SLING-8869?focusedCommentId=16988653=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16988653]
when it comes to freshness/staleness of the retrieved credentials-{{Map}}.
[^SLING-8869-new.patch] follows the latter and benefits from Executor/HTTP
client reuse (this is [in line with @bdelacretaz's suggestion
above|https://issues.apache.org/jira/browse/SLING-8869?focusedCommentId=16986813=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16986813]).
bq. With the approach taken by SLING-8869-new.patch we could reuse the client
and only fix the secret.
Thanks! the corresponding PR is
https://github.com/apache/sling-org-apache-sling-distribution-core/pull/28.
I'll try to update that with a commit applying the executor-eviction on 401
strategy.
\cc: [~mohiaror]
> SimpleHttpDistributionTransport does not refresh the secret for token based
> implementations.
>
>
> Key: SLING-8869
> URL: https://issues.apache.org/jira/browse/SLING-8869
> Project: Sling
> Issue Type: Bug
> Components: Content Distribution
>Reporter: Mohit Arora
>Assignee: Timothee Maret
>Priority: Critical
> Fix For: Content Distribution Core 0.4.2
>
> Attachments: SLING-8869-new.patch, SLING-8869.patch
>
> Time Spent: 20m
> Remaining Estimate: 0h
>
> While saving the {{contextKeyExecutor}} in {{DistributionTransportContext}}
> map, it is not expected that the secret associated with the executor could be
> expired. This can happen in case of access token based implementations where
> the token is expired after a certain period of time and has to be refreshed.
> The code to refresh the token is written in the secret provider but since the
> executor is [cached in the
> map|https://github.com/apache/sling-org-apache-sling-distribution-core/blob/master/src/main/java/org/apache/sling/distribution/transport/impl/SimpleHttpDistributionTransport.java#L208]
> the secrets are not refreshed. It works fine for credentials based secret
> provider but not for access token based.
> cc - [~marett]
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (SLING-8869) SimpleHttpDistributionTransport does not refresh the secret for token based implementations.
[
https://issues.apache.org/jira/browse/SLING-8869?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16988758#comment-16988758
]
Mohit Arora commented on SLING-8869:
[~marett] {quote}With the approach taken by SLING-8869-new.patch we could reuse
the client and only fix the secret.{quote}
Can you please review the PR [0] based on this approach and it seems Ok,
approve it?
[0] https://github.com/apache/sling-org-apache-sling-distribution-core/pull/28
> SimpleHttpDistributionTransport does not refresh the secret for token based
> implementations.
>
>
> Key: SLING-8869
> URL: https://issues.apache.org/jira/browse/SLING-8869
> Project: Sling
> Issue Type: Bug
> Components: Content Distribution
>Reporter: Mohit Arora
>Assignee: Timothee Maret
>Priority: Critical
> Fix For: Content Distribution Core 0.4.2
>
> Attachments: SLING-8869-new.patch, SLING-8869.patch
>
> Time Spent: 20m
> Remaining Estimate: 0h
>
> While saving the {{contextKeyExecutor}} in {{DistributionTransportContext}}
> map, it is not expected that the secret associated with the executor could be
> expired. This can happen in case of access token based implementations where
> the token is expired after a certain period of time and has to be refreshed.
> The code to refresh the token is written in the secret provider but since the
> executor is [cached in the
> map|https://github.com/apache/sling-org-apache-sling-distribution-core/blob/master/src/main/java/org/apache/sling/distribution/transport/impl/SimpleHttpDistributionTransport.java#L208]
> the secrets are not refreshed. It works fine for credentials based secret
> provider but not for access token based.
> cc - [~marett]
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (SLING-8869) SimpleHttpDistributionTransport does not refresh the secret for token based implementations.
[
https://issues.apache.org/jira/browse/SLING-8869?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16988664#comment-16988664
]
Timothee Maret commented on SLING-8869:
---
{quote}I still don't see in which case a
{{DistributionTransportSecretProvider}} can be created/updated _without_ a
corresponding creation of new instance of {{RemoteDistributionPackageImporter}}
{quote}
Any secret provider that changes secrets without deactivating/activating
itself. A concrete example, used in this ticket, is an access token provider.
{quote}None of the secret-providers can actually 'refresh' the token on a 401
because no such method exists in the API
{quote}
The implementation parses the response status code and can evict the Executor
from the cache. There would be no need to extend the API.
{quote}evicting a perfectly good HTTP Client
{quote}
It's questionable how the client is perfectly good when it is actually
configured to cache headers that will yields 401s. I understand your point
though. With the approach taken by [^SLING-8869-new.patch] we could reuse the
client and only fix the secret.
> SimpleHttpDistributionTransport does not refresh the secret for token based
> implementations.
>
>
> Key: SLING-8869
> URL: https://issues.apache.org/jira/browse/SLING-8869
> Project: Sling
> Issue Type: Bug
> Components: Content Distribution
>Reporter: Mohit Arora
>Assignee: Timothee Maret
>Priority: Critical
> Fix For: Content Distribution Core 0.4.2
>
> Attachments: SLING-8869-new.patch, SLING-8869.patch
>
> Time Spent: 20m
> Remaining Estimate: 0h
>
> While saving the {{contextKeyExecutor}} in {{DistributionTransportContext}}
> map, it is not expected that the secret associated with the executor could be
> expired. This can happen in case of access token based implementations where
> the token is expired after a certain period of time and has to be refreshed.
> The code to refresh the token is written in the secret provider but since the
> executor is [cached in the
> map|https://github.com/apache/sling-org-apache-sling-distribution-core/blob/master/src/main/java/org/apache/sling/distribution/transport/impl/SimpleHttpDistributionTransport.java#L208]
> the secrets are not refreshed. It works fine for credentials based secret
> provider but not for access token based.
> cc - [~marett]
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (SLING-8869) SimpleHttpDistributionTransport does not refresh the secret for token based implementations.
[
https://issues.apache.org/jira/browse/SLING-8869?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16988653#comment-16988653
]
Ashish Chopra commented on SLING-8869:
--
bq. DistributionTransportSecretProvider is an API and anyone can provide an
implementation for it.
Agreed. But as long as one is using {{RemoteDistributionPackageImporter}} (and
being an impl, the only way to _use_ it would be a via creation of configs for
{{DistributionAgentFactory}} impls and
{{RemoteDistributionPackageImporterFactory}}), I still don't see in which case
a {{DistributionTransportSecretProvider}} can be created/updated _without_ a
corresponding creation of new instance of {{RemoteDistributionPackageImporter}}.
{quote}
bq. Can you please help me understand the benefit of a failing HTTP call as you
propose?
It covers both dynamic and static secret providers without making any
assumption about the providers being dynamic or static.
{quote}
None of the secret-providers can actually 'refresh' the token on a 401 because
no such method exists in the API [0]. Depending on their impls they'd either
have to cache+refresh the credentials on every call, or just keep returning the
stale ones.
My concerns with 401/403 based executor eviction are:
* evicting a perfectly good HTTP Client (that backs the executor instance)
(afaik HTTP components advise HTTP Client reuse [1]) when all we need a header
value update
* retry semantics would need review (currently, a
{{RecoverableDistributionException}} is thrown for a 401/404 - its behaviour in
light of how the retry-attempt-counting and moving to error-queue works [2]
would likely need to be reviewed deeply)
[0]
https://github.com/apache/sling-org-apache-sling-distribution-api/blob/master/src/main/java/org/apache/sling/distribution/transport/DistributionTransportSecretProvider.java#L45
[1]
https://hc.apache.org/httpclient-3.x/performance.html#Reuse_of_HttpClient_instance
[2]
https://github.com/apache/sling-org-apache-sling-distribution-core/blob/af3140fc5641c6c394b1bbc4dbd4013965e6/src/main/java/org/apache/sling/distribution/agent/impl/SimpleDistributionAgentQueueProcessor.java#L147-L162
> SimpleHttpDistributionTransport does not refresh the secret for token based
> implementations.
>
>
> Key: SLING-8869
> URL: https://issues.apache.org/jira/browse/SLING-8869
> Project: Sling
> Issue Type: Bug
> Components: Content Distribution
>Reporter: Mohit Arora
>Assignee: Timothee Maret
>Priority: Critical
> Fix For: Content Distribution Core 0.4.2
>
> Attachments: SLING-8869-new.patch, SLING-8869.patch
>
> Time Spent: 20m
> Remaining Estimate: 0h
>
> While saving the {{contextKeyExecutor}} in {{DistributionTransportContext}}
> map, it is not expected that the secret associated with the executor could be
> expired. This can happen in case of access token based implementations where
> the token is expired after a certain period of time and has to be refreshed.
> The code to refresh the token is written in the secret provider but since the
> executor is [cached in the
> map|https://github.com/apache/sling-org-apache-sling-distribution-core/blob/master/src/main/java/org/apache/sling/distribution/transport/impl/SimpleHttpDistributionTransport.java#L208]
> the secrets are not refreshed. It works fine for credentials based secret
> provider but not for access token based.
> cc - [~marett]
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (SLING-8869) SimpleHttpDistributionTransport does not refresh the secret for token based implementations.
[
https://issues.apache.org/jira/browse/SLING-8869?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16988627#comment-16988627
]
Timothee Maret commented on SLING-8869:
---
{quote}If such a mechanism exists, can you please explain it here for my better
understanding
{quote}
DistributionTransportSecretProvider is an API and anyone can provide an
implementation for it.
{quote}Can you please help me understand the benefit of a failing HTTP call as
you propose?
{quote}
It covers both _dynamic_ and _static_ secret providers.
> SimpleHttpDistributionTransport does not refresh the secret for token based
> implementations.
>
>
> Key: SLING-8869
> URL: https://issues.apache.org/jira/browse/SLING-8869
> Project: Sling
> Issue Type: Bug
> Components: Content Distribution
>Reporter: Mohit Arora
>Assignee: Timothee Maret
>Priority: Critical
> Fix For: Content Distribution Core 0.4.2
>
> Attachments: SLING-8869-new.patch, SLING-8869.patch
>
> Time Spent: 20m
> Remaining Estimate: 0h
>
> While saving the {{contextKeyExecutor}} in {{DistributionTransportContext}}
> map, it is not expected that the secret associated with the executor could be
> expired. This can happen in case of access token based implementations where
> the token is expired after a certain period of time and has to be refreshed.
> The code to refresh the token is written in the secret provider but since the
> executor is [cached in the
> map|https://github.com/apache/sling-org-apache-sling-distribution-core/blob/master/src/main/java/org/apache/sling/distribution/transport/impl/SimpleHttpDistributionTransport.java#L208]
> the secrets are not refreshed. It works fine for credentials based secret
> provider but not for access token based.
> cc - [~marett]
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (SLING-8869) SimpleHttpDistributionTransport does not refresh the secret for token based implementations.
[
https://issues.apache.org/jira/browse/SLING-8869?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16988538#comment-16988538
]
Ashish Chopra commented on SLING-8869:
--
bq. If an Executor with a wrong pwd is cached then the only way to evict that
executor is to recreate the DistributionTransportContext cache entirely. This
happens currently because re-configuring a credential based secret provider
will force the components referencing it to restart.
I share your understanding on this. Currently, new instances of
{{RemoteDistributionPackageImporter}} will be created each time
credential-providers is reconfigured (e.g., due to credential-update), ensuring
that HTTP transport always works with most up-to-date credentials and executors
with stale credentials aren't accessible (and are candidates for GC). This is
true for both {{DistributionAgentFactory}} impls and
{{RemoteDistributionPackageImporterFactory}}.
bq. [it works] only by luck.
Erm, to me it seems to be as-designed - unless there's another supported
mechanism that can create/update credential-providers _without_ a corresponding
creation of {{RemoteDistributionPackageImporter}} with updated credentials.
If such a mechanism exists, can you please explain it here for my better
understanding?
bq. One way to handle this would be to evict the Executor based on the returned
status code, 401 and 403.
Assuming my understanding is correct, this seems like overkill for a credential
provider that contains static-secrets.
For dynamic-secrets, (even if there is 401/403 based executor-eviction,
followed by token-reacquisition) since the secret-provider can't be explicitly
asked for a token-update (which isn't possible because there's no such
capability in the API [0]), we anyway need to rely on the secret-provider
returning the most updated token when asked for.
In current proposal HTTP Transport impl doesn't concern itself with token
expiry/regeneration/caching and dumbly asks for the credentials from
secret-provider always, letting the secret-provider deal with all the
complexities of returning most up-to-date token always, which it leverages for
transport.
Can you please help me understand the benefit of a failing HTTP call as you
propose?
[0]
https://github.com/apache/sling-org-apache-sling-distribution-api/blob/master/src/main/java/org/apache/sling/distribution/transport/DistributionTransportSecretProvider.java#L45
> SimpleHttpDistributionTransport does not refresh the secret for token based
> implementations.
>
>
> Key: SLING-8869
> URL: https://issues.apache.org/jira/browse/SLING-8869
> Project: Sling
> Issue Type: Bug
> Components: Content Distribution
>Reporter: Mohit Arora
>Assignee: Timothee Maret
>Priority: Critical
> Fix For: Content Distribution Core 0.4.2
>
> Attachments: SLING-8869-new.patch, SLING-8869.patch
>
> Time Spent: 20m
> Remaining Estimate: 0h
>
> While saving the {{contextKeyExecutor}} in {{DistributionTransportContext}}
> map, it is not expected that the secret associated with the executor could be
> expired. This can happen in case of access token based implementations where
> the token is expired after a certain period of time and has to be refreshed.
> The code to refresh the token is written in the secret provider but since the
> executor is [cached in the
> map|https://github.com/apache/sling-org-apache-sling-distribution-core/blob/master/src/main/java/org/apache/sling/distribution/transport/impl/SimpleHttpDistributionTransport.java#L208]
> the secrets are not refreshed. It works fine for credentials based secret
> provider but not for access token based.
> cc - [~marett]
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (SLING-8869) SimpleHttpDistributionTransport does not refresh the secret for token based implementations.
[
https://issues.apache.org/jira/browse/SLING-8869?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16988514#comment-16988514
]
Mohit Arora commented on SLING-8869:
[~marett] I have raised PRs for both the approaches -
https://github.com/apache/sling-org-apache-sling-distribution-core/pull/28
https://github.com/apache/sling-org-apache-sling-distribution-core/pull/29
cc - [~ashishc]
> SimpleHttpDistributionTransport does not refresh the secret for token based
> implementations.
>
>
> Key: SLING-8869
> URL: https://issues.apache.org/jira/browse/SLING-8869
> Project: Sling
> Issue Type: Bug
> Components: Content Distribution
>Reporter: Mohit Arora
>Assignee: Timothee Maret
>Priority: Critical
> Fix For: Content Distribution Core 0.4.2
>
> Attachments: SLING-8869-new.patch, SLING-8869.patch
>
> Time Spent: 20m
> Remaining Estimate: 0h
>
> While saving the {{contextKeyExecutor}} in {{DistributionTransportContext}}
> map, it is not expected that the secret associated with the executor could be
> expired. This can happen in case of access token based implementations where
> the token is expired after a certain period of time and has to be refreshed.
> The code to refresh the token is written in the secret provider but since the
> executor is [cached in the
> map|https://github.com/apache/sling-org-apache-sling-distribution-core/blob/master/src/main/java/org/apache/sling/distribution/transport/impl/SimpleHttpDistributionTransport.java#L208]
> the secrets are not refreshed. It works fine for credentials based secret
> provider but not for access token based.
> cc - [~marett]
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (SLING-8869) SimpleHttpDistributionTransport does not refresh the secret for token based implementations.
[
https://issues.apache.org/jira/browse/SLING-8869?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16987967#comment-16987967
]
Timothee Maret commented on SLING-8869:
---
{quote}It works fine for credentials based secret provider but not for access
token based.
{quote}
True, but only by luck.
If an Executor with a wrong pwd is cached then the only way to evict that
executor is to recreate the DistributionTransportContext cache entirely. This
happens currently because re-configuring a credential based secret provider
will force the components referencing it to restart.
One way to handle this would be to evict the Executor based on the returned
status code, 401 and 403.
Please open PRs instead of attaching patches, it's much easier to
review/comment :)
> SimpleHttpDistributionTransport does not refresh the secret for token based
> implementations.
>
>
> Key: SLING-8869
> URL: https://issues.apache.org/jira/browse/SLING-8869
> Project: Sling
> Issue Type: Bug
> Components: Content Distribution
>Reporter: Mohit Arora
>Assignee: Timothee Maret
>Priority: Critical
> Fix For: Content Distribution Core 0.4.2
>
> Attachments: SLING-8869-new.patch, SLING-8869.patch
>
>
> While saving the {{contextKeyExecutor}} in {{DistributionTransportContext}}
> map, it is not expected that the secret associated with the executor could be
> expired. This can happen in case of access token based implementations where
> the token is expired after a certain period of time and has to be refreshed.
> The code to refresh the token is written in the secret provider but since the
> executor is [cached in the
> map|https://github.com/apache/sling-org-apache-sling-distribution-core/blob/master/src/main/java/org/apache/sling/distribution/transport/impl/SimpleHttpDistributionTransport.java#L208]
> the secrets are not refreshed. It works fine for credentials based secret
> provider but not for access token based.
> cc - [~marett]
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (SLING-8869) SimpleHttpDistributionTransport does not refresh the secret for token based implementations.
[
https://issues.apache.org/jira/browse/SLING-8869?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16987757#comment-16987757
]
Ashish Chopra commented on SLING-8869:
--
[~mohiaror], may I suggest raising a PR for SCD maintainers (and broader Sling
community) to review and merge? Thanks!
\cc: [~marett]
> SimpleHttpDistributionTransport does not refresh the secret for token based
> implementations.
>
>
> Key: SLING-8869
> URL: https://issues.apache.org/jira/browse/SLING-8869
> Project: Sling
> Issue Type: Bug
> Components: Content Distribution
>Reporter: Mohit Arora
>Assignee: Timothee Maret
>Priority: Critical
> Fix For: Content Distribution Core 0.4.2
>
> Attachments: SLING-8869-new.patch, SLING-8869.patch
>
>
> While saving the {{contextKeyExecutor}} in {{DistributionTransportContext}}
> map, it is not expected that the secret associated with the executor could be
> expired. This can happen in case of access token based implementations where
> the token is expired after a certain period of time and has to be refreshed.
> The code to refresh the token is written in the secret provider but since the
> executor is [cached in the
> map|https://github.com/apache/sling-org-apache-sling-distribution-core/blob/master/src/main/java/org/apache/sling/distribution/transport/impl/SimpleHttpDistributionTransport.java#L208]
> the secrets are not refreshed. It works fine for credentials based secret
> provider but not for access token based.
> cc - [~marett]
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (SLING-8869) SimpleHttpDistributionTransport does not refresh the secret for token based implementations.
[
https://issues.apache.org/jira/browse/SLING-8869?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16987641#comment-16987641
]
Mohit Arora commented on SLING-8869:
[~bdelacretaz], [~ashishc] I have attached another patch,
[^SLING-8869-new.patch] which keeps the caching as it is and always inject a
header in request if the credentials provider is authorization based
credentials provider. As Ashish mentioned, I do not see a clear advantage of
one approach on the other so we can choose either one.
> SimpleHttpDistributionTransport does not refresh the secret for token based
> implementations.
>
>
> Key: SLING-8869
> URL: https://issues.apache.org/jira/browse/SLING-8869
> Project: Sling
> Issue Type: Bug
> Components: Content Distribution
>Reporter: Mohit Arora
>Priority: Critical
> Fix For: Content Distribution Core 0.4.2
>
> Attachments: SLING-8869-new.patch, SLING-8869.patch
>
>
> While saving the {{contextKeyExecutor}} in {{DistributionTransportContext}}
> map, it is not expected that the secret associated with the executor could be
> expired. This can happen in case of access token based implementations where
> the token is expired after a certain period of time and has to be refreshed.
> The code to refresh the token is written in the secret provider but since the
> executor is [cached in the
> map|https://github.com/apache/sling-org-apache-sling-distribution-core/blob/master/src/main/java/org/apache/sling/distribution/transport/impl/SimpleHttpDistributionTransport.java#L208]
> the secrets are not refreshed. It works fine for credentials based secret
> provider but not for access token based.
> cc - [~marett]
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (SLING-8869) SimpleHttpDistributionTransport does not refresh the secret for token based implementations.
[
https://issues.apache.org/jira/browse/SLING-8869?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16987619#comment-16987619
]
Ashish Chopra commented on SLING-8869:
--
hi [~bdelacretaz], thanks for your comments!
bq. If it's a standard Java Executor that you're using you'd need an instanceof
+ cast to your expanded Executor to run the check, but that's a small price to
pay vs not caching at all.
This is a HTTP Components' Fluent API executor [0].
Per the documentation, the benefit for {{Executor}} is to retain authorization
information associated with it via the {{.auth}} invocations.
I see no other benefit of {{Executor}} from [0] - notably [1] doesn't invoke
{{.auth}}, but sets an HTTP header directly.
Perhaps we should be OK invoking the {{Request.}} directly (after all, that
_is_ the purported benefit of the Fluent API [0]) - but if you see merit in
reusing the {{Executor}} (and the {{HttpClient}} it backs) still, maybe we can
consider adding the {{Authorization}} header in the POST request itself [2]
instead of adding it as a default-header in the HttpClient that backs the
{{Executor}} in question.
Does above seem like a reasonable approach to you?
[0] https://hc.apache.org/httpcomponents-client-ga/tutorial/html/fluent.html
[1]
https://github.com/apache/sling-org-apache-sling-distribution-core/blob/deb3d2ae33c4f4678c8503091a9fffdbb141e569/src/main/java/org/apache/sling/distribution/transport/impl/SimpleHttpDistributionTransport.java#L228
[2]
https://hc.apache.org/httpcomponents-client-ga/fluent-hc/apidocs/org/apache/http/client/fluent/Request.html#addHeader(org.apache.http.Header)
> SimpleHttpDistributionTransport does not refresh the secret for token based
> implementations.
>
>
> Key: SLING-8869
> URL: https://issues.apache.org/jira/browse/SLING-8869
> Project: Sling
> Issue Type: Bug
> Components: Content Distribution
>Reporter: Mohit Arora
>Priority: Critical
> Fix For: Content Distribution Core 0.4.2
>
> Attachments: SLING-8869.patch
>
>
> While saving the {{contextKeyExecutor}} in {{DistributionTransportContext}}
> map, it is not expected that the secret associated with the executor could be
> expired. This can happen in case of access token based implementations where
> the token is expired after a certain period of time and has to be refreshed.
> The code to refresh the token is written in the secret provider but since the
> executor is [cached in the
> map|https://github.com/apache/sling-org-apache-sling-distribution-core/blob/master/src/main/java/org/apache/sling/distribution/transport/impl/SimpleHttpDistributionTransport.java#L208]
> the secrets are not refreshed. It works fine for credentials based secret
> provider but not for access token based.
> cc - [~marett]
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (SLING-8869) SimpleHttpDistributionTransport does not refresh the secret for token based implementations.
[
https://issues.apache.org/jira/browse/SLING-8869?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16986813#comment-16986813
]
Bertrand Delacretaz commented on SLING-8869:
I don't have much context about this so feel free to ignore this comment if
it's not relevant...AFAIU the current patch disables caching of any tokens that
might expire.
Isn't that a problem in terms of performance? You could maybe enhance the
Executor with a checkExpired() method that getExecutor() can call to refresh
the token only when needed. If it's a standard Java Executor that you're using
you'd need an instanceof + cast to your expanded Executor to run the check, but
that's a small price to pay vs not caching at all.
> SimpleHttpDistributionTransport does not refresh the secret for token based
> implementations.
>
>
> Key: SLING-8869
> URL: https://issues.apache.org/jira/browse/SLING-8869
> Project: Sling
> Issue Type: Bug
> Components: Content Distribution
>Reporter: Mohit Arora
>Priority: Critical
> Fix For: Content Distribution Core 0.4.2
>
> Attachments: SLING-8869.patch
>
>
> While saving the {{contextKeyExecutor}} in {{DistributionTransportContext}}
> map, it is not expected that the secret associated with the executor could be
> expired. This can happen in case of access token based implementations where
> the token is expired after a certain period of time and has to be refreshed.
> The code to refresh the token is written in the secret provider but since the
> executor is [cached in the map|#L208]] the secrets are not refreshed. It
> works fine for credentials based secret provider but not for access token
> based.
> cc - [~marett]
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (SLING-8869) SimpleHttpDistributionTransport does not refresh the secret for token based implementations.
[
https://issues.apache.org/jira/browse/SLING-8869?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16986688#comment-16986688
]
Mohit Arora commented on SLING-8869:
[~ashishc] Thanks for pointing it out. I have updated the patch.
> SimpleHttpDistributionTransport does not refresh the secret for token based
> implementations.
>
>
> Key: SLING-8869
> URL: https://issues.apache.org/jira/browse/SLING-8869
> Project: Sling
> Issue Type: Bug
> Components: Content Distribution
>Reporter: Mohit Arora
>Priority: Critical
> Fix For: Content Distribution Core 0.4.2
>
> Attachments: SLING-8869.patch
>
>
> While saving the \{{contextKeyExecutor}} in \{{DistributionTransportContext}}
> map, it is not expected that the secret associated with the executor could be
> expired. This can happen in case of access token based implementations where
> the token is expired after a certain period of time and has to be refreshed.
> The code to refresh the token is written in the secret provider but since the
> executor is [cached in the
> map|[https://github.com/apache/sling-org-apache-sling-distribution-core/blob/master/src/main/java/org/apache/sling/distribution/transport/impl/SimpleHttpDistributionTransport.java#L208]]
> the secrets are not refreshed. It works fine for credentials based secret
> provider but not for access token based.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (SLING-8869) SimpleHttpDistributionTransport does not refresh the secret for token based implementations.
[
https://issues.apache.org/jira/browse/SLING-8869?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16986649#comment-16986649
]
Ashish Chopra commented on SLING-8869:
--
[~mohiaror], IIUC the patch, once the secret is associated with an {{Executor}}
created via a call to {{buildAuthExecutor}}, it is being inserted in a map
named {{contextKeyExecutor}}. Upon the next invocation of {{getExecutor}}, that
map is queried first _irrespective_ of whether {{DistributionTransportSecret}}
was refreshed or not; if found in the Map, the executor is being reused with
(possibly stale) secret.
It is not immediately obvious to me how the executor with stale secret is being
evicted in [^SLING-8869.patch]. I'm likely missing something obvious here - can
you please explain?
> SimpleHttpDistributionTransport does not refresh the secret for token based
> implementations.
>
>
> Key: SLING-8869
> URL: https://issues.apache.org/jira/browse/SLING-8869
> Project: Sling
> Issue Type: Bug
> Components: Content Distribution
>Reporter: Mohit Arora
>Priority: Major
> Fix For: Content Distribution Core 0.4.2
>
> Attachments: SLING-8869.patch
>
>
> While saving the \{{contextKeyExecutor}} in \{{DistributionTransportContext}}
> map, it is not expected that the secret associated with the executor could be
> expired. This can happen in case of access token based implementations where
> the token is expired after a certain period of time and has to be refreshed.
> The code to refresh the token is written in the secret provider but since the
> executor is [cached in the
> map|[https://github.com/apache/sling-org-apache-sling-distribution-core/blob/master/src/main/java/org/apache/sling/distribution/transport/impl/SimpleHttpDistributionTransport.java#L208]]
> the secrets are not refreshed. It works fine for credentials based secret
> provider but not for access token based.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (SLING-8869) SimpleHttpDistributionTransport does not refresh the secret for token based implementations.
[
https://issues.apache.org/jira/browse/SLING-8869?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16985903#comment-16985903
]
Mohit Arora commented on SLING-8869:
Attached a sample patch to resolve the issue.
cc - [~ashishc]
> SimpleHttpDistributionTransport does not refresh the secret for token based
> implementations.
>
>
> Key: SLING-8869
> URL: https://issues.apache.org/jira/browse/SLING-8869
> Project: Sling
> Issue Type: Bug
> Components: Content Distribution
>Reporter: Mohit Arora
>Priority: Major
> Fix For: Content Distribution Core 0.4.2
>
> Attachments: SLING-8869.patch
>
>
> While saving the \{{contextKeyExecutor}} in \{{DistributionTransportContext}}
> map, it is not expected that the secret associated with the executor could be
> expired. This can happen in case of access token based implementations where
> the token is expired after a certain period of time and has to be refreshed.
> The code to refresh the token is written in the secret provider but since the
> executor is [cached in the
> map|[https://github.com/apache/sling-org-apache-sling-distribution-core/blob/master/src/main/java/org/apache/sling/distribution/transport/impl/SimpleHttpDistributionTransport.java#L208]]
> the secrets are not refreshed. It works fine for credentials based secret
> provider but not for access token based.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
