Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ACL-based auth policy and the @permission_required_for_context decorator. For now used with /users API #234

Merged
merged 24 commits into from Nov 24, 2021

Conversation

nhoening
Copy link
Contributor

@nhoening nhoening commented Nov 9, 2021

closes #201

  • we might not do table-based authorization for a while (table-level auth: let data classes define which roles are allowed #200), as I did not find a nice way in this PR to do it, without having to separate acl lists
  • I simplified how GET /users works - it can only get users per one account. Simplifying APIs is usually a good idea, I believe. It is a breaking change which I'll mention, but I'm very certain no-one relied on it yet.
  • The users API does some column-based checks (authorize a request based on what fields are affected). We could even address this in our auth policy, I believe, but I don't see a clean way. Let's leave this to the endpoints for now. There should not be many more examples, maybe none (users are a bit special).
  • We can still discuss what to do with service listings and /getService from here on out. They list required roles for each service. Will that persist?

@nhoening nhoening marked this pull request as ready for review November 10, 2021 08:47
@nhoening nhoening requested a review from Flix6x November 10, 2021 08:56
Copy link
Contributor

@Flix6x Flix6x left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides some minor fixes, I feel like we should be incorporating more Marshmallow functionality here.

And about your question:

We can still discuss what to do with service listings and /getService from here on out. They list required roles for each service. Will that persist?

I think this might be two questions actually:

  • Will the USEF roles in those service listings become obsolete? I believe they might, but I haven't completely thought this through.
  • Do we want the service listings to list relevant ACL? I believe we do.

documentation/changelog.rst Outdated Show resolved Hide resolved
flexmeasures/api/common/factories.py Outdated Show resolved Hide resolved
flexmeasures/auth/policy.py Outdated Show resolved Hide resolved
flexmeasures/api/v2_0/implementations/users.py Outdated Show resolved Hide resolved
flexmeasures/api/common/responses.py Outdated Show resolved Hide resolved
flexmeasures/api/common/factories.py Outdated Show resolved Hide resolved
flexmeasures/api/common/factories.py Outdated Show resolved Hide resolved
flexmeasures/api/common/factories.py Outdated Show resolved Hide resolved
flexmeasures/api/common/factories.py Outdated Show resolved Hide resolved
flexmeasures/api/common/factories.py Outdated Show resolved Hide resolved
@nhoening nhoening removed this from In progress in Authorization based on accounts Nov 17, 2021
@nhoening
Copy link
Contributor Author

nhoening commented Nov 17, 2021

I believe the way we list auth access for services will be addressed a bit later. I might be interesting to put the ACLs in natural language, actually. I'll add a card to this project (https://github.com/SeitaBV/flexmeasures/projects/4).

@nhoening nhoening requested a review from Flix6x November 17, 2021 14:56
@Flix6x Flix6x added this to In progress in Authorization based on accounts via automation Nov 17, 2021
Copy link
Contributor

@Flix6x Flix6x left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only small requests left: mainly some questions about documentation and the wish for a policy regarding raising errors or returning error messages in decorators.

flexmeasures/auth/decorators.py Show resolved Hide resolved
flexmeasures/data/models/user.py Show resolved Hide resolved
flexmeasures/auth/decorators.py Outdated Show resolved Hide resolved
flexmeasures/auth/decorators.py Outdated Show resolved Hide resolved
flexmeasures/auth/decorators.py Outdated Show resolved Hide resolved
@nhoening nhoening requested a review from Flix6x November 20, 2021 16:25
Copy link
Contributor

@Flix6x Flix6x left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly still needs an adjustment to the function signature of the error handler.

flexmeasures/auth/decorators.py Outdated Show resolved Hide resolved
flexmeasures/auth/decorators.py Outdated Show resolved Hide resolved
flexmeasures/auth/error_handling.py Show resolved Hide resolved
@nhoening nhoening merged commit c9c410f into main Nov 24, 2021
Authorization based on accounts automation moved this from In progress to Done Nov 24, 2021
@nhoening nhoening deleted the central-auth-policy-for-users branch November 24, 2021 10:12
@nhoening nhoening restored the central-auth-policy-for-users branch November 24, 2021 10:12
@nhoening nhoening deleted the central-auth-policy-for-users branch November 24, 2021 10:12
@Flix6x Flix6x added this to the 0.8.0 milestone Jan 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

row-level auth: Allow to protect account data
2 participants