feat: Add access rule CRD to appcred provider#806
Conversation
gtema
left a comment
There was a problem hiding this comment.
please sort methods in all traits and impls that you modify alphabetically sorted. Otherwise looks fine
gtema
left a comment
There was a problem hiding this comment.
https://github.com/openstack-experimental/keystone/blob/main/crates/identity-driver-sql/src/local_user/create.rs#L44, https://github.com/openstack-experimental/keystone/blob/main/crates/identity-driver-sql/src/password/set.rs#L44 show that in some cases we need to invoke backend functions in the transaction context. For access rules it is definitely the case where create/delete operations would be bound to the parenting application_credential create/delete. This is a small interface change that will not even require unittest changes. Otherwise fine
gtema
left a comment
There was a problem hiding this comment.
one, hopefully final thing. Please add "user_id" into the AccessRule core-types and correspondingly expose it from the database. It would be populated during creation and is definitely necessary in output. The AccessRule struct already contain a comment that in the DB user_id must be made not optional, but we can't do this in the keystone-rs project
| path: Set(rule.path), | ||
| service: Set(rule.service), | ||
| external_id: Set(Some(rule.id.unwrap_or(Uuid::new_v4().simple().to_string()))), | ||
| user_id: Set(Some(user_id.as_ref().to_string())), |
There was a problem hiding this comment.
we should extend the AccessRuleCreate to have user_id: String
Extends the application credential provider with access rule CRD (create, read, delete, no update), following the same layering as application credentials.
Closes #776
Note: this contribution was developed with AI assistance.