Open-MSS / MSS

A QT application, a OGC web map server, a collaboration server to plan atmospheric research flights.
https://open-mss.github.io
Apache License 2.0
60 stars 75 forks source link

Do not expose the testing IdP implementation #2072

Closed matrss closed 6 months ago

matrss commented 11 months ago

https://github.com/Open-MSS/MSS/blob/f6b3ad110dbfaa12d439cadea6476dcd54d9328d/localbuild/meta.yaml#L22 Do we really want to expose the IdP that was implemented? I still consider this to be testing infrastructure that should be moved to tests/ and be used as a lightweight target in integration tests of the SP functionality only.

_Originally posted by @matrss in https://github.com/Open-MSS/MSS/pull/2069#discussion_r1384668617_

nilupulmanodya commented 8 months ago

@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.

matrss commented 8 months ago

When we move IdP tests/, it means we are no longer offering testing IdP [...]

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.

[...] 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?

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?

ReimarBauer commented 7 months ago

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.

matrss commented 7 months ago

it is not helping a user when we have it under tests.

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?

On a further step we maybe can implement auth plugins.

What? If you mean what I think you mean, then please not. There is no point in re-implementing an IdP here.

ReimarBauer commented 7 months ago

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.

""" When running publicly rather than in development, you should not use the built-in development server ( flask run ). The development server is provided by Werkzeug for convenience, but is not designed to be particularly efficient, stable, or secure. """

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.

ReimarBauer commented 7 months ago

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.

ReimarBauer commented 7 months ago

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.

matrss commented 7 months ago

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.

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.

When it gets deployed the hard coded passwords have to become removed.

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 <username>:<password> combination would suffice for testing purposes and as soon as a password needs to be changed this is a clear sign that this IdP should no longer be used, IMO.

When it is run by a user it should show a big hint, that it is not for production

Yes.

ReimarBauer commented 7 months ago

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.