Skip to content

[Fixes #14309] Generalize remote service type registration and auth handling - #14328

Merged
etj merged 12 commits into
masterfrom
ISSUE_14309
Jul 6, 2026
Merged

[Fixes #14309] Generalize remote service type registration and auth handling#14328
etj merged 12 commits into
masterfrom
ISSUE_14309

Conversation

@sijandh35

Copy link
Copy Markdown
Contributor

Fixes #14309

Checklist

Reviewing is a process done by project maintainers, mostly on a volunteer basis. We try to keep the overhead as small as possible and appreciate if you help us to do so by completing the following items. Feel free to ask in a comment if you have troubles with any of them.

For all pull requests:

  • Confirm you have read the contribution guidelines
  • You have sent a Contribution Licence Agreement (CLA) as necessary (not required for small changes, e.g., fixing typos in the documentation)
  • Make sure the first PR targets the master branch, eventual backports will be managed later. This can be ignored if the PR is fixing an issue that only happens in a specific branch, but not in newer ones.

The following are required only for core and extension modules (they are welcomed, but not required, for contrib modules):

  • There is a ticket in https://github.com/GeoNode/geonode/issues describing the issue/improvement/feature (a notable exemption is, changes not visible to end-users)
  • The issue connected to the PR must have Labels and Milestone assigned
  • PR for bug fixes and small new features are presented as a single commit
  • PR title must be in the form "[Fixes #<issue_number>] Title of the PR"
  • New unit tests have been added covering the changes, unless there is an explanation on why the tests are not necessary/implemented

Submitting the PR does not require you to check all items, but by the time it gets merged, they should be either satisfied or inapplicable.

@cla-bot cla-bot Bot added the cla-signed CLA Bot: community license agreement signed label Jun 12, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors service type handling by introducing a centralized ServiceTypeRegistry and enhances remote resource upload handlers (including COG, FlatGeobuf, 3D Tiles, and WMS) to support authentication configurations (AuthConfig) during validation and import. It also integrates authentication credentials with GDAL configuration options for remote metadata extraction. The review feedback highlights several improvement opportunities: implementing lazy initialization in ServiceTypeRegistry to avoid inefficient re-initialization and loss of dynamic registrations, and adding safety guards in BaseRemoteResourceHandler to prevent potential AttributeErrors when service_url, _exec, or options are None or empty.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread geonode/services/serviceprocessors/registry.py
Comment thread geonode/upload/handlers/common/remote.py
Comment thread geonode/upload/handlers/common/remote.py Outdated
Comment thread geonode/upload/handlers/common/remote.py
@codecov

codecov Bot commented Jun 12, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 78.01205% with 73 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.90%. Comparing base (be64508) to head (71b81af).
⚠️ Report is 9 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##           master   #14328       +/-   ##
===========================================
+ Coverage   34.71%   74.90%   +40.18%     
===========================================
  Files         982      986        +4     
  Lines       60496    60862      +366     
  Branches     8247     8318       +71     
===========================================
+ Hits        21002    45586    +24584     
+ Misses      38332    13418    -24914     
- Partials     1162     1858      +696     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sijandh35
sijandh35 marked this pull request as draft June 12, 2026 10:11
Comment thread geonode/upload/handlers/common/remote.py Outdated
Comment thread geonode/upload/handlers/common/remote.py Outdated
@sijandh35
sijandh35 marked this pull request as ready for review June 17, 2026 07:30
@sijandh35
sijandh35 requested a review from mattiagiupponi June 17, 2026 07:33
@sijandh35
sijandh35 marked this pull request as draft June 17, 2026 08:04
@sijandh35
sijandh35 marked this pull request as ready for review June 17, 2026 14:39
Comment thread geonode/upload/handlers/common/remote.py Outdated
Comment thread geonode/upload/handlers/common/remote.py Outdated
Comment thread geonode/upload/handlers/common/remote.py Outdated
@sijandh35
sijandh35 requested a review from giohappy June 18, 2026 14:19
Comment thread geonode/upload/handlers/common/remote.py Outdated
Comment thread geonode/upload/handlers/common/remote.py
@mattiagiupponi
mattiagiupponi requested a review from giohappy June 26, 2026 08:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR generalizes two previously hard-coded areas in GeoNode: (1) remote service type registration by introducing a dynamic ServiceTypeRegistry, and (2) remote authentication handling by moving AuthConfig extraction/reuse into the common remote resource handler and extending auth handlers to support GDAL configuration.

Changes:

  • Introduces a dynamic service type registry (ServiceTypeRegistry) and routes get_available_service_types() / handler resolution through it.
  • Centralizes remote upload authentication extraction/reuse in BaseRemoteResourceHandler (payload authenticationAuthConfig, rollback cleanup, reuse service-owned credentials).
  • Adds GDAL auth support via AuthHandler.get_gdal_config() and applies it to remote GDAL-based handlers (COG / FlatGeobuf).

Reviewed changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
geonode/upload/handlers/remote/wms.py Switches WMS auth lookup to the shared execution-based auth resolution.
geonode/upload/handlers/remote/tiles3d.py Adds request auth usage for URL validation and import-time fetches; propagates auth_config_id into resource payload.
geonode/upload/handlers/remote/tests/test_wms.py Updates tests for the new auth config creation API and the pre-processing-based auth reuse flow.
geonode/upload/handlers/remote/serializers/wms.py Relies on the base remote serializer’s authentication field (no longer duplicated in WMS serializer).
geonode/upload/handlers/remote/flatgeobuf.py Adds request auth for reachability checks and GDAL config option support via auth handlers.
geonode/upload/handlers/remote/cog.py Adds request auth for reachability checks and GDAL config option support via auth handlers.
geonode/upload/handlers/common/test_remote.py Adds coverage for common remote auth extraction + rollback deletion behavior.
geonode/upload/handlers/common/serializer.py Adds and validates authentication on the base remote upload serializer.
geonode/upload/handlers/common/remote.py Centralizes auth config extraction, rollback cleanup, service matching, and request auth helpers.
geonode/upload/datastore.py Passes execution_id into is_valid_url() to allow authenticated validation.
geonode/thumbs/tests/test_unit.py Updates tests to the new create_auth_config(payload) signature.
geonode/services/tests.py Updates tests to patch the registry-based handler lookup and adds registry-specific unit tests.
geonode/services/serviceprocessors/registry.py Adds the new dynamic service type registry implementation.
geonode/services/serviceprocessors/init.py Switches service type listing/handler lookup to the registry.
geonode/services/models.py Makes service type choices dynamic at runtime and avoids KeyError in ptype.
geonode/services/migrations/0060_alter_service_type.py Alters Service.type choices to be dynamically provided.
geonode/services/forms.py Uses dynamic service type choices from get_service_type_choices.
geonode/services/apps.py Initializes the service type registry during app startup.
geonode/security/tests.py Updates tests and adds coverage for GDAL config generation in the basic auth handler.
geonode/security/auth_handlers.py Adds get_gdal_config() and updates create_auth_config() to accept a payload dict.
Comments suppressed due to low confidence (1)

geonode/services/serviceprocessors/init.py:50

  • get_service_handler() now directly looks up a handler class in service_type_registry and instantiates it. For service_type values like AUTO/OWS (and any unknown type), service_type_registry.get_handler_class() returns None, which will raise a TypeError: 'NoneType' object is not callable. This breaks the function’s documented behavior (“guessed from”) and makes callers vulnerable to hard crashes instead of a controlled error or auto-detection.
def get_service_handler(base_url, service_type=enumerations.AUTO, service_id=None, *args, **kwargs):
    """Return the appropriate remote service handler for the input URL.
    If the service type is not explicitly passed in it will be guessed from
    """

    if entry := service_cache.get(base_url):
        return entry

    handler = service_type_registry.get_handler_class(service_type)
    try:
        service_handler = handler(base_url, service_id, *args, **kwargs)
        service_cache.set(service_handler.url, service_handler, settings.SERVICE_CACHE_EXPIRATION_TIME)
    except Exception as e:
        logger.exception(e)
        logger.exception(msg=f"Could not parse service {base_url}")
        raise
    return service_handler

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread geonode/upload/handlers/remote/cog.py Outdated
Comment thread geonode/security/auth_handlers.py Outdated
Comment thread geonode/services/serviceprocessors/registry.py Outdated
Comment thread geonode/services/models.py
Comment thread geonode/services/serviceprocessors/registry.py
Comment thread geonode/services/apps.py Outdated
Comment thread geonode/services/models.py Outdated
Comment thread geonode/services/tests.py
Comment thread geonode/upload/handlers/common/remote.py Outdated
@sijandh35
sijandh35 requested a review from etj July 3, 2026 11:07
@etj
etj merged commit a9da5c0 into master Jul 6, 2026
15 checks passed
@etj
etj deleted the ISSUE_14309 branch July 6, 2026 15:19
etj added a commit that referenced this pull request Jul 8, 2026
Resolves conflicts from PR #14328 (generalize remote service type
registration via ServiceTypeRegistry) against the #14381 work
(relax Service.base_url uniqueness).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed CLA Bot: community license agreement signed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Generalize remote service registration and auth handling

5 participants