Skip to content

Do not call the callback if the selected auth method can't be retreived #28493

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.

Sign up for GitHub

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

Merged
PVince81 merged 1 commit into from
Jul 28, 2017
Merged

Do not call the callback if the selected auth method can't be retreived #28493

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions apps/files_external/js/settings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -692,7 +692,11 @@ MountConfigListView.prototype = _.extend({
var self = this;
this.$el.find('tbody tr:not(#addMountPoint)').each(function(i, tr) {
var authMechanism = $(tr).find('.selectAuthMechanism').val();
callback($(tr), authMechanism, self._allAuthMechanisms[authMechanism]['scheme']);
if (authMechanism !== undefined) {
var onCompletion = jQuery.Deferred();
Copy link
Contributor

@PVince81 PVince81 Jul 27, 2017

Choose a reason for hiding this comment

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

What's the point of this deferred/promise ? usually the purpose of a promise is to be returned and resolved later within a callback. Also it seems to be unused.

Copy link
Member Author

@jvillafanez jvillafanez Jul 27, 2017

Choose a reason for hiding this comment

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

gdrive uses the "onCompletition" callback, so unless we send something it will explode because the variable will be undefined. From my point of view, it was buggy from the beginning.
It's either use a dummy (the current solution) or try to remove these calls and force triggering an event at some point.

I've also tried to remove the callback. It seemed to work, so I don't know what was the reason to put that code in the first place.

I've already spent too much time just for this, so I've went for patching the bug.

Copy link
Contributor

@PVince81 PVince81 Jul 27, 2017

Choose a reason for hiding this comment

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

@Hemant-Mann I believe this might be related to your recent changes ?

Copy link
Contributor

@Hemant-Mann Hemant-Mann Jul 27, 2017

Choose a reason for hiding this comment

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

@jvillafanez Are you referring to this onCompletion callback?
https://github.com/owncloud/core/blob/master/apps/files_external/js/oauth2.js#L17

@PVince81 I don't think this Issue is related to my recent changes.
Hey @jvillafanez could you please describe the issue in a little more detail

Copy link
Contributor

@Hemant-Mann Hemant-Mann Jul 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@jvillafanez I have gone through the whole flow of selecting GDrive (when creating a PR) and was able to successfully authenticate it using OAuth2 so I am not sure what's the error

See this screenshot here

Copy link
Member Author

@jvillafanez jvillafanez Jul 27, 2017

Choose a reason for hiding this comment

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

Delay the https://github.com/owncloud/core/blob/master/apps/files_external/js/oauth2.js#L7 call 5 seconds with a setTimeout, something like:

setTimeout(function(){
  console.log('calling method');
  OCA.External.Settings.mountConfig.whenSelectAuthMechanism(function($tr, authMechanism, scheme, onCompletion) {
    .......
  });
}, 5000);

When you select a new mount gdrive mount, the button to grant access to the storage is missing.
This happens in the personal settings page, when several admin mount are presents for the user.

Note that this affects to other storages that are loaded before the files_external's settings js and are forced to use the "mountConfigLoaded" to register the auth mechanism. That delay intends to simulate that, as the call to that method will be done quite late.

As far as I know, the bug might have been there before @Hemant-Mann 's changes.

Copy link
Contributor

@PVince81 PVince81 Jul 28, 2017

Choose a reason for hiding this comment

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

ok so if @Hemant-Mann has no strong objection let's just merge this...

Copy link
Contributor

@Hemant-Mann Hemant-Mann Jul 28, 2017

Choose a reason for hiding this comment

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

No Objection from my side. This should be good to go

callback($(tr), authMechanism, self._allAuthMechanisms[authMechanism]['scheme'], onCompletion);
onCompletion.resolve();
}
});
this.on('selectAuthMechanism', callback);
},
Expand Down Expand Up @@ -1404,7 +1408,7 @@ OCA.External.Settings.UserStorageConfig = UserStorageConfig;
OCA.External.Settings.MountConfigListView = MountConfigListView;

/**
* @namespace OAuth2 namespace which is used to verify a storage adapter
* @namespace OAuth2 namespace which is used to verify a storage adapter
* using AuthMechanism as oauth2::oauth2
*/
OCA.External.Settings.OAuth2 = OCA.External.Settings.OAuth2 || {};
Expand All @@ -1413,7 +1417,7 @@ OCA.External.Settings.OAuth2 = OCA.External.Settings.OAuth2 || {};
* This function sends a request to the given backendUrl and gets the OAuth2 URL
* for any given backend storage, executes the callback if any, set the data-* parameters
* of the storage and REDIRECTS the client to Authentication page
*
*
* @param {String} backendUrl The backend URL to which request will be sent
* @param {Object} data Keys -> (backend_id, client_id, client_secret, redirect, tr)
*/
Expand Down Expand Up @@ -1454,7 +1458,7 @@ OCA.External.Settings.OAuth2.getAuthUrl = function (backendUrl, data) {
* This function verifies the OAuth2 code returned to the client after verification
* by sending request to the backend with the given CODE and if the code is verified
* it sets the data-* params to configured and disables the authorize buttons
*
*
* @param {String} backendUrl The backend URL to which request will be sent
* @param {Object} data Keys -> (backend_id, client_id, client_secret, redirect, tr, code)
* @return {Promise} jQuery Deferred Promise object
Expand Down Expand Up @@ -1487,7 +1491,7 @@ OCA.External.Settings.OAuth2.verifyCode = function (backendUrl, data) {
deferredObject.resolve(status);
});
} else {
deferredObject.reject(result.data.message);
deferredObject.reject(result.data.message);
}
}
);
Expand Down