OAuth2 support #919

nasli · 2017-07-11T07:25:11Z

Closes #716, closes #877

Description

Login will be done via OAuth2 protocol when server version supports it.

AC:

*Note: Tasks are not in priority order

How Has This Been Tested?

davivel · 2017-07-11T09:06:44Z

Owncloud iOs Client/DataBase/DTOs/CredentialsDto.h

 @property (nonatomic, copy) NSString *userName;
-@property (nonatomic, copy) NSString *password;
+@property (nonatomic, copy) NSString *password; //or accessToken in oauth


I would call it accessToken, seems more general; then explain in the comment that password goes there for basic auth.

davivel · 2017-07-11T09:07:30Z

Owncloud iOs Client/DataBase/DTOs/UserDto.h

@@ -33,6 +34,8 @@ typedef enum {
 @property (nonatomic, copy) NSString *url;
 @property (nonatomic, copy) NSString *username;
 @property (nonatomic, copy) NSString *password;


Since password will be contained in credDto, password property needs to disappear.

👍 pending, check also remove username

👍 Password already removed

davivel · 2017-07-11T10:28:28Z

Owncloud iOs Client/Login/Keychain/OCKeychain.h

@@ -19,6 +19,9 @@
 @interface OCKeychain : NSObject

 +(BOOL)setCredentialsById:(NSString *)idUser withUsername:(NSString *)userName andPassword:(NSString *)password;


This is the chance to rename the first parameter in all the other methods to userId instead of idUser.

👍 pending

davivel · 2017-07-11T10:30:49Z

Owncloud iOs Client/Login/Keychain/OCKeychain.m


    BOOL output = NO;

    NSMutableDictionary *keychainItem = [NSMutableDictionary dictionary];

-    [keychainItem setObject:(__bridge id)(kSecClassGenericPassword) forKey:(__bridge id)kSecClass];


I'm curious, why kSecClassInsternetPassword instead of kSecClassGenericPassword ?

Doesn't matter, just I want to use some key that are by default in the kSecClass kSecClassInsternetPassword as kSecAttrAuthenticationType. This was first approach but after I store all as data. This keychain method maybe not be the final, it is not in use yet. At this moment we can keep using kSecClassGenericPassword because I no longer use the other attributes

What it is recommended is for passcodes inside the app use GenericPassword and for pass used with connections in websites the kSecClassInsternetPassword that has more defaults attributes related connections

davivel · 2017-07-11T10:40:33Z

Owncloud iOs Client/Login/Login/UniversalLoginViewController.swift

+
+// MARK:  CheckAccessToServer delegate
+
+    func connection(toTheServer isConnection: Bool) {


Definitely, we need a better way to name delegate methods.

For this case, what do you think about something like connectionWasChecked

👍 but very careful with the refactor.. it it's used in a lot of places in the app

👍 Already improved

davivel · 2017-07-11T10:42:52Z

Owncloud iOs Client/Login/Login/UniversalLoginViewController.swift

+
+                    if (data != nil) {
+                        //getfiles, if ok store new account
+                    }


We should do something with the else case. At least, write an info log.

davivel · 2017-07-11T10:43:38Z

Owncloud iOs Client/Login/Login/UniversalLoginViewController.swift

+
+
+// MARK: segue
+    override func prepare(for segue: UIStoryboardSegue, sender: Any?) {


Is this the valid form or is the one in commented below?

This one, I will remove the commented one that is for previous swift 2.0 👍

DeepDiver1975 · 2017-07-11T14:55:10Z

Owncloud iOs Client/Branding/Customization.h

+#define k_oauth2_redirect_uri @"oc://android.owncloud.com"
+#define k_oauth2_client_id @"e4rAsNUSIUs0lF4nbv9FmCeUkTlV9GdgTLDH1b5uie7syb90SzEVrbN7HIpmWJeD"
+#define k_oauth2_client_secret @"dInFYGV33xKzhbRmpqQltYNdfLdJIfJ9L5ISoKhNoT9qZftpdWSP71VrpGR9pmoD"
+//#define k_oauth2_redirect_uri @"oc://ios.owncloud.com"


@nasli what will be the final values? I will adjust the oauth server app then thx

We can use this ones commented, I got them from core commented code

davivel · 2017-08-17T08:21:06Z

BUG [FIXED]

Problems when log-in is confirmed tapping on 'connect' button instead of on 'enter' in keyboard

Steps to reproduce

add account to server with basic auth support only, type URL, user and password
tap connect button to confirm password (do not tap 'enter' in soft keyboard

Expected behaviour

Keyboard is dismissed, account is created

Current behaviour

Keyboard is not dismissed, account is created twice

davivel · 2017-08-17T08:26:52Z

BUG [FIXED]

Crash on update of OAuth credentials

Steps to reproduce

add an account in a server supporting OAuth 2
tap on settings, then tap on button to edit the account added in 1

Expected behaviour

Log-in view in edit mode is shown

Current behaviour

Crash

davivel · 2017-08-17T09:03:04Z

BUG (1) [FIXED]

Log-in view for OAuth 2 appears from nowhere

Steps to reproduce

Add an OAuth2 account to the app.
Add a new account with Basic Auth to other server

Expected behaviour

The log-in view disappears and the list of accounts or the list of files is shown

Current behaviour

A new log-in view appears, showing the OAuth account as expired, and a cancel button disabled, blocking the usage of the app, til the point that requires reinstalling it .

davivel · 2017-08-17T09:05:58Z

BUG (2) [FIXED]

Log-in with HTTP prefix shows wrong error

Steps to reproduce

Add a new account; type an HTTP URL to a valid server that supports OAuth; type enter to check the URL

Expected behaviour:

Check is successful

Current behaviour:

"Redirect to an unsecure route" is shown after the check

davivel · 2017-08-17T09:10:13Z

BUG (3) [FIXED]

Connect button disabled after parsing URL that includes username and password

Steps to reproduce:

In the log-in view, type a URL including username and password inline, such as https://[user]:[pass]@[rest_of_the_url]
Tap on 'enter' in the keyboard

Expected behaviour:

After the URL is successfully checked, username and password input fields are shown, filled with the user and password from the URL; focus is on the password field; 'connect' button is enabled

Current behaviour:

After the URL is successfully checked, username and password input fields are shown, filled with the user and password from the URL; focus is on the username field; 'connect' button is disabled

davivel · 2017-08-17T09:14:50Z

BUG (4) [FIXED]

Wrong password not detected in edit mode

Steps to reproduce

Add an OAuth account (1)
Add a Basic Auth account (2)
Edit the account (2), change the password for a wrong one.
Tap on account (2) in list of accounts, wrong password is detected an log-in view shown in expired mode
Enter wrong password again

Expected behaviour

Wrong password is detected and account is not updated; user has to type in the correct password or cancel; the rest of the steps cannot be done

Current behaviour

Wrong password is accepted and stored.
App crashes.

NOTE @jesmrec (01/09/2017): Wrong passwords are detected but the app crashes if the "wrong" tries persists. Twice "Edit credentials" make the app crash

davivel · 2017-08-17T09:16:44Z

BUG (5) [FIXED]

Wrong padding between top bar and OC icon in log-in view

Steps to reproduce

Install the app
Open the app, log-in view appears

Expected behaviour

Space between top bar and OC icon is the same as in subsequent displays of log-in view

Current behaviour

Space between top bar and OC icon is smaller than in subsequent displays of log-in view

davivel · 2017-08-17T09:17:41Z

BUG (6) [NOT REPRODUCIBLE]

Disabled cancel button shown in navigation bar unexpectedly.

Steps to reproduce

Not clear.

Update hide bottom bar if needed

… the login view class. \n Not need to call addAccountDelegate, now is handle by the login view class

pablocarmu · 2017-11-07T09:48:49Z

Owncloud iOs Client/AppDelegate.m

+
+
+- (void)showLoginView:(UniversalLoginViewController *)loginView {
+//TODO: move to utils login, and window var


Here is a TODO pending i think

👍 Removed

pablocarmu · 2017-11-07T09:52:10Z

Owncloud iOs Client/AppDelegate.m

@@ -2836,17 +2769,87 @@ - (void) showSplashScreenFake {

 #pragma mark - CheckAccessToServerDelegate

-(void)connectionToTheServer:(BOOL)isConnection {
+-(void)connectionToTheServerWasChecked:(BOOL)isConnected withHttpStatusCode:(NSInteger)statusCode andError:(NSError *)error {


same code in 3 different functions(connectionToTheServerWasChecked, repeatTheCheckToTheServer, badCertificateNotAcceptedByUser), maybe we can extract this piece of code into a single one function and then call it inside every one of this functions.

👍 CheckAccessToServer still need improvements..

pablocarmu · 2017-11-07T10:12:09Z

Owncloud iOs Client/AppDelegate.m

+
+#pragma mark - Active User
+
+- (void) switchActiveUserTo:(UserDto *)user inHardMode:(BOOL)hardMode withCompletionHandler:(void (^)(void)) completionHandler {


This function is called only one time and with "inHardMode" set to NO, so why is this parameter useful?

Checking it, it's called also in login view with hardMode true, https://github.com/owncloud/ios/blob/oauth_support/Owncloud%20iOs%20Client/Login/Login/UniversalLoginViewController.swift#L896

pablocarmu · 2017-11-07T10:30:36Z

Owncloud iOs Client/ExternalLibreries/KKPasscodeLock/KKPasscodeSettingsViewController.m

@@ -122,7 +122,7 @@ - (void)actionSheet:(UIActionSheet*)actionSheet clickedButtonAtIndex:(NSInteger)
 - (void)eraseDataSwitchChanged:(id)sender
 {
 	if (_eraseDataSwitch.on) {
-		NSString* title = [NSString stringWithFormat:KKPasscodeLockLocalizedString(@"All data in this app will be erased after %d failed passcode attempts.", @""), [[KKPasscodeLock sharedLock] attemptsAllowed]];
+//		NSString* title = [NSString stringWithFormat:KKPasscodeLockLocalizedString(@"All data in this app will be erased after %d failed passcode attempts.", @""), [[KKPasscodeLock sharedLock] attemptsAllowed]];


Commented code

Let's leave this one, it's a feature in this library not in use right now

pablocarmu · 2017-11-07T10:32:23Z

Owncloud iOs Client/Log.swift

+import Foundation
+
+
+//class Log {


full commented class

Intent to log of swift classes, let's review in release state.

pablocarmu · 2017-11-07T11:42:43Z

Owncloud iOs Client/Network/ReadFolder/DetectListOfFiles.swift

+    self.readFolderOfURL(url, credentials: credentials, success: { (_ listOfFiles: [Any]?) in
+            var listOfFileDtos: [FileDto]? = nil
+
+            if (listOfFiles != nil && !((listOfFiles?.isEmpty)!)) {


this set of "(" and "!" , ((listOfFiles?.isEmpty)!), could be avoided if previously you ensures that the "listOfFiles" exist and is not nil, maybe with an "if let ..." statement.

👍 Improvement done to check files content

pablocarmu · 2017-11-07T11:44:59Z

Owncloud iOs Client/Tabs/FileTab/SimpleFileList/SimpleFileListTableViewController.m

@@ -554,7 +555,7 @@ - (void)showError:(NSString *) message {
        [alert addAction:ok];

        if ([self.navigationController isViewLoaded] && self.navigationController.view.window && self.resolveCredentialErrorViewController != nil) {
-            [self.resolveCredentialErrorViewController presentViewController:alert animated:YES completion:nil];
+          //TODO:check bridging  [self.resolveCredentialErrorViewController presentViewController:alert animated:YES completion:nil];


Yet Another TODO statement

pablocarmu · 2017-11-07T11:45:47Z

Owncloud iOs Client/Tabs/SettingTab/Account/Cell/AccountCell.h

@@ -31,7 +31,7 @@
 @property(nonatomic, weak) __weak id <AccountCellDelegate> delegate;


- (IBAction)activeAccount:(id)sender;
+//- (IBAction)activeAccount:(id)sender;


Commented statement

pablocarmu · 2017-11-07T11:48:40Z

Owncloud iOs Client/Tabs/SettingTab/Account/Cell/AccountCell.m

@@ -24,6 +24,7 @@

 @implementation AccountCell

+//TODO: use autolayout for cell 


Another TODO

pablocarmu · 2017-11-07T11:48:57Z

Owncloud iOs Client/Tabs/SettingTab/Account/Cell/AccountCell.m

@@ -40,16 +41,13 @@ - (void)setSelected:(BOOL)selected animated:(BOOL)animated
    // Configure the view for the selected state
 }

+/*


Commented Code

👍 Removed, and also removed remaining references in xib file, account cell classes an setting class

…d remove references not longer needed

nasli · 2017-12-20T08:37:13Z

Related bug (36) some downloads are not downloaded after an expiration, this already happens if you lost connection or session expires, but if you select the folder as available offline all pending downloads will be retry.

nasli · 2017-12-20T08:39:46Z

Related bug (34) a queue of uploads does not retry. Within the release branch new fix has been made to relaunch all pending uploads with credentials error when the user go to the uploads view. PR: #983

nasli · 2017-12-20T08:40:35Z

Related (33) and (40) improvements will be delayed

nasli requested a review from davivel July 11, 2017 07:25

jesmrec mentioned this pull request Jul 11, 2017

Authentication based in token (OAuth2) #716

Closed

9 tasks

nasli mentioned this pull request Jul 11, 2017

OAuth2 support owncloud/ios-library#159

Merged

nasli added this to the 3.7.0 milestone Jul 11, 2017

davivel reviewed Jul 11, 2017

View reviewed changes

nasli force-pushed the oauth_support branch from 30a5155 to 75e2d17 Compare July 11, 2017 13:51

DeepDiver1975 reviewed Jul 11, 2017

View reviewed changes

nasli force-pushed the oauth_support branch from dc73d59 to 27414ed Compare July 14, 2017 12:25

jesmrec changed the title ~~[WIP] Oauth2 support~~ [WIP] OAuth2 support Jul 18, 2017

jesmrec mentioned this pull request Jul 19, 2017

OAuth2 in mobile clients owncloud/QA#469

Closed

nasli force-pushed the oauth_support branch from 235fbcd to 4daf95e Compare August 3, 2017 11:25

davivel mentioned this pull request Aug 17, 2017

Silently refresh OAuth2 access token if expired #928

Closed

davivel added the login label Aug 17, 2017

davivel assigned pablocarmu, nasli and davivel Aug 17, 2017

nasli added 2 commits November 6, 2017 11:13

Remove old OAuth approach not in use

0290827

Add notification to relaunch credentials error

1359037

Update hide bottom bar if needed

nasli force-pushed the oauth_support branch from edf6d8b to 1359037 Compare November 6, 2017 12:56

nasli added 7 commits November 6, 2017 14:22

Reuse existing code

a143731

Not need to set cancel button on this point, it is going to be set in…

b928fb2

… the login view class. \n Not need to call addAccountDelegate, now is handle by the login view class

Not need to use notification of rotations in the new login view

9070c78

Remove TODOs already done

ae3bbed

Already set up preferredStatusBarStyle

6f625a1

Move update instant upload settings into store account method

ac04378

Remove todo already done

af837db

pablocarmu suggested changes Nov 7, 2017

View reviewed changes

Remove references to active account button in xib to avoid crashes an…

dd7d92f

…d remove references not longer needed

nasli force-pushed the oauth_support branch from ecbf6d9 to dd7d92f Compare November 7, 2017 16:19

nasli added 7 commits November 7, 2017 17:27

Remove not needed code in account cell

511dec4

Remove unnecessary code

4b324a1

Use just one method for now that they do the same

664d9eb

Remove pending todos

bde3176

Improve checks lis of files content

bbb3bcc

Rename user id name. Replace some deprecated method

118070e

Clean duplicated comments in the implementation file

63156dd

pablocarmu approved these changes Nov 8, 2017

View reviewed changes

nasli merged commit fd8e411 into master Nov 8, 2017

jesmrec mentioned this pull request Nov 8, 2017

[3.7.0] Release new version #923

Closed

33 tasks

nasli mentioned this pull request Nov 8, 2017

[UX] Make login page more intuitive #877

Closed

nasli deleted the oauth_support branch December 12, 2017 08:55

jesmrec mentioned this pull request Dec 14, 2017

Fix duplicate accounts #976

Merged

nasli mentioned this pull request Dec 19, 2017

Fix relaunch uploads after oauth exp #983

Merged

		@@ -19,6 +19,9 @@
		@interface OCKeychain : NSObject

		+(BOOL)setCredentialsById:(NSString )idUser withUsername:(NSString )userName andPassword:(NSString *)password;


		// MARK: CheckAccessToServer delegate

		func connection(toTheServer isConnection: Bool) {



		// MARK: segue
		override func prepare(for segue: UIStoryboardSegue, sender: Any?) {



		- (void)showLoginView:(UniversalLoginViewController *)loginView {
		//TODO: move to utils login, and window var


		#pragma mark - Active User

		- (void) switchActiveUserTo:(UserDto *)user inHardMode:(BOOL)hardMode withCompletionHandler:(void (^)(void)) completionHandler {

		@@ -24,6 +24,7 @@

		@implementation AccountCell

		//TODO: use autolayout for cell

		import Foundation


		//class Log {

OAuth2 support #919

OAuth2 support #919

Conversation

nasli commented Jul 11, 2017 • edited Loading

Description

How Has This Been Tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nasli Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nasli Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nasli Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davivel commented Aug 17, 2017 • edited by jesmrec Loading

BUG [FIXED]

Steps to reproduce

Expected behaviour

Current behaviour

davivel commented Aug 17, 2017 • edited by jesmrec Loading

BUG [FIXED]

Steps to reproduce

Expected behaviour

Current behaviour

davivel commented Aug 17, 2017 • edited by jesmrec Loading

BUG (1) [FIXED]

Steps to reproduce

Expected behaviour

Current behaviour

davivel commented Aug 17, 2017 • edited by jesmrec Loading

BUG (2) [FIXED]

Steps to reproduce

Expected behaviour:

Current behaviour:

davivel commented Aug 17, 2017 • edited by jesmrec Loading

BUG (3) [FIXED]

Steps to reproduce:

Expected behaviour:

Current behaviour:

davivel commented Aug 17, 2017 • edited by jesmrec Loading

BUG (4) [FIXED]

Steps to reproduce

Expected behaviour

Current behaviour

davivel commented Aug 17, 2017 • edited by jesmrec Loading

BUG (5) [FIXED]

Steps to reproduce

Expected behaviour

Current behaviour

davivel commented Aug 17, 2017 • edited by jesmrec Loading

BUG (6) [NOT REPRODUCIBLE]

Steps to reproduce

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nasli commented Jul 11, 2017 •

edited

Loading

nasli Jul 11, 2017 •

edited

Loading

nasli Jul 11, 2017 •

edited

Loading

nasli Jul 11, 2017 •

edited

Loading

davivel commented Aug 17, 2017 •

edited by jesmrec

Loading

davivel commented Aug 17, 2017 •

edited by jesmrec

Loading

davivel commented Aug 17, 2017 •

edited by jesmrec

Loading

davivel commented Aug 17, 2017 •

edited by jesmrec

Loading

davivel commented Aug 17, 2017 •

edited by jesmrec

Loading

davivel commented Aug 17, 2017 •

edited by jesmrec

Loading

davivel commented Aug 17, 2017 •

edited by jesmrec

Loading

davivel commented Aug 17, 2017 •

edited by jesmrec

Loading

nasli Nov 8, 2017 •

edited

Loading

nasli Nov 7, 2017 •

edited

Loading

nasli commented Dec 20, 2017 •

edited

Loading