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

Mask locations of inaccessible assets on map #409

Merged
merged 6 commits into from Mar 31, 2022
Merged

Conversation

Flix6x
Copy link
Contributor

@Flix6x Flix6x commented Mar 30, 2022

Workaround for #200.

Signed-off-by: F.N. Claessen <felix@seita.nl>
@Flix6x Flix6x requested a review from nhoening March 30, 2022 14:59
@Flix6x Flix6x self-assigned this Mar 30, 2022
@Flix6x Flix6x added this to the 0.9.1 milestone Mar 30, 2022
@coveralls
Copy link
Collaborator

coveralls commented Mar 30, 2022

Pull Request Test Coverage Report for Build 2071349103

  • 22 of 30 (73.33%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 68.475%

Changes Missing Coverage Covered Lines Changed/Added Lines %
flexmeasures/cli/init.py 10 11 90.91%
flexmeasures/api/common/responses.py 3 10 30.0%
Totals Coverage Status
Change from base Build 2060198507: 0.01%
Covered Lines: 7017
Relevant Lines: 9714

💛 - Coveralls

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.

We should not implement various maskX functions in various places.

We have flexmeasures.data.queries.utils.potentially_limit_query_to_account_assets, which is supposed to be used here by get_asset_group_queries (or actually by the query functions it is using).

So this PR is about figuring out why that is not happening.

@nhoening
Copy link
Contributor

I don't see how #200 could help here.

I also believe we don't need that issue anymore. Our auth policy is pretty powerful, and the create-children permission allows us to model the cases we needed.

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

I noticed we were using app.cli to test if we are running via the CLI, but that actually is not correct. That is the cli module, so testing for that is always true.
I'm not completely happy yet, but this would work.

@nhoening
Copy link
Contributor

Well, I broke a few tests, so I'll look into that tomorrow.

… a tuple)

Signed-off-by: Nicolas Höning <nicolas@seita.nl>
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
…they run via the CLI, so their querying is not hampered

Signed-off-by: Nicolas Höning <nicolas@seita.nl>
@nhoening nhoening assigned nhoening and unassigned Flix6x Mar 31, 2022
@nhoening
Copy link
Contributor

@Flix6x I can't set you as reviewer for this PR, so I request one this way :)

@Flix6x
Copy link
Contributor Author

Flix6x commented Mar 31, 2022

And I also can't file an approval for my own PR, so I approve it this way :)

Thanks for jumping on the opportunity to improve our auth!

@Flix6x Flix6x requested a review from nhoening March 31, 2022 13:19
@nhoening
Copy link
Contributor

Does this require a changelog entry?

@Flix6x
Copy link
Contributor Author

Flix6x commented Mar 31, 2022

Does this require a changelog entry?

Working on it.

Signed-off-by: F.N. Claessen <felix@seita.nl>
@Flix6x Flix6x merged commit a0f7857 into main Mar 31, 2022
@Flix6x Flix6x deleted the mask-asset-locations branch March 31, 2022 14:00
Flix6x added a commit that referenced this pull request Mar 31, 2022
Fix auth bug not masking locations of inaccessible assets on map.

* Mask inaccessible assets (workaround for issue #200)

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

* fix limiting query to current account

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

* fix checking for ResponseTuples in API code (type() will say they are a tuple)

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

* fix querying for null values

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

* introduce a way for tests that test backend functionality to pretend they run via the CLI, so their querying is not hampered

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

* Changelog entry

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

Co-authored-by: Nicolas Höning <nicolas@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants