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

Added HTTP authentication to the API. #115

Closed
wants to merge 5 commits into from

Conversation

ot2789
Copy link
Contributor

@ot2789 ot2789 commented May 25, 2021

  • Now the requests to HTTP can also contain Basic, Digest and Proxy
    authentication.

@asimell
Copy link
Contributor

asimell commented May 26, 2021

Instead of having four basically identical keywords, would it be more reasonable to have a single keyword Set Client Authentication that takes the type of authentication as an argument and handle passing it on in the keyword itself?
E.g

Set Client Authentication | basic | username | password
Set Client Authentication | digest | username | password
Set Client Authentication | | | 

@ot2789
Copy link
Contributor Author

ot2789 commented May 26, 2021

Instead of having four basically identical keywords, would it be more reasonable to have a single keyword Set Client Authentication that takes the type of authentication as an argument and handle passing it on in the keyword itself?
E.g

Set Client Authentication | basic | username | password
Set Client Authentication | digest | username | password
Set Client Authentication | | | 

Yes. I can do that it is not a problem. Two questions however.

  1. Is it fine if the first parameter is a string and the check is done through string comparison (case insensitive).
  2. In the situation in which a user wants to clear the authentication. Would the value ${NONE} be used or can the user call this keyword without any arguments?

In the code I also replaced deepcopy with copy because if the user adds the Digest authentication the deepcopy function cannot get the treading local structurest that are used in HTTPDigestAuth. I don't know if this is the best solution to this problem.

@asimell
Copy link
Contributor

asimell commented May 26, 2021

  1. I think case insensitive string comparison should work just fine as long as the supported values are documented.
  2. Using ${NONE} would probably make more sense than no arguments at all. This would mean that the first argument is mandatory (type of authentication) and the username and password are optional.

* Now the requests to HTTP can also contain Basic, Digest and Proxy
authentication.
@ot2789
Copy link
Contributor Author

ot2789 commented May 26, 2021

I updated the keywords according to your comment. Let me know if you have other ideas.

@asimell
Copy link
Contributor

asimell commented May 26, 2021

Could you also create tests to test the new functionality? Place the tests in the atest directory. Also, by looking at headers.robot it looks like the behaviour of this keyword is already somewhat supported just by using headers.

Could you also open this PR to eficode/restinstance? While we're waiting for repository transfer (ping @asyrjasalo) we're handling PRs by first merging them to Eficode fork and then creating a combined PR here with the next release.

@ot2789
Copy link
Contributor Author

ot2789 commented May 26, 2021

It is true that headers.robot can fix the Basic authentication. But it doesn't fix the Digest which is dynamic on connection.
I will add the test then continue with the PR on the other repository after you confirm here that we're good here.

Copy link
Contributor

@asimell asimell left a comment

Choose a reason for hiding this comment

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

Found some spots that could be cleaned. Otherwise looks good.

src/REST/keywords.py Show resolved Hide resolved
src/REST/keywords.py Outdated Show resolved Hide resolved
asimell
asimell previously approved these changes May 28, 2021
@asimell
Copy link
Contributor

asimell commented May 28, 2021

@ot2789 LGTM. Can you create a PR to Eficode/RESTinstance (I'll keep this one open as well). We'll merge this first there and then come back here with full release candidate.

@ot2789
Copy link
Contributor Author

ot2789 commented May 28, 2021

@asimell Ok. I'll create a PR there as well. Can you tell if there is a way to install the release that contains this PR through pip3?
Edit: Done, eficode#8

@asimell
Copy link
Contributor

asimell commented May 28, 2021

@ot2789 We will make a separate PR here that we'll merge here and release that to PyPI. We won't make the release through Eficode fork. Unfortunately, @asyrjasalo hasn't transferred this repo to Eficode org, even though he asked us to maintain this library. We've got our CI in place and full access rights in Eficode org, so this is how we roll for now.

@ot2789
Copy link
Contributor Author

ot2789 commented May 31, 2021

@asimell It is ok. I don't need to use the Eficode fork for updating. Just interested in installing it with pip3. So when this is approved, checked and merged. Let me know how I can install it. I expect to be something like pip3 install RESTinstance==1.x.xrc1 or something among those lines.

Copy link
Contributor

This pull request is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale label Jul 30, 2024
@asyrjasalo
Copy link
Owner

This was implemented and released. Thank you for your contribution!

@asyrjasalo asyrjasalo closed this Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants