Conversation
| // pub async fn user_roles(&self) -> Result<HashSet<Role>, Error<S::Error>> { | ||
| // self.regulator.user_roles(&User(self.user_id)).await | ||
| // } |
There was a problem hiding this comment.
You can leave this function, you don't have to make changes in parts of the code that'll go away soon. If you can remove them now, good, but if you need to adapt some part's that'll be removed later, it's not worth it.
There was a problem hiding this comment.
I removed the Authentication.user_roles because it was unused.
I removed the Authorizer.user_roles because it was unused (although this time the compiler didn't see it).
I didn't removed the Regulator.user_roles because it was used in other Regulator methods.
| let roles = match system.authorize(subject_roles).await?.access().await? { | ||
| Ok(roles) => roles, | ||
| Err(Rejection::NoSuchUser(_)) | Err(Rejection::NoSuchGroup(_)) => { | ||
| unreachable!("checked by parse_and_fetch_subject") |
There was a problem hiding this comment.
No the function only parses the argument, it does not always check the subjects exist in the database.
| Err(Rejection::NoSuchUser(_)) | Err(Rejection::NoSuchGroup(_)) => { | ||
| unreachable!("checked by parse_and_fetch_subject") | ||
| }, |
| assert_eq!( | ||
| regulator() | ||
| .group_roles(&Group(i64::MAX)) | ||
| // regulator() | ||
| // .group_roles(&Group(i64::MAX)) | ||
| // .await | ||
| // .expect("should query roles successfully"), | ||
| v2::subject_roles(Subject::Group(model::Group(i64::MAX))) | ||
| .authorize(&special_authorizers::Authorize(regulator().openfga())) | ||
| .await | ||
| .expect("should query roles successfully"), | ||
| HashSet::new() | ||
| .expect("should query group roles successfully") | ||
| .unwrap_authorized() | ||
| .await, | ||
| Vec::<Role>::new() | ||
| ); |
There was a problem hiding this comment.
It looks like this is the last part of the test that needed porting. Let's just rewrite the test without the old Regulator and Authorizer in v2.rs.
There was a problem hiding this comment.
The old Authorizer is still used for check_roles (which I don't see yet on dev), and the regulator is used for ensure_user and ensure_group (which I have no idea what they do).
| Ok(Json(WhoamiResponse { | ||
| // TODO: don't return -1 and a hardcoded name, return a different schema instead, requires frontend changes | ||
| id: auth.user_id()?.unwrap_or(-1), | ||
| name: auth.user_name()?.unwrap_or_else(|| "OSRD user".to_string()), | ||
| roles: auth.user_roles().await?.into_iter().collect(), | ||
| roles: roles, | ||
| })) |
There was a problem hiding this comment.
Best to get the Authentication as well and match on it to return the right roles (unauthenticated = 401, skip = admin, otherwise it's roles).
You should rebase on top of e2fda96 to get the last fix (should be merged soon).
There was a problem hiding this comment.
I don't understand.
SaumonDesMers
force-pushed
the
sga/authz_v2_subject_roles
branch
from
d8210aa to
74748c4
Compare
2 participants
I started migrating
user_rolestosubject_roles, but I'm a bit lost and the testviews::authz::tests::whoami_authorization_disabledfail (roles are empty instead of Admin).I would love a review if possible.