Improvement/cldsrv 431 misc api implicit deny by benzekrimaha · Pull Request #5479 · scality/cloudserver

benzekrimaha · 2023-12-07T15:17:14Z

Bucket policies are not correctly interpreted, this is part of the following epic to fix that: scality/Arsenal#2181

This PR is aiming to update get apis since object and bucket authorisations are made at API level and need to support implicit denies, ticket linked to this issue here : https://scality.atlassian.net/browse/CLDSRV-431

PRs providing implicit Deny logic to CS for processing in this PR
scality/Arsenal#2181 and scality/Arsenal#2193
https://github.com/scality/Vault/pull/2135
#5322
#5420
#5432
#5456
#5462
#5470

Here CI links for zenko tests :
https://github.com/scality/Zenko/actions/runs/7209044767
https://github.com/scality/Zenko/actions/runs/7209077500
https://github.com/scality/Zenko/actions/runs/7209082617

Seragonia · 2023-12-08T14:11:12Z

lib/routes/routeBackbeat.js

+    // eslint-disable-next-line no-param-reassign
+    request.actionImplicitDenies = false;


This can be done in the _normalizeBackbeatRequest function:

cloudserver/lib/routes/routeBackbeat.js

Line 71 in 15a1aa7

function _normalizeBackbeatRequest(req) {

Seragonia · 2023-12-08T14:36:19Z

lib/api/apiUtils/object/abortMultipartUpload.js

    // but the requestType is the more general 'objectDelete'
    const metadataValParams = Object.assign({}, metadataValMPUparams);
    metadataValParams.requestType = 'objectPut';
+    const authzIdentityResult = request ? request.actionImplicitDenies : true;


Maybe not useful anymore if we default to true (implicit deny) when the request is mising the property?

Indeed even if it's not changing the behaviour setting it to false makes more sens , good catch

Seragonia · 2023-12-08T14:37:36Z

lib/api/apiUtils/object/objectLockHelpers.js

+            const actionImplicitDenies = authorizationResults.reduce((acc, curr, idx) => {
+                const apiMethod = requestContextParams[idx].apiMethod;
+                return Object.assign({}, acc, { [apiMethod]: curr.isImplicit });
+            }, {});


Same comment as in other PR here to optimize the logic and avoid re-creating objects

Seragonia · 2023-12-08T14:38:31Z

lib/api/apiUtils/object/objectLockHelpers.js

+                log,
+                request);
+
+            return cb(areAllActionsAllowed ? null : errors.AccessDenied);


This was from the PoC but I would suggest being very strict on the check:

Suggested change

return cb(areAllActionsAllowed ? null : errors.AccessDenied);

return cb(areAllActionsAllowed === true ? null : errors.AccessDenied);

bert-e · 2023-12-11T07:53:49Z

Hello benzekrimaha,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Status report is not available.

benzekrimaha · 2023-12-11T11:00:43Z

ping

bert-e · 2023-12-11T11:00:44Z

Incorrect fix version

The Fix Version/s in issue CLDSRV-431 contains:

7.70.35

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

7.10.38
7.70.35
8.6.16
8.7.36
8.8.10

Please check the Fix Version/s of CLDSRV-431, or the target
branch of this pull request.

benzekrimaha · 2023-12-11T11:01:57Z

ping

bert-e · 2023-12-11T11:02:06Z

Request integration branches

Waiting for integration branch creation to be requested by the user.

To request integration branches, please comment on this pull request with the following command:

/create_integration_branches

Alternatively, the /approve and /create_pull_requests commands will automatically
create the integration branches.

benzekrimaha · 2023-12-14T11:45:32Z

/create_integration_branches

bert-e · 2023-12-14T11:45:42Z

Conflict

A conflict has been raised during the creation of
integration branch w/7.70/improvement/CLDSRV-431-misc-api-implicitDeny with contents from improvement/CLDSRV-431-misc-api-implicitDeny
and development/7.70.

I have not created the integration branch.

Here are the steps to resolve this conflict:

 $ git fetch
 $ git checkout -B w/7.70/improvement/CLDSRV-431-misc-api-implicitDeny origin/development/7.70
 $ git merge origin/improvement/CLDSRV-431-misc-api-implicitDeny
 $ # <intense conflict resolution>
 $ git commit
 $ git push -u origin w/7.70/improvement/CLDSRV-431-misc-api-implicitDeny

The following options are set: create_integration_branches

benzekrimaha · 2023-12-14T12:29:00Z

ping

bert-e · 2023-12-14T12:29:11Z

Conflict

A conflict has been raised during the creation of
integration branch w/8.6/improvement/CLDSRV-431-misc-api-implicitDeny with contents from w/7.70/improvement/CLDSRV-431-misc-api-implicitDeny
and development/8.6.

I have not created the integration branch.

Here are the steps to resolve this conflict:

 $ git fetch
 $ git checkout -B w/8.6/improvement/CLDSRV-431-misc-api-implicitDeny origin/development/8.6
 $ git merge origin/w/7.70/improvement/CLDSRV-431-misc-api-implicitDeny
 $ # <intense conflict resolution>
 $ git commit
 $ git push -u origin w/8.6/improvement/CLDSRV-431-misc-api-implicitDeny

The following options are set: create_integration_branches

benzekrimaha · 2023-12-14T12:32:52Z

ping

bert-e · 2023-12-14T12:33:04Z

Conflict

A conflict has been raised during the creation of
integration branch w/8.7/improvement/CLDSRV-431-misc-api-implicitDeny with contents from w/8.6/improvement/CLDSRV-431-misc-api-implicitDeny
and development/8.7.

I have not created the integration branch.

Here are the steps to resolve this conflict:

 $ git fetch
 $ git checkout -B w/8.7/improvement/CLDSRV-431-misc-api-implicitDeny origin/development/8.7
 $ git merge origin/w/8.6/improvement/CLDSRV-431-misc-api-implicitDeny
 $ # <intense conflict resolution>
 $ git commit
 $ git push -u origin w/8.7/improvement/CLDSRV-431-misc-api-implicitDeny

The following options are set: create_integration_branches

benzekrimaha · 2023-12-14T12:35:20Z

ping

bert-e · 2023-12-14T12:35:33Z

Conflict

A conflict has been raised during the creation of
integration branch w/8.8/improvement/CLDSRV-431-misc-api-implicitDeny with contents from w/8.7/improvement/CLDSRV-431-misc-api-implicitDeny
and development/8.8.

I have not created the integration branch.

Here are the steps to resolve this conflict:

 $ git fetch
 $ git checkout -B w/8.8/improvement/CLDSRV-431-misc-api-implicitDeny origin/development/8.8
 $ git merge origin/w/8.7/improvement/CLDSRV-431-misc-api-implicitDeny
 $ # <intense conflict resolution>
 $ git commit
 $ git push -u origin w/8.8/improvement/CLDSRV-431-misc-api-implicitDeny

The following options are set: create_integration_branches

benzekrimaha · 2023-12-14T12:37:39Z

/create_pull_requests

bert-e · 2023-12-14T12:37:57Z

Integration data created

I have created the integration data for the additional destination branches.

this pull request will merge improvement/CLDSRV-431-misc-api-implicitDeny into
development/7.10
w/7.70/improvement/CLDSRV-431-misc-api-implicitDeny will be merged into development/7.70 (pull request INTEGRATION [PR#5479 > development/7.70] Improvement/cldsrv 431 misc api implicit deny #5491)
w/8.6/improvement/CLDSRV-431-misc-api-implicitDeny will be merged into development/8.6 (pull request INTEGRATION [PR#5479 > development/8.6] Improvement/cldsrv 431 misc api implicit deny #5492)
w/8.7/improvement/CLDSRV-431-misc-api-implicitDeny will be merged into development/8.7 (pull request INTEGRATION [PR#5479 > development/8.7] Improvement/cldsrv 431 misc api implicit deny #5493)
w/8.8/improvement/CLDSRV-431-misc-api-implicitDeny will be merged into development/8.8 (pull request INTEGRATION [PR#5479 > development/8.8] Improvement/cldsrv 431 misc api implicit deny #5494)

The following branches will NOT be impacted:

development/7.4

Follow integration pull requests if you would like to be notified of
build statuses by email.

The following options are set: create_pull_requests, create_integration_branches

bert-e · 2023-12-14T12:37:59Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
2 peers

The following options are set: create_pull_requests, create_integration_branches

anurag4DSB

I tested this PR with E2E tests
LGTM
I was closely following there branch and code changes before the PR was opened, hence review was quick.
Integration PR: https://github.com/scality/Integration/pull/1215

benzekrimaha · 2023-12-14T13:11:19Z

@bert-e approve

bert-e · 2023-12-14T13:11:42Z

In the queue

The changeset has received all authorizations and has been added to the
relevant queue(s). The queue(s) will be merged in the target development
branch(es) as soon as builds have passed.

The changeset will be merged in:

✔️ development/7.10
✔️ development/7.70
✔️ development/8.6
✔️ development/8.7
✔️ development/8.8

The following branches will NOT be impacted:

development/7.4

There is no action required on your side. You will be notified here once
the changeset has been merged. In the unlikely event that the changeset
fails permanently on the queue, a member of the admin team will
contact you to help resolve the matter.

IMPORTANT

Please do not attempt to modify this pull request.

Any commit you add on the source branch will trigger a new cycle after the
current queue is merged.
Any commit you add on one of the integration branches will be lost.

If you need this pull request to be removed from the queue, please contact a
member of the admin team now.

The following options are set: approve, create_pull_requests, create_integration_branches

bert-e · 2023-12-14T13:11:58Z

I have successfully merged the changeset of this pull request
into targetted development branches:

✔️ development/7.10
✔️ development/7.70
✔️ development/8.6
✔️ development/8.7
✔️ development/8.8

The following branches have NOT changed:

development/7.4

Please check the status of the associated issue CLDSRV-431.

Goodbye benzekrimaha.

benzekrimaha force-pushed the improvement/CLDSRV-431-misc-api-implicitDeny branch from f545cc1 to c65aefb Compare December 8, 2023 13:26

Seragonia reviewed Dec 8, 2023

View reviewed changes

benzekrimaha force-pushed the improvement/CLDSRV-430-delete-api-implicitDeny branch from 76b8d1f to 74425d0 Compare December 8, 2023 17:29

Base automatically changed from improvement/CLDSRV-430-delete-api-implicitDeny to development/7.10 December 11, 2023 07:53

scality deleted a comment from bert-e Dec 11, 2023

benzekrimaha changed the base branch from development/7.10 to improvement/CLDSRV-474-fix-multiObjectDelete-api-aut December 12, 2023 15:13

benzekrimaha force-pushed the improvement/CLDSRV-431-misc-api-implicitDeny branch 2 times, most recently from 98e5efd to 55d741c Compare December 12, 2023 17:53

Base automatically changed from improvement/CLDSRV-474-fix-multiObjectDelete-api-aut to development/7.10 December 13, 2023 07:44

CLDSRV-431: changes on misc api for impDeny

4897b3c

benzekrimaha force-pushed the improvement/CLDSRV-431-misc-api-implicitDeny branch from 304181e to 4897b3c Compare December 13, 2023 10:14

CLDSRV-431: arsenal bump

dc39b37

benzekrimaha force-pushed the improvement/CLDSRV-431-misc-api-implicitDeny branch from ab42bc4 to dc39b37 Compare December 14, 2023 11:22

CLDSRV-431: cloudserver version bump

cdcdf8e

benzekrimaha marked this pull request as ready for review December 14, 2023 12:37

bert-e mentioned this pull request Dec 14, 2023

INTEGRATION [PR#5479 > development/7.70] Improvement/cldsrv 431 misc api implicit deny #5491

Merged

anurag4DSB approved these changes Dec 14, 2023

View reviewed changes

KazToozs approved these changes Dec 14, 2023

View reviewed changes

bert-e merged commit cdcdf8e into development/7.10 Dec 14, 2023

bert-e deleted the improvement/CLDSRV-431-misc-api-implicitDeny branch December 14, 2023 13:12

benzekrimaha mentioned this pull request Dec 14, 2023

improvement/CLDSRV-451-specific-7.70-apis-update #5495

Merged

		// eslint-disable-next-line no-param-reassign
		request.actionImplicitDenies = false;

	return cb(areAllActionsAllowed ? null : errors.AccessDenied);
	return cb(areAllActionsAllowed === true ? null : errors.AccessDenied);

Conversation

benzekrimaha commented Dec 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Seragonia Dec 8, 2023

Choose a reason for hiding this comment

Uh oh!

Seragonia Dec 8, 2023

Choose a reason for hiding this comment

Uh oh!

benzekrimaha Dec 12, 2023

Choose a reason for hiding this comment

Uh oh!

Seragonia Dec 8, 2023

Choose a reason for hiding this comment

Uh oh!

Seragonia Dec 8, 2023

Choose a reason for hiding this comment

Uh oh!

bert-e commented Dec 11, 2023

Hello benzekrimaha,

Uh oh!

benzekrimaha commented Dec 11, 2023

Uh oh!

bert-e commented Dec 11, 2023

Incorrect fix version

Uh oh!

benzekrimaha commented Dec 11, 2023

Uh oh!

bert-e commented Dec 11, 2023

Request integration branches

Uh oh!

benzekrimaha commented Dec 14, 2023

Uh oh!

bert-e commented Dec 14, 2023

Conflict

Uh oh!

benzekrimaha commented Dec 14, 2023

Uh oh!

bert-e commented Dec 14, 2023

Conflict

Uh oh!

benzekrimaha commented Dec 14, 2023

Uh oh!

bert-e commented Dec 14, 2023

Conflict

Uh oh!

benzekrimaha commented Dec 14, 2023

Uh oh!

bert-e commented Dec 14, 2023

Conflict

Uh oh!

benzekrimaha commented Dec 14, 2023

Uh oh!

bert-e commented Dec 14, 2023

Integration data created

Uh oh!

bert-e commented Dec 14, 2023

Waiting for approval

Uh oh!

anurag4DSB left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benzekrimaha commented Dec 14, 2023

Uh oh!

bert-e commented Dec 14, 2023

In the queue

Uh oh!

bert-e commented Dec 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

benzekrimaha commented Dec 7, 2023 •

edited

Loading

anurag4DSB left a comment •

edited

Loading