PHI permission Enhancements #877

hkethi002 · 2017-07-26T21:59:42Z

Fixes #869

Changes Made

New phi-access permission property blacklists phi fields (This is a frontend change)
- phi-access is a required property for every permission POST except at group level, defaults to true
phi flag can be used to return PHI fields on get_all calls

		phi=false/missing	phi=true/superuser invoked
phi=true	get_all	403	phi returned
phi=false/missing	get_all	phi not returned	phi not returned
phi=true	get	phi not returned	phi returned
phi=false/missing	get	phi not returned	phi returned*

* This is to support existing behavior

This PR has not applied PHI permissions to POST and PUT methods on containers

Review Checklist

Tests were added to cover all code changes
Documentation was added / updated
Code and tests follow standards in CONTRIBUTING.md

ryansanford · 2017-08-15T15:39:41Z

Confirmed today that the scope of this PR does not include ability to configure which fields are impacted.

kofalt · 2017-08-15T15:40:24Z

I'll need to hop on this to make sure the SDK will work with this change.

codecov-io · 2017-09-27T14:07:42Z

Codecov Report

Merging #877 into master will increase coverage by <.01%.
The diff coverage is 87.12%.

@@            Coverage Diff             @@
##           master     #877      +/-   ##
==========================================
+ Coverage   90.64%   90.64%   +<.01%     
==========================================
  Files          50       50              
  Lines        6763     6854      +91     
==========================================
+ Hits         6130     6213      +83     
- Misses        633      641       +8

kofalt · 2017-09-27T17:51:49Z

Questions:

How can I tell the difference between a field that was censored, and a field set to ***?
Any objection to changing no-phi-ro to ro-no-phi? Seems more logical.
You mention that PHI is returned on a get_all call when you have > ro-no-phi access. Can you give an example?

hkethi002 · 2017-09-27T17:54:59Z

We are removing ***, soon to be pushed
Nope
If your access is ro, and the phi param is set to true on a get_all, the PHI will be returned and logged.

nagem · 2017-09-29T15:44:27Z

api/auth/containerauth.py

            else:
                has_access = False

+            if method == 'GET' and exec_op is not noop:


What is the noop clause for? Usually we'll pass a noop when we want to see if the user had permission to do something but don't want to actually to follow through with the action yet.

nagem · 2017-09-29T15:55:02Z

api/auth/containerauth.py


+            if method == 'GET' and exec_op is not noop:
+                handler.phi = _get_access(handler.uid, container) > INTEGER_PERMISSIONS['ro-no-phi']
+                if not handler.phi:


For organizational purposes we should set the projection and view the query param in the handler logic. Then we can just pass a PHI=true/false to this method instead.

nagem · 2017-09-29T16:00:16Z

api/handlers/containerhandler.py

            result = self.handle_origin(result)
            modified_results.append(result)
+            if self.is_true('phi'):
+                self.log_user_access(AccessType.view_container, cont_name, result.get('_id'))


We'll probably want to create a more efficient way to call this with a list. Right now it will recreate the hierarchy from this container up. If this level is acquisitions, it means grabbing the same session-project-group from the db for each acquisition, even when it's the same session-project-group.

nagem · 2017-09-29T16:01:58Z

test/integration_tests/python/test_access_log.py

+
    r = as_admin.get('/projects/' + project)
    assert r.ok
    assert r.json()['files']


Why did this change order?

If I remember correctly, it had to do with some persistence of the validator when the test failed before the moved line that made it difficult to understand the underlying error.

nagem · 2017-10-02T18:42:21Z

I see some tests were added, mind adding a few more to get diff coverage closer to 100%? Looks like some of the missed lines involved PHI permissions, covering those lines are most important.

nagem · 2017-10-11T18:57:31Z

api/handlers/containerhandler.py

-            projection.pop('info')
+        projection = None
+        # if self.is_true('info'): Seems redundant with new phi functionality
+        #     projection.pop('info')


This can be removed

ryansanford · 2017-10-24T16:02:04Z

I need to create an instance tracking this branch for integration testing.

ryansanford · 2017-11-14T15:08:37Z

Was it intentional for phi-access to be a required parameter on group POST? Could we get a summary in PR description where this is to be required, and what the default is where it is not required?

2017-11-14 14:41:22      scitran.api                  base.py    54:INFO Initialized request request_id=d505f2d1-1510670482
2017-11-14 14:41:22      scitran.api                  base.py   340:WARN u'phi-access' is a required property

Failed validating u'required' in schema:
    {u'$schema': u'http://json-schema.org/draft-04/schema#',
     u'allOf': [{u'$ref': u'../definitions/permission.json#/definitions/permission'}],
     u'key_fields': [u'_id'],
     u'required': [u'_id', u'access', u'phi-access'],
     u'type': u'object'}

On instance:
    {u'_id': u'ci@flywheel.io', u'access': u'admin'} request_id=d505f2d1-1510670482
[pid: 23|app: 0|req: 28/58] 172.20.0.11 () {40 vars in 576 bytes} [Tue Nov 14 14:41:22 2017] POST /api/groups/scitran/permissions => generated 447 bytes in 14 msecs (HTTP/1.1 400) 3 headers in 121 bytes 
(2 switches on core 1)

hkethi002 · 2017-11-15T15:45:24Z

Having the phi-access property at the group level would simplify the propagation logic and allow projects and groups to keep the same permission logic. However I am not sure if that would complicate the frontend more than its worth, so I can change it if it would make more sense to only require it at the project and collection level permissions.
Made change in #991 but group perm post no longer requires phi-access, defaults to True

hkethi002 · 2017-11-15T15:47:27Z

Also, it may be better to track #991 as it is branched off of this PR and is ready for review.

nagem · 2018-02-19T22:24:41Z

Closing PR for now as requirements for this enhancement are reworked within the team.

hkethi002 requested a review from nagem July 26, 2017 21:59

hkethi002 force-pushed the PHI-perm-three branch 2 times, most recently from 49112df to 0b6e126 Compare August 2, 2017 18:17

hkethi002 force-pushed the PHI-perm-three branch from 0b6e126 to 6d5995b Compare August 9, 2017 18:26

kofalt self-assigned this Aug 15, 2017

kofalt added the Breaking Change label Aug 15, 2017

hkethi002 force-pushed the PHI-perm-three branch from 6d5995b to 19177b4 Compare August 18, 2017 20:49

hkethi002 force-pushed the PHI-perm-three branch from 19177b4 to 6e06632 Compare September 27, 2017 14:00

kofalt self-requested a review September 27, 2017 16:53

nagem reviewed Sep 29, 2017

View reviewed changes

hkethi002 force-pushed the PHI-perm-three branch from 075e5b2 to 17fb182 Compare October 2, 2017 17:13

nagem reviewed Oct 11, 2017

View reviewed changes

hkethi002 force-pushed the PHI-perm-three branch from bd74c4b to ba7c145 Compare October 11, 2017 22:08

ryansanford self-assigned this Oct 24, 2017

hkethi002 force-pushed the PHI-perm-three branch 3 times, most recently from 9d1743e to f9244ec Compare October 25, 2017 21:58

hkethi002 force-pushed the PHI-perm-three branch from 0664339 to ab677d5 Compare November 1, 2017 15:33

hkethi002 mentioned this pull request Nov 10, 2017

Custom PHI lists at site/project level #991

Closed

5 tasks

hkethi002 force-pushed the PHI-perm-three branch 2 times, most recently from f999ae5 to 2e81556 Compare November 13, 2017 15:09

hkethi002 force-pushed the PHI-perm-three branch from 400bfa2 to 79cae60 Compare December 4, 2017 21:27

hkethi002 added 12 commits December 21, 2017 10:44

loggin projects

5899926

new no-phi-ro access level

1f0ca26

phi flag and logging implemented on containers

b86460f

analyses follow phi rules

ceae24e

Renamed permission; blacklist PHI instead of scrub

8a54e92

reorganized phi logic into handlers

ea8cbc2

tests for collections added

cb8e2da

removed ro-no-phi and added no-phi boolean to permission

9ef9f4d

Removed commented code; phi for get_all_for_user

e125f9e

no-phi -> phi-access; get_ses/acq on collections use phi

d9d019e

Tested root access to phi fields

2dcc6e9

self.PHI for collections removing files.info projection

3d1438b

hkethi002 force-pushed the PHI-perm-three branch from 79cae60 to af4f116 Compare December 21, 2017 16:45

hkethi002 added 4 commits December 21, 2017 10:50

self.PHI is not modified

dae084c

allow additional properties on output schemas

82e1dc7

Propagating permissions of only one field of permission works

c6d7347

added parent to analysis schema

b4432f6

hkethi002 force-pushed the PHI-perm-three branch from af4f116 to 406ac2d Compare December 21, 2017 17:00

collection get_sessions uses SessionStorage

406ac2d

nagem closed this Feb 19, 2018

PHI permission Enhancements #877

PHI permission Enhancements #877

Uh oh!

Conversation

hkethi002 commented Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes Made

Review Checklist

Uh oh!

ryansanford commented Aug 15, 2017

Uh oh!

kofalt commented Aug 15, 2017

Uh oh!

codecov-io commented Sep 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kofalt commented Sep 27, 2017

Uh oh!

hkethi002 commented Sep 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nagem Sep 29, 2017

Choose a reason for hiding this comment

Uh oh!

nagem Sep 29, 2017

Choose a reason for hiding this comment

Uh oh!

nagem Sep 29, 2017

Choose a reason for hiding this comment

Uh oh!

nagem Sep 29, 2017

Choose a reason for hiding this comment

Uh oh!

hkethi002 Oct 2, 2017

Choose a reason for hiding this comment

Uh oh!

nagem commented Oct 2, 2017

Uh oh!

nagem Oct 11, 2017

Choose a reason for hiding this comment

Uh oh!

ryansanford commented Oct 24, 2017

Uh oh!

ryansanford commented Nov 14, 2017

Uh oh!

hkethi002 commented Nov 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hkethi002 commented Nov 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nagem commented Feb 19, 2018

Uh oh!

Uh oh!

hkethi002 commented Jul 26, 2017 •

edited

Loading

codecov-io commented Sep 27, 2017 •

edited

Loading

hkethi002 commented Sep 27, 2017 •

edited

Loading

hkethi002 commented Nov 15, 2017 •

edited

Loading

hkethi002 commented Nov 15, 2017 •

edited

Loading