-
Notifications
You must be signed in to change notification settings - Fork 95
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
Do not expose the testing IdP implementation #2072
Comments
@matrss When we move IdP tests/, it means we are no longer offering testing IdP and guidance to the user. When I think about the user's end, it can be a bit confusing, especially for users configuring first time with 3rd party IdP who are not familiar with configurations and use cases with SAML2 authentication. What are your insights on the user's perspective? If you think we no longer need further guidance, please assign me I will work on this issue. |
We would still offer it under tests/, but not as part of the published package e.g. installable from conda-forge. I consider this IdP implementation to be a lightweight one that would be easy to spin up in our tests (albeit we don't have any using it, yet), but not one we should expose into a production setting since it is limited in its functionality and not thoroughly checked for potential security issues.
I don't think the testing IdP implementation has that much value in this regard, anyway. If a mscolab server admin (other, less technical, users won't interact with this anyway, I'd assume) wants to integrate mscolab with a SAML IdP, then I'd expect the documentation integrating with keycloak (https://github.com/Open-MSS/MSS/blob/develop/docs/sso_via_saml_mscolab.rst) to be of more value, since that is closer to a real-world IdP they would encounter. This is just my opinion though. Maybe @ReimarBauer has a different view, e.g. from integrating MSColab with the Helmholtz AAI? |
it is not helping a user when we have it under tests. This is something similiar to the werkzeug/flask web server. We have to add a big hint simliar to that example. INFO: WARNING: This is a development server. Do not use it in a production deployment. Use a production e.g. keycloak server instead. For the next major I want to keep it as it is. Of course with the message. On a further step we maybe can implement auth plugins. |
I don't think it is helping a user where it is now either, is my point. So why expose a msidp entrypoint in the package?
What? If you mean what I think you mean, then please not. There is no point in re-implementing an IdP here. |
You are right that it is only something for the developer, but with the same arguments there won't be a flask werkzeug wsgi server available. """ So we should answer if it helps a developer and if it is not an entrypoint how the results of the documentation can be archieved. |
with auth plugins I meant more the server part lines, this did not scale when there now the next e.g. ldap needs routes etc to be used with us. |
Anyway I think we need to refactor it to avoid problems When it gets deployed the hard coded passwords have to become removed. is there a reason PASSWD is different.
When it is run by a user it should show a big hint, that it is not for production, and without the configuration it should not crash. |
Ah OK. I don't think this is much of a priority though, I am pretty sure LDAP wouldn't require these types of endpoints on our end and would instead rely purely on LDAP queries coming from our side. Other than SAML and OIDC, there is nothing relevant in this space, and an entire plugin system for two types of auth is overkill, IMO.
For testing purposes it doesn't matter, for anything else this shouldn't be used anyway. I don't see a purpose in making this configurable. A single hardcoded
Yes. |
we don't need to move the userata yet, we have something similiar in the example seed database for mscolab. At the moment an enterprise scanner annoyes me I change my mind. |
Originally posted by @matrss in #2069 (comment)
The text was updated successfully, but these errors were encountered: