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.

Sign up for GitHub

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

Fix adding multiple users to buckets and objects #151

Merged
merged 6 commits into from
Dec 13, 2023
Merged

Fix adding multiple users to buckets and objects #151

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9c14383
Fix adding multiple users to buckets and objects
wilwong89 Dec 8, 2023
ef0c176
Do not auto assign roles on adding user perms for bucket or object
wilwong89 Dec 8, 2023
e6686da
Fix typescript return signature in permissionStore.ts and return valu…
wilwong89 Dec 9, 2023
44d5096
Fix boolean return type, remove manager level perms check, copy perms…
wilwong89 Dec 9, 2023
f4b91e2
Clone permission object array by value
wilwong89 Dec 11, 2023
6b30b6a
Remove unused imports
wilwong89 Dec 11, 2023
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
34 changes: 3 additions & 31 deletions frontend/src/components/object/ObjectPermission.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,12 @@ import { onBeforeMount, ref } from 'vue';
import { FontAwesomeIcon } from '@fortawesome/vue-fontawesome';

import { ObjectPermissionAddUser, ObjectPublicToggle } from '@/components/object';
import { useAlert } from '@/composables/useAlert';
import { Button, Checkbox, Column, DataTable } from '@/lib/primevue';
import { useAuthStore, useObjectStore, usePermissionStore } from '@/store';
import { Permissions } from '@/utils/constants';

import type { Ref } from 'vue';
import type { COMSObject, UserPermissions } from '@/types';
import type { COMSObject } from '@/types';

// Props
type Props = {
Expand All @@ -30,46 +29,19 @@ const showSearchUsers: Ref<boolean> = ref(false);
const object: Ref<COMSObject | undefined> = ref(undefined);

// Actions
const removeManageAlert = useAlert('Warning', 'Cannot remove last user with MANAGE permission.');

const cancelSearchUsers = () => {
showSearchUsers.value = false;
};

const removeObjectUser = (userId: string) => {
const managers = getMappedObjectToUserPermissions.value.filter((x: UserPermissions) => x.manage);
if (managers.length === 1 && managers[0].userId === userId) {
removeManageAlert.show();
} else {
permissionStore.removeObjectUser(props.objectId, userId);
}
permissionStore.removeObjectUser(props.objectId, userId);
};

const updateObjectPermission = (value: boolean, userId: string, permCode: string) => {
if (value) {
permissionStore.addObjectPermission(props.objectId, userId, permCode);
} else {
const managers = getMappedObjectToUserPermissions.value.filter((x: UserPermissions) => x.manage);

const userPerms: UserPermissions = getMappedObjectToUserPermissions.value.find(
(x: UserPermissions) => x.userId === userId
) as UserPermissions;

// When READ is unticked check if they are the last remaining user with MANAGE
const lastManager = () => permCode === Permissions.READ && managers.length === 1 && userPerms.manage;

// Due to 2-way binding we check if there are no managers left when MANAGE is unticked
const noManagers = () => permCode === Permissions.MANAGE && !managers.length;

// Disallow removable of final MANAGE permission
if (lastManager() || noManagers()) {
removeManageAlert.show();

if (permCode === Permissions.READ) userPerms.read = true;
if (permCode === Permissions.MANAGE) userPerms.manage = true;
} else {
permissionStore.deleteObjectPermission(props.objectId, userId, permCode);
}
permissionStore.deleteObjectPermission(props.objectId, userId, permCode);
}
};

Expand Down
82 changes: 59 additions & 23 deletions frontend/src/store/permissionStore.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,28 +53,35 @@ export const usePermissionStore = defineStore('permission', () => {
};

// Actions
async function addBucketPermission(bucketId: string, userId: string, permCode: string) {
async function addBucketPermission(bucketId: string, userId: string, permCode: string): Promise<void> {
try {
appStore.beginIndeterminateLoading();
await permissionService.bucketAddPermissions(bucketId, [{ userId, permCode }]);
} catch (error: any) {
toast.error('Adding bucket permission', error);
} finally {
await fetchBucketPermissions();

const initialPerms = [...state.mappedBucketToUserPermissions.value];
await mapBucketToUserPermissions(bucketId);
const afterPerms = [...state.mappedBucketToUserPermissions.value];

const results = initialPerms.filter(({ userId: id1 }) => !afterPerms.some(({ userId: id2 }) => id2 === id1));
state.mappedBucketToUserPermissions.value = state.mappedBucketToUserPermissions.value.concat(results);

appStore.endIndeterminateLoading();
}
}

function addBucketUser(user: UserPermissions) {
function addBucketUser(user: UserPermissions): void {
try {
getters.getMappedBucketToUserPermissions.value.push(user);
} catch (error: any) {
toast.error('Adding bucket user', error);
}
}

async function addObjectPermission(objectId: string, userId: string, permCode: string) {
async function addObjectPermission(objectId: string, userId: string, permCode: string): Promise<void> {
try {
appStore.beginIndeterminateLoading();
await permissionService.objectAddPermissions(objectId, [{ userId, permCode }]);
Expand All @@ -87,33 +94,39 @@ export const usePermissionStore = defineStore('permission', () => {
toast.error('Adding object permission', error);
} finally {
await fetchObjectPermissions();

const initialPerms = [...state.mappedObjectToUserPermissions.value];
await mapObjectToUserPermissions(objectId);
const afterPerms = [...state.mappedObjectToUserPermissions.value];

const results = initialPerms.filter(({ userId: id1 }) => !afterPerms.some(({ userId: id2 }) => id2 === id1));
state.mappedObjectToUserPermissions.value = state.mappedObjectToUserPermissions.value.concat(results);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wonder if this before and after comparison is necessary. i think line 91 await permissionService.objectAddPermissions(objectId, [{ userId, permCode: Permissions.READ }]); returns the full details of the permission added

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The data returned by line 91 does not return the full details, it returns a different object that is slightly different. For example, the data returned does not have the name or email fields and if the previous user permission did not already have the READ perm code and another (non-READ) perm code is added, the data returned suggests that only READ was updated.


appStore.endIndeterminateLoading();
}
}

function addObjectUser(user: UserPermissions) {
function addObjectUser(user: UserPermissions): void {
try {
getters.getMappedObjectToUserPermissions.value.push(user);
} catch (error: any) {
toast.error('Adding object user', error);
}
}

async function deleteBucketPermission(bucketId: string, userId: string, permCode: string) {
async function deleteBucketPermission(bucketId: string, userId: string, permCode: string): Promise<void>{
try {
appStore.beginIndeterminateLoading();
await permissionService.bucketDeletePermission(bucketId, { userId, permCode });
} catch (error: any) {
toast.error('Deleting bucket permission', error);
} finally {
await fetchBucketPermissions();
await mapBucketToUserPermissions(bucketId);
await removeBucketUserPermsHandler(bucketId, userId);
appStore.endIndeterminateLoading();
}
}

async function deleteObjectPermission(objectId: string, userId: string, permCode: string) {
async function deleteObjectPermission(objectId: string, userId: string, permCode: string): Promise<void> {
try {
appStore.beginIndeterminateLoading();
await permissionService.objectDeletePermission(objectId, { userId, permCode });
Expand All @@ -125,13 +138,14 @@ export const usePermissionStore = defineStore('permission', () => {
} catch (error: any) {
toast.error('Deleting object permission', error);
} finally {
await fetchObjectPermissions();
await mapObjectToUserPermissions(objectId);
await removeObjectUserPermsHandler(objectId, userId);
appStore.endIndeterminateLoading();
}
}

async function fetchBucketPermissions(params: BucketSearchPermissionsOptions = {}) {
async function fetchBucketPermissions(
params: BucketSearchPermissionsOptions = {}
): Promise<Array<BucketPermission> | void> {
try {
appStore.beginIndeterminateLoading();

Expand All @@ -158,7 +172,9 @@ export const usePermissionStore = defineStore('permission', () => {
}
}

async function fetchObjectPermissions(params: ObjectSearchPermissionsOptions = {}) {
async function fetchObjectPermissions(
params: ObjectSearchPermissionsOptions = {}
): Promise<Array<COMSObjectPermission> | undefined> {
try {
appStore.beginIndeterminateLoading();

Expand Down Expand Up @@ -186,13 +202,13 @@ export const usePermissionStore = defineStore('permission', () => {
}
}

function isBucketActionAllowed(bucketId: string, userId?: string, permCode?: string) {
function isBucketActionAllowed(bucketId: string, userId?: string, permCode?: string): boolean {
return state.bucketPermissions.value.some(
(x: BucketPermission) => x.bucketId === bucketId && x.userId === userId && x.permCode === permCode
);
}

function isObjectActionAllowed(objectId: string, userId?: string, permCode?: string, bucketId?: string) {
function isObjectActionAllowed(objectId: string, userId?: string, permCode?: string, bucketId?: string): boolean {
const bucketPerm = state.bucketPermissions.value.some(
(x: BucketPermission) => x.bucketId === bucketId && x.userId === userId && x.permCode === permCode
);
Expand All @@ -204,15 +220,15 @@ export const usePermissionStore = defineStore('permission', () => {
return bucketPerm || objectPerm;
}

function isUserElevatedRights() {
function isUserElevatedRights(): boolean {
const idp = getConfig.value.idpList.find(
(provider: IdentityProvider) => provider.idp === getProfile.value?.identity_provider
);

return !!idp?.elevatedRights;
}

async function mapBucketToUserPermissions(bucketId: string) {
async function mapBucketToUserPermissions(bucketId: string): Promise<void> {
try {
appStore.beginIndeterminateLoading();
const bucketPerms = state.bucketPermissions.value.filter((x: BucketPermission) => x.bucketId === bucketId);
Expand Down Expand Up @@ -248,7 +264,7 @@ export const usePermissionStore = defineStore('permission', () => {
}
}

async function mapObjectToUserPermissions(objectId: string) {
async function mapObjectToUserPermissions(objectId: string): Promise<void> {
try {
appStore.beginIndeterminateLoading();
const objectPerms = state.objectPermissions.value.filter((x: COMSObjectPermission) => x.objectId === objectId);
Expand Down Expand Up @@ -284,7 +300,7 @@ export const usePermissionStore = defineStore('permission', () => {
}
}

async function removeBucketUser(bucketId: string, userId: string) {
async function removeBucketUser(bucketId: string, userId: string): Promise<void> {
try {
appStore.beginIndeterminateLoading();
for (const value of Object.values(Permissions)) {
Expand All @@ -293,13 +309,23 @@ export const usePermissionStore = defineStore('permission', () => {
} catch (error: any) {
toast.error('Removing bucket user', error);
} finally {
await fetchBucketPermissions();
await mapBucketToUserPermissions(bucketId);
await removeBucketUserPermsHandler(bucketId, userId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same with these.. not sure if you need the new handler function.. maybe you can drop this permission from the store here.
line 307:
deletedPermission = await permissionService.bucketDeletePermission(bucketId, { userId, permCode: value });

 state.mappedBucketToUserPermissions.value = state.mappedBucketToUserPermissions.value.filter(p => p.id !== deletedPermission.id)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using your method to read the deleted permission and then dropping it from the local state requires storing an array of deleted permissions returned form line, iteratively checking that list and removing to remove the permissions from the local state, and then finally another check to see if all permissions were removed from an entry in the local list before removing the entry entirely. I think instead the current method of doing a simple diff from before and after is the better method.

appStore.endIndeterminateLoading();
}
}

async function removeObjectUser(objectId: string, userId: string) {
async function removeBucketUserPermsHandler(bucketId: string, userId: string): Promise<void> {
await fetchBucketPermissions();

const initialPerms = [...state.mappedBucketToUserPermissions.value.filter(({ userId: id }) => id !== userId)];
await mapBucketToUserPermissions(bucketId);
const afterPerms = [...state.mappedBucketToUserPermissions.value];

const results = initialPerms.filter(({ userId: id1 }) => !afterPerms.some(({ userId: id2 }) => id2 === id1));
state.mappedBucketToUserPermissions.value = state.mappedBucketToUserPermissions.value.concat(results);
}

async function removeObjectUser(objectId: string, userId: string): Promise<void> {
try {
appStore.beginIndeterminateLoading();

Expand All @@ -312,12 +338,22 @@ export const usePermissionStore = defineStore('permission', () => {
} catch (error: any) {
toast.error('Removing object user', error);
} finally {
await fetchObjectPermissions();
await mapObjectToUserPermissions(objectId);
await removeObjectUserPermsHandler(objectId, userId);
appStore.endIndeterminateLoading();
}
}

async function removeObjectUserPermsHandler(objectId: string, userId: string): Promise<void> {
await fetchObjectPermissions();

const initialPerms = [...state.mappedObjectToUserPermissions.value.filter(({ userId: id }) => id !== userId)];
await mapObjectToUserPermissions(objectId);
const afterPerms = [...state.mappedObjectToUserPermissions.value];

const results = initialPerms.filter(({ userId: id1 }) => !afterPerms.some(({ userId: id2 }) => id2 === id1));
state.mappedObjectToUserPermissions.value = state.mappedObjectToUserPermissions.value.concat(results);
}

return {
// State
...state,
Expand Down
2 changes: 1 addition & 1 deletion frontend/src/views/list/ListObjectsView.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ onBeforeMount(async () => {
const router = useRouter();

const permResponse = await permissionStore.fetchBucketPermissions({ userId: getUserId.value, objectPerms: true });
if (!permResponse.some((x: BucketPermission) => x.bucketId === props.bucketId)) {
if (!permResponse?.some((x: BucketPermission) => x.bucketId === props.bucketId)) {
router.replace({ name: RouteNames.FORBIDDEN });
} else {
ready.value = true;
Expand Down