merge of Gsoc2023 nilupul manodya #2069

ReimarBauer · 2023-11-06T14:38:38Z

This is a direct merge request for the GSOC 2023 contribution of Nilupul Manodya. Beforehand develop was merged into his branch. We maybe can also squash and merge, what do you think?

The outcome of the project was tested on mss-stage. Further devlopment should be done in develop on that idea.

* configure sp and idp * update meta.yml remove cherypy * fixes previous * update notice * update readme * regroup idp_uwsgi * regroup app.py * regroup, change wsgi server to flask * Update conf_sp_idp/README.md Co-authored-by: Matthias Riße <[email protected]> * hide secrets by config * update copy-paste-able command for creating keys and certificates * Update README.md * correct copyright lines * remove make_metadata.py file and update doc with new flow * remove idp.xml file * remove condition libxmlsec1 * Update conf_sp_idp/sp/app/conf.py Co-authored-by: Matthias Riße <[email protected]> * Update conf_sp_idp/idp/idp.py Co-authored-by: Matthias Riße <[email protected]> * remove generate_metadatascript * remove hardcoded path * recorrect copyrights --------- Co-authored-by: Matthias Riße <[email protected]>

* split sp and idp * generate doc * remove prints idp.py * update comeponents.rst

* ui changes in qt for sso * fixes qt UI implementation * get idp_enabled response from server * update tests for test_hello * update test utils * Update mslib/msui/mscolab.py Co-authored-by: Matthias Riße <[email protected]> * fix typo * move downed idp_enabled exception * increase height ui_mscolab_connect_dialog * resolve comments --------- Co-authored-by: Matthias Riße <[email protected]>

* web browser implementation * update gitgnore * resolve comments * update docstring

* db modeling * add users into id[ * backend yaml implementation * set server conf * config server for sso * qt ui implmentation * backend html templates implementation * update testcases * config qt client app * update gitignore * set yaml endpoints * update docs * update test utill, and fix error * fix test utils * remove disabled pylint * add libxmlsec1 into dep * set IDP ENabled false * Update mslib/mscolab/server.py Co-authored-by: Matthias Riße <[email protected]> * recorrect commit * update db modeling with authentication_backend for multiple idps * update conf for the multiple idps * template implementation * msui update redirect url for multiple idps * saml update for multiple idps * update mscolab server for multiple idps * update doc for multiple idps * automate CERTs generation and paths * update doc * correct typo in doc * update doc * fix typos update gitignore * fix config idp_conf * update gitignore * set one time token access * add params for cert creation * set idp token for one time validation * fix unnnescessary debug * remove duplicate imports * Update mslib/mscolab/mscolab.py Co-authored-by: Matthias Riße <[email protected]> * automate saml yaml file and improve error handling * rename IDP_ENABLED to USE_SAML2 * update error template * update doc * add todo idp_wsgi * update db models * recorrect doc * add todo refactors --------- Co-authored-by: Matthias Riße <[email protected]>

* remove global var * remove idp.subjects file dirs * remove relaystste, rndstr and use secrets * remove shell=True * correct typos * fix group order * enable flake8 for GSOC2023-NilupulManodya * fix lint * fix lint * fixes comments * resolve comments * fix comments * update doc

…ltiple-IdPs improve mscolab for configure multiple Idps

…merge_current_develop

Merge current develop

* ssl verification enablement for SSO * add hint

* remove testing sp * remove documentation auth_client_sp

* create documentation sso integration * added into makefile components * change dir images * resolve comments, add sample files * resolve comments

* change cookies dir of web browser * Update mslib/msui/msui_web_browser.py Co-authored-by: Matthias Riße <[email protected]> --------- Co-authored-by: Matthias Riße <[email protected]>

* improve accessibility saml2 urls * resolve comments

joernu76 · 2023-11-06T15:23:13Z

docs/samples/config/mscolab/mss_saml2_backend.yaml.samlple

I think the ending should be ".sample"?

joernu76 · 2023-11-06T15:25:43Z

mslib/mscolab/mscolab.py

@@ -93,6 +96,264 @@ def handle_db_seed():
    print("Database seeded successfully!")


+def handle_mscolab_certificate_init():
+    print('generating CRTs for the mscolab server......')


There are some print-function calls in here already, but we should generally use the logging for console output.

IIRC this function is only called when initially generating a key and (self-signed) certificate for SAML, after which the program exits. It is not part of a long-running process when MSColab is running as a server. In this case one could argue that this print call is part of the direct user interaction when generating a key and (self-signed) certificate, not really logging.

Granted, in this line the distinction is not that clear, but in general I think everything that is required for a meaningful user interaction with the program should stay as a print (or the other way around: the program output should still make sense if someone disables all logging). Some other print statements in this file should therefore really stay that way (or maybe go to stderr), in my opinion.

joernu76 · 2023-11-06T15:26:23Z

mslib/mscolab/mscolab.py

+
+    try:
+        cmd = ["openssl", "req", "-newkey", "rsa:4096", "-keyout",
+               f"{mscolab_settings.MSCOLAB_SSO_DIR}/key_mscolab.key",


please us os.path.join (also below)

joernu76 · 2023-11-06T15:27:23Z

mslib/mscolab/mscolab.py

+  #       name_id_format_allow_create: true
+"""
+    try:
+        file_path = f"{mscolab_settings.MSCOLAB_SSO_DIR}/mss_saml2_backend.yaml"


os.path.join

joernu76 · 2023-11-06T15:27:36Z

mslib/mscolab/mscolab.py

+            file.write(saml_2_backend_yaml_content)
+        return True
+    except (FileNotFoundError, PermissionError) as error:
+        print(f"Error while generated backend .yaml for the local mscolabserver: {error}")


logging.error

joernu76 · 2023-11-06T15:28:35Z

mslib/mscolab/mscolab.py

+    print('generating metadata file for the mscolab server')
+
+    try:
+        command = ["python", "mslib/mscolab/mscolab.py", "start"] if repo_exists else ["mscolab", "start"]


The PYTHONPATH needs to be set properly for the first command to work. How do you ensure this?

The first command is only used if this is run from a git clone instead of the installed MSS package. Detecting git is used as a proxy to detect if this is run in a development context, I think. The assumption is that you have an adequate python environment enabled in that case.

I'd argue we should only use the second command though, and get rid of the git detection part entirely. In a development context one could then use pip install -e . to make the msui, mscolab, etc. commands use the code in the local checkout, instead of any other version that might be in the PATH. This would require that setup.py declares all entrypoints though, which it currently does not (only msui is in there).

Maybe mamba has a similar "editable install" feature.

it is quite simple to get the correct path by mscolab.__file__

Contriute to the idea pip install -e . on mamba-org/mamba#695

joernu76 · 2023-11-06T15:29:12Z

mslib/mscolab/mscolab.py

+    print('generating metadata for localhost identity provider')
+
+    try:
+        if os.path.exists(f"{mscolab_settings.MSCOLAB_SSO_DIR}/idp.xml"):


os.path.join

joernu76 · 2023-11-06T15:29:29Z

mslib/mscolab/mscolab.py

+        if os.path.exists(f"{mscolab_settings.MSCOLAB_SSO_DIR}/idp.xml"):
+            os.remove(f"{mscolab_settings.MSCOLAB_SSO_DIR}/idp.xml")
+
+        idp_conf_path = "mslib/msidp/idp_conf.py"


os.path.join

joernu76 · 2023-11-06T15:30:45Z

mslib/mscolab/mscolab.py

@@ -130,6 +400,13 @@ def main():
        print("Version:", __version__)
        sys.exit()

+    try:
+        _ = git.Repo(os.path.dirname(os.path.realpath(__file__)), search_parent_directories=True)


I am not sure that this is a reasonable use case. Useful for testing, but adds a lot of code that needs to be maintained.

joernu76 · 2023-11-06T15:34:51Z

Great work!

ReimarBauer · 2023-11-07T13:05:49Z

@joernu76 we should merge this and improve afterwards. I already did some changes on the staging system.

matrss · 2023-11-07T09:11:27Z

.gitignore

@@ -24,4 +24,5 @@ build/
 mss.egg-info/
 tutorials/recordings
 tutorials/cursor_image.png
-
+__pycache__/
+instance/


Where was that line coming from again? I cannot remember what this path was used for. Is it still required?

matrss · 2023-11-07T09:37:50Z

docs/samples/config/mscolab/mss_saml2_backend.yaml.samlple

+  #         discovery_response:
+  #         - [<base_url>/<name>/disco, 'urn:oasis:names:tc:SAML:profiles:SSO:idp-discovery-protocol']
+  #       name_id_format: 'urn:oasis:names:tc:SAML:2.0:nameid-format:transient'
+  #       name_id_format_allow_create: true


This file is missing a newline after the last line.

matrss · 2023-11-07T10:00:44Z

localbuild/meta.yaml

@@ -19,6 +19,7 @@ build:
    - mswms_demodata = mslib.mswms.demodata:main
    - mscolab = mslib.mscolab.mscolab:main
    - mssautoplot = mslib.utils.mssautoplot:main
+    - msidp = mslib.msidp.idp:main


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.

you are right, we can change this after the merge.

matrss · 2023-11-07T11:52:38Z

mslib/mscolab/conf.py

+    USE_SAML2 = False
+
+    # SSL certificates verification during SSO.
+    VERIFY_SSL_CERT = True


This config option name should somehow make clear that it only applies to the certificates used between MSColab as a SAML SP and the IdP, nothing else.

mslib/mscolab/conf.py

matrss · 2023-11-07T12:41:45Z

mslib/mscolab/mscolab.py

+        process = subprocess.Popen(command)
+
+        # Add a small delay to allow the server to start up
+        time.sleep(10)


Not really optimal to use sleep as a synchronization primitive here (ideally it should wait until process signals that it is ready in some way, e.g. when process' stdout contains something that indicates that the server is up and running). This is not a very critical code path though, so not too much of a concern.

matrss · 2023-11-07T12:44:47Z

mslib/mscolab/mscolab.py

+        cmd_curl = ["curl", "http://localhost:8083/metadata/localhost_test_idp",
+                    "-o", f"{mscolab_settings.MSCOLAB_SSO_DIR}/metadata_sp.xml"]
+        subprocess.run(cmd_curl, check=True)
+        process.kill()


Suggested change

process.kill()

process.terminate()

Sending SIGKILL seems a bit excessive.

matrss · 2023-11-07T12:49:34Z

mslib/mscolab/mscolab.py

+    if os.path.exists(mscolab_settings.MSCOLAB_SSO_DIR):
+        shutil.rmtree(mscolab_settings.MSCOLAB_SSO_DIR)
+    create_files()
+    if not handle_mscolab_certificate_init():


These could also use exceptions instead of boolean return values to indicate success/failure.

matrss · 2023-11-07T13:00:08Z

mslib/msui/mscolab.py

+                if idp_enabled:
+                    # Hide user creatiion seccion if IDP login enabled
+                    self.addUserBtn.setHidden(True)
+                    self.clickNewUserLabel.setHidden(True)


Shouldn't it still be possible to register a new non-IdP user on the MSColab instance, even if an IdP is configured? I feel like disabling "local" account registration should be a different config option (that would need to be enforced server-side).

also in the workflow of profile/usercreation by the IDP is not completed. The new profile is not activated, it would need a validation (recover password attempt currently) first. There some changes we have to think about after we merged this in.

We have to verify too that a user with a "None" Password because of IDP can't fetch one and update that 'None' over password recovery. Some of these parts need a coverage by tests.

matrss · 2023-11-07T13:19:21Z

mslib/mscolab/server.py

+        except TypeError:
+            return jsonify({"success": False}), 401
+
+    @APP.route("/metadata/<idp_identity_name>", methods=['GET'])


This route could also be defined in a loop for each IdP (similar to the acs_post_handler), so that it does not match too broadly. That would remove the requirement for an explicit 404 (and the need for the entire 404 template).

ReimarBauer · 2023-11-07T13:32:34Z

@matrss these are all things we have to improve / and figure out after the merge in develop. The GSOC branch can't have further changes.

ReimarBauer · 2023-11-07T14:12:26Z

Merging GSOC project by Nilupul Manodya - Implement a SAML 2.0 service provider (SP) into mscolab

nilupulmanodya and others added 30 commits June 10, 2023 10:06

remove inputs from conditions (#1808)

e06b396

Split conf sp idp (#1811)

f2b1434

* split sp and idp * generate doc * remove prints idp.py * update comeponents.rst

web browser implementation (#1814)

245d64e

* web browser implementation * update gitgnore * resolve comments * update docstring

improve code for multiple Idps

198cf88

conf routes for multiple conf

db82eb7

remove uncessary .yaml

90a1c62

update cmd metadata

df4ae37

update conf

8dd2c9e

update saml handler for multiple idps

05f3c2c

pinning of xmlschema

aa47a09

pin werkzeug

1912fd4

disable pytests for todo refactor

77f21d3

disbale whole file gsoc_testing

cf2a3b9

fix conf

c7ab2ae

resolve comments

34e7e33

resolve comments

b02854a

Merge pull request #2043 from nilupulmanodya/implement-mscolab-for-mu…

f94da54

…ltiple-IdPs improve mscolab for configure multiple Idps

Merge branch 'develop' of https://github.com/nilupulmanodya/MSS into …

458a978

…merge_current_develop

manual conflict resolve ui_mscolab_connect_dialog.ui file

2d59aa5

resolve flake8

18a9e7c

Merge pull request #2061 from nilupulmanodya/merge_current_develop

ef326c2

Merge current develop

set SSL certificate verification enablement (#2062)

3d328f9

* ssl verification enablement for SSO * add hint

Remove testing SP (#2066)

eefc67c

* remove testing sp * remove documentation auth_client_sp

Create documentation for SSO integration through SAML (#2064)

8d88932

* create documentation sso integration * added into makefile components * change dir images * resolve comments, add sample files * resolve comments

change cookies dir of web browser (#2063)

30a04ea

* change cookies dir of web browser * Update mslib/msui/msui_web_browser.py Co-authored-by: Matthias Riße <[email protected]> --------- Co-authored-by: Matthias Riße <[email protected]>

improve accessibility saml2 urls (#2068)

a2e10e1

* improve accessibility saml2 urls * resolve comments

ReimarBauer requested review from joernu76 and matrss November 6, 2023 14:39

ReimarBauer changed the title ~~Gsoc2023 nilupul manodya~~ merge of Gsoc2023 nilupul manodya Nov 6, 2023

joernu76 reviewed Nov 6, 2023

View reviewed changes

joernu76 approved these changes Nov 7, 2023

View reviewed changes

matrss reviewed Nov 7, 2023

View reviewed changes

ReimarBauer merged commit f6b3ad1 into develop Nov 7, 2023
6 checks passed

ReimarBauer mentioned this pull request Nov 8, 2023

rename example files to ".sample"? #2083

Closed

nilupulmanodya mentioned this pull request Nov 13, 2023

fixes i2087 #2092

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

merge of Gsoc2023 nilupul manodya #2069

merge of Gsoc2023 nilupul manodya #2069

ReimarBauer commented Nov 6, 2023 •

edited

Loading

joernu76 Nov 6, 2023

joernu76 Nov 6, 2023

matrss Nov 7, 2023

joernu76 Nov 6, 2023

joernu76 Nov 6, 2023

joernu76 Nov 6, 2023

joernu76 Nov 6, 2023

matrss Nov 7, 2023

ReimarBauer Nov 8, 2023 •

edited

Loading

joernu76 Nov 6, 2023

joernu76 Nov 6, 2023

joernu76 Nov 6, 2023

joernu76 commented Nov 6, 2023

ReimarBauer commented Nov 7, 2023

matrss Nov 7, 2023

matrss Nov 7, 2023

matrss Nov 7, 2023

ReimarBauer Nov 7, 2023

matrss Nov 7, 2023

matrss Nov 7, 2023

matrss Nov 7, 2023

matrss Nov 7, 2023

matrss Nov 7, 2023

ReimarBauer Nov 7, 2023

ReimarBauer Nov 7, 2023

matrss Nov 7, 2023

ReimarBauer commented Nov 7, 2023

ReimarBauer commented Nov 7, 2023

merge of Gsoc2023 nilupul manodya #2069

merge of Gsoc2023 nilupul manodya #2069

Conversation

ReimarBauer commented Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ReimarBauer Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joernu76 commented Nov 6, 2023

ReimarBauer commented Nov 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ReimarBauer commented Nov 7, 2023

ReimarBauer commented Nov 7, 2023

ReimarBauer commented Nov 6, 2023 •

edited

Loading

ReimarBauer Nov 8, 2023 •

edited

Loading