-
Notifications
You must be signed in to change notification settings - Fork 554
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
✨ Ozone team member manager #2460
Conversation
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.
general direction looks good!
"name": "ModeratorAlreadyExists", | ||
"description": "The user is already a moderator" |
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.
probably UserAlreadyExists
and "already a moderator" if we have a specific role called "Moderator"?
"description": "The user being deleted is not a moderator" | ||
}, | ||
{ | ||
"name": "LastAdmin", |
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.
maybe OnlyRemainingAdmin
? "LastAdmin" would be confusing to me out of context
"defs": { | ||
"main": { | ||
"type": "procedure", | ||
"description": "Add a user as moderator in the ozone service.", |
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.
Would be good docs to mention in each of these descriptions what privileges are required to use the endpoint. Eg, all require auth, but maybe listUsers
only requires an ozone user of any role?
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.
This is quite the chunk of work! Good stuff, going to be a big upgrade 👍
alright we should be close now. a few major things to review
|
packages/ozone/src/team/index.ts
Outdated
.orderBy('createdAt', 'asc') | ||
.execute() | ||
|
||
return { members, cursor: members.slice(-1)[0]?.createdAt.toISOString() } |
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.
return { members, cursor: members.slice(-1)[0]?.createdAt.toISOString() } | |
return { members, cursor: members.at(-1)?.createdAt.toISOString() } |
packages/ozone/src/team/index.ts
Outdated
updatedAt: now, | ||
createdAt: now, | ||
}) | ||
.onConflict((oc) => oc.column('did').doUpdateSet({ role })) |
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.
Should we maintain updatedAt
and lastUpdatedBy
here too?
packages/ozone/src/team/index.ts
Outdated
.updateTable('member') | ||
.where('did', '=', did) | ||
.set({ | ||
...updates, | ||
updatedAt: updates.updatedAt || new Date(), | ||
}) |
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.
There is a bit of a security issue here, since updates
may contain fields not explicitly Pick
ed out of Selectable<Member>
, and they end up passed directly through to the query. For example, imagine if did
were a field on updates
.
Looking at the team.updateMember
endpoint, I think the user could end-up successfully changing the user's createdAt
time through the API. On its own that isn't a critical issue, but as we add new columns to the members
table we might run into more sensitive issues.
packages/ozone/src/auth-verifier.ts
Outdated
// Always allow serviceDid to access as admin | ||
if (iss === this.serviceDid) { | ||
return { | ||
credentials: { | ||
type: 'standard', | ||
iss, | ||
aud: payload.aud, | ||
isAdmin: true, | ||
isModerator: true, | ||
isTriage: true, | ||
}, | ||
} | ||
} |
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.
Perhaps we could avoid this exception if we folded the account into seedInitialMembers()
, particularly now that there's a guard on deleting the user.
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.
You know, the more I think about it we may want to keep the deletion guard but not add any other exceptions for the service DID. I think there could be legitimate reasons for a deployment to not want to make the service DID a team member.
For the Ozone distribution we configure the service DID to be an admin anyway, so most deployments will have this by default. Perhaps the deletion rule should be to ensure you don't delete, demote, or disable yourself. That should be enough to ensure there's no way to lock yourself out, which I think is the main thing we're going for.
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.
yeah that makes sense!
packages/ozone/src/auth-verifier.ts
Outdated
const { isAdmin, isModerator, isTriage } = | ||
await this.teamService.getMemberRole(member) |
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.
const { isAdmin, isModerator, isTriage } = | |
await this.teamService.getMemberRole(member) | |
const { isAdmin, isModerator, isTriage } = | |
this.teamService.getMemberRole(member) |
Looking forward to this! Let me know when I can give it a spin in staging. |
This PR adds lexicons for a full set of CRUD actions to manage team members with 3 predefined roles (that bluesky currently uses).
Right now, team members are maintained via environment variables. This PR will move the storage to a table in the database. In order to maintain backward compatibility
In order to keep members configured via env vars compatible with this implementation, on boot, we iterate over those and upsert into the table as needed.