Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wgorman
Copy link

@wgorman wgorman commented Sep 1, 2017

Note, this change requires the upstream change egen/salesforce-wave-api#3

Copy link

@ghost ghost left a 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)
Copy link

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)
Copy link

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")
Copy link

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)) {
Copy link

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?

Copy link
Author

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?

Copy link

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

@phitoduck
Copy link

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
image

https://help.salesforce.com/s/articleView?id=sf.user_security_token.htm&type=5
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants