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.

Sign up for GitHub

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

Jump to bottom

[#2932] Skip KVK branch selection if vestigingsnummer already selected #1526

Merged
merged 2 commits into from
Dec 16, 2024
Merged

[#2932] Skip KVK branch selection if vestigingsnummer already selected #1526

merged 2 commits into from
Dec 16, 2024

Conversation

pi-sigma
Copy link
Contributor

@pi-sigma pi-sigma commented Dec 10, 2024 •
edited
Loading

The KVK branch selection page should be skipped for users who have already selected a branch on a different platform

  • Update eHerkenning OIDC flow: get vestigingsnummer from OIDC claim (if present) and store in session
  • Update eHerkenning SAML flow: get vestigingsnummer from SAML attributes (if present) and store in session
  • Skip KVK branch selection if vestigingsnummer already in session

Taiga: https://taiga.maykinmedia.nl/project/open-inwoner/task/2932

@pi-sigma pi-sigma force-pushed the task/2932-eherkenning-login-choice branch from c9f6c00 to 2403f68 Compare December 10, 2024 14:14
@codecov-commenter
Copy link

codecov-commenter commented Dec 10, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.26%. Comparing base (235ec5a) to head (fcb4c6b).

Additional details and impacted files 
@@           Coverage Diff            @@
##           develop    #1526   +/-   ##
========================================
  Coverage    94.25%   94.26%           
========================================
  Files         1068     1068           
  Lines        40466    40508   +42     
========================================
+ Hits         38141    38183   +42     
  Misses        2325     2325

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

@pi-sigma pi-sigma force-pushed the task/2932-eherkenning-login-choice branch from 2403f68 to 160e7c6 Compare December 10, 2024 14:39
@pi-sigma pi-sigma changed the title [#2932] Get vestigingsnummer from OIDC claim on login & store in session [#2932] Skip KVK branch selection if vestigingsnummer already selected Dec 10, 2024
@swrichards
Copy link
Contributor

Braindump: @pi-sigma this is technically orthogonal to the issue here (skipping the branch if already selected during the eHerkenning flow), but it's also important that users can see only vestigingen they're authorized to see according to the eHerkenning scope. I am not sure if that flows from this claim (Selected branch) versus some other claim that may or may not exist (all authorized branches).

@pi-sigma pi-sigma force-pushed the task/2932-eherkenning-login-choice branch from 160e7c6 to 90916fa Compare December 11, 2024 10:15
@pi-sigma
Copy link
Contributor Author

pi-sigma commented Dec 11, 2024 •
edited
Loading

@swrichards Good point. Is this already covered by our use of get_kvk_branch_number to scope the retrieval of roles, zaken, and vragen to a particular vestiging, or are we missing something?

@pi-sigma pi-sigma marked this pull request as ready for review December 11, 2024 13:23
@pi-sigma pi-sigma requested a review from swrichards December 11, 2024 13:23
@pi-sigma pi-sigma force-pushed the task/2932-eherkenning-login-choice branch 2 times, most recently from e9d46da to 32aff77 Compare December 12, 2024 07:53
swrichards
swrichards requested changes Dec 12, 2024
View reviewed changes
src/open_inwoner/accounts/backends.py Outdated Show resolved Hide resolved
src/open_inwoner/accounts/tests/test_oidc_views.py Show resolved Hide resolved
src/open_inwoner/accounts/backends.py Outdated Show resolved Hide resolved
src/open_inwoner/accounts/backends.py Outdated Show resolved Hide resolved
@pi-sigma pi-sigma force-pushed the task/2932-eherkenning-login-choice branch 3 times, most recently from 2ed8c7d to 2262b02 Compare December 12, 2024 10:04
@pi-sigma pi-sigma requested a review from swrichards December 12, 2024 10:27
@pi-sigma pi-sigma force-pushed the task/2932-eherkenning-login-choice branch 6 times, most recently from 46cac08 to bad1692 Compare December 12, 2024 14:29
swrichards
swrichards approved these changes Dec 12, 2024
View reviewed changes
Copy link
Contributor

@swrichards swrichards left a comment

Choose a reason for hiding this comment

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

Just one question about the claims: I'll leave it up to you whether we need to fix this now or whether it needs fixing at all.

src/open_inwoner/accounts/backends.py Outdated Show resolved Hide resolved
@pi-sigma pi-sigma marked this pull request as draft December 12, 2024 15:05
@pi-sigma pi-sigma force-pushed the task/2932-eherkenning-login-choice branch from bad1692 to 359d14c Compare December 12, 2024 15:15
@pi-sigma pi-sigma marked this pull request as ready for review December 12, 2024 15:16
swrichards
swrichards requested changes Dec 13, 2024
View reviewed changes
Copy link
Contributor

@swrichards swrichards left a comment

Choose a reason for hiding this comment

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

Very small nitpicks, otherwise GTG!

src/open_inwoner/accounts/backends.py Outdated
Comment on lines 184 to 186
msg = f"eHerkenning user (KVK/RSIN = {kvk_or_rsin}) retrieved with vestigingsnummer from IdP"
if user_created:
msg = f"eHerkenning user (KVK/RSIN = {kvk_or_rsin}) created with vestigingsnummer from IdP"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be in this method? It's strictly speaking about the vestigingsnummer. If we're logging anything here, I'd say we log that we did (or did not) find a vestigingsnummer, and leave the "we created or updated a user" in the function where that happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, that "retrieve/create" dichotomy indicates that the message is rather specific to the calling functions. I'll change and simplify accordingly.

src/open_inwoner/accounts/backends.py Outdated

if not (branch_number_claims := eherkenning_config.branch_number_claim):
return
if not (vestigingsnummer := claims.get(branch_number_claims[0])):
Copy link
Contributor

Choose a reason for hiding this comment

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

Just out of interest: did we figure out if there are ever multiple claims? No need to hold up the PR for that, just curious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. I'm not sure what the use case would be, but the library definitely makes room for this.

src/open_inwoner/accounts/backends.py Outdated
if not (identifier_type_claims := eherkenning_config.identifier_type_claim):
return

kvk_or_rsin = identifier_type_claims[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we fetching this only for the logging? In that case I'd also say we log this earlier in the chain, there's a lot of escape hatches in this function making it slightly harder to follow, if we can simplify it that'd be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the escape hatch for the identifier_type_claim is unnecessary. If you're logging in with eHerkenning, the KVK or RSIN needs to be in the claims

src/open_inwoner/accounts/backends.py Outdated
Comment on lines 172 to 173
if not (branch_number_claims := eherkenning_config.branch_number_claim):
return
Copy link
Contributor

Choose a reason for hiding this comment

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

My sense is this is an optional in a "you might not have eHerkenning set up" kind of way, but if we make it to this function, eHerkenning is evidently configured, and then I feel a Sentry ping would be in order to at least alert us to the incomplete configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact, the _check_candidate_backend already takes care of this, and since the config class provides a default for the claim field, this is guaranteed to exist. Will simplify

@pi-sigma pi-sigma marked this pull request as draft December 13, 2024 09:18
Paul Schilling added 2 commits December 13, 2024 10:45
[#2932] Update eHerkenning OIDC flow: get & store vestigingsnummer
28260d0 
    - When logging in with eHerkenning via OIDC, get the
      vestigingsnummer from the OIDC claim (if present) and
      store in session
[#2932] Update eHerkenning SAML flow: get & store vestigingsnummer
fcb4c6b
@pi-sigma pi-sigma force-pushed the task/2932-eherkenning-login-choice branch from 359d14c to fcb4c6b Compare December 13, 2024 09:45
@pi-sigma pi-sigma marked this pull request as ready for review December 13, 2024 10:47
@pi-sigma pi-sigma requested a review from swrichards December 13, 2024 10:47
@swrichards swrichards requested a review from alextreme December 16, 2024 15:43
@alextreme alextreme merged commit 1645f79 into develop Dec 16, 2024
20 checks passed
@alextreme alextreme deleted the task/2932-eherkenning-login-choice branch December 16, 2024 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swrichards swrichards swrichards approved these changes

@alextreme alextreme alextreme approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pi-sigma @codecov-commenter @swrichards @alextreme