Bug 25430 - HTTP(S) Test Script Recorder : Make it populate HTTP Authorization Manager
Summary: HTTP(S) Test Script Recorder : Make it populate HTTP Authorization Manager
Status: RESOLVED FIXED
Alias: None
Product: JMeter
Classification: Unclassified
Component: Main (show other bugs)
Version: 1.9.1
Hardware: All All
: P3 enhancement (vote)
Target Milestone: ---
Assignee: JMeter issues mailing list
URL:
Keywords: PatchAvailable
Depends on: 57381
Blocks:
  Show dependency tree
 
Reported: 2003-12-11 02:33 UTC by Andrew Roughan
Modified: 2015-03-01 09:11 UTC (History)
2 users (show)



Attachments
ProxyControl patch (11.21 KB, patch)
2014-11-03 12:06 UTC, Dzmitry Kashlach
Details | Diff
bug_25430_1.patch (13.19 KB, patch)
2014-11-05 11:11 UTC, Dzmitry Kashlach
Details | Diff
Add AuthManager as first node (1.24 KB, patch)
2014-12-18 08:09 UTC, Dzmitry Kashlach
Details | Diff
AuthManager_use_recording_controller (88.00 KB, image/png)
2014-12-21 07:52 UTC, Dzmitry Kashlach
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Roughan 2003-12-11 02:33:56 UTC
Have the Recording Controller record any authorisations that are
entered and populate the HTTP Authorisation component with the
values.
This would assist with setting up the HTTP Authorisation Manager
correctly.
Comment 1 Dzmitry Kashlach 2014-11-03 12:06:54 UTC
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.
Comment 2 Philippe Mouawad 2014-11-04 12:34:36 UTC
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
Comment 3 Dzmitry Kashlach 2014-11-04 12:48:43 UTC
Ok, Philippe.
Comment 4 Philippe Mouawad 2014-11-04 19:58:18 UTC
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
Comment 5 Dzmitry Kashlach 2014-11-05 11:11:55 UTC
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.
Comment 6 Philippe Mouawad 2014-12-16 21:23:51 UTC
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
Comment 7 Philippe Mouawad 2014-12-16 21:30:12 UTC
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
Comment 8 Philippe Mouawad 2014-12-16 21:36:25 UTC
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
Comment 9 Dzmitry Kashlach 2014-12-18 08:09:26 UTC
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?
Comment 10 Philippe Mouawad 2014-12-19 21:39:09 UTC
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
Comment 11 Dzmitry Kashlach 2014-12-21 07:52:02 UTC
Created attachment 32315 [details]
AuthManager_use_recording_controller

Phillipe,
Please, look.
Comment 12 Philippe Mouawad 2014-12-21 15:02:45 UTC
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.
Comment 13 Dzmitry Kashlach 2014-12-21 20:11:53 UTC
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);
             }