manager: Expose more nuanced methods for finding policies. #120
Conversation
Signed-off-by: Ugo Di Girolamo <ugo@compass.com>
manager.go
Outdated
| // FindPoliciesForSubject returns policies that could match the resource. It either returns | ||
| // a set that exactly matches the request, or a superset of it. If an error occurs, it returns nil and | ||
| // the error. | ||
| FindPoliciesForSubject(subject string) (Policies, error) |
There was a problem hiding this comment.
I know that this is kind of strange, because we already have GetPolicy and stuff like that, but it has been a while since this interface was defined. Best practice in Go is to define interfaces at the consumer rather than the producer. These methods are really only used in the tests at the moment (to hydrate the stores).
I'm not generally against exposing these methods as an interface here - but maybe it would make sense to have two interfaces RequestCandidateFinder and Manager? The former would only expose FindRequestCandidates (needed by Ladon) and the latter would expose management methods including these ones here? This would obviously be a breaking change but it would clean this up a bit.
What do you think?
There was a problem hiding this comment.
I have looked a bit to other code to try to understand the implications.
I agree completely that managing policies and actually authorizing could very well be two separate interfaces.
I struggle to visualize how that would be done nicely though, because I feel that most (all?) implementations should prefer keeping the code for management and for authentication close to each other since it's touching the same data. So, the postgres Manager and RequestCandidateFinder should probably be the same struct implementing both interfaces IMO.
Maybe a clean way to get this would be to introduce the new interfaces, but change SQLManager to something like:
type SQLPolicyDB struct {
db *sqlx.DB
database string
}
// ... All the same methods that are in SQLManager now, give or take
func (s *SQLPolicyDB) GetManager() Manager {
return s
}
func (s *SQLPolicyDB) GetRequestCandidateFinder() RequestCandidateFinder {
return s
}
Then the change to dependent systems maybe is not that bad?
manager/memory/manager_memory.go
Outdated
| return m.findAllPolicies() | ||
| } | ||
|
|
||
| // FindPoliciesForSubject returns policies that could match the resource. It either returns |
| return m.findAllPolicies() | ||
| } | ||
|
|
||
| // FindPoliciesForResource returns policies that could match the resource. It either returns |
manager/sql/databases.go
Outdated
| @@ -178,6 +179,30 @@ var Migrations = map[string]Statements{ | |||
| (subject.has_regex IS NOT TRUE AND subject.template = $1) | |||
| OR | |||
| (subject.has_regex IS TRUE AND $2 ~ subject.compiled)`, | |||
| QueryPoliciesForResource: ` | |||
There was a problem hiding this comment.
This is pretty much the same query as before - maybe this could be a helper function with Sprintf which replaces the subject.has_regex part with %s.has_regex and so on?
There was a problem hiding this comment.
Done. I put it in a separate commit because I felt not-completely sure that it is an upgrade.
It definitely removes a lot of repetition, but maybe it slightly obfuscates the SQL.
- Exposes FindPoliciesForSubject - Creates new method FindPoliciesForResource Signed-off-by: Ugo Di Girolamo <ugo@compass.com>
Signed-off-by: Ugo Di Girolamo <ugo@compass.com>
|
Thank you! |
Ref #119