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

Feature/add consultancy account #877

Merged
merged 56 commits into from Nov 3, 2023
Merged

Conversation

GustaafL
Copy link
Contributor

@GustaafL GustaafL commented Oct 17, 2023

Description

Add a Consultant account that has read access to accounts of clients.

Use case: Our customer is the supplier to a bunch of other (client) accounts on the server. They should have read-only access to some accounts. At least this is the first step. So they can oversee what happens, for instance for servicing questions and problems.

Look & Feel

The consultant will be able to see the accounts in their overview page and the assets they have access to. The consultant client will be able to see which account has read access to their account.

How to test

Tests are added in flexmeasures/api/v3_0/tests/test_assets_api.py
To test in the UI create a consultant account and a consultant client account in the CLI. Then go to the account pages and see the accounts represented.

Further Improvements

Potential improvements to be done in the same PR or follow up Issues/Discussions/PRs.

Related Items

The technical discussion can be found here: #868
The issue that this closes can be found here: #203


  • I agree to contribute to the project under Apache 2 License.
  • To the best of my knowledge, the proposed patch is not based on code under GPL or other license that is incompatible with FlexMeasures

GustaafL added 10 commits October 12, 2023 12:39
Signed-off-by: GustaafL <guus@seita.nl>
Signed-off-by: GustaafL <guus@seita.nl>
Signed-off-by: GustaafL <guus@seita.nl>
Signed-off-by: GustaafL <guus@seita.nl>
Signed-off-by: GustaafL <guus@seita.nl>
Signed-off-by: GustaafL <guus@seita.nl>
Signed-off-by: GustaafL <guus@seita.nl>
Signed-off-by: GustaafL <guus@seita.nl>
Signed-off-by: GustaafL <guus@seita.nl>
Signed-off-by: GustaafL <guus@seita.nl>
GustaafL added 4 commits October 17, 2023 15:19
Signed-off-by: GustaafL <guus@seita.nl>
Signed-off-by: GustaafL <guus@seita.nl>
Signed-off-by: GustaafL <guus@seita.nl>
Signed-off-by: GustaafL <guus@seita.nl>
Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

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

Good start!

I found a mix-up in how it's supposed to work, though.

Can you edit the PR description (only relevant sections), and then also reference discussion #868

Once we have this working well, I think I'd like to implement my idea of the required user role, as well (see discussion: "Addendum: also require a user role"). Better to do that right away, because otherwise the access might apply to too many people.

documentation/changelog.rst Outdated Show resolved Hide resolved
flexmeasures/conftest.py Outdated Show resolved Hide resolved
flexmeasures/api/v3_0/tests/test_assets_api.py Outdated Show resolved Hide resolved
flexmeasures/conftest.py Outdated Show resolved Hide resolved
flexmeasures/conftest.py Outdated Show resolved Hide resolved
flexmeasures/api/v3_0/tests/test_assets_api.py Outdated Show resolved Hide resolved
GustaafL added 7 commits October 18, 2023 10:24
…and fix 3.8 support

Signed-off-by: GustaafL <guus@seita.nl>
Signed-off-by: GustaafL <guus@seita.nl>
Signed-off-by: GustaafL <guus@seita.nl>
…to read

Signed-off-by: GustaafL <guus@seita.nl>
Signed-off-by: GustaafL <guus@seita.nl>
Signed-off-by: GustaafL <guus@seita.nl>
@Flix6x Flix6x added this to the 0.17.0 milestone Oct 18, 2023
…r role

Signed-off-by: GustaafL <guus@seita.nl>
@GustaafL
Copy link
Contributor Author

I've added the customer-manager user role as a requirement for accessing consultant client accounts by consultants.

GustaafL added 2 commits October 19, 2023 10:46
Signed-off-by: GustaafL <guus@seita.nl>
Signed-off-by: GustaafL <guus@seita.nl>
Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

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

Good iteration! Only smaller comments at this point.

Maybe we should add a small sentence/paragraph to https://flexmeasures.readthedocs.io/en/latest/concepts/security_auth.html#authorization, to explain this concept.

