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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,8 @@ jobs:
- run:
name: Add a dummy OPA policy.
command: |
curl -X PUT http://localhost:8181/v1/policies/publication_based_authorization -H 'Content-Type: text/plain' --data-raw 'package draft_annotations_api.publication_based_authorization is_authorized {true}'
curl -X PUT http://localhost:8181/v1/policies/read -H 'Content-Type: text/plain' --data-raw 'package draft_annotations_api.read is_authorized {true}'
curl -X PUT http://localhost:8181/v1/policies/write -H 'Content-Type: text/plain' --data-raw 'package draft_annotations_api.write is_authorized {true}'
- run:
name: Dredd API Testing
command: dredd
Expand Down
4 changes: 4 additions & 0 deletions annotations/annotations_api.go
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,10 @@ func (api *UPPAnnotationsAPI) getUPPAnnotationsResponse(ctx context.Context, con
params.Add("lifecycle", lc)
}

//by default publications are not returned from public-annotations-api,
//so we need to add this parameter to the query
params.Add("showPublication", "true")

baseURL.RawQuery = params.Encode()
apiReqURI = baseURL.String()
}
Expand Down
22 changes: 16 additions & 6 deletions annotations/annotations_api_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ func TestAnnotationsAPIGTGInvalidURL(t *testing.T) {
}

func TestAnnotationsAPIGTGConnectionError(t *testing.T) {
annotationsServerMock := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}))
annotationsServerMock := httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {}))
annotationsServerMock.Close()

log := logger.NewUPPLogger("draft-annotations-api", "INFO")
Expand All @@ -87,6 +87,7 @@ func TestHappyAnnotationsAPI(t *testing.T) {
annotationsAPI := NewUPPAnnotationsAPI(testClient, annotationsServerMock.URL+"/content/%v/annotations", testBasicAuthUsername, testBasicAuthPassword, log)
resp, err := annotationsAPI.getUPPAnnotationsResponse(ctx, uuid)
assert.NoError(t, err)
defer resp.Body.Close()
assert.Equal(t, http.StatusOK, resp.StatusCode)
}

Expand All @@ -95,13 +96,14 @@ func TestHappyAnnotationsAPIWithLifecycles(t *testing.T) {
tid := "tid_all-good"
ctx := tidUtils.TransactionAwareContext(context.TODO(), tid)

annotationsServerMock := newAnnotationsAPIServerMock(t, tid, uuid, "lifecycle=pac&lifecycle=v1&lifecycle=next-video", http.StatusOK, "I am happy!")
annotationsServerMock := newAnnotationsAPIServerMock(t, tid, uuid, "lifecycle=pac&lifecycle=v1&lifecycle=next-video&showPublication=true", http.StatusOK, "I am happy!")
defer annotationsServerMock.Close()

log := logger.NewUPPLogger("draft-annotations-api", "INFO")
annotationsAPI := NewUPPAnnotationsAPI(testClient, annotationsServerMock.URL+"/content/%v/annotations", testBasicAuthUsername, testBasicAuthPassword, log)
resp, err := annotationsAPI.getUPPAnnotationsResponse(ctx, uuid, pacAnnotationLifecycle, v1AnnotationLifecycle, nextVideoAnnotationLifecycle)
assert.NoError(t, err)
defer resp.Body.Close()
assert.Equal(t, http.StatusOK, resp.StatusCode)
}

Expand All @@ -117,6 +119,7 @@ func TestUnhappyAnnotationsAPI(t *testing.T) {
annotationsAPI := NewUPPAnnotationsAPI(testClient, annotationsServerMock.URL+"/content/%v/annotations", testBasicAuthUsername, testBasicAuthPassword, log)
resp, err := annotationsAPI.getUPPAnnotationsResponse(ctx, uuid)
assert.NoError(t, err)
defer resp.Body.Close()
assert.Equal(t, http.StatusServiceUnavailable, resp.StatusCode)
}

Expand All @@ -129,14 +132,17 @@ func TestNoTIDAnnotationsAPI(t *testing.T) {
annotationsAPI := NewUPPAnnotationsAPI(testClient, annotationsServerMock.URL+"/content/%v/annotations", testBasicAuthUsername, testBasicAuthPassword, log)
resp, err := annotationsAPI.getUPPAnnotationsResponse(context.TODO(), uuid)
assert.NoError(t, err)
defer resp.Body.Close()
assert.Equal(t, http.StatusServiceUnavailable, resp.StatusCode)
}

func TestRequestFailsAnnotationsAPI(t *testing.T) {
log := logger.NewUPPLogger("draft-annotations-api", "INFO")
annotationsAPI := NewUPPAnnotationsAPI(testClient, ":#", testBasicAuthUsername, testBasicAuthPassword, log)
resp, err := annotationsAPI.getUPPAnnotationsResponse(context.TODO(), "")

if err == nil {
defer resp.Body.Close()
}
assert.Error(t, err)
assert.Nil(t, resp)
}
Expand All @@ -145,7 +151,9 @@ func TestResponseFailsAnnotationsAPI(t *testing.T) {
log := logger.NewUPPLogger("draft-annotations-api", "INFO")
annotationsAPI := NewUPPAnnotationsAPI(testClient, "#:", testBasicAuthUsername, testBasicAuthPassword, log)
resp, err := annotationsAPI.getUPPAnnotationsResponse(context.TODO(), "")

if err == nil {
defer resp.Body.Close()
}
assert.Error(t, err)
assert.Nil(t, resp)
}
Expand All @@ -163,13 +171,15 @@ func TestAnnotationsAPITimeout(t *testing.T) {
ctx, cancel := context.WithTimeout(context.Background(), time.Millisecond*50)
defer cancel()

_, err := annotationsAPI.getUPPAnnotationsResponse(ctx, testContentUUID)
resp, err := annotationsAPI.getUPPAnnotationsResponse(ctx, testContentUUID)
if err == nil {
defer resp.Body.Close()
}
assert.Error(t, err)
assert.True(t, (err.(net.Error)).Timeout())
}

func TestGetAnnotationsHappy(t *testing.T) {

var testCases = []struct {
name string
annotationsStatus int
Expand Down
67 changes: 67 additions & 0 deletions handler/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"strings"
"time"

"github.com/Financial-Times/draft-annotations-api/policy"
"github.com/Financial-Times/go-logger/v2"
"github.com/gorilla/mux"

Expand Down Expand Up @@ -96,18 +97,36 @@ func (h *Handler) DeleteAnnotation(w http.ResponseWriter, r *http.Request) {
return
}

var scheduledForDelete interface{}
i := 0
for _, item := range uppList {
if item.(map[string]interface{})["id"] == conceptID {
scheduledForDelete = item
continue
}
uppList[i] = item
i++
}
uppList = uppList[:i]

if !isAuthorizedForDelete(r, scheduledForDelete) {
writeLog.Infof("Not authorized to delete annotation with current policy: %s", r.Header.Get("X-Policy"))
w.WriteHeader(http.StatusForbidden)
_, _ = w.Write([]byte("Forbidden"))
return
}

annotationsBody := make(map[string]interface{})
annotationsBody["annotations"] = uppList

//if the policy and the publication from the annotation match, set the publication in the annotations body
if scheduledForDelete != nil {
pub, ok := scheduledForDelete.(map[string]interface{})["publication"]
if ok {
annotationsBody["publication"] = pub
}
}

_, newHash, err := h.saveAndReturnAnnotations(ctx, annotationsBody, writeLog, oldHash, contentUUID)
if err != nil {
handleWriteErrors("Error writing draft annotations", err, writeLog, w, http.StatusInternalServerError)
Expand Down Expand Up @@ -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.

if scheduledForDelete == nil {
return true
}

publication, ok := scheduledForDelete.(map[string]interface{})["publication"].([]interface{})
if !ok {
//if no publication is returned, we assume its FT PINK
publication = []interface{}{"88fdde6c-2aa4-4f78-af02-9f680097cfd6"}
}

af := r.Header.Get("Access-From")
if af == "" {
//if access-from header is missing, we skip the policy check
return true
}

policyHeaders := r.Header.Get("X-Policy")
policyHeaders = strings.ReplaceAll(policyHeaders, " ", "")
splitPolicyHeaders := strings.Split(policyHeaders, ",")
allowDelete := false

for _, header := range splitPolicyHeaders {
//extract the publication from the policy header
incomingPublication := strings.ReplaceAll(header, policy.WritePBLC, "")

//verify if the extracted uuid is valid
_, err := uuid.Parse(incomingPublication)
if err != nil {
continue
}

if contains(incomingPublication, publication) {
allowDelete = true
}
}
return allowDelete
}

func contains(needle string, haystack []interface{}) bool {
for _, v := range haystack {
if v == needle {
return true
}
}
return false
}
Loading