Authorization core module

[ruiconti]

Contexts

The entire context about research, design decisions and everything are on #18.

Relevant implementation details and will be placed on code through mindful comments.

Checkers

Work is being split on these "greater" granularity:

1. Tiny DSL parser conditional attributes statements
2. Policy enforcement checks
3. Policy constraints
4. Proper packaging under core module
5. Compilation of docs to make mechanics clear

TDD is being applied to, within each of these tasks, find the thinnest and possible workable solution.

[ruiconti]

Tiny DSL Parser

Implementation for each conditional parsing is being done as a small state machine that iterates on a (conditional) expression,
"consumes" tokens and "advances" (through slices) a given expression. "consumes" and "advances" are quoted because we're not mutating the original expression.

Caveat: To split many conditionals in individual ones I've chosen a pythonic approach to save time and sanity.

[ruiconti]

As of now, all expected functionality have been implemented ✍️ These mechanics are exposed on kingdom.access.authorization.flow and tested on kingdom.access.tests.authorization.test_flow.

Implementation have been done using TDD: Breaking problem in small pieces then assembling them up. However, it is far from ready to go as it's the first working draft. What's left to do then, you ask?

Refactor on overall design

Namely:

1. Module—functionality—class cohesion. Functions are rather carelessly spread.
   1. Improve on namings, refer to industry standards
2. Public APIs: What is expected for services to use? Are we covering expected use cases? Are we supposed to enable extensibility to e.g. caching? How straightforward and sensible it is? How are we handling errors?
   1. Usage from services on an authorization/authentication flow
   2. Usage from ORM adapters to encode/decode policies on database
3. Innwards of functions: We can leverage from DRY, SRP, composability and cohesion principles here
4. Refactor and improve test cases to a broader range of scope when testing the Public API

[ruiconti]

@andreyrcdias Since you're doing a study on refactor, your views on these tasks I mentioned above are welcome :-)

[rafamelos]

First of all, really incredible implementation, nice job.

Before given more thoughts on it, I will raise a red flag about this PR size and enforce that our current movement about more atomic tasks that reflect in atomic PRs should be applied to this project as well. Even that I didn't ended up having many considerations it was really hard to evaluate this because of the size together with the high complexity.

Now, about the implementation, I overall approve this. I'm just in doubt about the READ evaluation, about a operation of this kind never being set as unauthorized and just having its scope restricted, to clarify, the idea is that the READ operation should filter the return using the scope instead of returning a clear message that the operation is not allowed?

[ruiconti]

Hey @rafamelos, thank you lots for taking the time to review this. I apologise for making you go through my refactoring thinking. I believe things are a bit simpler right now.

First of all, really incredible implementation, nice job.

Thanks you!

Before given more thoughts on it, I will raise a red flag about this PR size and enforce that our current movement about more atomic tasks that reflect in atomic PRs should be applied to this project as well. Even that I didn't ended up having many considerations it was really hard to evaluate this because of the size together with the high complexity.

Definitely. Totally agree with you. I've had a hard time splitting this up because we would need a intermediate stage between main and feature branches. But I'm stopping now with these published interfaces and for further integration (w/ ORM, GraphQL and others), I'm doing other (smaller) PRs. You have my word haha.

Now, about the implementation, I overall approve this. I'm just in doubt about the READ evaluation, about a operation of this kind never being set as unauthorized and just having its scope restricted, to clarify, the idea is that the READ operation should filter the return using the scope instead of returning a clear message that the operation is not allowed?

The rationale behind READ is that if a user is authenticated, then it is semantically authorised to ask for READ operations on any resource. In this sense, the access control happens on limiting the scope, as you noted yourself. I've documented this on flow.check_permission().

--

A few notes for further reviews. Currently, the public API (and this whole PR) consists of two modules:

• kingdom.access.flow, which is meant for services to use to properly verify incoming accesses. Its usage is documented on kingdom.access.tests.test_flow.
• kingdom.access.base, which is meant for access service to implement Permission encoding. Its usage is documented on kingdom.access.tests.test_base.

About the DSL, it's gonna be used for ORM serializaiton. So I'm leaving this for another PR to keep things consistent here.

[andreyrcdias]

Sorry for the delay to give my opinion here. Reiterating @rafamelos's words, a really good job 👏🏽.

I also found it a bit dense to understand, but I agree with this whole implementation. I left my simple contribution that has no impact on what was proposed.

[rafamelos]

Any big changes to point out, nice improvement since last viewed. Just one point about our discussion in friday, is inline policies going to be included in this?

[ruiconti]

Thanks @rafamelos and @andreyrcdias 🙏

Making explicit some of implementation premises:

 1. Write permission overrides read permission.
    e.g. If a Subject has any write permission, him/her implicitly
    have read permission.
 2. No write permissions override one-another.
    e.g. If a Subject has DELETE permission and it doesn't have
    UPDATE or CREATE permissions, it is only allowed to delete.

I've done and applied all suggested changes, thanks again guys.

[ruiconti]

@rafamelos about your question, IMO this could be another PR so it's small and easily reviewable. I think you'd agree with me on this hahaha.

[ruiconti]

This PR leads to the creation of four issues:

1. Split authorization module and all of commons into kingdom package & repo
2. Plugging authorization module to access aggregate and all of relevant refactors Adapt authorization module to work with access aggregate #20
3. Plugging authorization module to middlewares & directives Adapt authorization module to work with current entrypoints #21
4. Adding an inline policy & all of necessary interfaces Add inline policy capabilities to authorization #22

[andreyrcdias]

👀