Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SLING-8869 SimpleHttpDistributionTransport does not refresh the secret for token based implementations. #28

Merged
merged 2 commits into from
Dec 5, 2019

Conversation

mohiaror
Copy link
Contributor

@mohiaror mohiaror commented Dec 5, 2019

Do not set the default authorization header at the time of creating the executor but delegate this responsibility to the issuer of the request such that everytime POST request is made, if the executor is token based, check for the expiry of the token and add the relevant header to the request.

…t for token based implementations.

Do not set the default authorization header at the time of creating the executor but delegate this responsibility to the issuer of the request such that everytime POST request is made, if the executor is token based, check for the expiry of the token and add the relevant header to the request.
@tmaret
Copy link
Member

tmaret commented Dec 5, 2019

@mohiaror it seems that Sonar is correct here, DistributionTransportSecret#getAuthSecret may return null as documented in the API. Could you please add a null check ?

Copy link
Member

@tmaret tmaret left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR should handle null returned by DistributionTransportSecret#asCredentialsMap

@mohiaror
Copy link
Contributor Author

mohiaror commented Dec 5, 2019

@tmaret

it seems that Sonar is correct here, DistributionTransportSecret#getAuthSecret may return null as documented in the API.

The PR should handle null returned by DistributionTransportSecret#asCredentialsMap

We are already checking the null occurrence of DistributionTransportSecret#asCredentialsMap in the code. The sonar report is actually showing the NPE for secret returned by DistributionTransportSecretProvider#getSecret Line#235 which is old code and I assume this error must have been coming in previous reports as well. Nevertheless, I will add a patch for that in the PR.

…t for token based implementations.

Adding NullPointer check for DistributionTransportSecretProvider#getSecret.
@mohiaror
Copy link
Contributor Author

mohiaror commented Dec 5, 2019

@tmaret

I assume this error must have been coming in previous reports as well.

Looking at the sonar report [0], it indeed is a 2 years old bug. Nevertheless, I have added the patch with commit 49e8f9c. Kindly review.

[0] https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-core&pullRequest=28&resolved=false&types=BUG

@tmaret tmaret merged commit 28e9cf3 into apache:master Dec 5, 2019
@mohiaror mohiaror deleted the issues/SLING-8869 branch December 6, 2019 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants