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

Add digest authentication #583

Merged
merged 3 commits into from Mar 12, 2023
Merged

Add digest authentication #583

merged 3 commits into from Mar 12, 2023

Conversation

segevda
Copy link
Contributor

@segevda segevda commented Sep 28, 2022 •
edited by jeevatkm

  • Adhere to RFC 7616 ("HTTP Digest Access Authentication")
  • Added SetDigestAuth methods for Client and Request
  • Currently not supporting auth-int Quality of Protection

closes #467
closes #572

@segevda segevda mentioned this pull request Sep 29, 2022
Closed
jeevatkm
jeevatkm requested changes Mar 6, 2023
View reviewed changes
Copy link
Member

@jeevatkm jeevatkm left a comment

Choose a reason for hiding this comment

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

@segevda Thank you of your PR and contribution. I'm sorry for the delayed attention on the PR.
Can you please check the review comments?

digest.go Show resolved Hide resolved
digest.go Show resolved Hide resolved
jeevatkm
jeevatkm previously approved these changes Mar 6, 2023
View reviewed changes
Copy link
Member

@jeevatkm jeevatkm left a comment

Choose a reason for hiding this comment

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

@segevda Thanks for the PR.

@codecov
Copy link

codecov bot commented Mar 6, 2023 •
edited

Codecov Report

Merging #583 (59aaee7) into master (d54c956) will increase coverage by 0.05%.
The diff coverage is 96.29%.

@@            Coverage Diff             @@
##           master     #583      +/-   ##
==========================================
+ Coverage   95.77%   95.82%   +0.05%     
==========================================
  Files          10       11       +1     
  Lines        1395     1557     +162     
==========================================
+ Hits         1336     1492     +156     
- Misses         37       40       +3     
- Partials       22       25       +3
Impacted Files Coverage Δ
digest.go 95.58% <95.58%> (ø)
client.go 97.96% <100.00%> (+0.06%) ⬆️
request.go 95.66% <100.00%> (+0.19%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@jeevatkm
Copy link
Member

jeevatkm commented Mar 6, 2023

@segevda It seems test cases coverage check is failing, can you please check it?

@segevda
Copy link
Contributor Author

segevda commented Mar 6, 2023

@segevda It seems test cases coverage check is failing, can you please check it?

@jeevatkm just to make sure I understand, the failure here is because the total codebase coverage dropped due to this addition?

@jeevatkm
Copy link
Member

jeevatkm commented Mar 7, 2023

@segevda It seems test cases coverage check is failing, can you please check it?

@jeevatkm just to make sure I understand, the failure here is because the total codebase coverage dropped due to this addition?

Yes, @segevda.

@segevda
Copy link
Contributor Author

segevda commented Mar 7, 2023

@jeevatkm I don't see how I can improve the coverage on this file any more. All missing lines are error checks for std lib functions. Please assist.

@jeevatkm
Copy link
Member

jeevatkm commented Mar 8, 2023 •
edited

@segevda I see, I believe you have looked at this page. Which lists those return error lines, correct?
https://github.com/go-resty/resty/pull/583/checks?check_run_id=11833912592

Possibly, we could supply incorrect input values to produce the error in the test! what are your thoughts?

@segevda
Copy link
Contributor Author

segevda commented Mar 9, 2023

@jeevatkm I found a way to reduce error checking and thus to improve coverage:
Hash.Write calls are documented to never return an error (see here).
Also, changed the way we assign the digest Transport to avoid data races.

Please review 🙏

jeevatkm
jeevatkm requested changes Mar 11, 2023
View reviewed changes
Copy link
Member

@jeevatkm jeevatkm left a comment

Choose a reason for hiding this comment

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

@segevda Thanks for improving the PR in the last iteration. I have one last comment, can you have a look?

digest.go Show resolved Hide resolved
@segevda
Add digest authentication
c52c284 
- Adhere to RFC 7616 ("HTTP Digest Access Authentication")
- Added SetDigestAuth methods for Client and Request
- Currently not supporting auth-int Quality of Protection

fixes go-resty#467
@segevda
Use Hooks to avoid data races
3b6111c
@segevda
Rename error vars
59aaee7
jeevatkm
jeevatkm approved these changes Mar 12, 2023
View reviewed changes
Copy link
Member

@jeevatkm jeevatkm left a comment

Choose a reason for hiding this comment

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

Thanks @segevda for resolving the comments

@jeevatkm jeevatkm added this to the v2.8.0 Milestone milestone Mar 12, 2023
@jeevatkm jeevatkm merged commit a34adf1 into go-resty:master Mar 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeevatkm jeevatkm jeevatkm approved these changes

Assignees

@segevda segevda

Labels
feature
Milestone
v2.8.0 Milestone
Development

Successfully merging this pull request may close these issues.

Does resty support digest authentication
2 participants
@segevda @jeevatkm