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

step to support HTTP Basic Auth #145

Closed
wants to merge 2 commits into from
Closed

step to support HTTP Basic Auth #145

wants to merge 2 commits into from

Conversation

saxena-gaurav
Copy link

No description provided.

@@ -12,6 +13,10 @@
add_header(header, value)
end

When(/^the request auth basic header `([^`]*)` is assigned `([^`]*)`$/) do |header, value|
Copy link
Contributor

Choose a reason for hiding this comment

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

if doing basic auth then the arguments should be the username and password. The phrasing of the step should be considered relative to other steps...this step in particular is very confusing since auth is in the step but the header is an argument so the step could read very different than what is being done.

A better alternative may be something like:

When the request credentials are set for basic auth user `$user` and password `$password`

This also needs be accompanied by appropriate documentation and specification updates.

Copy link
Contributor

@mwhipple mwhipple left a comment

Choose a reason for hiding this comment

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

Requires documentation and specification.

Copy link
Contributor

@mwhipple mwhipple left a comment

Choose a reason for hiding this comment

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

I must have clicked the wrong button before, this won't be merged without corresponding docs and specs.

@@ -12,6 +13,10 @@
add_header(header, value)
end

When (/^the request credentials are set for basic auth user `([^`]*)` and password `([^`]*)`$/) do |user, password|
add_header('Authorization', 'Basic ' + Base64.strict_encode64(user + ':' + password))
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer string interpolation over concatenation. It's likely worth splitting this into two lines where the encoded value is assigned to a temporary value.

@grsin
Copy link

grsin commented Apr 8, 2019

I'm going to vote for closing this since there isn't any development on this. The issue is still open.
#141

@grsin grsin closed this Apr 8, 2019
@grsin grsin deleted the basic branch April 8, 2019 13:40
@Lewiscowles1986
Copy link
Contributor

Up for me picking this up and writing tests?

@mwhipple
Copy link
Contributor

mwhipple commented May 6, 2019

@Lewiscowles1986 Sure, that would be great, thanks! Sorry for the late reply, this got slightly buried in my inbox.

As part of the initial PR it should likely have an entry in the Step Reference and appropriate specs. An article is planned/desired for more in-depth info for authentication pieces.

@Lewiscowles1986
Copy link
Contributor

Yeah I got a bit lost in the specs using quoted strings.

@mwhipple
Copy link
Contributor

mwhipple commented May 7, 2019

Interesting issue.

I think it probably makes sense to shuffle the way the specs are executed around. The current approach of having one spec file which defines another one to run was I think borrowed from the Cucumber project, but the main files end up as mostly boilerplate. It probably makes more sense to change the way the tests are run so that the specs can just be what are currently the inner files which are all expected to pass. This should be a lot cleaner and will also likely make it easier to make the specs portable (rather than relying on or recreating potentially Ruby specific libraries like Aruba).

I'll see about swapping that over in the next day or so.

@mwhipple mwhipple mentioned this pull request May 7, 2019
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.

None yet

4 participants