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

Fix #784 Enable BasicAuth through Selenide proxy server #785

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion config/checkstyle/checkstyle.xml
Expand Up @@ -57,7 +57,7 @@
<module name="UnnecessaryParentheses"/>
<!--<module name="MutableException"/>-->
<module name="ClassFanOutComplexity">
<property name="max" value="28"/>
<property name="max" value="29"/>
</module>
<module name="CyclomaticComplexity">
<property name="max" value="20"/>
Expand Down
25 changes: 25 additions & 0 deletions src/main/java/com/codeborne/selenide/AuthenticationType.java
@@ -0,0 +1,25 @@
package com.codeborne.selenide;

/**
* Authentication schemes.
*
* @see <a href="https://developer.mozilla.org/en-US/docs/Web/HTTP/Authentication#Authentication_schemes">Web HTTP reference</a>
*/
public enum AuthenticationType {
BASIC("Basic"),
BEARER("Bearer"),
DIGEST("Digest"),
HOBA("HOBA"),
MUTUAL("Mutual"),
AWS4_HMAC_SHA256("AWS4-HMAC-SHA256");

private String value;
Copy link
Member

Choose a reason for hiding this comment

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

I would add private final here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@asolntsev Good catch!


AuthenticationType(final String value) {
Copy link
Member

Choose a reason for hiding this comment

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

I would remove final because it does't add any value.

this.value = value;
}

public String getValue() {
return value;
}
}
34 changes: 34 additions & 0 deletions src/main/java/com/codeborne/selenide/Credentials.java
@@ -0,0 +1,34 @@
package com.codeborne.selenide;

import java.util.Base64;

import static java.nio.charset.StandardCharsets.UTF_8;

public final class Credentials {
Copy link
Member

Choose a reason for hiding this comment

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

I would move this class to package com.codeborne.selenide.impl and make package-private. It's only used in Navigator.

Copy link
Collaborator Author

@rosolko rosolko Aug 24, 2018

Choose a reason for hiding this comment

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

@asolntsev
I consider to move this class to top level because I think about extra open method with signatures containing credentials instead of login and password pair.
What do you thinks?
It will be looks like - open(relativeOrAbsoluteUrl, authenticationType, credentials)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, one parameter Credentials is better than two Strings "login"/"password".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Or we can just overload method and give ability to do open with Credentials and login/password pair?

private String login;
private String password;

public Credentials(final String login, final String password) {
this.login = login;
this.password = password;
}

/**
* The resulting string is base64 encoded (YWxhZGRpbjpvcGVuc2VzYW1l).
*
* @return encoded string
*/
public String encode() {
final byte[] credentialsBytes = combine().getBytes(UTF_8);
return Base64.getEncoder().encodeToString(credentialsBytes);
}

/**
* Combine credentials with a colon (login:password).
*
* @return combined credentials
*/
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need javadoc for private methods?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@asolntsev
Yes we do.
The main point thats this object is public and method like this (I mean do some combining logic with special format) is necessary to de described in any way.

Copy link
Member

Choose a reason for hiding this comment

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

Let's make it public when it's really needed. Currently it's not used, so there is no reason to make it public.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree. Just remove javadoc since it's specially private method.

private String combine() {
return String.format("%s:%s", login, password);
}
}
22 changes: 22 additions & 0 deletions src/main/java/com/codeborne/selenide/Selenide.java
Expand Up @@ -101,6 +101,28 @@ public static void open(String relativeOrAbsoluteUrl, String domain, String logi
mockModalDialogs();
}

/**
*
* The main starting point in your tests.
* <p>
* Open browser and pass authentication using build-in proxy.
* <p>
* A common authenticationType is "Basic". See Web HTTP reference for other types.
* <p>
* {@code Configuration.fileDownload == Configuration.FileDownloadMode.PROXY;} - is required configuration.
Copy link
Member

Choose a reason for hiding this comment

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

I would rephrase it to something like "this method can only work if {@code Configuration.fileDownload == Configuration.FileDownloadMode.PROXY;}"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@asolntsev Understood. Thanks!

*
* @param relativeOrAbsoluteUrl
* @param authenticationType
* @param login
* @param password
Copy link
Member

Choose a reason for hiding this comment

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

I would remove all useless lines from javadoc, like @param login without description.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@asolntsev
We already had such empty javadocs lines for old open(relativeOrAbsoluteUrl, domain, login, password) method.
I do it in one style during new implementation.
In this case we had to reformat all methods related to this point.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, we can cleanup javadoc later.

*
* @see <a href="https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Proxy-Authorization">Web HTTP reference</a>
* @see AuthenticationType
*/
public static void open(String relativeOrAbsoluteUrl, AuthenticationType authenticationType, String login, String password) {
navigator.open(relativeOrAbsoluteUrl, authenticationType, login, password);
}

/**
* @see Selenide#open(URL, String, String, String)
*/
Expand Down
17 changes: 17 additions & 0 deletions src/main/java/com/codeborne/selenide/impl/Navigator.java
@@ -1,7 +1,10 @@
package com.codeborne.selenide.impl;

import com.codeborne.selenide.AuthenticationType;
import com.codeborne.selenide.Credentials;
import com.codeborne.selenide.logevents.SelenideLog;
import com.codeborne.selenide.logevents.SelenideLogger;
import io.netty.handler.codec.http.HttpHeaders;
import org.openqa.selenium.JavascriptExecutor;
import org.openqa.selenium.WebDriver;
import org.openqa.selenium.WebDriverException;
Expand All @@ -12,6 +15,7 @@
import static com.codeborne.selenide.Configuration.baseUrl;
import static com.codeborne.selenide.Configuration.captureJavascriptErrors;
import static com.codeborne.selenide.WebDriverRunner.getAndCheckWebDriver;
import static com.codeborne.selenide.WebDriverRunner.getSelenideProxy;
import static com.codeborne.selenide.WebDriverRunner.getWebDriver;
import static com.codeborne.selenide.WebDriverRunner.isIE;
import static com.codeborne.selenide.logevents.LogEvent.EventStatus.PASS;
Expand Down Expand Up @@ -39,6 +43,19 @@ public void open(URL url, String domain, String login, String password) {
navigateToAbsoluteUrl(url.toExternalForm());
}

public void open(String relativeOrAbsoluteUrl, AuthenticationType authenticationType, String login, String password) {
getAndCheckWebDriver();
Credentials credentials = new Credentials(login, password);
String authorization = String.format("%s %s", authenticationType.getValue(), credentials.encode());
getSelenideProxy().addRequestFilter("headers.request", (request, contents, messageInfo) -> {
final HttpHeaders headers = request.headers();
headers.add("Authorization", authorization);
headers.add("Proxy-Authorization", authorization);
Copy link
Member

Choose a reason for hiding this comment

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

@rosolko Isn't it a problem that we add this filter, but never remove?
It means that it will add Authorization header to ALL subsequent requests. Is it ok? Or we should remove this filter?

Copy link
Member

Choose a reason for hiding this comment

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

@rosolko I found that BrowserMobProxy already has built-in AutoBasicAuth filter. It's already enabled by default. So, this method can be simplified:

  public void open(String relativeOrAbsoluteUrl, AuthenticationType authenticationType, String login, String password) {
    getAndCheckWebDriver();
    getSelenideProxy().getProxy().autoAuthorization(domain(relativeOrAbsoluteUrl), login, password, AuthType.BASIC);
    open(relativeOrAbsoluteUrl);
  }

  private String domain(String relativeOrAbsoluteUrl) {
    String absoluteUrl = isAbsoluteUrl(relativeOrAbsoluteUrl) ? relativeOrAbsoluteUrl : absoluteUrl(relativeOrAbsoluteUrl);
    try {
      return new URL(absoluteUrl).getHost();
    }
    catch (MalformedURLException e) {
      throw new RuntimeException("invalid url: " + relativeOrAbsoluteUrl, e);
    }
  }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@asolntsev

  1. Previously you sad that we shouldn't call internal getProxy() and call only top level selenide proxy getSelenideProxy()
  2. autoAuthorization method supports only BASIC auth type
  3. If we remove filter isn't application logout user after another action in application?
  4. If remove - how can we remove filters from selenide proxy? I see only add functionality.

Copy link
Member

Choose a reason for hiding this comment

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

  1. Agree
  2. Agree
  3. I don't know exactly if logout happens. Probably. But anyway, we need to be able to somehow disable filter (when user calls open without basicauth) or reconfigure it (when user calls open with another username/password).
  4. Right, it seems that we cannot remove filter from BMP. So, we can add filter only once, and enable/disable or configure existing filter.

Copy link
Collaborator Author

@rosolko rosolko Aug 24, 2018

Choose a reason for hiding this comment

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

  1. I implement ability to remove filter. Let's try to remove filter after open authorized method and look at the effort.
    Looks like it isn't break flow.
  2. Chained with answer from 3 point.

return null;
});
open(relativeOrAbsoluteUrl);
}

protected String absoluteUrl(String relativeUrl) {
return baseUrl + relativeUrl;
}
Expand Down
10 changes: 10 additions & 0 deletions src/test/java/integration/BasicAuthTest.java
@@ -1,5 +1,8 @@
package integration;

import com.codeborne.selenide.AuthenticationType;
import com.codeborne.selenide.Configuration;
import com.codeborne.selenide.Configuration.FileDownloadMode;
import org.junit.jupiter.api.Test;

import static com.codeborne.selenide.Condition.text;
Expand All @@ -12,4 +15,11 @@ void canPassBasicAuth() {
open("/basic-auth/hello", "", "scott", "tiger");
$("body").shouldHave(text("Hello, scott:tiger!"));
}

@Test
void canAuthUsingProxy() {
Configuration.fileDownload = FileDownloadMode.PROXY;
open("/basic-auth/hello", AuthenticationType.BASIC, "scott", "tiger");
$("body").shouldHave(text("Hello, scott:tiger!"));
}
}