Skip to content

implement apply access control to /validate endpoints #2401

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 12 commits into from
Closed

implement apply access control to /validate endpoints #2401

wants to merge 12 commits into from

Conversation

aktaskaan
Copy link
Contributor

JIRA link (if applicable)

https://tools.hmcts.net/jira/browse/CCD-5344

Change description

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[ ] No

@aktaskaan aktaskaan force-pushed the ccd_5344 branch 4 times, most recently from b3c4a3e to f306ea3 Compare April 12, 2024 15:18
@hmcts-jenkins-a-to-c
Copy link
Contributor

Plan Result (aat)

⚠️ Resource Deletion will happen ⚠️

This plan contains resource delete operation. Please check the plan result very carefully!

Plan: 0 to add, 0 to change, 2 to destroy.
  • Delete
    • module.postgresql_v15.azurerm_postgresql_flexible_server_configuration.pgsql_server_config["logfiles.download_enable"]
    • module.postgresql_v15.azurerm_postgresql_flexible_server_configuration.pgsql_server_config["logfiles.retention_days"]
Change Result (Click me) 
  # module.postgresql_v15.azurerm_postgresql_flexible_server_configuration.pgsql_server_config["logfiles.download_enable"] will be destroyed
  # (because key ["logfiles.download_enable"] is not in for_each map)
  - resource "azurerm_postgresql_flexible_server_configuration" "pgsql_server_config" {
      - id        = "/subscriptions/1c4f0704-a29e-403d-b719-b90c34ef14c9/resourceGroups/ccd-data-store-api-postgres-db-v15-data-aat/providers/Microsoft.DBforPostgreSQL/flexibleServers/ccd-data-store-api-postgres-db-v15-aat/configurations/logfiles.download_enable" -> null
      - name      = "logfiles.download_enable" -> null
      - server_id = "/subscriptions/1c4f0704-a29e-403d-b719-b90c34ef14c9/resourceGroups/ccd-data-store-api-postgres-db-v15-data-aat/providers/Microsoft.DBforPostgreSQL/flexibleServers/ccd-data-store-api-postgres-db-v15-aat" -> null
      - value     = "ON" -> null
    }

  # module.postgresql_v15.azurerm_postgresql_flexible_server_configuration.pgsql_server_config["logfiles.retention_days"] will be destroyed
  # (because key ["logfiles.retention_days"] is not in for_each map)
  - resource "azurerm_postgresql_flexible_server_configuration" "pgsql_server_config" {
      - id        = "/subscriptions/1c4f0704-a29e-403d-b719-b90c34ef14c9/resourceGroups/ccd-data-store-api-postgres-db-v15-data-aat/providers/Microsoft.DBforPostgreSQL/flexibleServers/ccd-data-store-api-postgres-db-v15-aat/configurations/logfiles.retention_days" -> null
      - name      = "logfiles.retention_days" -> null
      - server_id = "/subscriptions/1c4f0704-a29e-403d-b719-b90c34ef14c9/resourceGroups/ccd-data-store-api-postgres-db-v15-data-aat/providers/Microsoft.DBforPostgreSQL/flexibleServers/ccd-data-store-api-postgres-db-v15-aat" -> null
      - value     = "7" -> null
    }

Plan: 0 to add, 0 to change, 2 to destroy.

@hmcts-jenkins-a-to-c hmcts-jenkins-a-to-c bot requested a deployment to preview April 17, 2024 19:42 Abandoned
danlysiak
danlysiak reviewed Apr 25, 2024
View reviewed changes
Copy link
Member

@danlysiak danlysiak left a comment

Choose a reason for hiding this comment

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

A few suggestions.

src/aat/resources/features/F-1017 - Validate Event to Update TTL/F-1017.feature Outdated
And the response [contains the TTL.SystemTTL for the case, that has been set to 20 days from today]
And the response [contains the TTL.OverrideTTL from the previouse data]
And the response [does not contain the TTL.Suspended as removed by callback (null -> missing)]
And the response [does not contain the TTL as citizen user has no access]
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to re-work the tests in this PR (whether as part of this ticket or temporarily disabling here to be fixed in another ticket) so that they retain their original purpose of what they were actually trying to test, as now we're applying AC here it's no longer actually verifying the scenario.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Assigning the necessary roles to the user for the TTL field solves the /validate scenarios but breaks about 20 different scenarios. Also, creating and assigning a new role just to access this field causes a similar problem. Since the TTL field is very dependent on scenarios, it would be better to reorganise the 5 scenarios that fail for /validate by adding a new field. Therefore I am disabling them and will create a new ticket.

src/main/java/uk/gov/hmcts/ccd/config/JacksonUtils.java Outdated
@@ -198,4 +200,12 @@ private static String getValue(@NonNull JsonNode jsonNode) {
}
return returnValue;
}

public static Map<String, JsonNode> convertValueInDataField(Object from) {
Map<String, JsonNode> map = MAPPER.convertValue(from, new TypeReference<HashMap<String, JsonNode>>() {});
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Map<String, JsonNode> map = MAPPER.convertValue(from, new TypeReference<HashMap<String, JsonNode>>() {});
Map<String, JsonNode> map = convertValue(from);

Looks like we are just re-using the implementation for this anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done 👍

@aktaskaan aktaskaan force-pushed the ccd_5344 branch 2 times, most recently from 29ec4b8 to 44e9c3d Compare May 8, 2024 12:19
@aktaskaan aktaskaan requested a review from danlysiak May 8, 2024 15:16
aktaskaan
aktaskaan commented May 8, 2024
View reviewed changes
Copy link
Contributor Author

@aktaskaan aktaskaan left a comment

Choose a reason for hiding this comment

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

@danlysiak @RebeccaBaker I have created CCD-5434 for the disabled FTs.

@aktaskaan
Copy link
Contributor Author

combined with ccd-6022 in #2545

@aktaskaan aktaskaan closed this Mar 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RebeccaBaker RebeccaBaker RebeccaBaker approved these changes

@danlysiak danlysiak Awaiting requested review from danlysiak

Assignees
No one assigned
Labels
aat/destroy enable_keep_helm keep-helm ns:ccd prd:ccd
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@aktaskaan @RebeccaBaker @danlysiak @mwallaceHMCTS