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

[ALS-7197] BDC Auth access: User able to access data #203

Merged
merged 12 commits into from
Sep 9, 2024
Merged

Conversation

Gcolon021
Copy link
Contributor

@Gcolon021 Gcolon021 commented Sep 5, 2024

  • The clinical query template has been updated to include the parentAccessField in the fields section. The parentAccessionField value is now set to \\\\_Parent Study Accession with Subject ID\\\\.

  • The TOPMED+PARENT access rule, associated with clinical privileges, has been fixed. The rule previously had misconfigured subAccessRules and gates:

    • The TOPMED_CONSENT gate consent rule was incorrectly set to IS_EMPTY instead of IS_NOT_EMPTY, causing queries to be erroneously rejected, even when the "categoryFilters": "\\_topmed_consents\\" section was correctly included in the query.
    • A missing subAccessRule named ALLOW_TOPMED_CONSENT was added. This rule ensures that all required consents are present in the \\_topmed_consents\\ section of the query template.
    • A new method, configureClinicalAccessRuleWithPhenoSubRule, was added to AccessRuleService. This method contains the configuration for this access rule.
  • Previously, there were many unassociated access rules. All access rules are now correctly linked to other rules and privileges. In Spring Boot, any entity property defining a parent-child relationship must use the cascade = CascadeType.ALL option in the @ManyToOne or @ManyToMany annotations to ensure updates to the parent or child entities on save.

  • A new role, MANUAL_ROLE_NAMED_DATASET, has been assigned to all BDC users. This role grants users access to the /named/dataset, /named/dataset/, /named/dataset/{uuid}, and /query/{uuid}/metadata endpoints. The role, along with its privilege and rule definition, is now part of the BDC infrastructure.

  • The createPhoneTypeSubRule method now correctly removes double-escaped characters. For example, a concept path formatted as \\\\phs000xxx\\\\ is now converted to \\phs000xxx\\.

  • The upsertConsentGate method now properly escapes backslashes in rules.

  • The extractAndCheckRule method now correctly evaluates access rules that need to check map nodes. When using JsonPath to retrieve a node value, it is naturally converted into a list. This list is now converted into a Map for key checking.

  • When evaluating subAccessRules for a given rule, the rules must first be merged before evaluation. Since sub access rules may overlap with other rules, the method preProcessARBySortedKeys is used to handle the merging process.

  • The pic-sure-auth-micro-app was originally designed with an ephemeral database. With each deployment, all roles, privileges, and access rules were re-created. This provided an opportunity to add new allowed query types and standard access rules. Now that our database is persisted, there was no mechanism to add new allowed query types or standard access rules. A new method has been added to PrivilegeService named updateAllPrivilegesOnStartup.

    • This method updates all privileges to include all current standard access rules. Any standard access rules that need to be removed from a privilege must be handled via a migration script.
    • The method also updates all allowed query types. All existing allowed query types are removed from each access_rule that currently has allowed query types as subAccessRules. Once these are removed, all currently defined allowed query types are added to the access_rule, enabling the addition or removal of allowed query types without needing migration scripts.

@Gcolon021 Gcolon021 self-assigned this Sep 5, 2024
@Gcolon021 Gcolon021 added the bug Something isn't working label Sep 5, 2024
…AccessField` in the fields section. The `parentAccessionField` value is now set to `\\\\_Parent Study Accession with Subject ID\\\\`.

- The `TOPMED+PARENT` access rule, associated with clinical privileges, has been fixed. The rule previously had misconfigured `subAccessRules` and `gates`:
    - The `TOPMED_CONSENT` gate consent rule was incorrectly set to `IS_EMPTY` instead of `IS_NOT_EMPTY`, causing queries to be erroneously rejected, even when the `"categoryFilters": "\\_topmed_consents\\"` section was correctly included in the query.
    - A missing `subAccessRule` named `ALLOW_TOPMED_CONSENT` was added. This rule ensures that all required consents are present in the `\\_topmed_consents\\` section of the query template.
    - A new method, `configureClinicalAccessRuleWithPhenoSubRule`, was added to `AccessRuleService`. This method contains the configuration for this access rule.

- Previously, there were many unassociated access rules. All access rules are now correctly linked to other rules and privileges. In Spring Boot, any entity property defining a parent-child relationship must use the `cascade = CascadeType.ALL` option in the `@ManyToOne` or `@ManyToMany` annotations to ensure updates to the parent or child entities on save.

- A new role, `MANUAL_ROLE_NAMED_DATASET`, has been assigned to all BDC users. This role grants users access to the `/named/dataset/`, `/named/dataset/{uuid}`, and `/query/{uuid}/metadata` endpoints. The role, along with its privilege and rule definition, is now part of the BDC infrastructure.

- The `createPhoneTypeSubRule` method now correctly removes double-escaped characters. For example, a concept path formatted as `\\\\phs000xxx\\\\` is now converted to `\\phs000xxx\\`.

- The `upsertConsentGate` method now properly escapes backslashes in rules.

- The `extractAndCheckRule` method now correctly evaluates access rules that need to check map nodes. When using JsonPath to retrieve a node value, it is naturally converted into a list. This list is now converted into a Map for key checking.

- When evaluating `subAccessRules` for a given rule, the rules must first be merged before evaluation. Since sub access rules may overlap with other rules, the method `preProcessARBySortedKeys` is used to handle the merging process.

- The `pic-sure-auth-micro-app` was originally designed with an ephemeral database. With each deployment, all roles, privileges, and access rules were re-created. This provided an opportunity to add new allowed query types and standard access rules. Now that our database is persisted, there was no mechanism to add new allowed query types or standard access rules. A new method has been added to `PrivilegeService` named `updateAllPrivilegesOnStartup`.
    - This method updates all privileges to include all current standard access rules. Any standard access rules that need to be removed from a privilege must be handled via a migration script.
    - The method also updates all allowed query types. All existing allowed query types are removed from each `access_rule` that currently has allowed query types as `subAccessRules`. Once these are removed, all currently defined allowed query types are added to the `access_rule`, enabling the addition or removal of allowed query types without needing migration scripts.
Replaced the custom query with direct calls to add access rules and save privileges. This simplifies the code and utilizes existing save method, ensuring transactional consistency.
@ramari16
Copy link
Contributor

ramari16 commented Sep 6, 2024

I don't think we have any external documentation for this functionality, which is pretty complicated. I think we should open a tech debt ticket to create some diagrams on how this all works.

@Gcolon021
Copy link
Contributor Author

@ramari16 I absolutely agree. I will ask for time to provide comprehensive documentation on all of this functionality for other developers.

@Gcolon021
Copy link
Contributor Author

@ramari16 A new technical debt ticket has been created.

Changed the logging level for `edu.harvard.hms.dbmi.avillach.auth.service.impl` from INFO to DEBUG to facilitate detailed debugging. This will help in tracking down issues more effectively during development and troubleshooting.
Changed the logging level from DEBUG to TRACE for specific methods in AccessRuleService to reduce the verbosity of the application logs. This will make debugging more efficient by including detailed logs specific to fine-grain debugging levels without cluttering higher-level log outputs.
Refactor log statement in AuthorizationService to log only when access is denied. This change reduces unnecessary log entries for granted access, improving log readability and performance.
Address an issue where double backslashes in the concept path were not appropriately converted, causing potential mismatches in rule text generation. This ensures consistency by replacing all instances of `\\\\` with `\\` before forming the rule text.
Changed log level from debug to trace for initial access rule evaluation steps. This change reduces verbosity in the logs, helping to focus on more critical debug information.
…lach/auth/service/impl/authorization/AuthorizationService.java
@Gcolon021 Gcolon021 merged commit a63a0bd into release Sep 9, 2024
2 checks passed
@Gcolon021 Gcolon021 deleted the ALS-7197 branch September 10, 2024 18:33
Luke-Sikina pushed a commit that referenced this pull request Oct 7, 2024
* - The clinical query template has been updated to include the `parentAccessField` in the fields section. The `parentAccessionField` value is now set to `\\\\_Parent Study Accession with Subject ID\\\\`.

- The `TOPMED+PARENT` access rule, associated with clinical privileges, has been fixed. The rule previously had misconfigured `subAccessRules` and `gates`:
    - The `TOPMED_CONSENT` gate consent rule was incorrectly set to `IS_EMPTY` instead of `IS_NOT_EMPTY`, causing queries to be erroneously rejected, even when the `"categoryFilters": "\\_topmed_consents\\"` section was correctly included in the query.
    - A missing `subAccessRule` named `ALLOW_TOPMED_CONSENT` was added. This rule ensures that all required consents are present in the `\\_topmed_consents\\` section of the query template.
    - A new method, `configureClinicalAccessRuleWithPhenoSubRule`, was added to `AccessRuleService`. This method contains the configuration for this access rule.

- Previously, there were many unassociated access rules. All access rules are now correctly linked to other rules and privileges. In Spring Boot, any entity property defining a parent-child relationship must use the `cascade = CascadeType.ALL` option in the `@ManyToOne` or `@ManyToMany` annotations to ensure updates to the parent or child entities on save.

- A new role, `MANUAL_ROLE_NAMED_DATASET`, has been assigned to all BDC users. This role grants users access to the `/named/dataset/`, `/named/dataset/{uuid}`, and `/query/{uuid}/metadata` endpoints. The role, along with its privilege and rule definition, is now part of the BDC infrastructure.

- The `createPhoneTypeSubRule` method now correctly removes double-escaped characters. For example, a concept path formatted as `\\\\phs000xxx\\\\` is now converted to `\\phs000xxx\\`.

- The `upsertConsentGate` method now properly escapes backslashes in rules.

- The `extractAndCheckRule` method now correctly evaluates access rules that need to check map nodes. When using JsonPath to retrieve a node value, it is naturally converted into a list. This list is now converted into a Map for key checking.

- When evaluating `subAccessRules` for a given rule, the rules must first be merged before evaluation. Since sub access rules may overlap with other rules, the method `preProcessARBySortedKeys` is used to handle the merging process.

- The `pic-sure-auth-micro-app` was originally designed with an ephemeral database. With each deployment, all roles, privileges, and access rules were re-created. This provided an opportunity to add new allowed query types and standard access rules. Now that our database is persisted, there was no mechanism to add new allowed query types or standard access rules. A new method has been added to `PrivilegeService` named `updateAllPrivilegesOnStartup`.
    - This method updates all privileges to include all current standard access rules. Any standard access rules that need to be removed from a privilege must be handled via a migration script.
    - The method also updates all allowed query types. All existing allowed query types are removed from each `access_rule` that currently has allowed query types as `subAccessRules`. Once these are removed, all currently defined allowed query types are added to the `access_rule`, enabling the addition or removal of allowed query types without needing migration scripts.

* Add transactional annotation to PrivilegeRepository

* Change event listener and improve privilege update logic

* Refactor privilege update method to avoid transactional query

Replaced the custom query with direct calls to add access rules and save privileges. This simplifies the code and utilizes existing save method, ensuring transactional consistency.

* Fix sub-access rule addition in PrivilegeService

* Refactor role name constants to uppercase

* Set logging level to DEBUG for auth service implementation

Changed the logging level for `edu.harvard.hms.dbmi.avillach.auth.service.impl` from INFO to DEBUG to facilitate detailed debugging. This will help in tracking down issues more effectively during development and troubleshooting.

* Switch logging level from DEBUG to TRACE in AccessRuleService

Changed the logging level from DEBUG to TRACE for specific methods in AccessRuleService to reduce the verbosity of the application logs. This will make debugging more efficient by including detailed logs specific to fine-grain debugging levels without cluttering higher-level log outputs.

* Restrict logging to denied access attempts

Refactor log statement in AuthorizationService to log only when access is denied. This change reduces unnecessary log entries for granted access, improving log readability and performance.

* Fix double backslash handling in Topmed access rule generation

Address an issue where double backslashes in the concept path were not appropriately converted, causing potential mismatches in rule text generation. This ensures consistency by replacing all instances of `\\\\` with `\\` before forming the rule text.

* Lower logging level in evaluateAccessRule method

Changed log level from debug to trace for initial access rule evaluation steps. This change reduces verbosity in the logs, helping to focus on more critical debug information.

* Update pic-sure-auth-services/src/main/java/edu/harvard/hms/dbmi/avillach/auth/service/impl/authorization/AuthorizationService.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants