Skip to content

Commit a954899

Browse files
jatindersingh93jatindersingh93
committed
Apply code review suggestion
1 parent 5005973 commit a954899

File tree

6 files changed

+49
-37
lines changed

6 files changed

+49
-37
lines changed

‎app/src/docs/v1.api-spec.yaml‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1818,7 +1818,7 @@ components:
18181818
example: ac246e31-c807-496c-bc93-cd8bc2f1b2b4
18191819
Query-BucketName:
18201820
in: query
1821-
name: name
1821+
name: displayName
18221822
description: A display name given to the bucket on creation
18231823
schema:
18241824
type: string
@@ -2735,7 +2735,7 @@ components:
27352735
status:
27362736
example: 403
27372737
detail:
2738-
example: The provided S3 credentials do not authorize this operation
2738+
example: User lacks permission to complete this action
27392739
Response-Gone:
27402740
title: Response Gone
27412741
type: object

‎app/src/middleware/authentication.js‎

Lines changed: 33 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -58,30 +58,40 @@ const currentUser = async (req, res, next) => {
5858
// Basic Authorization
5959
if (authorization.toLowerCase().startsWith('basic ')) {
6060
currentUser.authType = AuthType.BASIC;
61-
if (getConfigBoolean('basicAuth.s3AccessMode') && req.get('x-amz-endpoint') && req.get('x-amz-bucket')) {
62-
try {
63-
// This will validate with s3 bucket end point
64-
const base64Credentials = authorization.split(' ')[1];
65-
const credentials = Buffer.from(base64Credentials, 'base64').toString('utf-8');
66-
const [accessKeyId, secretAccessKey] = credentials.split(':');
67-
68-
const bucketSettings = {
69-
accessKeyId: accessKeyId,
70-
bucket: req.get('x-amz-bucket'),
71-
endpoint: req.get('x-amz-endpoint'),
72-
key: req.get('x-amz-key') ? req.get('x-amz-key') : '/',
73-
region: credentials.region || DEFAULTREGION,
74-
secretAccessKey: secretAccessKey,
75-
};
76-
const bucketHeader = await storageService.headBucket(bucketSettings);
77-
78-
if (bucketHeader?.$metadata?.httpStatusCode === 200) {
79-
currentUser.authType = AuthType.BASIC;
80-
delete bucketSettings.secretAccessKey;
81-
currentUser.bucketSettings = bucketSettings;
61+
if (getConfigBoolean('basicAuth.s3AccessMode')) {
62+
const amzEndpoint = req.get('x-amz-endpoint');
63+
const amzBucket = req.get('x-amz-bucket');
64+
65+
// Ensure both 'x-amz-endpoint' and 'x-amz-bucket' are either both provided or both missing
66+
if ((amzEndpoint && !amzBucket) || (!amzEndpoint && amzBucket)) {
67+
return next(new Problem(400,
68+
{ detail: 'Both x-amz-endpoint and x-amz-bucket must be provided together', instance: req.originalUrl }));
69+
}
70+
71+
if (amzEndpoint && amzBucket) {
72+
try {
73+
// This will validate with s3 bucket endpoint
74+
const base64Credentials = authorization.split(' ')[1];
75+
const credentials = Buffer.from(base64Credentials, 'base64').toString('utf-8');
76+
const [accessKeyId, secretAccessKey] = credentials.split(':');
77+
78+
const bucketSettings = {
79+
accessKeyId: accessKeyId,
80+
bucket: amzBucket,
81+
endpoint: amzEndpoint,
82+
region: credentials.region || DEFAULTREGION,
83+
secretAccessKey: secretAccessKey,
84+
};
85+
const bucketHeader = await storageService.headBucket(bucketSettings);
86+
87+
if (bucketHeader?.$metadata?.httpStatusCode === 200) {
88+
await storageService.headBucket(bucketSettings);
89+
delete bucketSettings.secretAccessKey;
90+
currentUser.bucketSettings = bucketSettings;
91+
}
92+
} catch (err) {
93+
return next(new Problem(403, { detail: 'Invalid authorization credentials', instance: req.originalUrl }));
8294
}
83-
} catch (err) {
84-
return next(new Problem(403, { detail: 'Invalid authorization credentials', instance: req.originalUrl }));
8595
}
8696
}
8797
}

‎app/src/middleware/authorization.js‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ const {
66
getAppAuthMode,
77
getCurrentIdentity,
88
getConfigBoolean,
9-
mixedQueryToArray } = require('../components/utils');
9+
mixedQueryToArray, stripDelimit } = require('../components/utils');
1010
const { NIL: SYSTEM_USER } = require('uuid');
1111
const {
1212
bucketPermissionService,
@@ -68,7 +68,8 @@ const checkS3BasicAccess = async (req, _res, next) => {
6868

6969
if (getConfigBoolean('basicAuth.s3AccessMode') && authType === AuthType.BASIC && bucketSettings) {
7070
// determine which buckets relate to the request
71-
let bucketIds = mixedQueryToArray(req.query.bucketId) || mixedQueryToArray(req.params.bucketId) || req.body.bucketId;
71+
let bucketIds = mixedQueryToArray(req.query.bucketId)
72+
|| mixedQueryToArray(req.params.bucketId) || req.body.bucketId;
7273
const objIds = mixedQueryToArray(req.query.objectId) || mixedQueryToArray(req.params.objectId) || req.body.objectId;
7374
const versionIds = mixedQueryToArray(req.query.versionId);
7475
const s3VersionIds = mixedQueryToArray(req.query.s3VersionId);
@@ -87,14 +88,16 @@ const checkS3BasicAccess = async (req, _res, next) => {
8788
const bucketData = {
8889
bucketId: bucketIds,
8990
bucket: bucketSettings.bucket,
90-
endpoint: bucketSettings.endpoint,
91+
endpoint: stripDelimit(bucketSettings.endpoint),
9192
accessKeyId: bucketSettings.accessKeyId,
9293
};
9394
const buckets = await bucketService.checkBucketBasicAccess(bucketData);
9495

9596
if (buckets.length != 0) {
9697
//bucketId params will be overwritten with passed or valid access bucketId.
9798
req.query.bucketId = buckets.length > 1 ? buckets : buckets[0];
99+
} else {
100+
return next(new Problem(403, { detail: 'Invalid authorization credentials', instance: req.originalUrl }));
98101
}
99102
} catch (err) {
100103
return next(new Problem(403, { detail: err.message, instance: req.originalUrl }));

‎app/src/routes/v1/user.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,14 @@ const router = require('express').Router();
22

33
const { userValidator } = require('../../validators');
44
const { userController } = require('../../controllers');
5-
const { checkAppMode } = require('../../middleware/authorization');
5+
const { checkAppMode, checkS3BasicAccess } = require('../../middleware/authorization');
66
const { requireSomeAuth } = require('../../middleware/featureToggle');
77

88
router.use(checkAppMode);
99
router.use(requireSomeAuth);
1010

1111
/** Search for users */
12-
router.get('/', userValidator.searchUsers, (req, res, next) => {
12+
router.get('/', checkS3BasicAccess, userValidator.searchUsers, (req, res, next) => {
1313
userController.searchUsers(req, res, next);
1414
});
1515

‎app/tests/unit/middleware/authentication.spec.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,7 @@ describe('currentUser', () => {
139139

140140
expect(req.currentUser).toBeTruthy();
141141
expect(req.currentUser).toHaveProperty('authType', AuthType.BASIC);
142-
expect(req.get).toHaveBeenCalledTimes(2);
142+
expect(req.get).toHaveBeenCalledTimes(3);
143143
expect(req.get).toHaveBeenCalledWith('Authorization');
144144
expect(checkBasicAuthSpy).toHaveBeenCalledTimes(1);
145145
});

‎app/tests/unit/services/object.spec.js‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,7 @@ describe('searchObjects', () => {
122122
objectPermission: [{
123123
permCode: 'READ',
124124
userId: 'ae8a58f6-62bc-4dd1-acef-79f123609d48'
125-
},{
125+
}, {
126126
permCode: 'UPDATE',
127127
userId: 'afcbad71-d2d5-4551-bd8b-18634fd8370e'
128128
}], ...params
@@ -150,9 +150,8 @@ describe('searchObjects', () => {
150150
expect(ObjectModel.modify).toHaveBeenNthCalledWith(4, 'filterPath', params.path);
151151
expect(ObjectModel.modify).toHaveBeenNthCalledWith(5, 'filterPublic', params.public);
152152
expect(ObjectModel.modify).toHaveBeenNthCalledWith(6, 'filterActive', params.active);
153-
expect(ObjectModel.modify).toHaveBeenNthCalledWith(
154-
7, 'filterVersionAttributes', params.mimeType, params.deleteMarker, params.latest
155-
);
153+
expect(ObjectModel.modify).toHaveBeenNthCalledWith(7, 'filterVersionAttributes',
154+
params.mimeType, params.deleteMarker, params.latest, params.versionId, params.s3VersionId);
156155
expect(ObjectModel.modify).toHaveBeenNthCalledWith(8, 'filterMetadataTag', {
157156
metadata: params.metadata,
158157
tag: params.tag
@@ -197,8 +196,8 @@ describe('searchObjects', () => {
197196
expect(ObjectModel.modify).toHaveBeenNthCalledWith(4, 'filterPath', params.path);
198197
expect(ObjectModel.modify).toHaveBeenNthCalledWith(5, 'filterPublic', params.public);
199198
expect(ObjectModel.modify).toHaveBeenNthCalledWith(6, 'filterActive', params.active);
200-
expect(ObjectModel.modify).toHaveBeenNthCalledWith(
201-
7, 'filterVersionAttributes', params.mimeType, params.deleteMarker, params.latest
199+
expect(ObjectModel.modify).toHaveBeenNthCalledWith(7, 'filterVersionAttributes',
200+
params.mimeType, params.deleteMarker, params.latest, params.versionId, params.s3VersionId
202201
);
203202
expect(ObjectModel.modify).toHaveBeenNthCalledWith(8, 'filterMetadataTag', {
204203
metadata: params.metadata,

0 commit comments

Comments
 (0)