(GH-14) Adding additional IconUrl check #23

gep13 · 2015-11-26T20:45:54Z

To ensure that the IconUrl either comes from the same domain as the
ProjectUrl, or comes from the RawGit CDN
Fixes Guidline: the use of iconurl not from domain of projecturl and not rawgit #14

ferventcoder · 2015-11-28T01:25:01Z

...alidator/infrastructure.app/rules/IconUrlNotSameDomainAsProjectUrlDomainOrRawGitGuideline.cs

+
+        protected override PackageValidationOutput is_valid(IPackage package)
+        {
+            var iconUrlHost = package.IconUrl.Host;


This could be null, so it may be helpful to return if the package.IconUrl is empty

ferventcoder · 2015-11-28T01:26:17Z

Fix up the checks a bit so there are no null reference exceptions.

gep13 · 2015-11-29T14:37:19Z

@ferventcoder ah, good shout!

I think I have corrected the logic flow. Let me know if you spot any other problems.

ferventcoder · 2015-11-29T17:46:18Z

Always resubmit the same commits unless the new changes add something additional. A code review is not one of them. (same comments for full effect ;) )

ferventcoder · 2015-11-29T17:47:45Z

...alidator/infrastructure.app/rules/IconUrlNotSameDomainAsProjectUrlDomainOrRawGitGuideline.cs

+
+            // Since there is an iconUrl, but no Project Url, the check has to return false
+            // since the domains are definitely not the same
+            if(package.ProjectUrl == null) return false;


We have a failure already accounted for projectUrl being empty, but I think it is okay to do this here. 👍

Agreed, this is a side effect of this specific test, and also a slight duplication of the other test, is it actually testing something else.

gep13 · 2015-11-29T21:23:37Z

@ferventcoder just away to head for the night, but will follow up with any comments in the morning. Let me know what you think.

ferventcoder · 2015-11-30T20:56:55Z

...alidator/infrastructure.app/rules/IconUrlNotSameDomainAsProjectUrlDomainOrRawGitGuideline.cs

+            // Use Regular Expression to extract the Domain Name, from the Uri Host
+            // Taken from example shown here http://stackoverflow.com/a/17091145/671491
+            var match = Regex.Match(host, "([^.]+\\.[^.]{1,3}(\\.[^.]{1,3})?)$");
+            return match.Groups[1].Success ? match.Groups[1].Value : string.Empty;


If there was ever a valid reason for adding tests, this is one.

yeah, I was actually thinking that as I was creating it. :-D Have you got an example of how you would like the test harness, etc, set up, adn i can take a stab at it?

There is a tests project. I'm taking it that there are no examples yet though?

The testing harness would be similar to choco's

I don't "think" there are, but ill admit, I only glanced. Will take a look, and see what I can come up with.

Would you see the get_domain_from_host method staying within the rule, or moved out into own class? Made static? Or create interface and constructor inject?

Just throwing ideas out just now. Not at computer now, so will be at least tomorrow.

Make it a public method and then test it there.

Okay, sounds like a plan 👍

gep13 · 2015-12-01T13:03:05Z

@ferventcoder as a start (work still required) is the second commit the type of thing that you had in mind?

gep13 · 2015-12-01T13:03:42Z

NOTE: Never used TinySpec before, so very much feeling my way around here, but used some examples from choco in order to create this.

ferventcoder · 2015-12-01T13:40:36Z

...tor.tests/infrastructure.app/IconUrlNotSameDomainAsProjectUrlDomainorRawGitGuidelineSpecs.cs

+            result.ShouldEqual("test.com");
+        }
+    }
+}


This is a great start! You could probably change the because to empty and then do this in all of the tests:

iconGuideline.get_domain_from_host("www.test.com").ShouldEqual("test.com")

And then add tests for the following scenarios:

test.com

www.test.com

somewildsubdomain.test.com

so, is each of those a separate test, or rather Fact? In other testing frameworks I have used/seen you can overload the test with multiple input and expected values, and it runs through the single test of each theory. Does that work here, or should I create three separate facts? If so, any suggestions as the the name of each fact?

It's using nunit in the background, so you could add that logic, I just prefer separate specs for each so the names are specific to what failed - unless you have a way of changing the test name based on the input.

It's really working with testing for so long - I should know from the name of the test alone exactly what failed before I go to dig into how to fix it. If I have to dig into the test itself to find out why it is failing then something is wrong. That's the theory anyway. It works out most of the time.

gep13 · 2015-12-01T20:37:53Z

@ferventcoder ok, let me know what you think.

ferventcoder · 2015-12-01T21:57:18Z

...tor.tests/infrastructure.app/IconUrlNotSameDomainAsProjectUrlDomainorRawGitGuidelineSpecs.cs

