-
Notifications
You must be signed in to change notification settings - Fork 5
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
Initial Remote Vetting cleanup #220
Initial Remote Vetting cleanup #220
Conversation
0ddb55c
to
24564d9
Compare
This is done to ease analysing the identity data.
01cd693
to
b436890
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like what I see here! See some feedback in the code review section below. I think this is good to ship. But maybe you want to address some of my points. It's up to you.
...pSelfService/SelfServiceBundle/Service/RemoteVetting/Encryption/IdentityFilesystemWriter.php
Show resolved
Hide resolved
...Surfnet/StepupSelfService/SelfServiceBundle/Service/RemoteVetting/ServiceProviderFactory.php
Show resolved
Hide resolved
- '%remote_vetting_sp%' | ||
$router: '@router' | ||
$entityId: "%remote_vetting_entity_id%" | ||
$assertionConsumerUrlSlug: "ss_second_factor_remote_vet_acs" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why hard-code it in the service container? Might be more predictable to hard code it in code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The slug of the route itself isn't hardcoded either so the slug is dependency of the ServiceProviderFactory and that's why i used DI.
remote_vetting_entity_id: https://selfservice.stepup.example.com/rv/metadata | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I lack context, but isn't the RV configuration not still configurable? Albeit it became less pronounced, but you can still configure the entitiy id in parameters.yml.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only the entityid and keys, but not the endpoint anymore, that seemed to be confusing because of the multiple acs endpoints that exist.
@@ -83,32 +33,58 @@ surfnet_stepup_self_service_saml_stepup_provider: | |||
pop_failed: "%gssp_webauthn_pop_failed%" | |||
app_android_url: "%gssp_webauthn_app_android_url%" | |||
app_ios_url: "%gssp_webauthn_app_ios_url%" | |||
azuremfa: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might lead to issues when building the dev environment. The application provisioning first composer-installs with scripts using the VCS provided parameters. It then dumps the StepupDeploy specific parameters. And again composer installs them. IIRC the first run requires the azuremfa gssp to be in config. I do not have time to re-provision my SelfService at this moment. But maybe this warning rings a bell with you? It might well be, I'm mistaken here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to change the config in order to get my dedicated RV Stepup-VM up and running with the config in the remote-vetting branch in Deploy. Not all GSSP's were and will be configured for RV so this was done to just reflect that config. This indeed should be addressed to reflect develop in Deploy but I think this should be addressed just before (and only if) RV lands back in develop. But because that could happen in a while and a lot of Deploy's config could have been changed in the meantime I'll prefer to keep this config in sync with the RV branch in Deploy.
Make the acs location static for remote vetting. This doesn't need to be configurable.
Remove the double escaping of objects in the process DTO so it would be easier to maintain later on.
Use dedicated mock endpoint for testing instead of relying on Irma configuration. So this won't conflict when using Deploy.
d37d020
to
d1fada0
Compare
Move the mock services out of the production service configuration. This is better in terms of defence-in-depth.
Add assertion to the session test to get rid of risky tests.
Use a dedicated encryption key-pair in tests to ease maintenance.
Use the ApplicationHelper itself instead of a mock in the integration test to test it's behavior.
d1fada0
to
c49e1c1
Compare
Some cleanup needed to be done in order to remove some shortcuts made
during development of the RV POC.
This refactoring consists of: