[#1874] Enabled selection of company branch for eHerkenning users #884

pi-sigma · 2023-12-07T15:55:06Z

Let eHerkenning users choose company branch on login
Refactored project structure and move KvK app out of contrib

Taiga 1874

src/eherkenning/mock/views/eherkenning.py

codecov-commenter · 2023-12-18T12:15:55Z

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (1d05ead) 92.93% compared to head (08bc4d0) 92.88%.

Files	Patch %	Lines
src/open_inwoner/kvk/views.py	87.75%	6 Missing ⚠️
src/open_inwoner/kvk/middleware.py	96.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #884      +/-   ##
===========================================
- Coverage    92.93%   92.88%   -0.05%     
===========================================
  Files          819      824       +5     
  Lines        28250    28474     +224     
===========================================
+ Hits         26255    26449     +194     
- Misses        1995     2025      +30

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pi-sigma · 2023-12-20T13:55:58Z

@Bartvaderkin test_any_page_for_digid_user_redirect_to_necessary_fields is failing locally for unknown reasons. I've tried disabling the kvk middleware, like I did for some of the other tests, but to no avail.

src/open_inwoner/openzaak/tests/test_cases.py

stevenbal · 2024-01-02T11:08:58Z

src/open_inwoner/kvk/views.py

+        )
+
+    def post(self, request):
+        form = self.get_form()


I tested this locally and it's possible to (after logging in with a KvK number) perform a request with the same session ID (with postman for example) and a branch number that does not belong to the KvK number you logged in with. I think it would be good to validate that the branch_number present in the data belongs to the KvK number, to avoid security issues

Please improve this

src/open_inwoner/kvk/views.py

src/open_inwoner/scss/components/Card/Card.scss

stevenbal · 2024-01-02T11:17:36Z

src/open_inwoner/kvk/views.py

+            },
+        )
+
+    def post(self, request):


I noticed that clicking Log in without selecting a branch returns you to the same page, but doesn't show an error message. I think it would be nice show some kind of error message

stevenbal · 2024-01-02T11:39:14Z

src/open_inwoner/accounts/tests/test_auth.py

@@ -1253,6 +1392,7 @@ def test_page_show_config_text(self):
        self.assertContains(response, "Hello registration text")
        self.assertContains(response, ' href="http://foo.bar/" ')

+    @skip("Debug")


Should be removed probably?

The NecessaryFieldsMiddleware was essentially disabled due to this change: 68ca2fc#diff-09a70cba3ae30157873d6826d98ae3cf634d9f0c764558ef37e98b4620443217R18. I removed that line and switched the order of the KvKLoginMiddleware and NecessaryFieldsMiddleware to fix other tests

stevenbal · 2024-01-02T11:53:58Z

@pi-sigma I'll pick up the requested changes for this PR

* add error message for company branch select form * refactor branches form page, remove unnecessary styling and use existing templatetags

* NecessaryFieldsMiddleware reenabled (because it was essentially disabled in a previous commit) * Switched order of KvKLoginMiddleware and NecessaryFieldsMiddleware, because the former should trigger first * Removed unnecessary test and fixed failing tests

to prevent eHerkenning authenticated users to try and post arbitrary vestigingsnummers from other companies

stevenbal · 2024-01-04T12:00:05Z

@alextreme this PR is ready for review. The only thing I just thought of is that it's currently not possible to switch branches after selecting one (you have to log out first), is this an issue?

alextreme · 2024-01-04T12:01:18Z

@alextreme this PR is ready for review. The only thing I just thought of is that it's currently not possible to switch branches after selecting one (you have to log out first), is this an issue?

In my eyes keeping the company branch set for the duration of the session seems fine

alextreme · 2024-01-04T12:59:02Z

This looks fine for now, I've added #1996 for further styling

outdated

github-advanced-security bot found potential problems Dec 7, 2023

View reviewed changes

src/eherkenning/mock/views/eherkenning.py Fixed Show fixed Hide fixed

src/eherkenning/mock/views/eherkenning.py Fixed Show fixed Hide fixed

pi-sigma force-pushed the feature/1874-company-branches-select branch 10 times, most recently from 0ddc3ad to a27391b Compare December 13, 2023 15:45

pi-sigma force-pushed the feature/1874-company-branches-select branch from a27391b to a7dc9f5 Compare December 18, 2023 09:14

alextreme requested a review from Bartvaderkin December 18, 2023 09:35

