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

AuthzPolicyEnforcement changes #2259

Merged
merged 1 commit into from
Apr 21, 2021
Merged

AuthzPolicyEnforcement changes #2259

merged 1 commit into from
Apr 21, 2021

Conversation

lmsurpre
Copy link
Member

@lmsurpre lmsurpre commented Apr 21, 2021

  1. getPatientRefValue now uses ReferenceUtil to obtain the id of the
    target Patient being referenced. This technique matches what we do
    during the extraction of the compartment membership param and is more
    robust to special reference values like versioned references or absolute
    references that match the base server url.

  2. getPatientIdFromToken will now split the patient_id claim on spaces.
    This sets us up for scenarios where a single token can grant access to
    multiple patient records. Additionally, I removed the log statement
    about expecting patient_id to be a string and not a list.

  3. Patient search is now allowed. Because we convert all searches into
    compartment searches, there is no longer a risk of exposing whether a
    given Patient resource by a given id exists or not. Therefore, we don't
    need to prevent such searches any longer. That said, because a Patient
    is not in its own compartment (unless it has a corresponding
    Patient.link), Patient search behavior will likely to be confusing to
    clients and so operators may want to continue to disallow Patient search
    from the REST layer in select deployments.

Signed-off-by: Lee Surprenant lmsurpre@us.ibm.com

1. getPatientRefValue now uses ReferenceUtil to obtain the id of the
target Patient being referenced. This technique matches what we do
during the extraction of the compartment membership param and is more
robust to special reference values like versioned references or absolute
references that match the base server url.

2. getPatientIdFromToken will now split the patient_id claim on spaces.
This sets us up for scenarios where a single token can grant access to
multiple patient records. Additionally, I removed the log statement
about expecting patient_id to be a string and not a list.

3. Patient search is now allowed. Because we convert all searches into
compartment searches, there is no longer a risk of exposing whether a
given Patient resource by a given id exists or not. Therefore, we don't
need to prevent such searches any longer. That said, because a Patient
is not in its own compartment (unless it has a corresponding
Patient.link), Patient search behavior will likely to be confusing to
clients, operators may want to continue to disallow Patient search from
the REST layer in select deployments.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
Copy link
Collaborator

@JohnTimm JohnTimm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

return claim.asList(String.class);
}

return Collections.singletonList(patientId);
return Stream.of(patientId.split(" "))
Copy link
Collaborator

@JohnTimm JohnTimm Apr 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we split on \s+ here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I think we're ok with supporting just space-delimited values for now, but do let me know if you feel we should change it.

@lmsurpre lmsurpre requested a review from d0roppe April 21, 2021 12:49
Copy link
Contributor

@michaelwschroeder michaelwschroeder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lmsurpre lmsurpre merged commit 3966a46 into main Apr 21, 2021
@lmsurpre lmsurpre deleted the lee-main branch April 21, 2021 15:17
Copy link
Collaborator

@d0roppe d0roppe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as far as providing a fix for the issue. I don't know much about how the code works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants