Repository navigation
refactor(server): access permissions - #2910
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
9e57248 to
cd46f29
Compare
brighteyed
left a comment
brighteyed
left a comment
There was a problem hiding this comment.
I like this solution! It really simplifies permission-checking for services
| const asset = await this._assetRepository.get(id); | ||
| if (!asset) { | ||
| const hasAccess = asset && (await this.access.hasPermission(authUser, Permission.ASSET_DELETE, id)); |
There was a problem hiding this comment.
Not a big deal, but maybe this code looks more natural:
const hasAccess = await this.access.hasPermission(authUser, Permission.ASSET_DELETE, id);
if (!hasAccess) {
result.push({ id, status: DeleteAssetStatusEnum.FAILED });
continue;
}
const asset = await this._assetRepository.get(id);
if (!asset) {
result.push({ id, status: DeleteAssetStatusEnum.FAILED });
continue;
}
| if (dto.userId && dto.userId !== authUser.id) { | ||
| await this.checkUserAccess(authUser, dto.userId); | ||
| } | ||
| await this.access.requirePermission(authUser, Permission.LIBRARY_READ, dto.userId || authUser.id); |
There was a problem hiding this comment.
I'd prefer to extract variable const userId = dto.userId || authUser.id like in getAssetByTimeBucket and getAssetCountByTimeBucket
Issue 1Not sure if this is related to this PR. In public shared links, drag and drop to upload returns Issue 2Cosmetic - on the web, with shared link disabled download, while multi selection enabled, the app bar showed the download button, although it doesn't work. Let me know if you want to address those two issues in this PR or we can do it in a subsequent one |
| // TODO: fix this to not use authUser.id for shared link access control | ||
| return this.repository.asset.hasOwnerAccess(authUser.id, id); | ||
|
|
||
| // case Permission.ALBUM_READ: |
There was a problem hiding this comment.
Is this for debugging, should we keep or remove these lines?
There was a problem hiding this comment.
I can remove them if you want. I believe all the commented out lines will be used in the future as more things migrate to this permission model.
| import { IAccessRepository } from './access.repository'; | ||
|
|
||
| export enum Permission { | ||
| // ASSET_CREATE = 'asset.create', |
There was a problem hiding this comment.
Are we planning to use these commented-out permission later?
There was a problem hiding this comment.
Yes, I added in everything when i was designing it, but decided not to add permission checks where none previously existed in this PR. In a subsequent one these will be enabled and used.
|
I'd guess issue one and two are pre-existing, and if so they can be fixed in a separate PR. If they are new bugs I'll fix them obviously. Those are good finds though and I can probably take a look at fixing them soon. |
|
I confirmed issue one and two exist in the demo site today, so they're not introduced with this PR. |
|
Created these to track the two (existing) bugs you found: |
Create an
AccessCorewhere all asset, album, etc. access checks can be consolidated. Eventually, hopefully, this logic can be enhanced with additional modifiers (additional share settings set on the user, album, asset, api key, etc.)