+
+            var domain = iconGuideline.get_domain_from_host("somewildsubdomain.test.co.uk");
+
+            domain.ShouldEqual("test.co.uk");


I would not have thought of this scenario, but that appears to be valid. :)

Can you add one more scenario for me?

subdomain.tla.com

gep13 · 2015-12-02T08:02:15Z

@ferventcoder ok, so that scenario results in a failed tests, which is what I think you thought would happen :-)

I will look at the regex, and see if there is anything that can be done.

gep13 · 2015-12-02T08:02:38Z

@ferventcoder I have added that scenario though, and pushed to this PR.

ferventcoder · 2015-12-02T14:38:33Z

ok, so that scenario results in a failed tests, which is what I think you thought would happen :-)

You caught me :D

gep13 · 2015-12-02T20:00:05Z

@ferventcoder what do you think of this change?

ferventcoder · 2015-12-02T20:07:54Z

...alidator/infrastructure.app/rules/IconUrlNotSameDomainAsProjectUrlDomainOrRawGitGuideline.cs

+                {
+                    domain = parts[1] + '.' + parts[0];
+
+                    if (host.ToLower().IndexOf(".co.uk") != -1 && parts.Count > 2)


is this the only domain like this?

https://en.wikipedia.org/wiki/.co_(second-level_domain)

https://en.wikipedia.org/wiki/Second-level_domain

http://stackoverflow.com/a/17886331/18475, but for real fun http://stackoverflow.com/q/10306690/18475

No, I didn't think it was, but I also didn't know of a clever way to encompass all the edge cases :-(

It's possible we just call this good enough.

I have one more approach that I want to try out, going to give it a whack at lunch time. If I don't get anywhere, I will fallback to what we have.

gep13 · 2015-12-03T13:17:57Z

@ferventcoder what do you think about this alternative approach?

ferventcoder · 2015-12-03T16:50:36Z

...tor.tests/infrastructure.app/IconUrlNotSameDomainAsProjectUrlDomainorRawGitGuidelineSpecs.cs

+        [Fact]
+        public void IsIconFromSameDomainAsProjectOrRawgit_should_return_true_from_rawgit_domain_with_single_level_top_level_domain()
+        {
+            Context();


Just noticed this. You don't need to call context. It is done automatically for you.

Oh really? I am fairly sure I saw this in some of the Chocolatey Tests, so I just copied it from there. But I will take this out, and give it a try. Cheers!

Ooooh. Yeah it was likely each of them was calling a reset to remove state.

"Under this context (arrange), because this happened (act), it should result in (assert)..."

Usually you set everything up in context, then because is the action (or the SUT), then all of the facts are just assertions. Sometimes you want to short circuit that because you have many, many scenarios that require little setup. I tend to prefer less ceremony because each arrange and act is usually an entire class with multiple assertions on that one action.

Gotcha, that makes sense now then.

I had been looking at this:

https://github.com/chocolatey/choco/blob/master/src/chocolatey.tests/infrastructure.app/services/FilesServiceSpecs.cs#L47

But this fits with exactly what you are describing.

Yes, that is one that is definitely a smaller setup. Would be a ton of spec classes for little benefit.

This is the norm - https://github.com/chocolatey/choco/blob/56677dfe853f9852b8292c9d4de4c7778dd6029e/src/chocolatey.tests/infrastructure.app/services/FilesServiceSpecs.cs#L135-L169

ferventcoder · 2015-12-03T16:55:04Z

...tor.tests/infrastructure.app/IconUrlNotSameDomainAsProjectUrlDomainorRawGitGuidelineSpecs.cs

+
+            result.ShouldBeTrue();
+        }
+    }


All trues, no falses. Need both sides of that.

Ah, good point! I guess by default I am a "happy path" kind of a guy 😄 I will add some failing steps as well.

gep13 · 2015-12-05T20:24:04Z

@ferventcoder I have fixed up with your comments, and re-pushed.

However... :-(

I started adding in the unhappy paths, as you suggested, and this highlighted an error in the logic. See the failing test.

Any ideas on best approach to go forward? It all comes down to not knowing the valid TLD's in order to move forward. I really don't want to have to start pulling in a complete list of TLD's and comparing against them, as these lists are always changing.

What are your thoughts?

ferventcoder · 2015-12-05T21:46:30Z

I'll need to pull in the PR to see the failing test.

gep13 · 2015-12-06T15:12:55Z

@ferventcoder did you have any suggestions about this one, or did you get pulled onto something else?

ferventcoder · 2015-12-06T17:37:00Z

I didn't see the failing test yet. Might be best to comment on the PR files where the failing test is.

ferventcoder · 2015-12-06T17:37:28Z

...tor.tests/infrastructure.app/IconUrlNotSameDomainAsProjectUrlDomainorRawGitGuidelineSpecs.cs

+        [Fact]
+        public void should_return_false_from_different_domain_and_double_level_top_level_domain()
+        {
+            guideline.is_icon_from_same_domain_as_project_or_rawgit("test1.co.uk", "test.co.uk").ShouldBeFalse();


Is this the failing test?

Yes.

And it is due to this line:

https://github.com/chocolatey/package-validator/pull/23/files#diff-947814a4acfa4b8c82b77c311b8656c3R48

I can change it to be > 3 but that then fails another test.

Dumb idea Monday - most double TLDs are both two letters.

I take it back. The last one is 2 letters. The other is either co or one of the well known ones like org, edu. We'd catch enough of the items, it's probably okay if a few false positives drop.

I had that idea on Dumb Idea Saturday, but wasn't sure if it was going to be a winner or not. Happy to proceed on that basis though.

I guess we can add some exemptions, as we see them start to come in.

ferventcoder · 2015-12-15T05:53:30Z

Is this ready to go?

gep13 · 2015-12-15T07:21:27Z

@ferventcoder no, this still has a failing test :-( I keep coming back to this one, and not liking the solutions that I come up with :-(

- To ensure that the IconUrl either comes from the same domain as the ProjectUrl, or comes from the RawGit CDN - Fixes chocolateyGH-14

- To cover the functionality of the get_domain_from_host method

gep13 force-pushed the Issue-14 branch from 5a9235b to b4647fa Compare November 26, 2015 21:04

ferventcoder reviewed Nov 28, 2015
View reviewed changes

ferventcoder changed the title ~~Adding additional IconUrl check~~ (GH-14) Adding additional IconUrl check Nov 28, 2015

ferventcoder reviewed Nov 29, 2015
View reviewed changes

gep13 force-pushed the Issue-14 branch 2 times, most recently from ce02888 to e070513 Compare November 29, 2015 21:22

ferventcoder reviewed Nov 30, 2015
View reviewed changes

ferventcoder reviewed Dec 1, 2015
View reviewed changes

gep13 force-pushed the Issue-14 branch from 3c75581 to 54e219f Compare December 1, 2015 20:37

gep13 force-pushed the Issue-14 branch from 54e219f to f36a76d Compare December 1, 2015 20:39

ferventcoder reviewed Dec 1, 2015
View reviewed changes

gep13 force-pushed the Issue-14 branch from f36a76d to 3148b8e Compare December 2, 2015 08:01

gep13 force-pushed the Issue-14 branch from 3148b8e to 2fce966 Compare December 2, 2015 19:59

ferventcoder reviewed Dec 2, 2015
View reviewed changes

ferventcoder reviewed Dec 3, 2015
View reviewed changes

gep13 force-pushed the Issue-14 branch 2 times, most recently from 56b9fd2 to 8217f67 Compare December 5, 2015 20:21

ferventcoder reviewed Dec 6, 2015
View reviewed changes

gep13 force-pushed the Issue-14 branch from 8217f67 to 3b1ca77 Compare December 7, 2015 22:08

gep13 force-pushed the Issue-14 branch from 3b1ca77 to 0b42e6d Compare January 7, 2016 14:36

gep13 added 2 commits May 6, 2016 22:02

(chocolateyGH-14) Adding IconUrl check

8715071

- To ensure that the IconUrl either comes from the same domain as the ProjectUrl, or comes from the RawGit CDN - Fixes chocolateyGH-14

(chocolateyGH-14) Adding tests

126077d

- To cover the functionality of the get_domain_from_host method

gep13 force-pushed the Issue-14 branch from 0b42e6d to 126077d Compare May 6, 2016 21:03


		var domain = iconGuideline.get_domain_from_host("somewildsubdomain.test.co.uk");

		domain.ShouldEqual("test.co.uk");

(GH-14) Adding additional IconUrl check #23

Are you sure you want to change the base?

(GH-14) Adding additional IconUrl check #23

Conversation

gep13 commented Nov 26, 2015

Choose a reason for hiding this comment

ferventcoder commented Nov 28, 2015

gep13 commented Nov 29, 2015

ferventcoder commented Nov 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gep13 commented Nov 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gep13 commented Dec 1, 2015

gep13 commented Dec 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gep13 commented Dec 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gep13 commented Dec 2, 2015

gep13 commented Dec 2, 2015

ferventcoder commented Dec 2, 2015

gep13 commented Dec 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gep13 commented Dec 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gep13 commented Dec 5, 2015

ferventcoder commented Dec 5, 2015

gep13 commented Dec 6, 2015

ferventcoder commented Dec 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferventcoder commented Dec 15, 2015

gep13 commented Dec 15, 2015