Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat(integrations): add support for salesforce cdp #3363

Merged
merged 13 commits into from
Feb 11, 2025

Conversation

hassan254-prog
Copy link
Contributor

@hassan254-prog hassan254-prog commented Jan 24, 2025

Describe the problem and your solution

Add support for Saleforce Data Cloud

Testing instructions (skip if just adding/editing providers)

Here’s a more grammatically polished version:

This provider supports the TWO_STEP authentication mode but requires some changes to fully implement it. Specifically, a second request is needed after obtaining the initial access_token from Salesforce to generate the Salesforce Data Cloud (CDP) access_token. Since I couldn't test this locally with Data Cloud, I created a mock server to simulate responses from both Salesforce and Data Cloud, similar to the approach with Perimeter81. The process of generating the encoded JWT (assertion) is somewhat technical for users, so I also included a script to enable offline generation, as we did with SharePoint Online v1.

Copy link

linear bot commented Jan 24, 2025

@hassan254-prog hassan254-prog marked this pull request as ready for review January 24, 2025 11:38
@hassan254-prog hassan254-prog self-assigned this Jan 24, 2025
@hassan254-prog hassan254-prog requested a review from a team January 24, 2025 13:52
Copy link
Collaborator

@TBonnin TBonnin left a comment

Choose a reason for hiding this comment

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

Yet another auth flow :/
I wish that instead of TWO_STEP we introduced a MULTI_STEP mode that takes a list of request params because now we have a TWO_STEPS provider which has 3 steps :(
Is it too late to refactor TWO_STEP? @hassan254-prog

@hassan254-prog
Copy link
Contributor Author

hassan254-prog commented Jan 28, 2025

Yet another auth flow :/ I wish that instead of TWO_STEP we introduced a MULTI_STEP mode that takes a list of request params because now we have a TWO_STEPS provider which has 3 steps :( Is it too late to refactor TWO_STEP? @hassan254-prog

TWO_STEP has proven to be quite useful for some of the more complex authentication modes from the provider 😅 🙌 . No, it’s not too late to refactor, as the main logic revolves around how we save the access_token and retrieve the expireAt value from the response. So, we should be fine to proceed with the refactor. However, I will need to review and test this further on my end. Do we anticipate having a provider with more than three steps in the future? Or work with the max current steps we have at the moment, three.

@TBonnin
Copy link
Collaborator

TBonnin commented Jan 28, 2025

Do we anticipate having a provider with more than three steps in the future? Or work with the max current steps we have at the moment, three.

I stopped trying to anticipate how providers implement authentication. 😆 So many slightly different implementations it is hard to foresee anything.
I don't know how busy you are and how urgent it is to support this flow. I'll let you and @khaliqgant decide about the necessity of the refactor.

Copy link
Collaborator

@bodinsamuel bodinsamuel left a comment

Choose a reason for hiding this comment

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

Auth is definitely becoming harder. Looks good, some open questions

const parsedCreds = this.parseRawCredentials(responseData, 'TWO_STEP', provider) as TwoStepCredentials;
const stepResponses: any[] = [responseData];
if (provider.additional_steps) {
for (let stepIndex = 1; stepIndex <= provider.additional_steps.length; stepIndex++) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not simply start at 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The loop starts at 1 and accesses provider.additional_steps[stepIndex - 1] to align with the step numbering convention used in providers.yaml. Starting from 1 ensures consistency in configuration, making it more intuitive for human readability—e.g., referencing step1 instead of step0. This approach maintains a natural mapping between the loop index and the defined steps in providers.yaml.

subject_token_type: urn:ietf:params:oauth:token-type:access_token
token_headers:
content-type: application/x-www-form-urlencoded
token_url: ${step1.instance_url}/services/a360/token
Copy link
Collaborator

Choose a reason for hiding this comment

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

step1 is what in this context?

Copy link
Contributor Author

@hassan254-prog hassan254-prog Feb 7, 2025

Choose a reason for hiding this comment

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

step1 refers to the response from the initial request made before any additional steps, if provided. In subsequent steps, parameters are accessed dynamically using step{request_number}.

@khaliqgant khaliqgant merged commit 0ba17f3 into master Feb 11, 2025
16 checks passed
@khaliqgant khaliqgant deleted the kelvinwari/ext-218-add-support-for-salesforce-cdp branch February 11, 2025 09:13
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants