Created attachment 32182 [details]
ProxyControl patch

Please, review patch from attachment.

With this change ProxyControl should grab "Authentication" header, add Authentication Manager(if there was no any), add authentication.
Duplicated Authentication records will be removed.

Reopening as it was closed by error.
Thanks for patch, for next time , only update Keywords with PatchAvailable but don't Mark bug as Resolved.
Thanks

Ok, Philippe.

Hello Dzimitry,
First thank you for your contribution.
Few notes on patch:
- There are no javadocs for many methods, can you add meaningful docs that explains them and comments for some processing
- AFAIU You code makes hypothesis that Authorization is Basic while it can be Digest. So you need to adapt code to handle this case, it would make it even smarter :-)
- getAuthorization should be named differently as it in fact removes from Headers the one named HTTPConstants.HEADER_AUTHORIZATION) once it has handled the creation of Auth Manager. 
- Proxy#run : add some docs to explain that HEADER_AUTHORIZATION is removed
- In AuthManager#addAuth , can you explain what removeMatchingAuthorizations exactly does

Thanks

Created attachment 32190 [details]
bug_25430_1.patch

> getAuthorization should be named differently as it in fact removes from Headers the one named HTTPConstants.HEADER_AUTHORIZATION) once it has handled the creation of Auth Manager. 

It extracts Authentication header from subconfigs and constructs Authentication object for further adding to Authentication Manager. Removing header is side-effect, not the main sense of this method. May be, it worth breaking it into two methods, but I have no clear idea about it.

> AFAIU You code makes hypothesis that Authorization is Basic while it can be Digest. So you need to adapt code to handle this case, it would make it even smarter :-)

Yes, agree. How about such code?
authorization.setMechanism(authType.equals("Basic")|authType.equals("Digest")?
                                AuthManager.Mechanism.BASIC_DIGEST:
                                AuthManager.Mechanism.KERBEROS);

> In AuthManager#addAuth , can you explain what removeMatchingAuthorizations exactly does
It iterates over Authorization objects in Authorization Manager and remove Authorization, which has the same fields as auth. I took this approach from Cookie Manager. I’ve reviewed this part and made changes. Now authentication object added only in case, if there is no the same one in manager.

public void addAuth(Authorization newAuthorization) {
        boolean alreadyExists=false;
        PropertyIterator iter = getAuthObjects().iterator();
        while (iter.hasNext()) {
            Authorization authorization = (Authorization) iter.next().getObjectValue();
            if (authorization == null) {
                continue;
            }
            if (match(authorization,newAuthorization)) {
                if (log.isDebugEnabled()) {
                    log.debug("Found the same Authorization object = " + newAuthorization.toString());
                }
                alreadyExists=true;
            }
        }
        if(!alreadyExists){
            getAuthObjects().addItem(newAuthorization);
        }
    }

Also added more comments to patch.

Date: Tue Dec 16 21:23:26 2014
New Revision: 1646084

URL: http://svn.apache.org/r1646084
Log:
Bug 25430 - HTTP(S) Test Script Recorder : Make it populate HTTP Authorisation Manager
Bugzilla Id: 25430

Modified:
    jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/control/AuthManager.java
    jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/proxy/Proxy.java
    jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/proxy/ProxyControl.java
    jmeter/trunk/xdocs/changes.xml
    jmeter/trunk/xdocs/usermanual/component_reference.xml

Hi Dzmitry,
Thanks for Patch, I commited it with a bunch of changes:
- Base64 conversion cannot work with Digest
- Authorization header was not correctly removed
- URL of Authorization was not created correctly, Domain was wrongly used
- When I fail parsing or guessing I put in authorization:
* ${AUTH_BASE_URL}
* ${AUTH_LOGIN}
* ${AUTH_PASSWORD}

1/ Note I am not satisfied currently with the URL Matching nor guessing , but I don't see how to improve this.
2/ I think it would be better to put AuthorizationManager as first child of Recording controller if possible it would be more visible

Please make as much tests as you can on your side.

Thanks

Date: Tue Dec 16 21:35:47 2014
New Revision: 1646093

URL: http://svn.apache.org/r1646093
Log:
Bug 25430 - HTTP(S) Test Script Recorder : Make it populate HTTP Authorisation Manager
Matching should ignore login/password
Bugzilla Id: 25430

Modified:
    jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/control/AuthManager.java

Created attachment 32300 [details]
Add AuthManager as first node

Hi Philippe,
Thank you for feedback.

1/ Yes, seems for now your variant is the best;
2/ Please, review patch proxy_control.patch. I've noticed, that if Test tree is new, Test Recorder has default value "Use recording controller". In this case all recorded samples are added below WorkBench. I've implemented addition of AuthManager as 3rd element(TestPlan,WorkBench,...) in this case. Is it ok?

Hi Dzmitry,
I don't think it is OK from what I understand but to be sure, could you show a screenshot of what you get without patch and what you get with it ?

Thx

Created attachment 32315 [details]
AuthManager_use_recording_controller

Phillipe,
Please, look.

This is an existing behaviour and is due to wrong configuration of Recording.
I close this one and opened a new Bugzilla 57381 to change general recording behaviour if team is ok with it.

So, what do you think about suggestion to make HTTP Auth Manager first element of node?

It may look like this:

src/protocol/http/org/apache/jmeter/protocol/http/proxy/ProxyControl.java
  try {
                 log.debug("Creating HTTP Authentication manager for authorization:"+authorization);
                 AuthManager authManager = newAuthorizationManager(authorization);
                JMeterTreeNode authManagerNode = new JMeterTreeNode(authManager,jmeterTreeModel);
                int index=(target.getChildCount()>0&&
                        ((JMeterTreeNode)target.getFirstChild()).getTestElement() instanceof TestPlan)?2:0;
                jmeterTreeModel.insertNodeInto(authManagerNode, target,index);
             } catch (IllegalUserActionException e) {
                 log.error("Failed to add Authorization Manager to target node:" + target.getName(), e);
             }

This issue has been migrated to GitHub: https://github.com/apache/jmeter/issues/1276