flexmeasures/api/v3_0/tests/test_assets_api.py Outdated Show resolved Hide resolved
flexmeasures/api/v3_0/tests/test_assets_api.py Outdated Show resolved Hide resolved
flexmeasures/conftest.py Outdated Show resolved Hide resolved
flexmeasures/conftest.py Show resolved Hide resolved
GustaafL added 7 commits October 30, 2023 10:01
Signed-off-by: GustaafL <guus@seita.nl>
Signed-off-by: GustaafL <guus@seita.nl>
Signed-off-by: GustaafL <guus@seita.nl>
Signed-off-by: GustaafL <guus@seita.nl>
Signed-off-by: GustaafL <guus@seita.nl>
Signed-off-by: GustaafL <guus@seita.nl>
Signed-off-by: GustaafL <guus@seita.nl>
Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

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

Very close to done.
I'd like to add the consultant's account ID to the client account when I'm creating it on the CLI (we discussed that once in person).
That would also make it easy enough for me to test this PR locally.

documentation/changelog.rst Outdated Show resolved Hide resolved
flexmeasures/ui/templates/crud/account.html Outdated Show resolved Hide resolved
GustaafL added 3 commits November 2, 2023 08:10
…client accounts

Signed-off-by: GustaafL <guus@seita.nl>
Signed-off-by: GustaafL <guus@seita.nl>
Signed-off-by: GustaafL <guus@seita.nl>
@GustaafL
Copy link
Contributor Author

GustaafL commented Nov 2, 2023

I've added the option to add a consultancy-account-id in the CLI and a test that this works as expected.

Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

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

I tested it (via the UI) and it works.
I feel that one part is too confusing for later, and needs more clarity. Then we can merge!

flexmeasures/ui/templates/defaults.jinja Outdated Show resolved Hide resolved
GustaafL added 2 commits November 3, 2023 12:26
Signed-off-by: GustaafL <guus@seita.nl>
Signed-off-by: GustaafL <guus@seita.nl>
@Flix6x
Copy link
Contributor

Flix6x commented Nov 3, 2023

I used the following psql statements to get a short demo going. Perhaps the first statement is suited to already be included in the database migration.

insert into role (name, description) values ('customer-manager', 'Consultant that manages another customer');
insert into roles_users (user_id, role_id) values (<user ID of consultant>, <role ID that was just created>);
update account set consultancy_account_id=<account ID of consultant organisation> where id=<account ID of consultee organisation>;

UPDATE: we found out that we haven't created new user roles before within database migrations, so my suggestion is maybe not the way to go. What we had done before was to add new user roles to a list in data.scripts.data_gen.add_default_user_roles, so it would get picked up when running flexmeasures add initial-structure. That list currently consists of just the admin and admin-reader roles, so I believe we had missed adding the account-admin to it at some point in the past.

Signed-off-by: GustaafL <guus@seita.nl>
@nhoening
Copy link
Contributor

nhoening commented Nov 3, 2023

So we should add two new roles there in our current PR? Thanks.

Signed-off-by: GustaafL <guus@seita.nl>
@GustaafL
Copy link
Contributor Author

GustaafL commented Nov 3, 2023

So we should add two new roles there in our current PR? Thanks.

Done

Flix6x and others added 2 commits November 3, 2023 21:29
… Sensor (#890)

* fix: hierarchical read rights from Account to Asset and from Asset to Sensor

Signed-off-by: F.N. Claessen <felix@seita.nl>

* style: flake8

Signed-off-by: F.N. Claessen <felix@seita.nl>

* feat(test): add test for getting single asset in consultantclient account

Signed-off-by: GustaafL <guus@seita.nl>

* update docstrings to match new reality

Signed-off-by: Nicolas Höning <nicolas@seita.nl>

---------

Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: GustaafL <guus@seita.nl>
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
Co-authored-by: GustaafL <guus@seita.nl>
Co-authored-by: Nicolas Höning <nicolas@seita.nl>
@nhoening nhoening self-requested a review November 3, 2023 20:37
Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

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

Nice work!

@nhoening nhoening merged commit 72b4935 into main Nov 3, 2023
8 of 9 checks passed
@nhoening nhoening deleted the feature/add-consultancy-account branch November 3, 2023 20:38
@Flix6x Flix6x mentioned this pull request Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consultant accounts ― who can watch normal non-admin accounts
3 participants