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

add support for dev app #63

Merged
merged 5 commits into from
Jun 28, 2019
Merged

add support for dev app #63

merged 5 commits into from
Jun 28, 2019

Conversation

tobiasKaminsky
Copy link
Member

Signed-off-by: tobiasKaminsky tobias@kaminsky.me

@tobiasKaminsky
Copy link
Member Author

SSO has hard dependendcies to Nextcloud File app:

public static int getNextcloudFilesVersionCode(Activity activity) throws PackageManager.NameNotFoundException {
PackageInfo pInfo = activity.getPackageManager().getPackageInfo("com.nextcloud.client", 0);
int verCode = pInfo.versionCode;
Log.e("VersionCheckHelper", "Version Code: " + verCode);
return verCode;
}

Intent intentService = new Intent();
intentService.setComponent(new ComponentName("com.nextcloud.client",
"com.owncloud.android.services.AccountManagerService"));
if (!mContext.bindService(intentService, mConnection, Context.BIND_AUTO_CREATE)) {
Log.d(TAG, "Binding to AccountManagerService returned false");
throw new IllegalStateException("Binding to AccountManagerService returned false");

private static Intent buildRequestAuthTokenIntent(Context context, Intent intent) throws NextcloudFilesAppAccountPermissionNotGrantedException {
String accountName = intent.getStringExtra(AccountManager.KEY_ACCOUNT_NAME);
Account account = AccountImporter.getAccountForName(context, accountName);
if(account == null) {
throw new NextcloudFilesAppAccountPermissionNotGrantedException();
}
Intent authIntent = new Intent();
authIntent.setComponent(new ComponentName("com.nextcloud.client",
"com.owncloud.android.ui.activity.SsoGrantPermissionActivity"));
authIntent.putExtra(NEXTCLOUD_FILES_ACCOUNT, account);
return authIntent;
}

Question is how / if we can also support dev version there.

@David-Development
Copy link
Member

For reference from our Telegram chat:

Es gibt wirklich noch einiges zu beachten da der Service entsprechend der jeweiligen App gebunden werden muss. Bin mir noch nicht über Seiteneffekte bei Verwendung von multi Accounts im klaren. Erste Idee ist, dass wir uns für jeden Account speichern müssen von welchem package er ist und dementsprechend den Service binden müssen. Tobi hatte ja bereits die entsprechenden Stellen im Code angegeben.

Der Vollständigkeit wegen: hier der Bug-Report der schon seit einigen Monaten offen ist: #46

@tobiasKaminsky Just thought about #49 .. this will make it quite hard to support both apps (normal and dev) as we need to connect to both apps and get an account list.

@tobiasKaminsky
Copy link
Member Author

need to connect to both apps and get an account list.

as this is a "one time" action, only when adding an account, I think this is ok.

Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
@tobiasKaminsky
Copy link
Member Author

This should be ready to test

@tobiasKaminsky
Copy link
Member Author

@stefan-niedermann @desperateCoder can you both include this in your app and test?

@David-Development
Copy link
Member

as this is a "one time" action, only when adding an account, I think this is ok.

@tobiasKaminsky so for all the requests we'll just use the nextcloud files app? (and not the dev app?)

Does the normal app has access to the account data of the dev app? The actual account data is required for the actual network call to the nextcloud, isn't it?

@tobiasKaminsky
Copy link
Member Author

as this is a "one time" action, only when adding an account, I think this is ok.

@tobiasKaminsky so for all the requests we'll just use the nextcloud files app? (and not the dev app?)

Does the normal app has access to the account data of the dev app? The actual account data is required for the actual network call to the nextcloud, isn't it?

No, I was referring to "fetch accounts from both apps". This is a "one time" action while adding accounts, which does not happen that offen.

As far as I tested, this PR allows to use both apps.
Please test it again so that it is really working as desired. I tested like this:

  • official app: account on server A
  • dev app: account on server B
  • 3rd party app sees both accounts
  • 3rd party app can switch between both accounts on the fly and do stuff
    (I tested it with deck)

Copy link
Member

@stefan-niedermann stefan-niedermann left a comment

Choose a reason for hiding this comment

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

lgtm - looking forward to use it in deck :)

@David-Development
Copy link
Member

David-Development commented Jun 16, 2019

@tobiasKaminsky I was a little confused at first how the network requests will be made but I just saw that you added some logic to whether to connect to the nextcloud service or the nextcloud dev service. This line made things much clearer to me:

String componentName = Constants.PACKAGE_NAME_PROD;

After running some tests, I couldn't select an account from the dev app as the line final Account[] accounts = accMgr.getAccountsByType("nextcloud"); in the AccountImporter was only returning accounts of the type "nextcloud". After changing it to final Account[] accounts = accMgr.getAccounts(); the login worked. How did you manage to get in running without that line?

This PR looks good to me! Great work! :) I created a PR with a few minor fixes in #65.

And sorry for the delay!..

@tobiasKaminsky
Copy link
Member Author

How did you manage to get in running without that line?

Good question, :/
Maybe I tested it wrongly, but your fixes make sense.
Thanks for testing!

@David-Development David-Development merged commit eb3627a into master Jun 28, 2019
@delete-merged-branch delete-merged-branch bot deleted the allowDevApp branch June 28, 2019 16:58
@desperateCoder
Copy link
Contributor

Yay, thank you! When are you gonna build a release approximately?

@David-Development
Copy link
Member

@desperateCoder right now! https://github.com/nextcloud/Android-SingleSignOn/releases/tag/0.4.0

@desperateCoder
Copy link
Contributor

Thanks buddy! Hope you will be @ the NC conference, so I can offer you a beer for your support! Same for @tobiasKaminsky btw!

@desperateCoder
Copy link
Contributor

@David-Development @tobiasKaminsky , just tested it for Deck, works great! Thanks again!

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.

4 participants