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

split policy reading #81

Merged

Conversation

angelraynovft
Copy link
Contributor

@angelraynovft angelraynovft commented Jul 24, 2024

Description

What

split the policy reading. Before that all the rules were defined in a single auth file, now there are 2 files one for read rules and one for write. I have excluded the get-schema and validation endpoints from the authorization as they are not exposed and to reduce the number of rules.

As this remained not reviewed I had the time to add the authorization for the delete endpoint. Please review thoroughly.

Why

https://financialtimes.atlassian.net/browse/UPPSF-5436

Anything, in particular, you'd like to highlight to reviewers

Mention here sections of code which you would like reviewers to pay extra attention to .E.g

Would appreciate a second pair of eyes on the test
I am not quite sure how this bit works
Is there a better library for doing x

Scope and particulars of this PR (Please tick all that apply)

  • Tech hygiene (dependency updating & other tech debt)
  • Bug fix
  • Feature
  • Documentation
  • Breaking change
  • Minor change (e.g. fixing a typo, adding config)

DoD - Ensure all relevant tasks are completed before marking this PR as "Ready for review"

  • Test coverage is not significantly decreased
  • All PR checks have passed
  • Changes are deployed on dev before asking for review
  • Documentations remains up-to-date
    • OpenAPI definition file is updated
    • README file is updated
    • Documentation is updated in upp-docs and upp-public-docs
    • Architecture diagrams are updated

This Pull Request follows the rules described in our Pull Requests Guide

@coveralls
Copy link

coveralls commented Jul 24, 2024

Coverage Status

coverage: 75.11% (+0.9%) from 74.233%
when pulling 157860e on feature/UPPSF-5436-split-policy-reading
into 1627136 on publishing-cluster.

@angelraynovft angelraynovft force-pushed the feature/UPPSF-5436-split-policy-reading branch from dd6ddd5 to b643fbc Compare July 25, 2024 09:02
@angelraynovft angelraynovft marked this pull request as ready for review July 25, 2024 09:46
@angelraynovft angelraynovft requested a review from a team as a code owner July 25, 2024 09:46
@angelraynovft angelraynovft requested review from agrancharova and a team July 31, 2024 09:14
Copy link
Contributor

@ManoelMilchev ManoelMilchev left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -640,3 +659,51 @@ func switchToIsClassifiedBy(toChange []interface{}) []interface{} {
}
return changed
}

func isAuthorizedForDelete(r *http.Request, scheduledForDelete interface{}) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

comment: Unfortunately we didn't write a middleware which calls a specific handle based on the opa evaluation, if we had such, you would have been able to get the annotations, use opa for the access control instead of this custom function, and later call the handler to actually delete the resource, we can think about this in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe, but if you call the handler in a middleware the annotation will be already deleted

Copy link
Contributor

Choose a reason for hiding this comment

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

No, no, maybe I didn't make myself clear, imagine a response middleware which only gets the annotation, evaluates if it is eligible for deletion and after that is able to call another handler that deletes the annotation itself.

@angelraynovft angelraynovft merged commit 36f9029 into publishing-cluster Aug 2, 2024
3 checks passed
@angelraynovft angelraynovft deleted the feature/UPPSF-5436-split-policy-reading branch October 4, 2024 11:27
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.

4 participants