fix: rbac fixes and some refactoring #281
Conversation
* Call GetAllSubjects() instead for listing RBAC roles. GetAllRoles() only gets roles that have actually been assigned to a user. This is because 'p' policy lines are defined for the roles. 'g' policy lines are for assignment of a role to a user. GetAllRoles() calls the 'g' line at index 2 to list roles. * Fixed a bug with role:readonly looking for wildcard 'get' action which no longer exists because our actions became more specific - e.g. 'pipelines:runs, get-run' rather than 'pipelines:runs, get' * Added a check to make sure all newly created roles have a prefix of 'role:x' * Made the instantiation of the rbac more testable by using DI instead of instantiating concrete resources within the constructor * Add missing service.go unit tests
|
@Skarlso some fixes for RBAC, more tests, and general quality of life changes. |
|
/test |
|
Command received. Building new test image. |
|
Failed to fetch and build pr. |
|
Oh f*ck off. :D @speza You have a |
|
@Skarlso 🤣 just finding bugs in your code bro! |
|
yeah :D |
|
/test rbac-fixes-281 |
3 similar comments
|
/test rbac-fixes-281 |
|
/test rbac-fixes-281 |
|
/test rbac-fixes-281 |
|
Command received. Building new test image. |
|
Failed to fetch and build pr. |
|
@gaia-bot OH FUCK OFF |
|
/test rbac-fixes-281 |
|
Command received. Building new test image. |
|
Failed to fetch and build pr. |
|
/test gaiapipeline:testing/rbac-fixes-281 |
|
Command received. Building new test image. |
|
Failed to fetch and build pr. |
|
/test gaiapipeline/testing:rbac-fixes-281 |
|
Command received. Building new test image. |
|
The new test version has been pushed. |
|
@Skarlso would be cool if the bot posted the link to the running service... I'm lazy! 🤣 |
|
https://gaia.cronohub.org :P It didn't change. :D |
|
/help |
|
Hello, I'm Gaia Bot. I help the team build, test and manage PRs and other issues. |
| svc, err := rbac.NewEnforcerSvc(gaia.Cfg.RBACDebug, store.CasbinStore()) | ||
| model, err := rbac.LoadModel() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error loading model: %w", err) |
There was a problem hiding this comment.
Please Log the error and return a userfriendly message. The log is enough to check out the rest. And below too.
There was a problem hiding this comment.
We wrap the error and log in the calling code. I think these errors are only really for us anyway?
We could return a user friendly error in the calling code that is just a blanket "failed to init RBAC". But here, I'd prefer to wrap and then log where we call initRBACService.
| } | ||
|
|
||
| func loadModel() (model.Model, error) { | ||
| // LoadModel loads the rbac model string from the assethelper and parses it into a Casbin model.Model. | ||
| func LoadModel() (model.Model, error) { | ||
| modelStr, err := assethelper.LoadRBACModel() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error loading rbac model from assethelper: %w", err) |
There was a problem hiding this comment.
Here too, please return something user friendly and log the actual error. And everywhere else too. :) These usually are user facing and note that a large enough error might break the output too because the div can't handle log strings. So make it something userfriendly instead of concatenating the error. And log the error so it can be investigated. :) Thanks.
There was a problem hiding this comment.
@Skarlso I prefer to wrap errors so we can more easily debug. Its up to the caller of this method to log. Therefore I'd prefer adding the logf into server.go#initRBACService (for which I definitely forgot!). If this specific error occurred it would be more for us to diagnose a problem than a user issue. So I'd rather include as much context as possible?
There was a problem hiding this comment.
fmt.Errorf("error loading rbac model from assethelper: %w", err) is wrapping errors? Its the Go std lib way of wrapping errors introduced in 1.13 :)
There was a problem hiding this comment.
Ah right, I forgot. :D But then in the upper calls we'll need to do some logic in unwrapping?
There was a problem hiding this comment.
When you do the log on the error it will show the full error string of all errors. So something like error a: error b: error c all the way down the error stack). But other benefits are being able to use errors.Is and errors.As to look for any errors in the wrapped error.
There was a problem hiding this comment.
Yes, that's why I asked that we would need to inspect the errors if it's something we can recover from. Anyways, fine then. I just don't want anything to creep up to the user. But I followed through that calls so this should be fine.
There was a problem hiding this comment.
👍 Let me just make this code a little bit more consistent dude. You're right though, in some circumstances we'd definitely want to recover. I feel like here if something fails to load (like the model) we're buggered though! Recovery could be turning RBAC off, but that would be bad if it was meant to be on!
There was a problem hiding this comment.
Yes, let's not do that. :) ( turning it off I mean. )
|
Holy shit. |
Hoping it was just a wobble... https://status.npmjs.org/ |
Call GetAllSubjects() instead for listing RBAC roles. GetAllRoles() only gets roles that have actually been assigned to a user. This is because 'p' policy lines are defined for the roles. 'g' policy lines are for assignment of a role to a user. GetAllRoles() calls the 'g' line at index 2 to list roles.
Fixed a bug with role:readonly looking for wildcard 'get' action which no longer exists because our actions became more specific - e.g. 'pipelines:runs, get-run' rather than 'pipelines:runs, get'
Added a check to make sure all newly created roles have a prefix of 'role:x'
Made the instantiation of the rbac more testable by using DI instead of instantiating concrete resources within the constructor
Add missing service.go unit tests