pi-sigma force-pushed the feature/1874-company-branches-select branch 2 times, most recently from b818b88 to 6102f0f Compare December 18, 2023 12:02

pi-sigma force-pushed the feature/1874-company-branches-select branch from 6102f0f to 0fdd219 Compare December 18, 2023 12:58

pi-sigma changed the title ~~Feature/1874 company branches select~~ [#1874] Enabled selection of company branch for eHerkenning users Dec 18, 2023

pi-sigma force-pushed the feature/1874-company-branches-select branch 6 times, most recently from cc716e8 to 68ca2fc Compare December 20, 2023 13:03

pi-sigma marked this pull request as ready for review December 20, 2023 13:56

alextreme requested a review from stevenbal January 2, 2024 09:42

stevenbal previously requested changes Jan 2, 2024

View reviewed changes

stevenbal self-assigned this Jan 2, 2024

stevenbal marked this pull request as draft January 2, 2024 13:41

stevenbal force-pushed the feature/1874-company-branches-select branch 4 times, most recently from 51a433d to 9a4045e Compare January 2, 2024 16:00

pi-sigma and others added 7 commits January 4, 2024 10:48

[#1874] Refactor KvK API client to support different queries

bfd79f2

[#1874] Enable selection of company branch at eHerkenning login

2a9bfc0

[#1874] Move KvK app out of contrib

bf307a9

💄 [#1874] Changes to KvK select branch page

ce370ce

* add error message for company branch select form * refactor branches form page, remove unnecessary styling and use existing templatetags

🔒 [#1874] Throw 401 for POST on KvK select branch page

3078289

🔒 [#1874] Validate vestigingsnummer when selecting company branch

71fe064

to prevent eHerkenning authenticated users to try and post arbitrary vestigingsnummers from other companies

stevenbal force-pushed the feature/1874-company-branches-select branch from 54d3d08 to 71fe064 Compare January 4, 2024 09:48

✅ [#1874] Fix tests failing due to KvKLoginMiddleware

bd53fd2

stevenbal force-pushed the feature/1874-company-branches-select branch from c44b8e3 to bd53fd2 Compare January 4, 2024 11:32

stevenbal requested a review from alextreme January 4, 2024 11:59

stevenbal marked this pull request as ready for review January 4, 2024 12:00

stevenbal self-requested a review January 4, 2024 12:00

alextreme approved these changes Jan 4, 2024

View reviewed changes

alextreme merged commit e2baecf into develop Jan 4, 2024
14 checks passed

alextreme deleted the feature/1874-company-branches-select branch January 4, 2024 12:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#1874] Enabled selection of company branch for eHerkenning users #884

[#1874] Enabled selection of company branch for eHerkenning users #884

pi-sigma commented Dec 7, 2023 •

edited

Loading

codecov-commenter commented Dec 18, 2023 •

edited

Loading

pi-sigma commented Dec 20, 2023

stevenbal Jan 2, 2024

alextreme Jan 2, 2024

stevenbal Jan 2, 2024

stevenbal Jan 2, 2024

stevenbal Jan 2, 2024

stevenbal Jan 2, 2024

stevenbal Jan 2, 2024 •

edited

Loading

stevenbal commented Jan 2, 2024

stevenbal commented Jan 4, 2024

alextreme commented Jan 4, 2024

alextreme commented Jan 4, 2024

[#1874] Enabled selection of company branch for eHerkenning users #884

[#1874] Enabled selection of company branch for eHerkenning users #884

Conversation

pi-sigma commented Dec 7, 2023 • edited Loading

codecov-commenter commented Dec 18, 2023 • edited Loading

Codecov Report

pi-sigma commented Dec 20, 2023

stevenbal Jan 2, 2024

Choose a reason for hiding this comment

alextreme Jan 2, 2024

Choose a reason for hiding this comment

stevenbal Jan 2, 2024

Choose a reason for hiding this comment

stevenbal Jan 2, 2024

Choose a reason for hiding this comment

stevenbal Jan 2, 2024

Choose a reason for hiding this comment

stevenbal Jan 2, 2024

Choose a reason for hiding this comment

stevenbal Jan 2, 2024 • edited Loading

Choose a reason for hiding this comment

stevenbal commented Jan 2, 2024

stevenbal commented Jan 4, 2024

alextreme commented Jan 4, 2024

alextreme commented Jan 4, 2024

pi-sigma commented Dec 7, 2023 •

edited

Loading

codecov-commenter commented Dec 18, 2023 •

edited

Loading

stevenbal Jan 2, 2024 •

edited

Loading