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/retrieve saml metadata via url #16

Merged
merged 42 commits into from
Oct 27, 2022

Conversation

Ewen-Lgy
Copy link

@Ewen-Lgy Ewen-Lgy commented Aug 31, 2022

Related to ticket open-formulieren/open-forms#1849

I might not have time to close this pull request since it is my last day so here is a summary of what has been done so far and what is left to do.

DONE :

  • Created solo models for Ehekernning and Digid
  • Created endpoint/urls to generate : Eherkenning, Digid and DiesntCatalogus metadata
  • created custom change_form.html admin template so we can have buttons (like history) to generate metadata
  • Updated tests from command to models
  • Added mixins for generating certificate and fill digid/eherkenning metadata config
  • Added tests for metadata endpoints
  • Deleted commands for generating metadata

TODO:

  • Complete the error message if metadata genreration has failed
  • Add more model field verification to generate properly the metadata (especially for OIN and makelaar_id in Eherkenning model)
  • Add various dropdowns for different fields
  • Update saml2 clients in order to not use EHERKENNING and DIGID settings anymore (only the metadata related settings)
  • !! Update the documentation

UNDO:

  • commands to generate metadata still have to be there
  • Put back metadata commands tests

@Ewen-Lgy Ewen-Lgy force-pushed the feature/retrieve-saml-metadata-via-url branch from 6bd6ec8 to e3a5e3c Compare August 31, 2022 10:07
@codecov
Copy link

codecov bot commented Aug 31, 2022

Codecov Report

Merging #16 (4b9a3d4) into master (34c5a0e) will increase coverage by 2.46%.
The diff coverage is 98.71%.

@@            Coverage Diff             @@
##           master      #16      +/-   ##
==========================================
+ Coverage   93.78%   96.25%   +2.46%     
==========================================
  Files          44       60      +16     
  Lines        2172     2880     +708     
==========================================
+ Hits         2037     2772     +735     
+ Misses        135      108      -27     
Impacted Files Coverage Δ
digid_eherkenning/mock/views/digid.py 72.54% <ø> (+5.27%) ⬆️
digid_eherkenning/utils.py 92.59% <ø> (ø)
digid_eherkenning/views/digid.py 94.78% <ø> (+0.71%) ⬆️
tests/project/urls.py 100.00% <ø> (ø)
digid_eherkenning/settings.py 92.30% <90.00%> (-7.70%) ⬇️
...t/commands/generate_eherkenning_dienstcatalogus.py 93.54% <92.30%> (+8.64%) ⬆️
digid_eherkenning/management/commands/_base.py 94.93% <94.93%> (ø)
digid_eherkenning/models/digid_metadata_config.py 95.00% <95.00%> (ø)
digid_eherkenning/saml2/base.py 94.90% <96.29%> (+7.24%) ⬆️
..._eherkenning/models/eherkenning_metadata_config.py 97.22% <97.22%> (ø)
... and 31 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@sergei-maertens sergei-maertens left a comment

Choose a reason for hiding this comment

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

  • Re-introduce management commands, but instead let them configure/use the solo model config classes. Any CLI flags result in the config class being overridden on the fly. Possibly we can add a --save-config flag to also persist that.
  • Regular views/SAML flows etc. should fetch config from the classes rather than from the Django setting
  • Optimize the config classes so they always select_related the certificate - this query will always be done and should be avoided.

digid_eherkenning/saml2/eherkenning.py Outdated Show resolved Hide resolved
digid_eherkenning/saml2/eherkenning.py Outdated Show resolved Hide resolved
digid_eherkenning/saml2/eherkenning.py Outdated Show resolved Hide resolved
digid_eherkenning/saml2/eherkenning.py Outdated Show resolved Hide resolved
digid_eherkenning/saml2/eherkenning.py Outdated Show resolved Hide resolved
requirements/base.txt Show resolved Hide resolved
tests/mixins.py Outdated Show resolved Hide resolved
tests/mixins.py Outdated Show resolved Hide resolved
tests/project/settings.py Show resolved Hide resolved
tests/project/urls.py Outdated Show resolved Hide resolved
@sergei-maertens sergei-maertens force-pushed the feature/retrieve-saml-metadata-via-url branch 3 times, most recently from 1cef1cb to fe1314e Compare October 21, 2022 10:53
tests/mixins.py Outdated Show resolved Hide resolved
digid_eherkenning/models/eherkenning_metadata_config.py Outdated Show resolved Hide resolved
digid_eherkenning/migrations/0001_initial.py Outdated Show resolved Hide resolved
digid_eherkenning/models/metadata_config.py Outdated Show resolved Hide resolved
digid_eherkenning/models/metadata_config.py Outdated Show resolved Hide resolved
digid_eherkenning/models/eherkenning_metadata_config.py Outdated Show resolved Hide resolved
digid_eherkenning/saml2/digid.py Outdated Show resolved Hide resolved
digid_eherkenning/saml2/digid.py Outdated Show resolved Hide resolved
digid_eherkenning/views/eherkenning.py Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
Ewen and others added 14 commits October 21, 2022 15:14
... and use this configuration source for metadata generation.

This is a squashed commit of about 8 commits by Ewen-Lgy, many
thanks for the initial work.
There are some configuration params/flags that are now included that may
be relevant.
* Added language option to eherkenning config
* Management command now uses the eHerkenning client to generate metadata
* Added EH/eidas-specific requested (additional) attributes
@sergei-maertens sergei-maertens force-pushed the feature/retrieve-saml-metadata-via-url branch from 9c12026 to 6513ee5 Compare October 21, 2022 13:14
@sergei-maertens sergei-maertens self-assigned this Oct 21, 2022
@sergei-maertens sergei-maertens force-pushed the feature/retrieve-saml-metadata-via-url branch 2 times, most recently from a8918aa to e1b1940 Compare October 27, 2022 07:55
@sergei-maertens sergei-maertens force-pushed the feature/retrieve-saml-metadata-via-url branch from e1b1940 to 238b12c Compare October 27, 2022 09:47
Converted a number of fields into dropdowns in the admin UI by specifying
the valid choices.
@sergei-maertens sergei-maertens force-pushed the feature/retrieve-saml-metadata-via-url branch from 0f16f92 to 0fb0907 Compare October 27, 2022 14:02
... and configure them at the URL level rather than duplicating the
code and error messages.
And fix privates/sendfile setup
As the config is used at runtime too, it would be misleading to keep this part in the model names
@sergei-maertens sergei-maertens merged commit 6762a18 into master Oct 27, 2022
@sergei-maertens sergei-maertens deleted the feature/retrieve-saml-metadata-via-url branch October 31, 2022 14:18
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.

3 participants