OAuth2 flow changes by Hemant-Mann · Pull Request #28179 · owncloud/core

Hemant-Mann · 2017-06-21T11:43:11Z

Description

Instead of hard coding the request to the backend in oauth2.js i have triggered some new events on which respective event handlers can be attached in app specific js which will handle those events. Two new methods have been added to OCA.External.Settings namespace which will abstract the details of sending the request to backend and saving the mount configuration.

The developer of new apps don't need to write custom javascript for the same. All they will need to provide will the be the url to backend which will handle oauth2 steps like it is done in files_external/ajax/oauth2.php

Related Issue

#27049

Motivation and Context

This PR aims to solve the problem of authentication of storage apps using OAuth2. New apps will be need to attach event handlers on 2 events and just the call the related OCA.External.Settings.OAuth2.{function} by providing the backendUrl (based on lines of ajax/oauth2.php) which does the necessary OAuth2 tasks like generating Authorization URL and verifying access token

How Has This Been Tested?

This has been tested with the OwnCloud version 10.0.2 (git) with browser cache turned off.
Google Drive was mounted as an external storage successfully

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

CLAassistant · 2017-06-21T11:43:17Z

All committers have signed the CLA.

PVince81 · 2017-06-22T07:46:12Z

@@ -1,12 +1,14 @@
 $(document).ready(function() {
+	var storageId = 'googledrive';
+	var backendUrl = OC.filePath('files_external', 'ajax', 'oauth2.php');


OC.generateUrl('apps/files_external/ajax/oauth2.php') is better and is the new way since many OC versions

Ok didn't know about this, will use this though

PVince81 · 2017-06-22T07:46:55Z

@@ -1,12 +1,14 @@
 $(document).ready(function() {
+	var storageId = 'googledrive';


uh oh, please don't call it "storageId" because storage ID in OC is a complete different thing.

Please call it "backend" or "backendId" instead

Okay I'll change it to backendId
I was doubtful about storageId in the first place

PVince81 · 2017-06-22T07:47:21Z

+	/**
+	 * ------- OAUTH2 Events ----------
+	 * 
+	 * The files_external_{storageId} app's CUSTOM JS should handle the OAuth2 events itself


s/storageId/backendId/

PVince81 · 2017-06-22T13:25:38Z

+		}
+
+		OCA.External.Settings.OAuth2.getAuthUrl(backendUrl, data, function (authUrl) {
+			// (Optional) do some extra task - then control shifts back to getAuthUrl


if this is not needed for GDrive let's remove it ?

Yes the callback function is optional (I think I over engineered it)

But we need thisOCA.External.Settings.OAuth2.getAuthUrl because as you might have seen it sends request to backend to get the authUrl and it also redirect's the user to that URL (if status - success) else it display error message to the user

So I guess the name getAuthUrl does not fully describe what the function

Can you remove the empty callback and make it optional ? An empty function here doesn't make sense.

PVince81 · 2017-06-22T14:00:34Z

+ * @param  {Object}   data       Keys -> (client_id, client_secret, redirect, tr)
+ * @param  {Function} callback   Callback function to be called once authUrl is fetched
+ */
+OCA.External.Settings.OAuth2.getAuthUrl = function (backendUrl, data, callback) {


now it would also be nice to provide the promise pattern by returning a $.Deferred object and resolving it on success

Okay so if we are returning a Promise then it will be upto the caller of the function to redirect the user on success right?
because we can't do both - return promise and also redirect?

Ah, I understand now. Then let's forget about the Promise for now. The callback is fine.

One thing I'm wondering is whether we should check for the return value of the callback. If it's false (not undefined or null), then block the redirect. This way a callback could decide to do the a custom redirect.

I have done away with callback in Step 1: Auth URL Removed the callback argument from the function OCA.External.Settings.OAuth2.getAuthUrl

In Step 2 I have provided promise pattern using $.Deferred object

PVince81 · 2017-06-22T18:34:59Z

+ * @param  {String}   backendUrl The backend URL to which request will be sent
+ * @param  {Object}   data       Keys -> (backend_id, client_id, client_secret, redirect, tr)
+ */
+OCA.External.Settings.OAuth2.getAuthUrl = function (backendUrl, data) {


ok so do I understand it correctly that we don't need a promise here and just redirect directly ?

Yes we dont need promise here as far as i think, just redirecting the user on success will be fine and if there is error in backend it is displayed to the user using OC.dialogs.alert
I think this will make life easy because the AuthUrl is generated by backend nothing much can be done in frontend (client side)

PVince81 · 2017-06-23T08:35:57Z

+
+		OCA.External.Settings.OAuth2.verifyCode(backendUrl, data)
+		.done(function (storageConfig) {
+			console.log('Oauth 2 successful');


remove logging statement and function

PVince81 · 2017-06-23T08:36:32Z

Please remove that logging statement and the empty function that will result once removed.

Then this is good for merge from my POV 👍

Hemant-Mann · 2017-06-23T16:25:19Z

@PVince81 for some reason jenkins build failed

PVince81 · 2017-06-26T07:15:19Z

Please backport to stable10

PVince81 · 2017-06-26T07:15:36Z

submit pr with same commits to stable10

Hemant-Mann · 2017-06-26T09:50:50Z

PR #28210

lock · 2019-08-02T08:02:55Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

OAuth2 flow changes

409b5d7

Hemant-Mann mentioned this pull request Jun 21, 2017

Move files_external backends to separate apps #27049

Closed

16 tasks

PVince81 added this to the 10.1 milestone Jun 22, 2017

PVince81 reviewed Jun 22, 2017

View reviewed changes

Renamed storageId to backendId

539fda3

PVince81 reviewed Jun 22, 2017

View reviewed changes

Remove Callbacks, introduce promise pattern

33f8437

PVince81 reviewed Jun 22, 2017

View reviewed changes

PVince81 reviewed Jun 23, 2017

View reviewed changes

PVince81 added backport-request 3 - To Review labels Jun 23, 2017

Remove unused done callback

81c45c1

PVince81 merged commit 2897882 into owncloud:master Jun 26, 2017

Hemant-Mann mentioned this pull request Jun 26, 2017

New oauth2 JS #28210

Merged

9 tasks

This was referenced Jul 4, 2017

Oauth does not work owncloud/files_external_gdrive#1

Closed

Fix OAuth2 Event Trigger #28345

Merged

PVince81 removed the backport-request label Aug 15, 2017

DeepDiver1975 modified the milestones: 10.1, development Oct 10, 2017

lock Bot locked as resolved and limited conversation to collaborators Aug 2, 2019

		@@ -1,12 +1,14 @@
		$(document).ready(function() {
		var storageId = 'googledrive';

Uh oh!

Conversation

Hemant-Mann commented Jun 21, 2017

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

CLAassistant commented Jun 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hemant-Mann Jun 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PVince81 Jun 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hemant-Mann Jun 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PVince81 commented Jun 23, 2017

Uh oh!

Hemant-Mann commented Jun 23, 2017

Uh oh!

PVince81 commented Jun 26, 2017

Uh oh!

PVince81 commented Jun 26, 2017

Uh oh!

Hemant-Mann commented Jun 26, 2017

Uh oh!

lock Bot commented Aug 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CLAassistant commented Jun 21, 2017 •

edited

Loading

Hemant-Mann Jun 22, 2017 •

edited

Loading

PVince81 Jun 22, 2017 •

edited

Loading

Hemant-Mann Jun 23, 2017 •

edited

Loading