refactor server.py for using Blueprint

[JanEisermann]

Purpose of PR?:

Fixes #2081

Does this PR introduce a breaking change?
included Blueprint, fixed test_load_no_file

If the changes in this PR are manually verified, list down the scenarios covered::
tests succeed, not manually verified

Additional information for reviewer? :
Mention if this PR is part of any design or a continuation of previous PRs

Does this PR results in some Documentation changes?
If yes, include the list of Documentation changes

Checklist:

• Bug fix. Fixes #
• New feature (Non-API breaking changes that adds functionality)
• PR Title follows the convention of <type>: <subject>
• Commit has unit tests

[ReimarBauer]

That's good progress. We can already see it's becoming much more readable. You need to rearrange a few things.

The blueprints should contain as few additional functions as possible.

Especially not ones that need to be imported from other blueprints.

see comments

[ReimarBauer]

see comments

[joernu76]

Prompt:

Perform a senior developer like review of the current branch in relation to origin/develop. Take into account a former review contained in review.md related to commit ee1d8b4.Split review results in general comments, security related issues, performance issues, and logic/algorithmic issues.

Result:

General comments

• The blueprintification has clearly progressed: mslib/mscolab/server.py shrank from ~830 to ~230 lines, blueprints have moved into their own packages (auth, chat, docs, operation, user, gallery), and the previously offending top-level coupling of mslib/utils/auth.py to MSColab internals has been cleaned up (helpers moved to mslib/mscolab/auth.py). Good work.
• The "managers in-function-import" workaround is replaced with app.extensions['fm'|'cm'|'sockio']. Functionally fine, but app.extensions is documented for actual Flask extensions, not arbitrary services. A typed mini-registry (app.config['SERVICES'] = SimpleNamespace(...) or a small accessor module) would be more honest. Tolerable as a stop-gap.
• Commit hygiene is mixed: there are several "fix flake8", "tried to remove workarounds (does not work)", "blank line", and "URL fix" micro-commits. Squashing/rebasing before merge would help reviewers in the future.
• mslib/utils/auth.py still hosts send_email, which is purely an MSColab concern. Now that the rest of the MSColab-coupled helpers were moved to mslib/mscolab/auth.py, send_email belongs there too — leaving it in mslib.utils keeps the same architectural smell that motivated the move.
• Several auth_basic_auth = HTTPBasicAuth() instances are scattered across blueprints (mscolab auth, mscolab docs, mswms docs) plus the original auth = HTTPBasicAuth() in server.py/wms.py. These are independent objects; see the security section.
• Multiple from mslib.mscolab.app import _xstatic (function-local) imports inside DOCS_BP.files look like a leftover circular-import workaround. Now that app/init.py owns _xstatic, this can be a module-level import.
• tests/_test_mscolab/test_server_auth_required.py:33 imports initialize_managers from mslib.mscolab.server, but the module renamed it to _initialize_managers. The import is wrapped in try/except ImportError: pytest.skip(...), so the entire test file silently disappears from the suite. This is exactly the failure mode the previous review warned about with renames; please re-export or fix the import.

---

Security-related issues

1. Basic-auth callbacks are not wired to the blueprint HTTPBasicAuth instances. In mslib/mscolab/blueprints/auth/init.py:49, mslib/mscolab/blueprints/docs/init.py:43, and mslib/mswms/blueprints/docs/init.py:48, each blueprint creates its own auth_basic_auth = HTTPBasicAuth(). The corresponding @auth.verify_password is registered only on the unrelated auth = HTTPBasicAuth() in server.py/wms.py. When enable_basic_http_authentication=True:
   - In mscolab, optional_auth calls auth_basic_auth.login_required(...) on an instance with no callback → flask_httpauth defaults to denying every request, so /status, /token, /register, and the / page are unreachable.
   - Same regression in mswms/blueprints/docs/init.py for the main application() route, which previously used @conditional_decorator(auth.login_required, ...) on the wired-up auth in wms.py.
   This is a hard regression in the basic-HTTP-auth code path. Either share a single HTTPBasicAuth instance via current_app.extensions['basic_auth'] (set when verify_password is registered) or move the verify_password registration into the blueprint.
2. status_password.html template flow now produces a werkzeug.routing.BuildError. Commit a396cbc changed uri={"path": "auth.reset_request", ...} → uri={"path": url_for("auth.reset_request"), ...} (i.e. an absolute URL). But the template (mslib/mscolab/blueprints/auth/templates/auth/user/status_password.html:35) still does . url_for("/reset_request") is not a valid endpoint name and raises at render time. The previous review's recommendation was to verify the
   template; this fix went the wrong direction. Either revert to the endpoint name and keep the template's url_for, or change the template to render uri.path directly ().
3. fetch_profile_image returns any user's image without an authorization check. mslib/mscolab/blueprints/user/init.py:63 reads request.form['user_id'] and serves whatever fm.get_user_profile_image(user_id) returns. There is no check that the requester is allowed to view that user's profile (e.g., shared operation membership or owner). This is pre-existing on develop, but since the file is being re-written it's a good moment to fix. Also: this is a GET handler reading request.form — request.form is
   empty for a typical GET, so the route only "works" when callers send a body with Content-Type: application/x-www-form-urlencoded. Use request.args.get('user_id') and add an explicit auth check.
4. upload_profile_image trusts client-supplied mimetype and content_length. Same file, lines 51–54: file.mimetype is the multipart header from the client, so a malicious client can claim image/png and upload anything; file.content_length likewise is a request-header value and can be 0 or unset. Validate using Pillow.Image.open(...).verify() (or imghdr/puremagic) and check the actual byte count after read, not the declared length. Also pre-existing, but worth fixing while the file is touched.
5. Defense in depth: DIRECT_LOGIN/USE_SAML2 in /status. auth/init.py:64-83 returns the SAML config flags unconditionally, which is fine, but the duplicated json.dumps(...) branches all return identical bodies regardless of the basic-auth check. A short comment or refactor to a single dict and a single return statement would prevent future drift between the three copies.
6. gallery/init.py code() opens user-supplied filenames after safe_join. The safe_join(STATIC_LOCATION, "code", filename) is correct, but STATIC_LOCATION may be "" (when mswms_settings import fails — see gallery_builder.py:38). safe_join("", "code", filename) returns code/, which is then passed straight to open(), opening files relative to the CWD of whatever process serves the WMS. Add an os.path.isabs(STATIC_LOCATION) or STATIC_LOCATION != "" guard at the top of code() and return 404 /
   a clear error instead.

---

Performance issues

1. DNS / MX deliverability check on every registration. mslib/mscolab/auth.py:70 calls email_validator.validate_email(email, check_deliverability=current_app.config['MAIL_ENABLED']). With MAIL_ENABLED=True this performs a synchronous DNS/MX lookup on every /register POST, blocking the request thread. Previously the code only did syntax validation. If you want deliverability checks, do them in the background or only when actually sending the confirmation email, not as an inline gate.
2. SAML metadata regeneration per request. metadata(idp_identity_name) (auth/init.py:287-297) calls create_metadata_string(...) on every GET. The IDP metadata only changes when configuration changes — cache the per-IDP XML in module/process scope (or current_app.extensions).
3. add_image mutates module-global end and begin strings via repeated .replace(...). mslib/mswms/gallery_builder.py:712 and add_levels/add_times keep doing string-level rewrites on end/begin. For large galleries this is O(N²) on the document size. Pre-existing and largely orthogonal to this branch, but you've touched neighbouring lines (the default_plot_type change), so worth flagging.
4. Mail(APP) and _handle_db_upgrade() run unconditionally at module import time. server.py:136-139 runs create_app(...) (which now also imports/registers all blueprints), then immediately runs _handle_db_upgrade() and instantiates Mail. This makes any test that does from mslib.mscolab.server import APP pay for a database upgrade and a Mail-extension setup, which is the main reason the test suite is slow and fragile here. The blueprint refactor is a good moment to push these into a main()/start_server
   path.

---

Logic / algorithmic issues

1. EmailUndeliverableError is still NOT being caught. mslib/mscolab/auth.py:71:

except (email_validator.exceptions.EmailSyntaxError or email_validator.exceptions.EmailUndeliverableError):

1. or is the Python boolean operator; it short-circuits to the first truthy operand. Both classes are truthy, so this expression evaluates to EmailSyntaxError only — EmailUndeliverableError is dropped silently. The intended syntax is a tuple:

