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

Accept array of permCodes for user creating a bucket #264

Merged
merged 1 commit into from
Jun 20, 2024
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
8 changes: 6 additions & 2 deletions app/src/controllers/bucket.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ const Problem = require('api-problem');
const { UniqueViolationError } = require('objection');
const { NIL: SYSTEM_USER } = require('uuid');

const { DEFAULTREGION } = require('../components/constants');
const { DEFAULTREGION, Permissions } = require('../components/constants');
const errorToProblem = require('../components/errorToProblem');
const log = require('../components/log')(module.filename);
const {
Expand Down Expand Up @@ -117,11 +117,15 @@ const controller = {
await controller._validateCredentials(data);
data.userId = await userService.getCurrentUserId(getCurrentIdentity(req.currentUser, SYSTEM_USER));

// if permCodes array (eg: ['READ', 'UPDATE'] or []) was provided use that (de-duped),
// otherwise assign all permissions
data.permCodes = req.body.permCodes ? Array.from(new Set(req.body.permCodes)) : Object.values(Permissions);

response = await bucketService.create(data);
} catch (e) {
// If bucket exists, check if credentials precisely match
if (e instanceof UniqueViolationError) {
// Grant all permissions if credentials precisely match
// Grant permissions if credentials precisely match
response = await bucketService.checkGrantPermissions(data).catch(permErr => {
next(new Problem(403, { detail: permErr.message, instance: req.originalUrl }));
});
Expand Down
14 changes: 12 additions & 2 deletions app/src/docs/v1.api-spec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,9 @@ paths:
description: >-
Creates a bucket record. Bucket should exist in S3. If the set of
`bucket`, `endpoint` and `key` match an existing record, the user will
be added to that existing bucket with full permissions instead of
generating a new bucket record.
be added to that existing bucket with the provided permissions instead of
generating a new bucket record. This endpoint can be used to grant the current user
permission to upload to a new or existing bucket.
operationId: createBucket
tags:
- Bucket
Expand Down Expand Up @@ -2395,6 +2396,15 @@ components:
type: string
default: /
example: coms/env
permCodes:
type: array
items:
type: string
description: >-
Optional array of permCodes for each permission being given to the current user.
Accepts any of `"READ", "CREATE", "UPDATE", "DELETE", "MANAGE"`.
Defaults to all permCodes if unspecified.
example: ["READ", "CREATE", "UPDATE"]
Request-CreateBucketChild:
title: Request Create Bucket Child
type: object
Expand Down
19 changes: 10 additions & 9 deletions app/src/services/bucket.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
const { v4: uuidv4, NIL: SYSTEM_USER } = require('uuid');

const bucketPermissionService = require('./bucketPermission');
const { Permissions } = require('../components/constants');
const { Bucket } = require('../db/models');

/**
Expand All @@ -10,12 +9,13 @@ const { Bucket } = require('../db/models');
const service = {
/**
* @function checkGrantPermissions
* Grants a user full permissions to the bucket if the data precisely matches
* Grants a user provided permissions to the bucket if the data precisely matches
* accessKeyId and secretAccessKey values.
* @param {string} data.accessKeyId The S3 bucket access key id
* @param {string} data.bucket The S3 bucket identifier
* @param {string} data.endpoint The S3 bucket endpoint
* @param {string} data.key The relative S3 key/subpath managed by this bucket
* @param {string} data.permCodes Permissions to give to current user for the bucket
* @param {string} data.secretAccessKey The S3 bucket secret access key
* @param {object} [etrx=undefined] An optional Objection Transaction object
* @returns {Promise<object>} The result of running the insert operation
Expand All @@ -37,9 +37,9 @@ const service = {
bucket.accessKeyId === data.accessKeyId &&
bucket.secretAccessKey === data.secretAccessKey
) {
// Add all permission codes for the uploader
if (data.userId && data.userId !== SYSTEM_USER) {
const perms = Object.values(Permissions).map((p) => ({
// Add permissions
if (data.permCodes.length && data.userId && data.userId !== SYSTEM_USER) {
TimCsaky marked this conversation as resolved.
Show resolved Hide resolved
const perms = data.permCodes.map((p) => ({
userId: data.userId,
permCode: p
}));
Expand All @@ -59,12 +59,13 @@ const service = {

/**
* @function create
* Create a bucket record and give the uploader (if authed) permissions
* Create a bucket record and give the current user the provided permissions
* @param {string} data.bucketName The user-defined bucket name identifier
* @param {string} data.accessKeyId The S3 bucket access key id
* @param {string} data.bucket The S3 bucket identifier
* @param {string} data.endpoint The S3 bucket endpoint
* @param {string} data.key The relative S3 key/subpath managed by this bucket
* @param {string} data.permCodes Permissions to give to current user for the bucket
* @param {string} data.secretAccessKey The S3 bucket secret access key
* @param {string} [data.region] The optional S3 bucket region
* @param {boolean} [data.active] The optional active flag - defaults to true if undefined
Expand Down Expand Up @@ -93,9 +94,9 @@ const service = {

const response = await Bucket.query(trx).insert(obj).returning('*');

// Add all permission codes for the uploader
if (data.userId && data.userId !== SYSTEM_USER) {
const perms = Object.values(Permissions).map((p) => ({
// Add permissions for a current oidc user
if (data.permCodes.length && data.userId && data.userId !== SYSTEM_USER) {
TimCsaky marked this conversation as resolved.
Show resolved Hide resolved
const perms = data.permCodes.map((p) => ({
userId: data.userId,
permCode: p
}));
Expand Down
4 changes: 3 additions & 1 deletion app/src/validators/bucket.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ const Joi = require('joi');

const { scheme, type } = require('./common');
const { validate } = require('../middleware/validation');
const { Permissions } = require('../components/constants');

const schema = {
createBucket: {
Expand All @@ -13,7 +14,8 @@ const schema = {
key: Joi.string().max(255).trim().strict(),
secretAccessKey: Joi.string().max(255).required(),
region: Joi.string().max(255),
active: type.truthy
active: type.truthy,
permCodes: Joi.array().items(...Object.values(Permissions))
}).required(),
},

Expand Down
5 changes: 3 additions & 2 deletions app/tests/unit/controllers/bucket.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const {
userService,
} = require('../../../src/services');
const utils = require('../../../src/components/utils');
const { Permissions } = require('../../../src/components/constants');

const mockResponse = () => {
const res = {};
Expand Down Expand Up @@ -74,7 +75,7 @@ describe('createBucket', () => {
expect(getCurrentUserIdSpy).toHaveBeenCalledTimes(1);
expect(getCurrentUserIdSpy).toHaveBeenCalledWith(USR_IDENTITY);
expect(createSpy).toHaveBeenCalledTimes(1);
expect(createSpy).toHaveBeenCalledWith({ ...req.body, userId: USR_ID });
expect(createSpy).toHaveBeenCalledWith({ ...req.body, userId: USR_ID, permCodes: Object.values(Permissions) });

expect(res.status).toHaveBeenCalledWith(201);
});
Expand Down Expand Up @@ -168,7 +169,7 @@ describe('createBucket', () => {
expect(getCurrentUserIdSpy).toHaveBeenCalledTimes(1);
expect(getCurrentUserIdSpy).toHaveBeenCalledWith(USR_IDENTITY);
expect(createSpy).toHaveBeenCalledTimes(1);
expect(createSpy).toHaveBeenCalledWith({ ...req.body, userId: USR_ID });
expect(createSpy).toHaveBeenCalledWith({ ...req.body, userId: USR_ID, permCodes: Object.values(Permissions) });

expect(next).toHaveBeenCalledTimes(1);
expect(next).toHaveBeenCalledWith(new Problem(500, 'Internal Server Error'));
Expand Down
1 change: 1 addition & 0 deletions app/tests/unit/services/bucket.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ const data = {
secretAccessKey: 'secretaccesskey',
region: 'region',
active: 'true',
permCodes: ['READ'],
createdBy: SYSTEM_USER,
userId: SYSTEM_USER,
lastSyncRequestedDate: new Date().toISOString()
Expand Down
Loading