-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feat: namespace access management #4576
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
base: main
Are you sure you want to change the base?
Conversation
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
first batch of comments
await knex.schema.createTable(TableName.NamespaceRole, (t) => { | ||
t.uuid("id").primary().defaultTo(knex.fn.uuid()); | ||
t.string("name").notNullable(); | ||
t.string("description"); | ||
t.string("slug").notNullable(); | ||
t.jsonb("permissions").notNullable(); | ||
t.uuid("namespaceId").notNullable(); | ||
t.foreign("namespaceId").references("id").inTable(TableName.Namespace).onDelete("CASCADE"); | ||
t.timestamps(true, true, true); | ||
t.unique(["name", "namespaceId"]); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we were moving away from slugs? or is this to keep consistency with org/project roles?
If so, shouldn't the unique be on slug
and not name
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FIxed and yes it's for keeping it similiar in all sides
name: "", | ||
slug: "", | ||
description: "", | ||
permissions: {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue with creating namespace role being blocked by permission validation error:
CleanShot.2025-09-23.at.14.00.11.mp4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
formName: "role" | ||
}, | ||
{ | ||
title: "Namespace Profile", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feel like this should probably be Namespace Settings? Profile confused me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm! May be let's discuss this - project permission follows the same convention
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
audit endpoints
}); | ||
ForbiddenError.from(namespacePermission).throwUnlessCan( | ||
NamespacePermissionIdentityActions.Edit, | ||
subject(NamespacePermissionSubjects.Identity, { identityId }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
if (identityOrgMembership.identity.namespace) | ||
throw new BadRequestError({ | ||
message: `Namespace identity with id ${identityId} membership cannot be deleted` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: extra space in error message
|
||
const totalCount = await namespaceIdentityMembershipDAL.getCountByNamespaceId(namespace.id, { search }); | ||
|
||
return { identityMemberships, totalCount }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we filter the memberships based off condition permission?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conditions are just not pushed yet. Will hold it as separate part.
namespaceId: namespace.id | ||
}); | ||
|
||
return { identityMemberships: docs, totalCount }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same thing; shouldn't we filter results based off conditional read with id?
Description 📣
Completed namespace first part of phase 1 which is the access management
Type ✨
Tests 🛠️
# Here's some code block to paste some code snippets