Feature/convert to web extension

[khlr]

Hello!

This is a pull request for issue #22. Please note that it's still in progress (the necessary changes for converting SAML tracer to a Web Extension are way more substantial than I thought in the first place ;-)

Anyways, I thought opening this pull request would be a good idea to get some early feedback. Maybe my effort is going in the wrong direction? Hence I really appreciate any comments! Some parts may be a bit hacky. Please tell me, if there's something inacceptable.

What's still on my todo-list:

• pretty up the yet only functional UI
• make the export/import functionality work (this seems to be quite effortful) to be done in another pull request
• use the strings from the locale-files to be done in another pull request
• fix incorrect request count (see here)
• tidy up everything
• spend time with "minor" things that aren't on my list yet ;-)

[thijskh]

Hi! Great to see you are working on this! I'll give it a try soon and let you know my findings.

I'm wondering what it is that makes that import/export require more effort than other parts?

[khlr]

Hi Thijs! I'm looking forward to your response!

Well, I think the import/export feature isn't more complex than other parts. It just seems to be the last "big" thing for me to convert. But I'll see... there still might be other things which seem "small" but may take some time, too 😉

[thijskh]

I've given it a try and it works quite fine already!

I did notice that AuthnRequests seem to be missing from my SAML tracer window. The GET request rows are there, but show the wrong URL (referrer?) and have no SAML label next to them. If you look at the request details. the GET request does seem to be to the right host.

[khlr]

Where did you experience that behaviour? As an example I tried to login to https://foodl.org using "Universität Hohenheim" as IdP. This seems to work:

[thijskh]

If I go to Foodl and select my IdP SURFnet I get the following:

Right above the first green coloured line, there's three identical black lines. However, the bottom two of those are GET requests to respectively engine.surfconext.nl and idp.surfnet.nl and they both have a SAML AuthnRequest parameter.

[khlr]

Thank you for the details. I will look after it the next days.

[khlr]

Sorry for the late response. You're absolutely right, Thijs. Good catch!

The reason for this behavior is that the webRequest API keeps a request's ID if it's a redirect. Hence results were overwritten.

I have an idea to work around this, but it'll take some more days 😉

[lonoak]

Hi,

have to say it works more-than-enough-well-for-me™ to probably replace the now missing add-on in Firefox v57... what additional fixes (the duplicate URLs/missing SAML tag problem reported by Thijskh?) would it need to merge the PR, build a new release, and publish it again?

Jose.

note: I can live without import/export, or a non-translated add-on... and those are minor problems IMO compared to not having it...

[khlr]

Hello there!

Thijs' findings should be fixed with the latest commits.

In theory I'd now go on with the import/export. But as @lonoak suggested it'd be nice to have SAML Tracer back as a compatible extension soon. So I could put a little effort in prettifying the UI and deliver the other features with subsequent pull requests.

Maybe @jaimeperez or @olavmrk could tell what they'd prefer?

[thijskh]

Yes, if it was up to me I'd definitely prefer to have something that has basic functionality uploaded now, than the situation that we have and that is that there is nothing. And then release updated versions to complete the rest. Hope @jaimeperez or @olavmrk can help with getting it published at Mozilla.

[jaimeperez]

I agree too. It's better to have it out with basic functionality rather than waiting to have everything in place (especially given that import/export is something rarely used).

We can definitely help releasing it and submitting the package to Mozilla.

[thijskh]

I've tested some more and indeed it works better now. Two items stand out:

When I start the latest master, the window is completely blank until I right-click it.

Try logging in to https://wiki.surfnet.nl (top-right) with the tracer open. Then click one of the IdP's. The authnrequest to engine.surfconext.nl is shown, but the subsequent authnrequest to the IdP does not show.

[khlr]

Thanks again, Thijs!

I was able to reproduce the blank-problem on my linux machine. It doesn't occur on my windows machine. I think this behavior started with FF57, right? I'll look for the cause - and for a fix ;-) - on the weekend.

Your second finding seems to be related to accessing some undefined objects. I'll look for that on the weekend, too! :-)

[thijskh]

Agreed that it is since FF57. Thanks for all your efforts so far!

[khlr]

So far, I focused on your second finding. Unfortunately, when it comes to redirects, it seems as if the webRequest-API behaves differently form the way browsers behave... The subsequent request vanishes, because the webRequest-API handles it as a POST although the browser issues a GET...

Errors occur hereafter, because undefined members are invoked, since the request is no real POST and therefore has no postdata etc.

I described this unexpected behaviour in a Stackoverflow question. Maybe someone has an idea...

[khlr]

From what I found out yet, the webRequest-API seems to act way more RFC-compliant than modern browsers do...

Anyways, we need a way to let SAML Tracer imitate the browsers' special-302-treatment. The webRequest-API allows several things to be modified. Although this doesn't include the method directly, maybe the redirectUrl-property could be "abused" to achieve this:

...initiated by a redirect action use the original request method for the redirect, with one exception: If the redirect is initiated at the onHeadersReceived stage, then the redirect will be issued using the GET method.

That's exactly what's necessary. But somehow it seems quite dirty 😆

[thijskh]

A valid point indeed about whether the HTTP status codes are correct, but indeed one would assume that in any case the reported behaviour of the API is identical to the actual behaviour of the browser...

