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

org-oidc auth plugin employee login #4947

Merged
merged 2 commits into from
Dec 19, 2024

Conversation

robinmolen
Copy link
Contributor

@robinmolen robinmolen commented Dec 18, 2024

Closes #4863

Changes

This should solve the problem of employee login on forms with the org-oidc auth plugin.

Currently this is a WIP. I'm not sure if this is the right approach, and solves the right issue (this might be a very superficial solution, but im not sure). This currently contains:

  • Test that showcases the failing scenario
  • Solution for employee_id authentication
    (Which is highly experimental, and probably should/will change)

This that still should be done:

  • Add employee id to user
  • Use employee_id as identifier for employee case in to_auth_context_data
    (I think this requires passing employee id from the org-oidc plugin handle_return())

Checklist

Check off the items that are completed or not relevant.

  • Impact on features

    • Checked copying a form
    • Checked import/export of a form
    • Config checks in the configuration overview admin page
    • Problem detection in the admin email digest is handled
  • Release management

    • I have labelled the PR as "needs-backport" accordingly
  • I have updated the translations assets (you do NOT need to provide translations)

    • Ran ./bin/makemessages_js.sh
    • Ran ./bin/compilemessages_js.sh
  • Dockerfile/scripts

    • Updated the Dockerfile with the necessary scripts from the ./bin folder
  • Commit hygiene

    • Commit messages refer to the relevant Github issue
    • Commit messages explain the "why" of change, not the how

@robinmolen robinmolen changed the title Bug/4863 OIDC org employee login org-oidc auth plugin employee login Dec 18, 2024
@robinmolen robinmolen force-pushed the bug/4863-oidc-org-employee-login branch from e7374cd to e70fab7 Compare December 18, 2024 12:40
Comment on lines 356 to 369
case (AuthAttribute.employee_id, ""):
employee_context: EmployeeContext = {
"source": "custom",
"levelOfAssurance": self.loa,
"representee": {
"identifierType": "employee_id",
"identifier": self.value,
},
}
return employee_context
Copy link
Member

Choose a reason for hiding this comment

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

Some background to explain more about how things work in Open Forms!

The model AuthInfo (which defines the to_auth_context_data method) captures who is authenticated for the submission. This means that if an employee of the organization (after OIDC login) logs in for the form for themselves (the other flow I will describe below), they are the authorizee.legalSubject (representing themselves). This is a rather simple setup of employee context that needs to be returned if we follow the established pattern (where representee is implied from the authorizee/legal subject).

{
  "source": "custom",
  "loa": "unknown",
  "authorizee": {
    "legalSubject": {
      "identifierType": "opaque",
      "identifier": self.value
    }
  }
}

Identifier type opaque means you can't derive any meaning/format validation from it.

The other flow is where an employee is logged in with OIDC, and they enter the BSN or KVK of the customer. In the code, this brings you to openforms.authentication.signals.set_auth_attribute_on_session in the branch with if registrator_subject. In that situation, on the AuthInfo instance not the employee details are stored, but the BSN/KVK nr of the actual customer, and the attribute is then set to bsn or kvk rather than employee_id, so you can never end up in this case in those flows. Meaning, the to_auth_context_data only needs to consider the situation where an employee is acting on their own behalf which is nice because it simplifies things a bit.

Looking at the Sentry error, this is the right place to patch this up.

So, key takeaways:

  • the context dict/shape here needs to use authorizee > legalSubject structure, similar to DigiD without machtigen (first case)
  • we don't need to consider "acting on behalf of someone else" for employee_id attribute

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay.. I think i'm starting to understand this a bit more..

Should the context then not also contain the representee data? (Although this would probably be the same information.., right?)

Copy link
Member

@sergei-maertens sergei-maertens Dec 19, 2024

Choose a reason for hiding this comment

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

if no representee is specified, it is indeed (implicitly) equal to authorizee.legalSubject. Therefore, this information is not duplicated.



class EmployeeRepresenteeEntity(TypedDict):
identifierType: Literal["employee_id"]
Copy link
Member

Choose a reason for hiding this comment

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

I'd use either opaque or custom here. I gravitate towards the former because it's already used for the eHerkenning acting subject too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check, yeah i'll use opaque then it that case, to keep things consistent 👍

@@ -52,6 +52,7 @@ VCR.py). The primary reason this setup exists, is for automated testing reasons.
attributes from `eherkenning-bewindvoering`.
- `admin` / `admin`, intended to create as django user (can be made staff). The email address is
`[email protected]`.
- `employee` / `employee`, has the `employee_id` attribute (for org-oidc)
Copy link
Member

Choose a reason for hiding this comment

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

this isn't necessary, since you can populate the user model field from already existing claims, see the documentation: https://open-forms.readthedocs.io/en/stable/configuration/authentication/oidc_org.html#configuring-oidc-for-login-of-organization-members

In these cases I would probably just map the sub or username or email claim to the employee_id field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But isn't this needed for the login itself?

Copy link
Member

Choose a reason for hiding this comment

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

you can use the admin/admin user for that :)

@robinmolen robinmolen force-pushed the bug/4863-oidc-org-employee-login branch 2 times, most recently from b03d4c5 to d2e30b2 Compare December 19, 2024 11:47
@robinmolen robinmolen force-pushed the bug/4863-oidc-org-employee-login branch from d2e30b2 to 147b2cf Compare December 19, 2024 11:53
@robinmolen robinmolen added the needs-backport Fix must be backported to stable release branch label Dec 19, 2024
@robinmolen robinmolen marked this pull request as ready for review December 19, 2024 11:55
Copy link

codecov bot commented Dec 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.62%. Comparing base (e11c687) to head (147b2cf).
Report is 6 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4947   +/-   ##
=======================================
  Coverage   96.61%   96.62%           
=======================================
  Files         760      760           
  Lines       25825    25837   +12     
  Branches     3384     3385    +1     
=======================================
+ Hits        24952    24964   +12     
  Misses        608      608           
  Partials      265      265           

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

@sergei-maertens sergei-maertens merged commit 260a985 into master Dec 19, 2024
34 of 37 checks passed
@sergei-maertens sergei-maertens deleted the bug/4863-oidc-org-employee-login branch December 19, 2024 13:32
sergei-maertens pushed a commit that referenced this pull request Dec 19, 2024
sergei-maertens pushed a commit that referenced this pull request Dec 19, 2024
@sergei-maertens
Copy link
Member

Backports:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-backport Fix must be backported to stable release branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

oidc_org: RuntimeError Unknown attribute: employee_id
2 participants