Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for validating FDL links from custom domains. #1962

Merged
merged 10 commits into from Nov 12, 2018
Merged

Conversation

@dmandar
Copy link
Contributor

@dmandar dmandar commented Oct 16, 2018

This feature is not yet available for public consumption.

Copy link
Member

@paulb777 paulb777 left a comment

Please address the two failing unit tests on travis:

https://travis-ci.org/firebase/firebase-ios-sdk/jobs/442453702

Looks like an extra call to [FIRApp configure]?
Done.

FIRDynamicLinksTest
testInvalidCustomDomainNames, failed: caught "com.firebase.core", "Default app has already been configured."
/Users/travis/build/firebase/firebase-ios-sdk/Example/DynamicLinks/Tests/FIRDynamicLinksTest.m:1054

[FIRApp configure];
NSArray<NSString *> *urlStrings = @[

testValidCustomDomainNames, failed: caught "com.firebase.core", "Default app has already been configured."
/Users/travis/build/firebase/firebase-ios-sdk/Example/DynamicLinks/Tests/FIRDynamicLinksTest.m:1027

[FIRApp configure];
NSArray<NSString *> *urlStrings = @[
 Executed 129 tests, with 2 failures (2 unexpected) in 1.578 (1.952) seconds

2018-10-17 00:56:17.268 xcodebuild[8624:17626] [MT] IDETestOperationsObserverDebug: 157.318 elapsed -- Testing started completed.
2018-10-17 00:56:17.268 xcodebuild[8624:17626] [MT] IDETestOperationsObserverDebug: 0.000 sec, +0.000 sec -- start
2018-10-17 00:56:17.268 xcodebuild[8624:17626] [MT] IDETestOperationsObserverDebug: 157.318 sec, +157.318 sec -- end
Failing tests:
-[FIRDynamicLinksTest testInvalidCustomDomainNames]
-[FIRDynamicLinksTest testValidCustomDomainNames]
** TEST FAILED **

mh6o
Copy link

mh6o commented on 8656130 Oct 17, 2018

Always include the scheme. This string with scheme will correspond to that in FDL console.

mh6o
Copy link

mh6o commented on 8656130 Oct 17, 2018

Why this internal go/ link?

mh6o
Copy link

mh6o commented on 8656130 Oct 17, 2018

Remove all URLs of g.co, https://go, https://g.co, and alikes. While some are correctly expressed, this is not really an FDL (not a realistic example for test). My comment applies to rest (there are several of the same occurrences).

mh6o
Copy link

mh6o commented on 8656130 Oct 17, 2018

I think it's reasonable to have an upper bound of 5 (the max in a project). That'd also avoid running into dev specifying arbitrarily many URLs.

mh6o
Copy link

mh6o commented on 8656130 Oct 17, 2018

Can enforce it no dups instead? That is, throw a terminal exception, than fail silently (which can imply that >5 domains are allowed).

mh6o
Copy link

mh6o commented on 8656130 Oct 17, 2018

Do you need the last condition? FIRDLAddToWhiteListForCustomDomainsArray() seems to run safe with with 0 size array.

mh6o
Copy link

mh6o commented on 8656130 Oct 17, 2018

nit, early return true

mh6o
Copy link

mh6o commented on 8656130 Oct 17, 2018

nit, return false ; you can avoid have customDomainMatchFound variable altogether.

mh6o
Copy link

mh6o commented on 8656130 Oct 17, 2018

Pls ensure that given a whitelist https://foo.bar/stuff , it can match short FDL https://foo.bar/stuff/suffix & long FDL https://foo.bar/stuff?link=...

mh6o
Copy link

mh6o commented on 8656130 Oct 17, 2018

Pls see CIL.

dmandar
Copy link
Contributor

dmandar commented on 8656130 Oct 17, 2018

Yes, this is in the test app on purpose to make sure it is not treated as a valid entry.

dmandar
Copy link
Contributor

dmandar commented on 8656130 Oct 17, 2018

These are only test entries (many of them invalid)

dmandar
Copy link
Contributor

dmandar commented on 8656130 Oct 17, 2018

These are test entries (have used domains that google owns to test).

dmandar
Copy link
Contributor

dmandar commented on 8656130 Oct 17, 2018

Sure. seems redundant..will remove.

dmandar
Copy link
Contributor

dmandar commented on 8656130 Oct 17, 2018

Does the limit of 5 include all possible paths for a given subdomain as well? e.g. https://custom.com/wlpath1 and https://custom.com/wlpath2

dmandar
Copy link
Contributor

dmandar commented on 8656130 Oct 17, 2018

Sure. we can check the counts here and do that.

mh6o
Copy link

mh6o commented on 8656130 Oct 17, 2018

Do the test call it out that they're invalid? Are you expecting a failure?

mh6o
Copy link

mh6o commented on 8656130 Oct 17, 2018

Should be invalid (without scheme)?

mh6o
Copy link

mh6o commented on 8656130 Oct 17, 2018

Replied.

dmandar
Copy link
Contributor

dmandar commented on 8656130 Nov 9, 2018

Yes, these should be weeded out. Also replacing this entry with google

dmandar
Copy link
Contributor

dmandar commented on 8656130 Nov 9, 2018

Replaced.

dmandar
Copy link
Contributor

dmandar commented on 8656130 Nov 9, 2018

Correct. This test is testing for all invalid custom domain names.

dmandar
Copy link
Contributor

dmandar commented on 8656130 Nov 9, 2018

Yes, pls see testValidCustomDomainNames test above.

@dmandar dmandar self-assigned this Nov 10, 2018
Copy link
Contributor Author

@dmandar dmandar left a comment

Fixed. PTAL.

<string>https://mydomain.com</string>
<string>https://mydomain2.com</string>
<string>https://google.com</string>
<string>https://google.com</string>
Copy link
Member

@paulb777 paulb777 Nov 12, 2018

duplicate intentional?

&lt;string&gt;https://mydomain.com&lt;/string&gt;
&lt;string&gt;https://mydomain2.com&lt;/string&gt;
&lt;string&gt;https://google.com&lt;/string&gt;
&lt;string&gt;https://google.com&lt;/string&gt;
Copy link
Member

@paulb777 paulb777 Nov 12, 2018

duplicate

@dmandar dmandar merged commit bf1ce88 into master Nov 12, 2018
2 checks passed
@paulb777 paulb777 deleted the md-fdl branch Mar 20, 2019
@firebase firebase locked and limited conversation to collaborators Oct 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants