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.

Sign up for GitHub

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

Jump to bottom

Sanitize host name for AWS requests before signing in AWSAuthV4 trans… #2090

Merged
merged 2 commits into from
Aug 30, 2022
Merged

Sanitize host name for AWS requests before signing in AWSAuthV4 trans… #2090

merged 2 commits into from
Aug 30, 2022

Conversation

jhuebner79
Copy link
Contributor

@jhuebner79 jhuebner79 commented Jun 24, 2022 •
edited
Loading

...port

@jhuebner79 jhuebner79 force-pushed the fix-issue-1777-trailing-dot-during-request-signing branch 2 times, most recently from 0616fed to ea5d6fb Compare June 26, 2022 21:26
jhuebner79 pushed a commit to jhuebner79/Elastica that referenced this pull request Jun 27, 2022
Sanitize host name for AWS requests before signing in AWSAuthV4 trans…
b376ef5 
…port (ruflin#2090)
@jhuebner79 jhuebner79 force-pushed the fix-issue-1777-trailing-dot-during-request-signing branch from ac171b2 to b376ef5 Compare June 27, 2022 09:37
jhuebner79 pushed a commit to jhuebner79/Elastica that referenced this pull request Jun 27, 2022
Sanitize host name for AWS requests before signing in AWSAuthV4 trans…
54b5234 
…port (ruflin#2090)
@jhuebner79 jhuebner79 force-pushed the fix-issue-1777-trailing-dot-during-request-signing branch from b376ef5 to 54b5234 Compare June 27, 2022 09:39
@jhuebner79 jhuebner79 marked this pull request as ready for review June 27, 2022 09:42
@jhuebner79 jhuebner79 mentioned this pull request Jun 27, 2022
Open
@ruflin
Copy link
Owner

ruflin commented Jun 28, 2022

The change LGTM. Thanks @jhuebner79 . As I don't have any AWS setup and it is not part of the test suite, I don't really have a way to test this. Did you by chance run this code with one of your setups?

@jhuebner79
Copy link
Contributor Author

The change LGTM. Thanks @jhuebner79 . As I don't have any AWS setup and it is not part of the test suite, I don't really have a way to test this. Did you by chance run this code with one of your setups?

Yes, we were able to reference my branch in our composer.json - it resolves our issue 👍

jhuebner79 pushed a commit to jhuebner79/Elastica that referenced this pull request Jun 29, 2022
Sanitize host name for AWS requests before signing in AWSAuthV4 trans…
630b3f2 
…port (ruflin#2090)
@jhuebner79 jhuebner79 force-pushed the fix-issue-1777-trailing-dot-during-request-signing branch from 54b5234 to 630b3f2 Compare June 29, 2022 12:04
@jhuebner79
Copy link
Contributor Author

@franmomu I force pushed the code I suggested earlier, and your suggestion as well

@jhuebner79
Copy link
Contributor Author

oh no, the local tests are not running successfully anymore.. hang on please

jhuebner79 pushed a commit to jhuebner79/Elastica that referenced this pull request Jun 29, 2022
Sanitize host name for AWS requests before signing in AWSAuthV4 trans…
d0905bf 
…port (ruflin#2090)
@jhuebner79 jhuebner79 force-pushed the fix-issue-1777-trailing-dot-during-request-signing branch from 630b3f2 to d0905bf Compare June 29, 2022 12:57
@jhuebner79
Copy link
Contributor Author

Found it - looks good locally now (did another force push)

@franmomu franmomu mentioned this pull request Jun 29, 2022
Merged
@jhuebner79
Copy link
Contributor Author

@ruflin @franmomu do you need to give approval for another workflow run? Is there anything else I can do?

@franmomu
Copy link
Contributor

franmomu commented Jul 1, 2022

@ruflin @franmomu do you need to give approval for another workflow run? Is there anything else I can do?

The workflow must be approved, I'm not a maintainer, so you'll need @thePanz or @deguif approval

jhuebner79 pushed a commit to jhuebner79/Elastica that referenced this pull request Jul 11, 2022
Sanitize host name for AWS requests before signing in AWSAuthV4 trans…
bcb873c 
…port (ruflin#2090)
@jhuebner79 jhuebner79 force-pushed the fix-issue-1777-trailing-dot-during-request-signing branch from d0905bf to bcb873c Compare July 11, 2022 14:35
jhuebner79 pushed a commit to jhuebner79/Elastica that referenced this pull request Jul 11, 2022
Sanitize host name for AWS requests before signing in AWSAuthV4 trans…
857717a 
…port (ruflin#2090)
@jhuebner79 jhuebner79 force-pushed the fix-issue-1777-trailing-dot-during-request-signing branch from bcb873c to 857717a Compare July 11, 2022 14:44
@jhuebner79
Copy link
Contributor Author

We added another exclusion to phpstan-baseline.neon - would you please be so kind and allow for another workflow run please @thePanz @deguif ?

thePanz
thePanz reviewed Jul 11, 2022
View reviewed changes
phpstan-baseline.neon
@@ -135,3 +135,7 @@ parameters:
count: 1
path: tests/SnapshotTest.php

-
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if this is the right indentation level

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'll check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, basically I copied lines 28-31 and did not change indentation at all 🤔

It's the same like in the whole file, four spaces everywhere..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thePanz anything I can do here?

@thePanz
Copy link
Collaborator

thePanz commented Aug 2, 2022 •
edited
Loading

@jhuebner79 I will check the indentation later, would you be able to provide some tests covering the added lines?
Looks like the code-coverage is not picking them up 🤔

@jhuebner79
Copy link
Contributor Author

@jhuebner79 I will check the indentation later, would you be able to provide some tests covering the added lines? Looks like the code-coverage is not picking them up 🤔

We honestly have no idea why the added lines are not picked up by codecov, I'd be very happy if someone who's more familiar with the project could help us a bit here.

@franmomu
Copy link
Contributor

franmomu commented Aug 9, 2022

I've created #2113 that should help with this (it adds a job to test with lowest dependencies)

@ruflin
Copy link
Owner

ruflin commented Aug 15, 2022

Lets get this moving again. I resolved the Changelog conflict which triggered another CI run and got the PR from @franmomu merged.

@jhuebner79 @thePanz What are the other things we still need?

@jhuebner79
Copy link
Contributor Author

@ruflin thanks for that!

I still don't know why codecov is not happy or what I can do about it, but I think I would need to rebase to current master?

ruflin
ruflin reviewed Aug 17, 2022
View reviewed changes
Copy link
Owner

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

Could you try to rebase on master? But not sure if this is the issue.

The part I was not sure about is if the test you added executes all these lines of code but based on your exception check around host, it should.

tests/Transport/AwsAuthV4Test.php
$client = $this->_getClient($config);

try {
$client->request('_stats');
Copy link
Owner

Choose a reason for hiding this comment

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

Is this executing all the new code? Maybe that is the issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're able to have a look later on today or tomorrow.

Copy link
Contributor Author

@jhuebner79 jhuebner79 Aug 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

So, we hacked the make targets and the Dockerfile for elastica_php to be able to generate phpunit code coverage reports, and it doesn't look too bad there.. see attached screenshot. 🤔

Screenshot 2022-08-25 at 12-23-31 Code Coverage for var www html src Transport AwsAuthV4 php

krasilnikovs
krasilnikovs reviewed Aug 22, 2022
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@jhuebner79 jhuebner79 force-pushed the fix-issue-1777-trailing-dot-during-request-signing branch from 6e93bae to ea3f4b6 Compare August 25, 2022 10:31
@ruflin
Copy link
Owner

ruflin commented Aug 26, 2022

I suggest we just leave the bots to themself and agree that codecov is fine even thought it shows it isn't? Any objections?

@jhuebner79
Copy link
Contributor Author

By reacting with the heart smiley I meant "not from my side", of course :)

@ruflin
Copy link
Owner

ruflin commented Aug 29, 2022

I resolved a changelog conflict to get this mergeable. @thePanz As you were also involved in this PR, ok for you if we get this in?

@ruflin ruflin merged commit 878acd2 into ruflin:master Aug 30, 2022
@ruflin
Copy link
Owner

ruflin commented Aug 30, 2022

I merged this PR now. If we see issues, we can always revert. Anyone using / testing this, please report back.

@jhuebner79 jhuebner79 deleted the fix-issue-1777-trailing-dot-during-request-signing branch August 30, 2022 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krasilnikovs krasilnikovs krasilnikovs left review comments

@franmomu franmomu franmomu left review comments

@ruflin ruflin ruflin left review comments

@thePanz thePanz thePanz left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jhuebner79 @ruflin @franmomu @thePanz @krasilnikovs