{"action":"opened","number":24,"pull_request":{"url":"https://api.github.com/repos/sakaicontrib/turnitin/pulls/24","id":78911904,"html_url":"https://github.com/sakaicontrib/turnitin/pull/24","diff_url":"https://github.com/sakaicontrib/turnitin/pull/24.diff","patch_url":"https://github.com/sakaicontrib/turnitin/pull/24.patch","issue_url":"https://api.github.com/repos/sakaicontrib/turnitin/issues/24","number":24,"state":"open","locked":false,"title":"TII-201 fix isUrlAccessed field updating, modify logic to allow or deny serving the file","user":{"login":"bjones86","id":10403943,"avatar_url":"https://avatars.githubusercontent.com/u/10403943?v=3","gravatar_id":"","url":"https://api.github.com/users/bjones86","html_url":"https://github.com/bjones86","followers_url":"https://api.github.com/users/bjones86/followers","following_url":"https://api.github.com/users/bjones86/following{/other_user}","gists_url":"https://api.github.com/users/bjones86/gists{/gist_id}","starred_url":"https://api.github.com/users/bjones86/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bjones86/subscriptions","organizations_url":"https://api.github.com/users/bjones86/orgs","repos_url":"https://api.github.com/users/bjones86/repos","events_url":"https://api.github.com/users/bjones86/events{/privacy}","received_events_url":"https://api.github.com/users/bjones86/received_events","type":"User","site_admin":false},"body":"https://jira.sakaiproject.org/browse/TII-201\r\n\r\nThere is a new contentreviewitem property called isUrlAccessed which controls whether or not a submission in the queue (file) can be retrieved by Turnitin. Before granting access to the file resource (in BaseAssignmentService.getHttpAccess()), it checks this property and returns early if it is true.\r\n\r\nAfter many, many iterations of testing and debugging, it seems that there's a race condition happening here.\r\n\r\nThe TII callback to retrieve the file happens very quickly, and sets the isUrlAccessed to true. Milliseconds later, the queue job continues on, and does some unrelated updates to the same contentReviewItem. Now, because the queue job is using the DAO to update the object, regardless of if it's explicitly setting the isUrlAccessed attribute or not, Hibernate will update this field to the default value.\r\n\r\nSo, TII gets the file, sets the isURLAccessed to true, then the queue job continues and sets it almost immediately back to false. In effect, this leaves the URLs to access these content review files open for anyone to access once more, as there is no authentication performed on these requests.\r\n\r\nThe solution to this, is marking the urlAccessed column as insert=true and update=false in the Hibernate mapping XML file. In this way, when creating a new contentReviewItem, the urlAccessed parameter will default to false, and any code updating this object via the DAO will reject any changes to this column.\r\n\r\nThen, in the one and only place we want to set the urlAccessed value to true (after TII has called us back and retrieved the file), we call a method that runs some custom HQL to manually update the value to true for the given row.\r\n\r\nWe've also added some extra obfuscation to these URLs because they are not authenticated. Currently, the URL could technically be determined by an attacker (with a combination of sleuthing/inspecting elements and brute force), as it contains the site ID, the submission ID, and the (positive, sequential integer) ID of the content review item.\r\n\r\nThe extra protection we've implemented, is by appending the hash value of the contentId to the access URL. Due to the fact that the content ID is a combination of two additional, non-user facing UUIDs and the actual file name and extension of the file in question, it is near impossible for an attacker to determine this string and it's subsequent hash code. When the URL is accessed, it will first check to see that the hash codes match, and short circuit if not.\r\n\r\nLastly, we've modified the conditional logic around actually serving up the file. Previously, the file was only served if and only if the isUrlAccessed property was false. However, due to the calls to and from TII being asynchronous, we cannot safely assume that the status of the item will be either 1 or 2 by the time TII attempts to retrieve the file. Depending on timing, it could very well be one or the other (coupled with the fact that we've seen TII make multiple requests to get the same file), and we need to allow TII to get the file in both of these circumstances.\r\n\r\nTherefore, we've determined that the file should be served in only the following circumstances:\r\n\r\n* if the URL has not yet been accessed\r\n* if the URL has been accessed, but the status is one of the following: 1 (not yet submitted), 2 (submitted, waiting for report), and 4 (submission error, will retry)\r\n\r\nOf course, the hashing protection and validation happens prior to checking these conditions, serving as both another layer of obfuscation as well as a short circuit for the process.","created_at":"2016-07-26T19:37:31Z","updated_at":"2016-07-26T19:37:31Z","closed_at":null,"merged_at":null,"merge_commit_sha":null,"assignee":null,"assignees":[],"milestone":null,"commits_url":"https://api.github.com/repos/sakaicontrib/turnitin/pulls/24/commits","review_comments_url":"https://api.github.com/repos/sakaicontrib/turnitin/pulls/24/comments","review_comment_url":"https://api.github.com/repos/sakaicontrib/turnitin/pulls/comments{/number}","comments_url":"https://api.github.com/repos/sakaicontrib/turnitin/issues/24/comments","statuses_url":"https://api.github.com/repos/sakaicontrib/turnitin/statuses/ac5ef5f71eae4e850fc63339999a4e0ba5d2646e","head":{"label":"bjones86:TII-201","ref":"TII-201","sha":"ac5ef5f71eae4e850fc63339999a4e0ba5d2646e","user":{"login":"bjones86","id":10403943,"avatar_url":"https://avatars.githubusercontent.com/u/10403943?v=3","gravatar_id":"","url":"https://api.github.com/users/bjones86","html_url":"https://github.com/bjones86","followers_url":"https://api.github.com/users/bjones86/followers","following_url":"https://api.github.com/users/bjones86/following{/other_user}","gists_url":"https://api.github.com/users/bjones86/gists{/gist_id}","starred_url":"https://api.github.com/users/bjones86/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bjones86/subscriptions","organizations_url":"https://api.github.com/users/bjones86/orgs","repos_url":"https://api.github.com/users/bjones86/repos","events_url":"https://api.github.com/users/bjones86/events{/privacy}","received_events_url":"https://api.github.com/users/bjones86/received_events","type":"User","site_admin":false},"repo":{"id":63079865,"name":"turnitin","full_name":"bjones86/turnitin","owner":{"login":"bjones86","id":10403943,"avatar_url":"https://avatars.githubusercontent.com/u/10403943?v=3","gravatar_id":"","url":"https://api.github.com/users/bjones86","html_url":"https://github.com/bjones86","followers_url":"https://api.github.com/users/bjones86/followers","following_url":"https://api.github.com/users/bjones86/following{/other_user}","gists_url":"https://api.github.com/users/bjones86/gists{/gist_id}","starred_url":"https://api.github.com/users/bjones86/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bjones86/subscriptions","organizations_url":"https://api.github.com/users/bjones86/orgs","repos_url":"https://api.github.com/users/bjones86/repos","events_url":"https://api.github.com/users/bjones86/events{/privacy}","received_events_url":"https://api.github.com/users/bjones86/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/bjones86/turnitin","description":"Content Review integration for Turnitin","fork":true,"url":"https://api.github.com/repos/bjones86/turnitin","forks_url":"https://api.github.com/repos/bjones86/turnitin/forks","keys_url":"https://api.github.com/repos/bjones86/turnitin/keys{/key_id}","collaborators_url":"https://api.github.com/repos/bjones86/turnitin/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/bjones86/turnitin/teams","hooks_url":"https://api.github.com/repos/bjones86/turnitin/hooks","issue_events_url":"https://api.github.com/repos/bjones86/turnitin/issues/events{/number}","events_url":"https://api.github.com/repos/bjones86/turnitin/events","assignees_url":"https://api.github.com/repos/bjones86/turnitin/assignees{/user}","branches_url":"https://api.github.com/repos/bjones86/turnitin/branches{/branch}","tags_url":"https://api.github.com/repos/bjones86/turnitin/tags","blobs_url":"https://api.github.com/repos/bjones86/turnitin/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/bjones86/turnitin/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/bjones86/turnitin/git/refs{/sha}","trees_url":"https://api.github.com/repos/bjones86/turnitin/git/trees{/sha}","statuses_url":"https://api.github.com/repos/bjones86/turnitin/statuses/{sha}","languages_url":"https://api.github.com/repos/bjones86/turnitin/languages","stargazers_url":"https://api.github.com/repos/bjones86/turnitin/stargazers","contributors_url":"https://api.github.com/repos/bjones86/turnitin/contributors","subscribers_url":"https://api.github.com/repos/bjones86/turnitin/subscribers","subscription_url":"https://api.github.com/repos/bjones86/turnitin/subscription","commits_url":"https://api.github.com/repos/bjones86/turnitin/commits{/sha}","git_commits_url":"https://api.github.com/repos/bjones86/turnitin/git/commits{/sha}","comments_url":"https://api.github.com/repos/bjones86/turnitin/comments{/number}","issue_comment_url":"https://api.github.com/repos/bjones86/turnitin/issues/comments{/number}","contents_url":"https://api.github.com/repos/bjones86/turnitin/contents/{+path}","compare_url":"https://api.github.com/repos/bjones86/turnitin/compare/{base}...{head}","merges_url":"https://api.github.com/repos/bjones86/turnitin/merges","archive_url":"https://api.github.com/repos/bjones86/turnitin/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/bjones86/turnitin/downloads","issues_url":"https://api.github.com/repos/bjones86/turnitin/issues{/number}","pulls_url":"https://api.github.com/repos/bjones86/turnitin/pulls{/number}","milestones_url":"https://api.github.com/repos/bjones86/turnitin/milestones{/number}","notifications_url":"https://api.github.com/repos/bjones86/turnitin/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/bjones86/turnitin/labels{/name}","releases_url":"https://api.github.com/repos/bjones86/turnitin/releases{/id}","deployments_url":"https://api.github.com/repos/bjones86/turnitin/deployments","created_at":"2016-07-11T15:30:59Z","updated_at":"2016-07-13T12:33:50Z","pushed_at":"2016-07-26T19:36:10Z","git_url":"git://github.com/bjones86/turnitin.git","ssh_url":"git@github.com:bjones86/turnitin.git","clone_url":"https://github.com/bjones86/turnitin.git","svn_url":"https://github.com/bjones86/turnitin","homepage":null,"size":1005,"stargazers_count":1,"watchers_count":1,"language":"Java","has_issues":false,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":0,"mirror_url":null,"open_issues_count":0,"forks":0,"open_issues":0,"watchers":1,"default_branch":"master"}},"base":{"label":"sakaicontrib:master","ref":"master","sha":"6f7a520ad182499974ade9316e2816aec6d8d4f0","user":{"login":"sakaicontrib","id":16902267,"avatar_url":"https://avatars.githubusercontent.com/u/16902267?v=3","gravatar_id":"","url":"https://api.github.com/users/sakaicontrib","html_url":"https://github.com/sakaicontrib","followers_url":"https://api.github.com/users/sakaicontrib/followers","following_url":"https://api.github.com/users/sakaicontrib/following{/other_user}","gists_url":"https://api.github.com/users/sakaicontrib/gists{/gist_id}","starred_url":"https://api.github.com/users/sakaicontrib/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sakaicontrib/subscriptions","organizations_url":"https://api.github.com/users/sakaicontrib/orgs","repos_url":"https://api.github.com/users/sakaicontrib/repos","events_url":"https://api.github.com/users/sakaicontrib/events{/privacy}","received_events_url":"https://api.github.com/users/sakaicontrib/received_events","type":"Organization","site_admin":false},"repo":{"id":36799378,"name":"turnitin","full_name":"sakaicontrib/turnitin","owner":{"login":"sakaicontrib","id":16902267,"avatar_url":"https://avatars.githubusercontent.com/u/16902267?v=3","gravatar_id":"","url":"https://api.github.com/users/sakaicontrib","html_url":"https://github.com/sakaicontrib","followers_url":"https://api.github.com/users/sakaicontrib/followers","following_url":"https://api.github.com/users/sakaicontrib/following{/other_user}","gists_url":"https://api.github.com/users/sakaicontrib/gists{/gist_id}","starred_url":"https://api.github.com/users/sakaicontrib/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sakaicontrib/subscriptions","organizations_url":"https://api.github.com/users/sakaicontrib/orgs","repos_url":"https://api.github.com/users/sakaicontrib/repos","events_url":"https://api.github.com/users/sakaicontrib/events{/privacy}","received_events_url":"https://api.github.com/users/sakaicontrib/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/sakaicontrib/turnitin","description":"Content Review integration for Turnitin","fork":false,"url":"https://api.github.com/repos/sakaicontrib/turnitin","forks_url":"https://api.github.com/repos/sakaicontrib/turnitin/forks","keys_url":"https://api.github.com/repos/sakaicontrib/turnitin/keys{/key_id}","collaborators_url":"https://api.github.com/repos/sakaicontrib/turnitin/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/sakaicontrib/turnitin/teams","hooks_url":"https://api.github.com/repos/sakaicontrib/turnitin/hooks","issue_events_url":"https://api.github.com/repos/sakaicontrib/turnitin/issues/events{/number}","events_url":"https://api.github.com/repos/sakaicontrib/turnitin/events","assignees_url":"https://api.github.com/repos/sakaicontrib/turnitin/assignees{/user}","branches_url":"https://api.github.com/repos/sakaicontrib/turnitin/branches{/branch}","tags_url":"https://api.github.com/repos/sakaicontrib/turnitin/tags","blobs_url":"https://api.github.com/repos/sakaicontrib/turnitin/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/sakaicontrib/turnitin/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/sakaicontrib/turnitin/git/refs{/sha}","trees_url":"https://api.github.com/repos/sakaicontrib/turnitin/git/trees{/sha}","statuses_url":"https://api.github.com/repos/sakaicontrib/turnitin/statuses/{sha}","languages_url":"https://api.github.com/repos/sakaicontrib/turnitin/languages","stargazers_url":"https://api.github.com/repos/sakaicontrib/turnitin/stargazers","contributors_url":"https://api.github.com/repos/sakaicontrib/turnitin/contributors","subscribers_url":"https://api.github.com/repos/sakaicontrib/turnitin/subscribers","subscription_url":"https://api.github.com/repos/sakaicontrib/turnitin/subscription","commits_url":"https://api.github.com/repos/sakaicontrib/turnitin/commits{/sha}","git_commits_url":"https://api.github.com/repos/sakaicontrib/turnitin/git/commits{/sha}","comments_url":"https://api.github.com/repos/sakaicontrib/turnitin/comments{/number}","issue_comment_url":"https://api.github.com/repos/sakaicontrib/turnitin/issues/comments{/number}","contents_url":"https://api.github.com/repos/sakaicontrib/turnitin/contents/{+path}","compare_url":"https://api.github.com/repos/sakaicontrib/turnitin/compare/{base}...{head}","merges_url":"https://api.github.com/repos/sakaicontrib/turnitin/merges","archive_url":"https://api.github.com/repos/sakaicontrib/turnitin/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/sakaicontrib/turnitin/downloads","issues_url":"https://api.github.com/repos/sakaicontrib/turnitin/issues{/number}","pulls_url":"https://api.github.com/repos/sakaicontrib/turnitin/pulls{/number}","milestones_url":"https://api.github.com/repos/sakaicontrib/turnitin/milestones{/number}","notifications_url":"https://api.github.com/repos/sakaicontrib/turnitin/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/sakaicontrib/turnitin/labels{/name}","releases_url":"https://api.github.com/repos/sakaicontrib/turnitin/releases{/id}","deployments_url":"https://api.github.com/repos/sakaicontrib/turnitin/deployments","created_at":"2015-06-03T11:30:08Z","updated_at":"2016-07-13T12:33:54Z","pushed_at":"2016-07-26T19:37:31Z","git_url":"git://github.com/sakaicontrib/turnitin.git","ssh_url":"git@github.com:sakaicontrib/turnitin.git","clone_url":"https://github.com/sakaicontrib/turnitin.git","svn_url":"https://github.com/sakaicontrib/turnitin","homepage":null,"size":1537,"stargazers_count":1,"watchers_count":1,"language":"Java","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":10,"mirror_url":null,"open_issues_count":1,"forks":10,"open_issues":1,"watchers":1,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/sakaicontrib/turnitin/pulls/24"},"html":{"href":"https://github.com/sakaicontrib/turnitin/pull/24"},"issue":{"href":"https://api.github.com/repos/sakaicontrib/turnitin/issues/24"},"comments":{"href":"https://api.github.com/repos/sakaicontrib/turnitin/issues/24/comments"},"review_comments":{"href":"https://api.github.com/repos/sakaicontrib/turnitin/pulls/24/comments"},"review_comment":{"href":"https://api.github.com/repos/sakaicontrib/turnitin/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/sakaicontrib/turnitin/pulls/24/commits"},"statuses":{"href":"https://api.github.com/repos/sakaicontrib/turnitin/statuses/ac5ef5f71eae4e850fc63339999a4e0ba5d2646e"}},"merged":false,"mergeable":null,"mergeable_state":"unknown","merged_by":null,"comments":0,"review_comments":0,"commits":1,"additions":42,"deletions":28,"changed_files":5}}
{"actor":{"display_login":"bjones86"}}