Feature/dis 4606 allow preview users to view bundles bugfix#131
Feature/dis 4606 allow preview users to view bundles bugfix#131BarathaAberathne wants to merge 8 commits intodevelopfrom
Conversation
api/api.go
Outdated
| // getDatasetEditionAttributeForBundle provides the "dataset_edition" attribute required | ||
| // for conditional preview-team policies to apply to bundle read endpoints. | ||
| func (api *BundleAPI) getDatasetEditionAttributeForBundle(req *http.Request) (map[string]string, error) { | ||
| attrs, err := auth.GetCollectionIDAttribute(req) |
There was a problem hiding this comment.
We're not using collections anymore, so would it not be better to just make attrs an empty map instead of pulling from GetCollectionIDAttribute?
|
|
||
| bundleID := mux.Vars(req)[RouteVariableBundleID] | ||
| if bundleID == "" { | ||
| return attrs, nil |
There was a problem hiding this comment.
What happens if the bundle id is "" and we just pass through an empty map to the RequireWithAttributes function?
There was a problem hiding this comment.
We are fetching the bundle id from the HTTP request /bundles/{bundle-id} and /bundles/{bundle-id}/contents which will always contains the bundle id. otherwise the malformed http request will be handle by the router it self. So this scenario is unlikely to happen.
| return nil, err | ||
| } | ||
| if len(contentItems) == 0 { | ||
| return attrs, nil |
There was a problem hiding this comment.
Same as the above comment. What happens if the there are no content items and we just pass through an empty map to the RequireWithAttributes function?
There was a problem hiding this comment.
The only edge case would be when a bundle has no content items yet, meaning there are no attributes to pass to RequireWithAttributes. However, this is unlikely to cause issues, as preview teams are typically added later in the process rather than at bundle creation. in this scenario an admin can view the bundles successfully.
| return attrs, nil | ||
| } | ||
|
|
||
| datasetID := contentItems[0].Metadata.DatasetID |
There was a problem hiding this comment.
Is it safe to always be checking just the first item in the contentItems array?
There was a problem hiding this comment.
The purpose of introducing the RequireWithAttributes method is to prevent the authentication layer from repeatedly parsing an empty value, which previously resulted in a 403 response. By using RequireWithAttributes, we avoid passing a null value into the authentication function and instead trigger the permissions checker directly.
When preview teams are added to a bundle, the permissions are correctly updated to include all associated content items. For example, if a bundle contains two content items, the permissions table will store a policy that includes the dataset_editions from both items. Since the policy already covers all related editions, validating against a single value is sufficient to determine whether the user has the bundles:read permission for that preview team.
Therefore, keeping this approach should be safe from a functional standpoint.
| @@ -0,0 +1,7 @@ | |||
| ignore: | |||
There was a problem hiding this comment.
Why has this file been added when the repo has already been upgraded to go 1.26 and make audit passes without it?
There was a problem hiding this comment.
i have updated the go versions in the commit 51bd99651bd996 which resulted with an component failure -> https://concourse.dp-ci.aws.onsdigital.uk/builds/121598194. Hence decided to move forward with the ignore file.
3 participants
What
Jira ticket: https://officefornationalstatistics.atlassian.net/browse/DIS-4606
Updated /bundles/{bundle-id} & /bundles/{bundle-id}/contents with RequireWithAttributes() to allow preview users to view bundles.
How to review
Steps:
When the user belongs to the newly added preview team calls {{localhost_bundle}}/bundles/{bundle_id}, response should be 200.
Who can review
Anyone