[Fixes #14177] Implement storage and handling of authentications for external resources#14192
[Fixes #14177] Implement storage and handling of authentications for external resources#14192
Conversation
|
Warning Gemini is experiencing higher than usual traffic and was unable to create the review. Please try again in a few hours by commenting |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #14192 +/- ##
==========================================
+ Coverage 64.87% 74.70% +9.83%
==========================================
Files 960 967 +7
Lines 58866 59201 +335
Branches 8070 8106 +36
==========================================
+ Hits 38188 44228 +6040
+ Misses 19054 13167 -5887
- Partials 1624 1806 +182 🚀 New features to boost your workflow:
|
| return custom_services_types | ||
|
|
||
|
|
||
| def get_basic_auth_credentials(auth_config): |
There was a problem hiding this comment.
@sijandh35 I would move these utils under the security module
| def clean(self): | ||
| cleaned_data = super().clean() | ||
|
|
||
| if cleaned_data.get("type") == "basic" and not self.instance.pk and not cleaned_data.get("password"): |
There was a problem hiding this comment.
Call the registry to obtain the handler for the type, and deledate payload validation to t handler classmethod, like AuthHandler.validate(cleaned_data)
| def save(self, commit=True): | ||
| instance = super().save(commit=False) | ||
|
|
||
| if instance.type == "basic": |
There was a problem hiding this comment.
Let's avoid conditions on specific types. This couples the Admin to the implemented handlers.
For safety and simplicity, let's encrypt/decrypt the entire payload.
| auth_handler_registry = AuthHandlerRegistry() | ||
|
|
||
|
|
||
| def build_auth_handler(config): |
There was a problem hiding this comment.
we don't need this util. The caller can just use auth_handler_registry.build(config) itself.
| return (auth_handler.username, auth_handler.password) | ||
|
|
||
|
|
||
| def create_basic_auth_config(username, password): |
There was a problem hiding this comment.
This can be a class method on the AuthBasicHandler
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def get_basic_auth_credentials(auth_config): |
There was a problem hiding this comment.
This can be a class methos on BasicAuthHandler.
However, see my comment inside services...
| service.id, | ||
| username=service.username if service.needs_authentication else None, | ||
| password=service.get_password() if service.needs_authentication else None, | ||
| username=username, |
There was a problem hiding this comment.
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 32555648 | Triggered | Authentication Tuple | eebd47c | geonode/security/tests.py | View secret |
| 32555648 | Triggered | Authentication Tuple | eebd47c | geonode/services/tests.py | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
| auth = self.kwargs.get("auth") | ||
| auth_config = self.kwargs.get("auth_config") | ||
| if auth is None and auth_config is not None: | ||
| auth = BasicAuthHandler(auth_config).get_request_auth() |
There was a problem hiding this comment.
This shouldn't be tied to the Basic auth handler. Any auth type could be used for a service. We must retrieve the correct auth handler from the registry, based on the config.
| auth_config = None | ||
| if username is not None or password is not None: | ||
| payload = {"username": username, "password": password} | ||
| BasicAuthHandler.validate(payload) |
There was a problem hiding this comment.
Same here @sijandh35 . We create the config from username/passowrd (we only suport basic auth when creating a new service for the moment), but the auth handler can be obtained from the config instead of hardcodign the basichandler
| """Gets authentication info for a dataset if it's a remote service requiring auth.""" | ||
| auth_info = {} | ||
| if dataset.remote_service and dataset.remote_service.needs_authentication: | ||
| from geonode.security.auth_registry import auth_handler_registry |
There was a problem hiding this comment.
For thumbnails, we can make similar changes to what has been done for the WMSService.
Instead of passing around the username/password, and so limit them to Basic Auth, we can send an auth dowin the chain.
At the end of the chain, you will see that owslib.OpenURL is used, which accepts an auth parameter, which we currently set to None. We can use it, instead of preparing the basic auth headers ourselves.
3 participants
Fixes #14177
Checklist
For all pull requests:
The following are required only for core and extension modules (they are welcomed, but not required, for contrib modules):
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.