except (email_validator.exceptions.EmailSyntaxError,
email_validator.exceptions.EmailUndeliverableError):

1. Or simply except email_validator.exceptions.EmailNotValidError: (parent of both). The previous review's bug Integrate support for multiple simultaneous WMS layers into msui client #3 is therefore still open. Note: flake8/pyflakes will not flag this, but ruff B-014-style checks would.
2. status_password.html BuildError. Already covered under Security Integrate prefetch functionality into msui client to speed up map loading #2, but it is also a logic regression — in the password-reset failure path, the user now hits a 500 instead of seeing the "Resend authentication email" link.
3. Blueprint registration is still not idempotent. All blueprints are now registered inside create_app() (good), but there is no guard against create_app() being called twice. Re-registration raises ValueError: The name 'auth' is already registered for a different blueprint. The clean version is:

if 'auth' not in APP.blueprints:
APP.register_blueprint(AUTH_BP)
...

3. This matters because the previous-review note about double-registration came from tests calling create_app directly — that pattern is likely to come back.
4. DOCS_BLUEPRINTS_DIR / DOCS_TEMPLATES_DIR still point at non-existent paths. mslib/mscolab/app/init.py:46-51:

DOCS_SERVER_PATH = os.path.dirname(os.path.abspath(mslib.file)) # .../mslib
DOCS_BLUEPRINTS_DIR = os.path.join(DOCS_SERVER_PATH, 'blueprints') # .../mslib/blueprints ← doesn't exist
DOCS_STATIC_DIR = os.path.join(DOCS_BLUEPRINTS_DIR, 'static') # .../mslib/blueprints/static ← doesn't exist
DOCS_TEMPLATES_DIR = os.path.join(DOCS_STATIC_DIR, 'templates') # ← doesn't exist
APP = Flask(name, template_folder=os.path.join(DOCS_TEMPLATES_DIR))

4. mslib/blueprints does not exist. Same shape in mslib/mswms/app/init.py:40-44 (os.path.join(mslib.__file___dir, 'static') — mslib/static doesn't exist either). It "works" only because each blueprint sets template_folder='templates' itself. Flag this as dead code and either delete the constants or point them at the real per-blueprint directories. Leaving misleading globals in place is a footgun for the next person who adds a non-blueprint template.
5. Duplicate /static rule in mscolab. Commit f9f4a52 fixed the /static collision in mswms (DOCS_BP → /docs-static, GALLERY_BP → /gallery-static), but the same fix wasn't applied in mscolab/blueprints/docs/init.py:42, where DOCS_BP still uses static_url_path='/static'. Combined with Flask's automatic app.static rule (which mscolab inherits because Flask(name) registers it by default), there are two rules at /static/ with different endpoints — the registration order decides which
   wins. Apply the same /docs-static rename here.
6. mswms_settings / STATIC_LOCATION empty-string fallback. gallery_builder.py:38 sets STATIC_LOCATION = "" on mswms_settings import failure. app/init.py:88 then does Flask(name, static_url_path="/static", static_folder=STATIC_LOCATION) — i.e. static_folder="". Flask resolves "" to the application root; in practice this means an unconfigured WMS happily serves files relative to its own package directory. Replace with static_folder=None (no static endpoint when not configured) or STATIC_LOCATION
   = None.
7. init_saml is correctly fixed via record_once, but the surrounding code still has dead try/except paths. register_saml_routes() for idp_config in setup_saml2_backend.CONFIGURED_IDPS: try: ... except (NameError, AttributeError, KeyError) as ex: logging.warning("USE_SAML2 is %s, ...", current_app.config['USE_SAML2'], ex). We're inside blueprint registration — current_app is unbound here, so the warning itself raises a RuntimeError: Working outside of application context. Use
   state.app.config['USE_SAML2'] (you have state in the outer function) or just log without it.
8. hello() calls auth_basic_auth.login_required() without a function. auth/init.py:67: auth_basic_auth.login_required() — note the trailing (). flask_httpauth.HTTPBasicAuth.login_required is a decorator; called with no argument it returns a partial that expects a view function. The call's return value is discarded, so this line is a no-op. The intended behaviour was likely auth_basic_auth.login_required(...) decorating hello, but optional_auth already does that. Delete the line.
9. jinja_env.globals['imprint'] / ['gdpr'] defaults are mscolab-only. mslib/mscolab/app/init.py:64-65 adds module-level setdefault("imprint", "")/setdefault("gdpr", "") (good — addresses old review Add a button to reverse the flight path #11). mslib/mswms/app/init.py does not have the equivalent, so any template rendered before create_app runs in mswms still hits a NameError. Apply the same setdefault there.
10. gdpr / imprint redirect to empty body. mscolab/blueprints/docs/init.py:115-132 and mswms/blueprints/docs/init.py:163-180 both return "" when the configured file does not exist. An empty 200 leaks the fact that there is no imprint configured but is otherwise harmless. Prefer abort(404) for consistency.
11. fetch_profile_image uses request.form for a GET handler (also noted in Security). request.form is empty for a normal GET; request.args is the right accessor.
12. mss_plot_driver.py:489,832 — if hasattr(jump, "size") and jump.size > 0. jump = np.where(dlon_data > 2 * dlon)[0] always returns a 1-D ndarray, so .size is guaranteed. The hasattr is dead code — defensive but misleading. Either trust the type and use if jump.size > 0: (the previous review's preferred form), or, if you genuinely want list/tuple support, branch on len(jump) > 0 (works for both, no .size access).
13. gallery_builder.write_html default_plot_type. The fix to prefer "Top" (f9f4a52) is correct. Worth noting: the for l_type in plots: loop still iterates the module-global plots dict in insertion order, then filters by plot_types. If a caller passes plot_types=["Linear", "Top"], the visible tab will still be "Top" (good) but the rendered tab order will be Top, Side, Linear (the dict's order), not the caller's. Document or fix.
14. test_server_auth_required.py import drift. Already mentioned; the test file effectively no-ops.
15. register_user test data. tests/_test_mscolab/test_server.py:50 was changed to 'UV10@uv10.de' to satisfy the new validator's domain requirement, but tests/_test_mscolab/test_server_auth_required.py:45 still uses 'UV10@uv10', 'UV10', 'uv10', ..., and register_user("test@test.io", "test", "pwdtest", "UserPWD") returns a dict, while the test asserts r.status_code == 401. Both will fail now.
16. mscolab/server.py:140 CORS(APP, origins=APP.config['CORS_ORIGINS'] if hasattr(APP, "CORS_ORIGINS") else [""]). hasattr(APP, "CORS_ORIGINS") checks an attribute on the Flask instance, but CORS_ORIGINS is in APP.config, not as an attribute on APP. The condition is always false → CORS always opens with [""]. Pre-existing on develop, but a clear logic bug worth fixing while server.py is being shrunk.
17. server.py:155 if APP.dict.get('enable_basic_http_authentication', False): — same shape: this checks a dict on the Flask instance (which it isn't), so the basic-auth branch is dead. Should be APP.config.get('enable_basic_http_authentication', False). Pre-existing, but again you're inside this file already.
18. mscolab/auth.py:71 — is_valid_username is computed before email validation but evaluated after it. Minor: if both checks fail, the user gets the email error first. Fine, but the variable computation can move next to its use.

---

Summary verdict

The architecture moves are mostly the right direction — blueprints, decoupled mslib.utils.auth, app.extensions for managers — and several critical bugs from the previous review are genuinely fixed (SAML2 routing via record_once, top-level MSColab coupling). However, three of the previous review's items have been "fixed" in a way that is itself broken:

• the EmailUndeliverableError or-in-except (logic Remove old OWSlib from repository and replace by current version #1),
• the status_password.html url_for(uri.path) (security Integrate prefetch functionality into msui client to speed up map loading #2 / logic Integrate prefetch functionality into msui client to speed up map loading #2),
• the test rename leaving test_server_auth_required.py silently skipped (general / logic Add support for visualisation of gravity wave forecasts #14),

and a regression was introduced in basic-HTTP-auth wiring across blueprints (security #1). Those four should block merge. The rest are quality issues that can land in a follow-up.

[ReimarBauer]

Several auth_basic_auth = HTTPBasicAuth() instances are scattered across blueprints (mscolab auth, mscolab docs, mswms docs) plus the original auth = HTTPBasicAuth() in server.py/wms.py. These are independent objects; see the security section.

wms and mscolab do have to use two different HTTPBasicAuth. They should not depent on each other.

[ReimarBauer]

see comments