feat(@xen-orchestra/rest-api): expose get user and get users #8494
Conversation
| href: '/rest/v0/users/722d17b9-699b-49d2-8193-be1ac573d3de', | ||
| }, | ||
| { | ||
| email: 'testName', |
| async getUsers( | ||
| @Request() req: ExRequest, | ||
| @Query() fields?: string, | ||
| @Query() filter?: string, | ||
| @Query() limit?: number | ||
| ): Promise<string[] | WithHref<Partial<Unbrand<XoUser>>>[]> { | ||
| return this.sendObjects(Object.values(await this.getObjects({ filter, limit })), req) | ||
| } | ||
|
|
||
| /** | ||
| * @example id "722d17b9-699b-49d2-8193-be1ac573d3de" | ||
| */ | ||
| @Example(user) | ||
| @Get('{id}') | ||
| @Response(notFoundResp.status, notFoundResp.description) | ||
| getUser(@Path() id: string): Promise<Unbrand<XoUser>> { | ||
| return this.getObject(id as XoUser['id']) | ||
| } |
There was a problem hiding this comment.
These methods will return the pw_hash of a user.
I don't think it's a good practice to return the password (even if it's a hash).
@fbeauchamp Are you agree to not return the hashed password? (The JSON-RPC API does not return it)
There was a problem hiding this comment.
I agree, even if it's only a salted hash, it has no value , and only add one more risks (at leats questions by user asking if it's safe)
Co-authored-by: Mathieu <[email protected]>
| partialUser(user: XoUser): Partial<Unbrand<XoUser>> { | ||
| return { | ||
| ...user, | ||
| pw_hash: '***obfuscated***', | ||
| } | ||
| } |
There was a problem hiding this comment.
The type is not valid.
Partial<T> means that all properties of T become optional.
We don't want all other props to be optional simply because of pw_hash
There was a problem hiding this comment.
Only use Unbrand<T> when documenting an endpoint.
Unbrand is a type that converts Branded<string> to string to make it compatible with OpenAPI.
| @Query() limit?: number | ||
| ): Promise<string[] | WithHref<Partial<Unbrand<XoUser>>>[]> { | ||
| const users = Object.values(await this.getObjects({ filter, limit })).map(this.partialUser) | ||
| return this.sendObjects(users as XoUser[], req) |
There was a problem hiding this comment.
Avoid to use the as keyword expect if no other solution
| getAllCollectionObjects(): Promise<XoUser[]> { | ||
| return this.restApi.xoApp.getAllUsers() | ||
| } | ||
| getCollectionObject(id: XoUser['id']): Promise<XoUser> { | ||
| return this.restApi.xoApp.getUser(id) | ||
| } |
There was a problem hiding this comment.
IMO, these methods should return sanitized users. This way, every time we call this.getObject/this.getObjects, we will get a sanitized user. The REST API does not need to manage user passwords.
| { | ||
| permission: 'admin', | ||
| name: '[email protected]', | ||
| id: '722d17b9-699b-49d2-8193-be1ac573d3de', | ||
| href: '/rest/v0/users/722d17b9-699b-49d2-8193-be1ac573d3de', | ||
| }, |
There was a problem hiding this comment.
Try to add another user in your example.
You should have the same number of result between userIds and partialUsers.
| partialUser(user: XoUser): Partial<Unbrand<XoUser>> { | ||
| return { | ||
| ...user, | ||
| pw_hash: '***obfuscated***', |
There was a problem hiding this comment.
pw_hash can be undefined. Don't obfuscate the password if no password is defined
Description
Short explanation of this PR (feel free to re-use commit message)
Checklist
Fixes #007,See xoa-support#42,See https://...)Introduced byCHANGELOG.unreleased.mdReview process
Notes: