secret management #104

marjo-luc · 2024-08-28T18:37:55Z

Added maap-py support for user secrets management. Users can now use the following four endpoints to get, add, and delete secrets:

maap.secrets.get_secrets()
maap.secrets.add_secret("my_secret_name", "my_secret_value")
maap.secrets.get_secret("my_secret_name")
maap.secrets.delete_secret("my_secret_name")

Closes #102 & 1048

maap/Secrets.py

sujen1412 · 2024-08-28T21:30:44Z

maap/Secrets.py

+                headers=self._api_header
+            )
+
+            return json.loads(response.text)


I think this should just return a value and not a dict.
Returning a dict object can complicate users code using maag.py
For eg.

token = maap.Secrets().get_secret("name").get("name")

vs if we return just the value it could look like

token = maap.Secrets().get_secret("name")

Updated to support this.

sujen1412 · 2024-08-28T21:33:34Z

maap/Secrets.py

+
+    def add_secret(self, secret_name=None, secret_value=None):
+        """
+        Adds a secret. Secret name must be provided. Secret value may be null.


What is the use-case of having a secret without a value ?

Mostly because an empty string is a valid value and I don't see a good reason placing additional constraints on this. I was also thinking of cases where users might need to have the secret parameter exist but not necessarily care about the value itself.

Saw this later, commented below.

sujen1412 · 2024-08-28T21:35:43Z

maap/config_reader.py

@@ -81,6 +81,7 @@ def __init__(self, maap_host):
        self.algorithm_build = self._get_api_endpoint("algorithm_build")
        self.mas_algo = self._get_api_endpoint("mas_algo")
        self.dps_job = self._get_api_endpoint("dps_job")
+        self.member_secrets = self._get_api_endpoint("member_secrets")


Has this new config also been added to the maap_api_settings file as default config ? here https://github.com/MAAP-Project/maap-api-nasa/blob/main/api/settings.py#L119-L132

I went with the existing member endpoint.

sujen1412 · 2024-08-28T21:36:43Z

maap/Secrets.py

+from maap.utils import requests_utils
+
+
+class Secrets:


Could you add a section in the functional test to test adding, retrieving and deleting a secret?
https://github.com/MAAP-Project/maap-py/blob/master/test/functional_test.py

Yeah, this is next on my to-do.

Implemented.

bsatoriu

Looks good. Are we allowing empty secret values then? I'll need to update the API if so: https://github.com/MAAP-Project/maap-api-nasa/blob/develop/api/endpoints/members.py#L457-L459

marjo-luc · 2024-08-29T23:26:40Z

I think we should allow empty strings for secret values, but not None. I just pushed a commit to ensure the secret_value is not None.

sujen1412 · 2024-08-29T23:27:36Z

I think we should allow empty strings for secret values, but not None. I just pushed a commit to ensure the secret_value is not None.

Can you provide an example where this would be used ? I dont think we should allow empty strings either, unless we have a good reason.

Edited
Saw your response above. Again, I dont think the usage of a presence of a secret without a value should be promoted. And why is it a secret in that case ? Since names of secrets are committed to public code anyway.
It could might as well be a boolean input parameter.

marjo-luc · 2024-08-29T23:37:18Z

The good reason I see is that an empty string is a valid string so we shouldn't arbitrarily not allow that. But if that doesn't sway you, I'm fine updating the code to not allow empty strings as values.

sujen1412 · 2024-08-29T23:44:46Z

At present, since I do not see a convincing use for an empty string as a secret it does not sway me. Maybe in the future this need might arise.

I agree that an empty string is a valid string, but for secrets to me it implies an error on the user/systems part of not providing a valid (length > 0) value.

We do not want to promote our secrets database being used as a boolean/parameter flag store.

sujen1412 · 2024-08-30T01:48:11Z

maap/Secrets.py

+                headers=self._api_header
+            )
+            logger.debug(response.text)
+            return json.loads(response.text)


Should this be of type list or dict to match the doc string?

It's a list of dicts. I've updated the doc string to provide an example.

maap/Secrets.py

sujen1412 · 2024-08-30T01:52:19Z

maap/Secrets.py

+                response = response.json()
+                return response["secret_value"]
+
+            logger.debug(response.text)


Suggested change

logger.debug(response.text)

We should not be logging secrets anywhere.

This is logging the api response and not the secret value.

maap/Secrets.py

sujen1412 · 2024-08-30T01:54:11Z

maap/Secrets.py

+                url = self._members_endpoint,
+                headers=self._api_header
+            )
+            logger.debug(response.text)


Suggested change

logger.debug(response.text)

sujen1412 · 2024-08-30T01:55:20Z

maap/Secrets.py

+        Returns:
+            list: Secret names for a given user.
+        """
+        try:


Suggested change

try:

try:

logger.debug("Fetching all secrets")

Just more informative to log what function is being called than just logging response.text.

maap/Secrets.py

test/functional_test.py

* Remove the need to track maap.cfg file (#93) * Remove the need to track maap.cfg * Fix merge issues * Allow overriding of MAAP_API_HOST * Split config url construction into functions * Add test for config reader * Add script to perform functional test of maap-py * Construct api root url from provided maap host * Apply suggestions from code review Co-authored-by: Chuck Daniels <chuck@developmentseed.org> * Apply suggestions from code review to update url handling Co-authored-by: Chuck Daniels <chuck@developmentseed.org> * Remove unused code and memoize get config function * Update request utils to drop class and use methods * Use new request utils and pass maap config object * Update imports and url handling * Simplify boolean self-signed for request utils * Remove usage of os.path.join for constructing urls * Fix variable name in exception message * Remove incorrect import * Update functional tests --------- Co-authored-by: Chuck Daniels <chuck@developmentseed.org> * added pagination to listJobs endpoint * change request * Issues/95: Add CICD pipeline (#101) * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Fetch full repo in checkout for sonar analysis * tweak pr template wording * /version 4.0.1a0 * /version 4.1.0a1 * added more query params (#98) * added more query params * added get_job_details flag * cleanup * Update maap/maap.py Co-authored-by: Chuck Daniels <chuck@developmentseed.org> * Update maap/maap.py Co-authored-by: Chuck Daniels <chuck@developmentseed.org> * Update maap/maap.py Co-authored-by: Chuck Daniels <chuck@developmentseed.org> * added docstring | updated param handling * review updates * review updates * renamed file --------- Co-authored-by: Chuck Daniels <chuck@developmentseed.org> * /version 4.1.0a2 * secret management (#104) * wip * cleanup * updated get_secret endpoint to return value * added tests for secrets management * updated error handling | added logging * review comments * /version 4.1.0a3 * /version 4.2.0a0 * added changelog entry * /version 4.1.0rc1 --------- Co-authored-by: Sujen Shah <sujen1412@users.noreply.github.com> Co-authored-by: Chuck Daniels <chuck@developmentseed.org> Co-authored-by: Frank Greguska <89428916+frankinspace@users.noreply.github.com> Co-authored-by: frankinspace <frankinspace@users.noreply.github.com> Co-authored-by: marjo-luc <marjo-luc@users.noreply.github.com>

* Release/4.1.0 (#106) * Remove the need to track maap.cfg file (#93) * Remove the need to track maap.cfg * Fix merge issues * Allow overriding of MAAP_API_HOST * Split config url construction into functions * Add test for config reader * Add script to perform functional test of maap-py * Construct api root url from provided maap host * Apply suggestions from code review Co-authored-by: Chuck Daniels <chuck@developmentseed.org> * Apply suggestions from code review to update url handling Co-authored-by: Chuck Daniels <chuck@developmentseed.org> * Remove unused code and memoize get config function * Update request utils to drop class and use methods * Use new request utils and pass maap config object * Update imports and url handling * Simplify boolean self-signed for request utils * Remove usage of os.path.join for constructing urls * Fix variable name in exception message * Remove incorrect import * Update functional tests --------- Co-authored-by: Chuck Daniels <chuck@developmentseed.org> * added pagination to listJobs endpoint * change request * Issues/95: Add CICD pipeline (#101) * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Fetch full repo in checkout for sonar analysis * tweak pr template wording * /version 4.0.1a0 * /version 4.1.0a1 * added more query params (#98) * added more query params * added get_job_details flag * cleanup * Update maap/maap.py Co-authored-by: Chuck Daniels <chuck@developmentseed.org> * Update maap/maap.py Co-authored-by: Chuck Daniels <chuck@developmentseed.org> * Update maap/maap.py Co-authored-by: Chuck Daniels <chuck@developmentseed.org> * added docstring | updated param handling * review updates * review updates * renamed file --------- Co-authored-by: Chuck Daniels <chuck@developmentseed.org> * /version 4.1.0a2 * secret management (#104) * wip * cleanup * updated get_secret endpoint to return value * added tests for secrets management * updated error handling | added logging * review comments * /version 4.1.0a3 * /version 4.2.0a0 * added changelog entry * /version 4.1.0rc1 --------- Co-authored-by: Sujen Shah <sujen1412@users.noreply.github.com> Co-authored-by: Chuck Daniels <chuck@developmentseed.org> Co-authored-by: Frank Greguska <89428916+frankinspace@users.noreply.github.com> Co-authored-by: frankinspace <frankinspace@users.noreply.github.com> Co-authored-by: marjo-luc <marjo-luc@users.noreply.github.com> * /version 4.1.0 --------- Co-authored-by: Marjorie Lucas <47004511+marjo-luc@users.noreply.github.com> Co-authored-by: Sujen Shah <sujen1412@users.noreply.github.com> Co-authored-by: Chuck Daniels <chuck@developmentseed.org> Co-authored-by: Frank Greguska <89428916+frankinspace@users.noreply.github.com> Co-authored-by: frankinspace <frankinspace@users.noreply.github.com> Co-authored-by: marjo-luc <marjo-luc@users.noreply.github.com>

rebased develop onto master and tweaked versions rebased develop onto master and tweaked versions Resolve conflicts (#112) * Release/4.1.0 (#106) * Remove the need to track maap.cfg file (#93) * Remove the need to track maap.cfg * Fix merge issues * Allow overriding of MAAP_API_HOST * Split config url construction into functions * Add test for config reader * Add script to perform functional test of maap-py * Construct api root url from provided maap host * Apply suggestions from code review Co-authored-by: Chuck Daniels <chuck@developmentseed.org> * Apply suggestions from code review to update url handling Co-authored-by: Chuck Daniels <chuck@developmentseed.org> * Remove unused code and memoize get config function * Update request utils to drop class and use methods * Use new request utils and pass maap config object * Update imports and url handling * Simplify boolean self-signed for request utils * Remove usage of os.path.join for constructing urls * Fix variable name in exception message * Remove incorrect import * Update functional tests --------- Co-authored-by: Chuck Daniels <chuck@developmentseed.org> * added pagination to listJobs endpoint * change request * Issues/95: Add CICD pipeline (#101) * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Fetch full repo in checkout for sonar analysis * tweak pr template wording * /version 4.0.1a0 * /version 4.1.0a1 * added more query params (#98) * added more query params * added get_job_details flag * cleanup * Update maap/maap.py Co-authored-by: Chuck Daniels <chuck@developmentseed.org> * Update maap/maap.py Co-authored-by: Chuck Daniels <chuck@developmentseed.org> * Update maap/maap.py Co-authored-by: Chuck Daniels <chuck@developmentseed.org> * added docstring | updated param handling * review updates * review updates * renamed file --------- Co-authored-by: Chuck Daniels <chuck@developmentseed.org> * /version 4.1.0a2 * secret management (#104) * wip * cleanup * updated get_secret endpoint to return value * added tests for secrets management * updated error handling | added logging * review comments * /version 4.1.0a3 * /version 4.2.0a0 * added changelog entry * /version 4.1.0rc1 --------- Co-authored-by: Sujen Shah <sujen1412@users.noreply.github.com> Co-authored-by: Chuck Daniels <chuck@developmentseed.org> Co-authored-by: Frank Greguska <89428916+frankinspace@users.noreply.github.com> Co-authored-by: frankinspace <frankinspace@users.noreply.github.com> Co-authored-by: marjo-luc <marjo-luc@users.noreply.github.com> * /version 4.1.0 --------- Co-authored-by: Marjorie Lucas <47004511+marjo-luc@users.noreply.github.com> Co-authored-by: Sujen Shah <sujen1412@users.noreply.github.com> Co-authored-by: Chuck Daniels <chuck@developmentseed.org> Co-authored-by: Frank Greguska <89428916+frankinspace@users.noreply.github.com> Co-authored-by: frankinspace <frankinspace@users.noreply.github.com> Co-authored-by: marjo-luc <marjo-luc@users.noreply.github.com> Resolve conflicts (#111) * Release/4.1.0 (#106) * Remove the need to track maap.cfg file (#93) * Remove the need to track maap.cfg * Fix merge issues * Allow overriding of MAAP_API_HOST * Split config url construction into functions * Add test for config reader * Add script to perform functional test of maap-py * Construct api root url from provided maap host * Apply suggestions from code review Co-authored-by: Chuck Daniels <chuck@developmentseed.org> * Apply suggestions from code review to update url handling Co-authored-by: Chuck Daniels <chuck@developmentseed.org> * Remove unused code and memoize get config function * Update request utils to drop class and use methods * Use new request utils and pass maap config object * Update imports and url handling * Simplify boolean self-signed for request utils * Remove usage of os.path.join for constructing urls * Fix variable name in exception message * Remove incorrect import * Update functional tests --------- Co-authored-by: Chuck Daniels <chuck@developmentseed.org> * added pagination to listJobs endpoint * change request * Issues/95: Add CICD pipeline (#101) * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Fetch full repo in checkout for sonar analysis * tweak pr template wording * /version 4.0.1a0 * /version 4.1.0a1 * added more query params (#98) * added more query params * added get_job_details flag * cleanup * Update maap/maap.py Co-authored-by: Chuck Daniels <chuck@developmentseed.org> * Update maap/maap.py Co-authored-by: Chuck Daniels <chuck@developmentseed.org> * Update maap/maap.py Co-authored-by: Chuck Daniels <chuck@developmentseed.org> * added docstring | updated param handling * review updates * review updates * renamed file --------- Co-authored-by: Chuck Daniels <chuck@developmentseed.org> * /version 4.1.0a2 * secret management (#104) * wip * cleanup * updated get_secret endpoint to return value * added tests for secrets management * updated error handling | added logging * review comments * /version 4.1.0a3 * /version 4.2.0a0 * added changelog entry * /version 4.1.0rc1 --------- Co-authored-by: Sujen Shah <sujen1412@users.noreply.github.com> Co-authored-by: Chuck Daniels <chuck@developmentseed.org> Co-authored-by: Frank Greguska <89428916+frankinspace@users.noreply.github.com> Co-authored-by: frankinspace <frankinspace@users.noreply.github.com> Co-authored-by: marjo-luc <marjo-luc@users.noreply.github.com> * /version 4.1.0 --------- Co-authored-by: Marjorie Lucas <47004511+marjo-luc@users.noreply.github.com> Co-authored-by: Sujen Shah <sujen1412@users.noreply.github.com> Co-authored-by: Chuck Daniels <chuck@developmentseed.org> Co-authored-by: Frank Greguska <89428916+frankinspace@users.noreply.github.com> Co-authored-by: frankinspace <frankinspace@users.noreply.github.com> Co-authored-by: marjo-luc <marjo-luc@users.noreply.github.com>

wip

fd01642

marjo-luc self-assigned this Aug 28, 2024

sujen1412 reviewed Aug 28, 2024

View reviewed changes

marjo-luc added 3 commits August 28, 2024 16:39

cleanup

5e4cf56

updated get_secret endpoint to return value

dfc6cd7

added tests for secrets management

c797c89

marjo-luc requested a review from sujen1412 August 29, 2024 21:09

marjo-luc marked this pull request as ready for review August 29, 2024 21:09

marjo-luc requested a review from bsatoriu August 29, 2024 21:28

bsatoriu previously approved these changes Aug 29, 2024

View reviewed changes

updated error handling | added logging

b5d833d

marjo-luc dismissed bsatoriu’s stale review via b5d833d August 29, 2024 23:26

sujen1412 requested changes Aug 30, 2024

View reviewed changes

review comments

b001b4e

marjo-luc requested a review from sujen1412 September 4, 2024 21:28

sujen1412 approved these changes Sep 5, 2024

View reviewed changes

marjo-luc merged commit 46754f6 into develop Sep 9, 2024
2 of 3 checks passed

marjo-luc deleted the feature/secrets branch September 9, 2024 18:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

secret management #104

secret management #104

marjo-luc commented Aug 28, 2024 •

edited

Loading

sujen1412 Aug 28, 2024

marjo-luc Aug 29, 2024

sujen1412 Aug 28, 2024

marjo-luc Aug 28, 2024

marjo-luc Aug 29, 2024

sujen1412 Aug 29, 2024

sujen1412 Aug 28, 2024

marjo-luc Aug 28, 2024

sujen1412 Aug 28, 2024

marjo-luc Aug 28, 2024

marjo-luc Aug 29, 2024

bsatoriu left a comment •

edited

Loading

marjo-luc commented Aug 29, 2024

sujen1412 commented Aug 29, 2024 •

edited

Loading

marjo-luc commented Aug 29, 2024

sujen1412 commented Aug 29, 2024

sujen1412 Aug 30, 2024

marjo-luc Sep 4, 2024

sujen1412 Aug 30, 2024

marjo-luc Sep 4, 2024

sujen1412 Aug 30, 2024

sujen1412 Aug 30, 2024

secret management #104

secret management #104

Conversation

marjo-luc commented Aug 28, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsatoriu left a comment • edited Loading

Choose a reason for hiding this comment

marjo-luc commented Aug 29, 2024

sujen1412 commented Aug 29, 2024 • edited Loading

marjo-luc commented Aug 29, 2024

sujen1412 commented Aug 29, 2024

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

marjo-luc commented Aug 28, 2024 •

edited

Loading

bsatoriu left a comment •

edited

Loading

sujen1412 commented Aug 29, 2024 •

edited

Loading