Supporting Client Secret rotation programatically + documentation update by pitchmuc · Pull Request #15 · adobe/aepp

pitchmuc · 2023-07-27T14:51:12Z

Client Rotating Option + ConnectInstance Documentation

Providing the option to rotate client secrets for Oauth V2 connection as described in the documentation : https://developer.adobe.com/developer-console/docs/guides/authentication/ServerToServerAuthentication/implementation/

At the same time, documenting the ConnectInstance class.

Motivation and Context

When clients wants to change their secret, they may want to do it programmatically and not via UI.
We now support that option via the ConnectInstance object.

How Has This Been Tested?

Tested on client implementation.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

I have signed the Adobe Open Source CLA.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

…ocumentation on ConnectInstance class

skanjila · 2023-07-27T23:34:39Z

aepp/configs.py

        self.__configObject__["sandbox"] = sandbox
        return self.getConfigObject()
+
+    def setOauthV2setup(self,credentialId:str=None,orgDevId:str=None)->bool:


can we use python style conventions in functions, namely with underscores for functions

I would have agreed but the whole package is using the camelCase notation.
I would think that we should be consistent over the different modules and keep camelCase usage.
It does not hurt in the end.

Also changing the method names is a massive breaking change.

skanjila · 2023-07-27T23:35:15Z

aepp/configs.py

+        if self.connectionType != "oauthV2":
+            raise Exception("You are trying to set credential ID or orgDevId for auth that is not OauthV2. We do not support these auth type.")
+        if credentialId is None:
+            raise ValueError("credentialId is None")


With these exceptions what are the expectations on how the user recovers, what do they need to do

The rotating client capability are not supported for JWT or Oauth V1.
There is no recovery, except moving to OauthV2 credential type.
How do you recommend to provide that feedback ?

skanjila · 2023-07-27T23:35:56Z

aepp/configs.py

+            raise ValueError("orgDevId is None")
+        self.credentialId = credentialId
+        self.orgDevId = orgDevId
+        return True


why are we returning boolean from a mutator function, typically a mutator would return void and an accessor function would return a datatype

True, for some reason, I thought it would be interesting to get when the function has correctly executed but definitely not normal setup, I will change it in the next commit.

skanjila · 2023-07-27T23:36:52Z

aepp/configs.py

+            orgDevId : OPTIONAL : the org Id but NOT the IMS string. It is defined on your project page.
+                Example : https://developer.adobe.com/console/projects/<orgId>/<projectId>/credentials/<credentialId>/details/oauthservertoserver
+        """
+        if self.connectionType != "oauthV2":


should we move these checks into Utils instead

I am not sure what part you are referring about.
the configs part ? We can, we just need to change the reference everywhere.
If it is in configs.py vs utils.py it does not matter.
Let me know.
I would believe that it would be a different PR to be honest.

skanjila · 2023-07-27T23:37:37Z

aepp/configs.py

+            'x-api-key' : self.client_id
+        }
+        endpoint = f"https://api.adobe.io/console/organizations/{orgDevId}/credentials/{credentialId}/secrets"
+        res = self.connector.getData(endpoint,headers=myheader)


do we need to do any validations on res?

skanjila · 2023-07-27T23:39:52Z

aepp/configs.py

@@ -12,7 +12,7 @@
 import os


please add unit tests to this PR based on our new convention with all features

skanjila · 2023-07-27T23:40:18Z

aepp/configs.py

+            cf.write(json.dumps(json_data, indent=4))
+        return True
+
+    def deleteSecrete(self,secretUID:str=None,credentialId:str=None,orgDevId:str=None,)->None:


lets rename to delete_secret

As explained above, the package current naming convention is using lower camelCase, so I think we should keep it consistent.
Otherwise, we would need to rename all existing methods which is a massive breaking change.

skanjila · 2023-07-27T23:41:22Z

aepp/configs.py

+        }
+        with open(destination, "w") as cf:
+            cf.write(json.dumps(json_data, indent=4))
+        return True


again lets not return bool from these functions, typically set and update either return void or a data structure that contains a status

skanjila · 2023-07-27T23:41:44Z

aepp/configs.py

+            orgDevId : OPTIONAL : the org Id but NOT the IMS string. It is defined on your project page.
+                Example : https://developer.adobe.com/console/projects/<orgId>/<projectId>/credentials/<credentialId>/details/oauthservertoserver
+        """
+        if self.connectionType != "oauthV2":


see my comments above on these if statements

skanjila · 2023-07-27T23:42:41Z

docs/connectObject.md

@@ -0,0 +1,258 @@
+# Connect Object class
+


can we move this to the Readme or code, not sure we need an individual markdown file, was there a reason to create separate documentation around this

As you can see the ConnectObject Instance is actually a module by itself providing way to connect to the AEP API without the need of other modules, it can also serve as a way to rotate your client secret programmatically.
As a specific class and method available, it has to follow the current structure and get its own markdown file.

The readMe would be pretty long with that.

liqunc · 2023-07-31T20:47:07Z

aepp/configs.py

+            'Authorization' : 'Bearer '+self.token,
+            'x-api-key' : self.client_id
+        }
+        endpoint = f"https://api.adobe.io/console/organizations/{orgDevId}/credentials/{credentialId}/secrets"


will this endpoint be the same for stage and prod?

Good question. I am only able to follow what is actually providing on the official documentation. https://developer.adobe.com/developer-console/docs/guides/authentication/ServerToServerAuthentication/implementation/

I do not think that you are using OauthV2, but Oauth V1.
I would suggest that we go live so provide clients with this functionality and if you ever need it and it is required to have a different url, we can use a variable to modify the endpoint automatically.
We can even use the environment parameter from the configuration file or configure method to define the correct endpoint if it turns out to be different from the stage and prod.

pitchmuc · 2023-08-21T14:03:02Z

I have added unit tests and modified the return statement for the void functions as asked.
Let me know if this is now good to go.

skanjila · 2023-08-22T14:43:41Z

tests/connectInstance_test.py

+from unittest.mock import patch, MagicMock, Mock, ANY
+
+@patch("aepp.configs.importConfigFile")
+def test_importConfigFile_Oauth(mock_data):


nit change 0auth to oauth

skanjila · 2023-08-22T14:44:44Z

tests/connectInstance_test.py

+
+@patch("aepp.configs.importConfigFile")
+def test_importConfigFile_Oauth(mock_data):
+    mock_data.return_value = {


I'm not sure we need this test, from what I see its just saying that mock_data is a dict, is there a case ever where it wouldnt be a dict?

You can just define mock_data as a fixture that can be leveraged in the methods below

pitchmuc added 3 commits June 14, 2023 09:30

adding token rotation capability for Oauth V2

b61c665

Supporting Rotating client secret methods for ConnectObject class + d…

13f0d76

…ocumentation on ConnectInstance class

Merge branch 'main' of https://github.com/adobe/aepp into enhanceOauth

7b43432

pitchmuc added documentation Improvements or additions to documentation enhancement New feature or request labels Jul 27, 2023

pitchmuc requested review from cmenguy and rezpe July 27, 2023 14:51

skanjila requested a review from Saikat1911 July 27, 2023 14:57

pitchmuc requested review from liqunc and skanjila and removed request for Saikat1911 July 27, 2023 15:18

skanjila reviewed Jul 27, 2023

View reviewed changes

liqunc reviewed Jul 31, 2023

View reviewed changes

pitchmuc added 2 commits August 16, 2023 14:05

removing boolean return for void function

2fb15cf

fixing return statement & adding unit tests

f045045

skanjila reviewed Aug 22, 2023

View reviewed changes

Conversation

pitchmuc commented Jul 27, 2023

Client Rotating Option + ConnectInstance Documentation

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitchmuc Aug 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitchmuc Aug 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitchmuc commented Aug 21, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pitchmuc Aug 3, 2023 •

edited

Loading

pitchmuc Aug 4, 2023 •

edited

Loading