[lonoak]

Wondering if the 'Web Developers' tool -> Network makes use of this API. At least it seems to handle redirections correctly:

[lonoak]

Forget about my previous comment... unless firefox uses internally this API itself. I hadn't read your post to StackOverflow... :(

[khlr]

With commit a0ca642 I now applied the workaround described here. I'm still not quite sure if I feel comfortable with this, but for the moment it seems to work ;-)

[khlr]

By now the most severe issue should be solved. The blank-page-problem unfortunately still exists, when using SAML tracer on Linux. I have no real clue yet.

But I think the new UI looks quite nice. I tried to give it a developer menu look. What do you think?

So what's still on my list:

• fix the blank-page-problem
• tidy up everything (remove old code fragments, update readme, ...)

All other issues should be fixed with subsequent pull requests.

[jkakavas]

Hi, @khlr thanks a ton for your efforts on this. I wanted to add that I did some testing on Ubuntu 16.04 with Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0 .

I can verify the issue with the blank screen until the first right click and I would like to suggest marking the selected line in resources somehow so that it is obvious which request the user is looking at. I will open this as an issue in due time.

Thanks again for making this usable in Quantum

[khlr]

Thanks for your feedback, Ioannis!

What I noticed while debugging is that there's an error occurring on Linux machines that doesn't show up on Windows machines:

TypeError: invalid 'in' operand browser[Learn More] tabbrowser.xml:2426:1

I'll continue my investigations in this direction.

[tvdijen]

Here's a c;lue: https://issues.adblockplus.org/ticket/5817#no1

[thijskh]

Hi @khlr, I've given it some more tests and it works very well for me! Also like the improved layout. Impressive work.

The Linux display bug is indeed still there but I could live with it even if it was not fixed in the first upload: this is already much better than we have now. I would be very much in favour of uploading the extension to Mozilla (with or without Linux display fix) in the short term…

[khlr]

Thanks for the hint, @tvdijen! But unfortunately the fix suggested there doesn't seem to work in our case. At least I'm not able to catch the exception.

Meanwhile I found other error reports of this kind, too. It's believed to be a Firefox bug. So for the moment I think it's a good idea to not spend further time in this and fix this later.

Thank you for your feedback, @thijskh! I'm looking forward to uploading the extension to Mozilla in the near future, too! Presumably tomorrow I'll have some more time to highlight the selected request row and tidy up the codebase.

[khlr]

If I did not miss anything (which I probably have 😉), the pull request is finished from what I think.

What's still to do with future pull requests:

• re-introduce the import/export feature
• re-introduce localization

And additionally very useful:

• update the build script (if necessary)
• provide a more appropriate extension icon

@jaimeperez, could you please review the changes and then submit the extension to Mozilla? I think it's just you and @olavmrk who are able to upload updated versions of the extension, aren't you? Aprospos "new version": Do you create something like a release branch or should I increase the version number directly in this pull request. 0.4.0 to 0.5.0?

[jaimeperez]

Thanks so much for this @khlr! This is an impressive piece of work!

Since we haven't been doing much development on the SAML tracer, I think it's perfectly fine to just merge it into master and tag it with 0.5.0. One could argue that there are enough changes to justify a new major, but on the other hand there's functionality that's been lost, so I'm not sure this deserves a major.

In any case, I'm running really tight on time these days, so I don't think I'll be able to test and review in the near future. Since I don't want to delay this, maybe @thijskh can give the code a review?

I don't know either if the build script will work any longer. If it just works ™ it should be simple for me to build and upload the new release, but if it doesn't, I'm not sure I'll have time to fix it myself...

[thijskh]

build.sh probably needs to be updated, not to create a .xpi but a .zip. Maybe just git archive --format zip suffices...

[thijskh]

I've tested it and it works for me, as others have also confirmed above. A review did not turn up any real problems. Good to see that most of the actual SAML-specific code has been reused.

@jaimeperez I think that dropping functionality is per definition a new major? In any case increasing the major version seems appropriate for such a fundamental change. Building it for upload is trivial as far as I understand. So let's go for it?

Our remaining wishlist item is restoring import/export... seems ideal for a next iteration after this has been submitted?

[jkakavas]

:+1: from me for a new major

[jaimeperez]

Great, thanks a lot for taking the time to review, Thijs!

I've merged and uploaded a new 1.0 release, which is now pending review. Good work everybody!

[jaimeperez]

... and there it was approved and published! That was fast! 🎉

[igorvulovic]

And it is working perfectly, I already tested it 🥇 Thank you so much!

[thijskh]

Thanks a lot @khlr!

[jkakavas]

Magnificent work @khlr , much appreciated !

[lonoak]

It really works well, @khlr! Thanks for your work, and thanks Jaime for re-publishing it quick!

[DanielMalmgren]

Great work everyone involved! I've really been missing this in my day-to-day work...

One tiny comment: When I click somewhere in the SAML and press ctrl-A (something I do kinda often because I want to copy the ticket somewhere) not only the SAML is marked but also the entire list of calls, I'm quite sure the behaviour was different in the old version. Anything that's easy to change?

[khlr]

Thank you all for your support and your feedback. It was my pleasure! 🤗

@danielmalmgren, would you mind creating a new issue for your finding?