Expose user update profile bindings #9418
Conversation
| use super::utils::client_factory; | ||
|
|
||
| #[parsec_test(testbed = "coolorg", with_server)] | ||
| async fn ok(env: &TestbedEnv) { |
There was a problem hiding this comment.
We should probably test all combinations for the ok case:
- Standard -> Admin
- Standard -> Outsider
- Admin -> Standard
- Admin -> Outsider
- Outsider -> Standard
- Outsider -> Admin
Also we could add another test ok_idempotent for:
- Admin -> Admin
- Standard -> Standard
- Outsider -> Outsider
Also we should add tests for the "self downgrading" case (either ok or not ok depending on what is decided):
- Admin (self) -> Standard
- Admin (self) -> Outsider
| } | ||
|
|
||
| #[parsec_test(testbed = "coolorg", with_server)] | ||
| async fn not_allowed(env: &TestbedEnv) { |
There was a problem hiding this comment.
This tests that a user with Standard profile cannot update other user's profile.
We should test the same thing for a user with Outsider profile.
There was a problem hiding this comment.
No need for a news fragment since this PR does not have an impact on the end-user.
The news fragment should be added once the feature is implemented in the GUI.
| Stopped, | ||
| #[error("Cannot reach the server")] | ||
| Offline, | ||
| #[error("Cannot revoke oneself")] |
There was a problem hiding this comment.
The error message doesn't seem right (there is no revoking here)
| // TODO: should an admin be able to downgrade themselves | ||
| return Err(CertifUpdateUserProfileError::UserIsSelf); |
There was a problem hiding this comment.
This TODO needs to be addressed before merging the PR --> please update the PR description to add it as an open question.
Also, tests will need to be added either to accept downgrading or test that is not possible.
There was a problem hiding this comment.
You should have a look at the certificate ops validation code (look for check_user_update_certificate_consistency function)
tl;dr: the code contains // 2) Check the certificate is not self-signed ;-)
| fn from(value: ConnectionError) -> Self { | ||
| match value { | ||
| ConnectionError::NoResponse(_) => Self::Offline, | ||
| // TODO: handle organization expired and user revoked here ? |
There was a problem hiding this comment.
Should we address this TODO before merging the PR? If not, please should make there is an issue to track it.
There was a problem hiding this comment.
No this is unrelated to this PR: unfortunately all errors handle ConnectionError this may :'(
| // TODO: handle `strictly_greater_than` out of the client ballpark by | ||
| // returning an error |
There was a problem hiding this comment.
Should we address this TODO before merging the PR? If not, please should make there is an issue to track it.
There was a problem hiding this comment.
No, this comment is present in multiple place in the code, it is unrelated to this PR
(but we may want to check if there is an issue about this)
| return Err(CertifUpdateUserProfileError::UserIsSelf); | ||
| } | ||
|
|
||
| // 1) Build role certificate |
There was a problem hiding this comment.
| // 1) Build role certificate | |
| // 1) Build update user profile certificate |
| // TODO: handle `strictly_greater_than` out of the client ballpark by | ||
| // returning an error |
There was a problem hiding this comment.
No, this comment is present in multiple place in the code, it is unrelated to this PR
(but we may want to check if there is an issue about this)
| // TODO: should an admin be able to downgrade themselves | ||
| return Err(CertifUpdateUserProfileError::UserIsSelf); |
There was a problem hiding this comment.
You should have a look at the certificate ops validation code (look for check_user_update_certificate_consistency function)
tl;dr: the code contains // 2) Check the certificate is not self-signed ;-)
| fn from(value: ConnectionError) -> Self { | ||
| match value { | ||
| ConnectionError::NoResponse(_) => Self::Offline, | ||
| // TODO: handle organization expired and user revoked here ? |
There was a problem hiding this comment.
No this is unrelated to this PR: unfortunately all errors handle ConnectionError this may :'(
| ballpark_client_late_offset: f64, | ||
| }, | ||
| #[error("There is no key available in this realm for encryption")] | ||
| NoKey, |
There was a problem hiding this comment.
Please, take some extra time when creating an error to make sure the variants are meaningful for the use case 🙏
The whole point of having each function with a dedicated error is to precisely describe what can go wrong. I strongly suggest you to any new error empty, then gradually add items to it instead of copy/pasting from another one.
| ) -> Result<DoServerCommandOutcome, CertifUpdateUserProfileError> { | ||
| // 0) Sanity check to prevent generating and invalid certificate | ||
|
|
||
| if recipient == ops.device.user_id { |
There was a problem hiding this comment.
This check has already been done by the caller (i.e. update_profile), so two possibilities:
- remove the check from
update_profile - consider the check should be done in
update_profile, in this case the check done here is only for consistency and hence you should do an assert instead of returning an error
| use crate::EventTooMuchDriftWithServerClock; | ||
|
|
||
| #[derive(Debug, thiserror::Error)] | ||
| pub enum CertifUpdateUserProfileError { |
There was a problem hiding this comment.
Note every variant here should lead to a corresponding test
(protip: you can copy server_error/invalid_keys_bundle/invalid_response tests from libparsec/crates/client/tests/unit/certif/validate_child_manifest.rs to handle the InvalidKeysBundle / InvalidCertificate / Internal variants)
Fix #9381