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 (#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 Nov 10, 2021
@nhoening nhoening requested a review from Flix6x Nov 10, 2021
Copy link
Contributor

@Flix6x Flix6x left a comment

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 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 Nov 17, 2021
nhoening
Copy link
Contributor Author

@nhoening nhoening commented on 2aaeb70 Nov 17, 2021

Choose a reason for hiding this comment

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

My custom factory approach replaced the id argument in the list of args. Marshmallow only adds new things to the arguments. I spent considerable time on this, as I share your view, but that is how it is.

Flix6x
Copy link
Contributor

@Flix6x Flix6x commented on 2aaeb70 Nov 17, 2021

Choose a reason for hiding this comment

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

a service (singular)

Flix6x
Copy link
Contributor

@Flix6x Flix6x commented on 2aaeb70 Nov 17, 2021

Choose a reason for hiding this comment

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

  • I wouldn't presume that these authorization decorators are straightforward to the reader.
  • You mention a consequence, but its practical implications aren't really clear to me.

Flix6x
Copy link
Contributor

@Flix6x Flix6x commented on 2aaeb70 Nov 17, 2021

Choose a reason for hiding this comment

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

Is the original resource_id also passed next to the deserialized resource? I expected to see only the latter here.

Flix6x
Copy link
Contributor

@Flix6x Flix6x commented on 2aaeb70 Nov 17, 2021

Choose a reason for hiding this comment

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

One means for this

Missing context; what does "this" refer to?

@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

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 Nov 20, 2021
Copy link
Contributor

@Flix6x Flix6x left a comment

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
Flix6x
Flix6x approved these changes Nov 24, 2021
@nhoening nhoening merged commit c9c410f into main Nov 24, 2021
2 checks passed
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 Nov 24, 2021
@nhoening nhoening restored the central-auth-policy-for-users branch Nov 24, 2021
@nhoening nhoening deleted the central-auth-policy-for-users branch Nov 24, 2021
@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.

2 participants