[PLUGIN-1953] Support for client credentials Grant type#353
[PLUGIN-1953] Support for client credentials Grant type#353vikasrathee-cs wants to merge 1 commit intodevelopfrom
Conversation
itsankit-google
left a comment
There was a problem hiding this comment.
it is okay to remove null check because it is not required in certain cases.
But we should also add validation for the type when these configs are required.
As of now, there is no explicit variable defined which calculates whether user is supporting password grant or client_creds. Based on the values, if user name and password are null, it will fallback to client creds automatically. This was done to support DTS use case I guess. |
As discussed offline, this way we will not be able to maintain the extensibility of CDF plugin's ability to support multiple auth types. We can add a We can also hide certain fields in CDF Plugin UI based on |
| getConsumerSecret())) | ||
| .put(SalesforceConstants.CONFIG_LOGIN_URL, Objects.requireNonNull(config.getConnection().getLoginUrl())); | ||
| switch (config.getConnection().getAuthenticationGrantType()) { | ||
| case BASIC_AUTHENTICATION: |
There was a problem hiding this comment.
there is no basic auth in salesforce, previously password grant type was supported and now it is client creds also
|
|
||
| public AuthenticationGrantType getAuthenticationGrantType() { | ||
| if (!Strings.isNullOrEmpty(authenticationGrantType) && | ||
| authenticationGrantType.equals(AuthenticationGrantType.BASIC_AUTHENTICATION.getValue())) { |
There was a problem hiding this comment.
make it password grant type as default
There was a problem hiding this comment.
this is handeled by the else case
// Default auth, handles null case when upgrading pipeline
return SalesforceConstants.DEFAULT_GRANT_TYPE
widgets/Salesforce-batchsink.json
Outdated
| { | ||
| "name": "GrantTypeBasicAuthentication", | ||
| "condition": { | ||
| "expression": "authenticationGrantType == 'basicAuthentication'" |
There was a problem hiding this comment.
make it not clientCredentialsAuthentication, then it will be backward compatible.
vikasrathee-cs
left a comment
There was a problem hiding this comment.
change the auth type from based to password grant type
| * Licensed under the Apache License, Version 2.0 (the "License"); you may not | ||
| * use this file except in compliance with the License. You may obtain a copy of | ||
| * the License at | ||
| * |
There was a problem hiding this comment.
AuthenticatorCredentials class already has a GrantType available which is getting used in DTS also, it should be set based on this UI property.
That existing class can be used instead of this.
| .put(SalesforceConstants.CONFIG_CONSUMER_SECRET, | ||
| Objects.requireNonNull(config.getConnection().getConsumerSecret())) | ||
| .put(SalesforceConstants.CONFIG_LOGIN_URL, Objects.requireNonNull(config.getConnection().getLoginUrl())); | ||
| if (config.getConnection().getAuthenticationGrantType() == AuthenticatorCredentials.GrantType.PASSWORD) { |
There was a problem hiding this comment.
if grant type is password then only this configuration is mandatory
Support for client credentials Grant type
Jira : PLUGIN-1953
Summary
Adds support for the Client Credentials OAuth grant type to all Salesforce plugins (source, sink, streaming, connector).
Files updated
Widgets (UI)
Expose the OAuth Grant Type radio group and add a filter that hides
username/password/securityTokenwhenclient_credentialsis selected.widgets/Salesforce-connector.jsonwidgets/Salesforce-batchsink.jsonwidgets/Salesforce-batchsource.jsonwidgets/Salesforce-streamingsource.jsonwidgets/SalesforceMultiObjects-batchsource.jsonDocs
Document the new Grant Type field, clarify which fields are required per grant type, and note that
client_credentialsrequires an instance-specific login URL.docs/Salesforce-connector.mddocs/Salesforce-batchsink.mddocs/Salesforce-batchsource.mddocs/Salesforce-streamingsource.mddocs/SalesforceMultiObjects-batchsource.mdWhat's new
New config property
authenticationGrantTypebacked by the pre-existingAuthenticatorCredentials.GrantTypeenum (password/client_credentials). Null is treated aspasswordso upgraded pipelines that were saved before this change keep working without manual edits.Validation
New
validateAuthenticationFields(FailureCollector)onSalesforceConnectorBaseConfig:consumerKey,consumerSecret,loginUrl.password:username,password,securityToken.withConfigProperty(...), so the UI highlights the exact field.UI changes
password) added to all five widget JSONs.GrantTypePasswordfilter hidesusername/password/securityTokenwhenclient_credentialsis selected, so users only see the fields they need to fill in.Screenshots
Tests added
New file:
SalesforceConnectorBaseConfigTest.java(250 lines) coveringvalidateAuthenticationFields():consumerKey,consumerSecret,loginUrl,username,password,securityToken), including empty-string handling, plus an all-missing test that asserts exactly 6 failures on the right fields.consumerKey/consumerSecret/loginUrl, explicit assertion thatusername/password/securityTokenare not flagged, and an all-missing test that asserts exactly 3 failures.authenticationGrantTypefalls back toPASSWORDvalidation rules.Additional notes for reviewer
Backward compatibility: existing pipelines persisted without
authenticationGrantTypewill deserialize with a null value, whichgetAuthenticationGrantType()maps toPASSWORD.Login URL gotcha:
client_credentialsrequires an instance-specific URL (e.g.https://<instance>.my.salesforce.com/services/oauth2/token) rather than the genericlogin.salesforce.comendpoint. This is called out in the updated docs.