GMOD / Apollo

Genome annotation editor with a Java Server backend and a Javascript client that runs in a web browser as a JBrowse plugin.
http://genomearchitect.readthedocs.io/
Other
124 stars 85 forks source link

Bug in JBrowse authentication in development code. #25

Closed spficklin closed 9 years ago

spficklin commented 9 years ago

At the hackaton this past January, some changes were made to the server/WebApollo/src/org/bbop/apollo/web/Login.java file to support OAuth authentication. It changed the way HTTP GETs were handled. This inadvertantly broke the logins through JBrowse. I have committed a fix to my forked copy of Apollo, and I have a patch I can supply.

nathandunn commented 9 years ago

That would be great.

Sent from my iPhone

On Sep 12, 2014, at 11:52 PM, spficklin notifications@github.com wrote:

At the hackaton this past January, some changes were made to the server/WebApollo/src/org/bbop/apollo/web/Login.java file to support OAuth authentication. It changed the way HTTP GETs were handled. This inadvertantly broke the logins through JBrowse. I have committed a fix to my forked copy of Apollo, and I have a patch I can supply.

— Reply to this email directly or view it on GitHub.

cmdcolin commented 9 years ago

Can you clarify the bug you're seeing? I haven't seen any problems with logins so I just want to understand more what you mean.

spficklin commented 9 years ago

Each time I would click the 'Login' button in JBrowse nothing seemed to occur. I was never presented with the login dialogue. I suppose if you were logged in previously via WebApollo first then this problem would not occur. To see if you have the same problem, try logging in using the 'Login' button in JBrowse rather than through the WebApollo pages.

So, here's the problem.... In the stable version of WebApollo, an HTTP GET to the Login servlet (Login.java) would result in the login form being delivered to the user so that the user could provide credentials to login. The JBrowse "Login" button passed the parameters "operation=login" (e.g. http://localhost/WebApollo/Login?operation=login"). But, it really didn't matter that the operation parameter was there, WebApollo would always return the login dialogue.

At the Hackathon this June, because the OAuth protocol uses GET to pass parameters between the Oauth server and the client, Ed changed the doGet() function of the Login servlet so that if the 'operation' parameter was part of the GET URL and it's value was 'login' that it would instead pass handling of the GET to the authentication plugin (in our case the OAuth plugin). This inadvertently broke the JBrowse login button because it passes the parameter "operation=login". Therefore, the GET URL was expected to be handled by the authentication plugin rather than delivering the login form like it should have.

spficklin commented 9 years ago

Another comment.... A quick fix for this, could be to remove the 'operation=login' from the URL when the JBrowse "Login" button is clicked. Then the doGet() function would no longer see the "operation" parameter and would return the login dialogue as expected. I wasn't comfortable digging around in JBrowse to change that so I altered the code in my fork of WebApollo to handle the JBrowse case. If you have this same problem and would like to see the patch let me know and I can send it along. Or if you want to change the link in JBrowse then let me know and I can revert my changes to the Login.java file.

Thanks...

cmdcolin commented 9 years ago

I don't see the parameter ?operation=login currently being used for the login button inside jbrowse, which hopefully means that there is no issue! Currently it says the following for the login function (see AnnotTrack.js:4151)

login: function() {
    var track = this;
    dojo.xhrGet( {
        url: context_path + "/Login",
        handleAs: "text",
        timeout: 5 * 60,
        load: function(response, ioArgs) {
//            track.openDialog("Login", response);
            var dialog = new dojoxDialogSimple({
                preventCache: true,
                refreshOnShow: true,
                executeScripts: true 
            });  
            if (track.config.disableJBrowseMode) {
                dialog.hide = function() { }; 
            }    
            dialog.startup();
            dialog.set("title", "Login");
            dialog.set("content", response);
            dialog.show();
        }    
    });  
},   

Previously it was as @spficklin said, passing operation=login to the login button, but as i said above, it was changed.

login: function() {
    var track = this;
    dojo.xhrGet( {
        url: context_path + "/Login?operation=login",
        handleAs: "text",
        timeout: 5 * 60,
        load: function(response, ioArgs) {
            track.openDialog("Login", response);
        }
    });
},
spficklin commented 9 years ago

Oh, so I guess this means, I'm using an older version of JBrowse that still has the operation=login parameter in the Login button. How do I find the version of JBrowse that is compatible with the current development head?

cmdcolin commented 9 years ago

This code is actually part of WebApollo (the login button is aadded dynamically to jbrowse as a part of WebApollo being a jbrowse plugin), so if you have pulled the latest branch of GMOD/Apollothen it should be fine. I actually checked your fork of the code (https://github.com/spficklin/Apollo/blob/master/client/apollo/js/View/Track/AnnotTrack.js) and it seems to be up to date.

spficklin commented 9 years ago

Ah... I see. Okay. I'll back out those updates to my fork to the Login.java code.

Thanks for your help.

spficklin commented 9 years ago

For my development work I have to create new WAR files for WebApollo. This is because GenSAS will install WebApollo on the fly for each project it will manage, and I need the code from my fork for Drupal authentication. JBrowse comes packaged in the WAR files. I've been using JBrowse that came pre-packaged in the WebApollo-2014-04-03 version.... although now with the updated WebApollo plugin.... But, I wonder, should i continue to still use that version of JBrowse? Does it matter what version I use so long as the WebApollo plugin is present? Or is there a recommended version at any given time during development? I'd like to add this information to the documentation for setting up Eclipse for WebApollo development work...

cmdcolin commented 9 years ago

You can use a plain github checkout of jbrowse, or you can use the "dev" versions that are offered for download for each release (ex. http://jbrowse.org/jbrowse-1-11-5/)

Note that we are also working on improving the build process, and the complex Eclipse setup may be updated by a new Maven build script.

cmdcolin commented 9 years ago

If there's no issue here I'll close this for now. Thanks for the concern :+1: