-
Notifications
You must be signed in to change notification settings - Fork 68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
allow authentication via access token in addition to username/password #16
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your pull request. Could you please add few unit tests for your change?
Please make sure the existing tests pass with your changes as well.
Please create PR for https://github.com/springml/salesforce-wave-api with your changes
@@ -40,7 +41,7 @@ class DataWriter ( | |||
def writeMetadata(metaDataJson: String, | |||
mode: SaveMode, | |||
upsert: Boolean): Option[String] = { | |||
val partnerConnection = createConnection(userName, password, login, version) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to support existing login as well. i.e with username and password
if (username.isDefined) { | ||
waveAPI = APIFactory.getInstance.waveAPI(username.get, password, serverUrl, version) | ||
} else { | ||
waveAPI = APIFactory.getInstance.waveAPIwAuthToken(authToken.get, serverUrl, version) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need PR for salesforce-wave-api as well
password = param(parameters, "SF_PASSWORD", "password") | ||
serverUrl = parameters.getOrElse("login", "https://login.salesforce.com") | ||
} else { | ||
serverUrl = parameters.getOrElse("instanceUrl", "https://login.salesforce.com") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we use the existing property for instanceUrl?
@@ -202,7 +243,7 @@ class DefaultSource extends RelationProvider with SchemaRelationProvider with Cr | |||
|
|||
if (monitorJob) { | |||
logger.info("Monitoring Job status in Salesforce wave") | |||
if (Utils.monitorJob(writtenId.get, username, password, login, version)) { | |||
if (Utils.monitorJob(writtenId.get, username, password, authToken, serverUrl, version)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is the authToken expiry should be handled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great question, would you like automatic logic built into the spark layer or would you expect that to take place outside of spark?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wgorman I would like to handle in spark layer. There might be a chance that the access token got expired while fetching millions of rows
Is there any hope of this PR getting merged? If you have MFA enabled for API access to salesforce, you can't get a security key. This means that the only way to access Salesforce APIs for accounts with MFA is via OAuth. Submitting a username, password, and security token simply isn't an option in that case :( Salesforce has said that they are not responsible for security breaches for any customers that don't have MFA enabled on their accounts. (makes sense). This means that more and more SF accounts are enabling MFA. I think that means that this library is useless for those accounts in it's current state. (unless I'm wrong, which I'd love to be) References: https://help.salesforce.com/s/articleView?id=sf.security_require_2fa_api.htm&type=5 https://help.salesforce.com/s/articleView?id=sf.user_security_token.htm&type=5 |
Note, this change requires the upstream change egen/salesforce-wave-api#3