{"action":"created","issue":{"url":"https://api.github.com/repos/datatheorem/TrustKit/issues/159","repository_url":"https://api.github.com/repos/datatheorem/TrustKit","labels_url":"https://api.github.com/repos/datatheorem/TrustKit/issues/159/labels{/name}","comments_url":"https://api.github.com/repos/datatheorem/TrustKit/issues/159/comments","events_url":"https://api.github.com/repos/datatheorem/TrustKit/issues/159/events","html_url":"https://github.com/datatheorem/TrustKit/pull/159","id":302453261,"number":159,"title":"Support custom reporting class in configuration","user":{"login":"Jakenberg","id":1630090,"avatar_url":"https://avatars1.githubusercontent.com/u/1630090?v=4","gravatar_id":"","url":"https://api.github.com/users/Jakenberg","html_url":"https://github.com/Jakenberg","followers_url":"https://api.github.com/users/Jakenberg/followers","following_url":"https://api.github.com/users/Jakenberg/following{/other_user}","gists_url":"https://api.github.com/users/Jakenberg/gists{/gist_id}","starred_url":"https://api.github.com/users/Jakenberg/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Jakenberg/subscriptions","organizations_url":"https://api.github.com/users/Jakenberg/orgs","repos_url":"https://api.github.com/users/Jakenberg/repos","events_url":"https://api.github.com/users/Jakenberg/events{/privacy}","received_events_url":"https://api.github.com/users/Jakenberg/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":3,"created_at":"2018-03-05T20:39:03Z","updated_at":"2018-03-12T21:51:29Z","closed_at":null,"author_association":"NONE","pull_request":{"url":"https://api.github.com/repos/datatheorem/TrustKit/pulls/159","html_url":"https://github.com/datatheorem/TrustKit/pull/159","diff_url":"https://github.com/datatheorem/TrustKit/pull/159.diff","patch_url":"https://github.com/datatheorem/TrustKit/pull/159.patch"},"body":"Hello! First and foremost, thank you very much for making this excellent library.\r\n\r\nIn this PR, we propose a new feature: custom pin failure reporting.\r\n\r\n# Use Case\r\n\r\nOur reporting server is strict about the JSON format it will receive. It cannot be updated by us because it's a 3rd-party service (i.e. Airbrake). Using this PR, we are able to customize the format that will be sent to our reporting URI, thus conforming to what our 3rd-party service expects.\r\n\r\n# Implementation\r\n\r\n- Define new configuration key: `kTSKPinFailureReportClassName`.\r\n- When custom reporting class is used, throw an exception _unless_ it inherits from `TSKPinFailureReport`.\r\n- When no custom reporting class is used, default to using original `TSKPinFailureReport`.\r\n- Write a test confirming the intended usage behavior for `kTSKPinFailureReportClassName`.\r\n- Add \"Custom Pin Failure Reporting\" section in `docs/getting-started.md`.\r\n\r\nGod speed 🖖"},"comment":{"url":"https://api.github.com/repos/datatheorem/TrustKit/issues/comments/372474983","html_url":"https://github.com/datatheorem/TrustKit/pull/159#issuecomment-372474983","issue_url":"https://api.github.com/repos/datatheorem/TrustKit/issues/159","id":372474983,"user":{"login":"Jakenberg","id":1630090,"avatar_url":"https://avatars1.githubusercontent.com/u/1630090?v=4","gravatar_id":"","url":"https://api.github.com/users/Jakenberg","html_url":"https://github.com/Jakenberg","followers_url":"https://api.github.com/users/Jakenberg/followers","following_url":"https://api.github.com/users/Jakenberg/following{/other_user}","gists_url":"https://api.github.com/users/Jakenberg/gists{/gist_id}","starred_url":"https://api.github.com/users/Jakenberg/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Jakenberg/subscriptions","organizations_url":"https://api.github.com/users/Jakenberg/orgs","repos_url":"https://api.github.com/users/Jakenberg/repos","events_url":"https://api.github.com/users/Jakenberg/events{/privacy}","received_events_url":"https://api.github.com/users/Jakenberg/received_events","type":"User","site_admin":false},"created_at":"2018-03-12T21:51:29Z","updated_at":"2018-03-12T21:51:29Z","author_association":"NONE","body":"@adamkaplan:\r\n\r\n> Why would this need to be configured on a per-domain basis?\r\n\r\nThis PR doesn't actually allow this. `TSKPinFailureReportClassName` is declared as a top-level key; higher-order than `TSKPinnedDomains`.\r\n\r\n>  ...it could be easily provided programmatically rather than by reflection.\r\n\r\nIndeed. We went with this approach initially, but realized it wasn't compatible with the ideology of configuring TrustKit from a plist file. Frankly, we are neutral on either approach.\r\n\r\n> ...generate JSON reports via class method without new allocation each time... that would let you pass an actual instance instead of Class.\r\n\r\nWe disagree. In `TSKPinFailureReport`, only the `json` method is called, but it's still useful as a vehicle of data (i.e. bundle ID, app version, etc) to be passed/read from by other objects ([example](https://github.com/datatheorem/TrustKit/blob/master/TrustKit/Reporting/TSKReportsRateLimiter.m#L44)).\r\n\r\n-----------\r\n\r\n@nabla-c0d3:\r\n\r\n@adamkaplan is right about all-or-nothing customization. We wanted to keep all of the battle-tested reporting functionality that's already there while simply customizing the format of the JSON payload sent to our reporting URLs.\r\n\r\n> ...avoid adding yet another configuration key and more complexity...\r\n\r\nThis isn't a required customization key. In fact, there's no mention of `TSKPinFailureReportClassName` excluding the excerpt we wrote for it in the docs. For this reason we do not believe this adds any unnecessary complexity for the implementor unless they intentionally seek it out.\r\n\r\n> ...the callback feature should give you all the flexibility you need.\r\n\r\nWe could update the callback, but we found some limitations:\r\n\r\n- [`sendValidationReport:` is called on a different thread than `pinningValidatorCallback`](https://github.com/datatheorem/TrustKit/blob/master/TrustKit/TrustKit.m#L178-L188).\r\n- `pinningValidatorCallback` would need to be extended. For example, it's return value could be any instance of `TSKPinFailureReportClass`, which is then used in `sendValidationReport`.\r\n\r\nThere is likely more to it than this, but these two raise an interesting point. Primarily due to the second point being a breaking change, we don't believe this would be a better approach than what we've done in this PR.\r\n\r\n-----------\r\n\r\nAfter a week of using this PR, we have had excellent results. We wouldn't be utilizing the full capability of this library without it so we intend to continue using it. However, it's obviously up to you whether or not you believe we are the only implementors of this library out there that want customization _only_ for their failure report schema. We think it's highly unlikely that we're the only ones ;) \r\n\r\nThank you for taking time out to discuss this PR with us!"}}
{"actor":{"display_login":"Jakenberg"}}