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
base: master
from

Conversation

Projects
None yet
3 participants
@rosolko
Copy link
Collaborator

rosolko commented Aug 21, 2018

Proposed changes

Fix #784 issue. Enable BasicAuth through Selenide proxy server

Checklist

  • Checkstyle and unit tests pass locally with my changes by running gradle check chrome htmlunit command
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
@@ -57,7 +57,7 @@
<module name="UnnecessaryParentheses"/>
<!--<module name="MutableException"/>-->
<module name="ClassFanOutComplexity">
<property name="max" value="28"/>
<property name="max" value="31"/>

This comment has been minimized.

@BorisOsipov

BorisOsipov Aug 21, 2018

Member

not related to this PR. We need to do smth with Selenide.java class

This comment has been minimized.

@rosolko

rosolko Aug 21, 2018

Author Collaborator

Agree. It was one of the point to create separate AuthenticationType and Credentials classes to make logic easier a little bit.

@BorisOsipov BorisOsipov added the feature label Aug 21, 2018

@BorisOsipov BorisOsipov added this to the 4.14.0 milestone Aug 21, 2018

*/
public static void open(final String relativeOrAbsoluteUrl, final AuthenticationType authenticationType,
final String login, final String password) {
getAndCheckWebDriver();

This comment has been minimized.

@asolntsev

asolntsev Aug 22, 2018

Contributor

Here is too much logic for Selenide class. All this stuff should be in SelenideProxyServer and/or navigator.open. In this case we would not need to increase ClassFanOutComplexity in checkstyle.xml

getAndCheckWebDriver();
final BrowserMobProxy proxy = getSelenideProxy().getProxy();
final Credentials credentials = new Credentials(login, password);
final String authorization = String.format("%s %s", authenticationType.getValue(), credentials.encode());

This comment has been minimized.

@asolntsev

asolntsev Aug 22, 2018

Contributor

I personally do not like final near every local variable and method parameter. It's a noise.

public static void open(final String relativeOrAbsoluteUrl, final AuthenticationType authenticationType,
final String login, final String password) {
getAndCheckWebDriver();
final BrowserMobProxy proxy = getSelenideProxy().getProxy();

This comment has been minimized.

@asolntsev

asolntsev Aug 22, 2018

Contributor

It's not a good idea to use BrowserMobProxy directly. Much better would be to add RequestFilter using getSelenideProxy().addRequestFilter(). This filter could add required headers.

@rosolko

This comment has been minimized.

Copy link
Collaborator Author

rosolko commented Aug 23, 2018

@asolntsev Done.

getSelenideProxy().addRequestFilter("headers.request", (request, contents, messageInfo) -> {
final HttpHeaders headers = request.headers();
headers.add("Authorization", authorization);
headers.add("Proxy-Authorization", authorization);

This comment has been minimized.

@asolntsev

asolntsev Aug 23, 2018

Contributor

@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?

This comment has been minimized.

@asolntsev

asolntsev Aug 23, 2018

Contributor

@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);
    }
  }

This comment has been minimized.

@rosolko

rosolko Aug 24, 2018

Author Collaborator

@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.

This comment has been minimized.

@asolntsev

asolntsev Aug 24, 2018

Contributor
  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.

This comment has been minimized.

@rosolko

rosolko Aug 24, 2018

Author Collaborator
  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.
* Combine credentials with a colon (login:password).
*
* @return combined credentials
*/

This comment has been minimized.

@asolntsev

asolntsev Aug 23, 2018

Contributor

Do we really need javadoc for private methods?

This comment has been minimized.

@rosolko

rosolko Aug 24, 2018

Author Collaborator

@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.

This comment has been minimized.

@asolntsev

asolntsev Aug 24, 2018

Contributor

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

This comment has been minimized.

@rosolko

rosolko Aug 24, 2018

Author Collaborator

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


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

public final class Credentials {

This comment has been minimized.

@asolntsev

asolntsev Aug 23, 2018

Contributor

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

This comment has been minimized.

@rosolko

rosolko Aug 24, 2018

Author Collaborator

@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)

This comment has been minimized.

@asolntsev

asolntsev Aug 24, 2018

Contributor

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

This comment has been minimized.

@rosolko

rosolko Aug 24, 2018

Author Collaborator

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

MUTUAL("Mutual"),
AWS4_HMAC_SHA256("AWS4-HMAC-SHA256");

private String value;

This comment has been minimized.

@asolntsev

asolntsev Aug 23, 2018

Contributor

I would add private final here.

This comment has been minimized.

@rosolko

rosolko Aug 24, 2018

Author Collaborator

@asolntsev Good catch!


private String value;

AuthenticationType(final String value) {

This comment has been minimized.

@asolntsev

asolntsev Aug 23, 2018

Contributor

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

* @param relativeOrAbsoluteUrl
* @param authenticationType
* @param login
* @param password

This comment has been minimized.

@asolntsev

asolntsev Aug 23, 2018

Contributor

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

This comment has been minimized.

@rosolko

rosolko Aug 24, 2018

Author Collaborator

@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.

This comment has been minimized.

@asolntsev

asolntsev Aug 24, 2018

Contributor

I agree, we can cleanup javadoc later.

* <p>
* A common authenticationType is "Basic". See Web HTTP reference for other types.
* <p>
* {@code Configuration.fileDownload == Configuration.FileDownloadMode.PROXY;} - is required configuration.

This comment has been minimized.

@asolntsev

asolntsev Aug 23, 2018

Contributor

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

This comment has been minimized.

@rosolko

rosolko Aug 24, 2018

Author Collaborator

@asolntsev Understood. Thanks!

getSelenideProxy().addRequestFilter("headers.request", (request, contents, messageInfo) -> {
final HttpHeaders headers = request.headers();
headers.add("Authorization", authorization);
headers.add("Proxy-Authorization", authorization);

This comment has been minimized.

@asolntsev

asolntsev Aug 23, 2018

Contributor

@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);
    }
  }

asolntsev added a commit that referenced this pull request Aug 26, 2018

asolntsev added a commit that referenced this pull request Aug 26, 2018

#785 hold only one instance of Authentication filter
set/reset login+password in every open() method

@asolntsev asolntsev referenced this pull request Aug 26, 2018

Merged

Fix #784 and close #785 #787

3 of 3 tasks complete

asolntsev added a commit that referenced this pull request Aug 27, 2018

asolntsev added a commit that referenced this pull request Aug 27, 2018

asolntsev added a commit that referenced this pull request Aug 27, 2018

@rosolko rosolko deleted the rosolko:784 branch Aug 27, 2018

@asolntsev

This comment has been minimized.

Copy link
Contributor

asolntsev commented Aug 27, 2018

Improved and merged in PR #787

asolntsev added a commit that referenced this pull request Aug 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.