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

feat: Add support for browser-based logins #45

Closed
wants to merge 8 commits into from

Conversation

xitij2000
Copy link
Contributor

@xitij2000 xitij2000 commented Sep 5, 2023

For sites that only support logging in using a third-party login provider, allow logging in via a browser.
This new flow will open a custom tab where a user can log in using the supported mechanisms of the site,
then have the client id redirect back to to the app using a callback URL.

The auth code returned by the callback URL can then be used instead of a username and password to log in.

This change adds support for specifying an alternative resource directory, allowing resources to be overridden without needing to fork the repository.
This change makes it possible to specify an alternative location for the
config.yaml file using an environment variable. This allows the developers to
override all important data without messing up any files that can be committed
to the repository.
@xitij2000 xitij2000 changed the base branch from main to develop September 5, 2023 14:53
@xitij2000 xitij2000 changed the title kshitij/custom auth feat: Add support for browser-based logins Sep 5, 2023
For sites that only support logging in using a third-party login provider, allow logging in via a browser.
This new flow will open a custom tab where a user can log in using the supported mechanisms of the site,
then have the client id redirect back to to the app using a callback URL.
The auth code returned by the callback URL can then be used instead of a username and password to log in.
android {
compileSdk 34

defaultConfig {
applicationId "org.openedx.app"
applicationId config?.application_id ?: "org.openedx.app"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Allow customising the applicationId via the config file, but fall back to org.openedx.app as before.

Copy link
Contributor Author

@xitij2000 xitij2000 left a comment

Choose a reason for hiding this comment

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

I've added some comments to help the reviewer at OpenCraft work on this PR during my leave.

Comment on lines +45 to +49
applicationIdSuffix '.dev'
}
stage {
dimension 'env'
applicationIdSuffix '.stage'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added suffixes for each environment since AFAIK this is also done for iOS.

Comment on lines +38 to +43
<intent-filter>
<action android:name="android.intent.action.VIEW" />
<category android:name="android.intent.category.DEFAULT" />
<category android:name="android.intent.category.BROWSABLE" />
<data android:scheme="${applicationId}" />
</intent-filter>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will have the app handle the scheme matching the applicationId.

i.e. if the application id is "org.openedx.app" then this will handle: com.openedx.app://

Comment on lines +53 to +61
private val authCode: String?
get() {
val data = intent?.data
if (data is Uri && data.scheme == BuildConfig.APPLICATION_ID && data.host == "oauth2Callback") {
return data.getQueryParameter("code")
}
return null
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the application was opened using the auth intent filter,
<applicationid>://oauth2Callback?code=<some_code> it will get the code out of the URL and return that.

Comment on lines +116 to +121
val fragment = SignInFragment()
val bundle = Bundle()
bundle.putString("auth_code", authCode)
fragment.arguments = bundle
supportFragmentManager.beginTransaction()
.add(R.id.container, SignInFragment())
.add(R.id.container, fragment)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pass the code to the login fragment if available.

Comment on lines +16 to +24
private suspend fun processAuthResponse(authResponse: AuthResponse) {
if (authResponse.error != null) {
throw EdxError.UnknownException(authResponse.error!!)
}
preferencesManager.accessToken = authResponse.accessToken ?: ""
preferencesManager.refreshToken = authResponse.refreshToken ?: ""
val user = api.getProfile().mapToDomain()
preferencesManager.user = user
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extracted out the common code since both login functions only have a slightly different API call but the same response.

Comment on lines +60 to +63
val authCode = arguments?.getString("auth_code")
if (authCode is String) {
viewModel.login(authCode)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this view was passed the auth_code then we can just log in straight away.

Comment on lines 97 to 101
val intent = CustomTabsIntent.Builder()
.setUrlBarHidingEnabled(true)
.setShowTitle(true)
.build()
intent.launchUrl(context, uri)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using the custom tab mechanism allows any browser to handle this AFAIK and that can use the users saved password etc.

@@ -33,7 +33,7 @@ ext {
firebase_version = "32.1.0"

retrofit_version = '2.9.0'
logginginterceptor_version = '4.9.1'
logginginterceptor_version = '4.11.0'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This gets rids of some errors

Comment on lines +2 to +12
buildscript {
repositories {
mavenCentral()
maven {
url = uri("https://storage.googleapis.com/r8-releases/raw")
}
}
dependencies {
classpath("com.android.tools:r8:8.2.24")
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was suggested somewhere on StackOverflow for an error I was getting.

@openedx-webhooks
Copy link

openedx-webhooks commented Oct 12, 2023

Thanks for the pull request, @xitij2000! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

This is currently a draft pull request. When it is ready for our review and all tests are green, click "Ready for Review", or remove "WIP" from the title, as appropriate.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Oct 12, 2023
@e0d
Copy link

e0d commented Nov 21, 2023

@xitij2000 was a product ticket created for this work? We're pushing to have all PRs tie back to a product ticket that is, ideally, prioritized before the development happens.

@xitij2000
Copy link
Contributor Author

@xitij2000 was a product ticket created for this work? We're pushing to have all PRs tie back to a product ticket that is, ideally, prioritized before the development happens.

I think this PR predates the move to the openedx org. Can a ticket be created now?

@mphilbrick211
Copy link

@xitij2000 was a product ticket created for this work? We're pushing to have all PRs tie back to a product ticket that is, ideally, prioritized before the development happens.

I think this PR predates the move to the openedx org. Can a ticket be created now?

@e0d - flagging this

@xitij2000
Copy link
Contributor Author

Replaced by #190

@xitij2000 xitij2000 closed this Jan 30, 2024
@openedx-webhooks
Copy link

@xitij2000 Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

farhan-arshad-dev pushed a commit to farhan-arshad-dev/openedx-app-android that referenced this pull request Sep 20, 2024
We were incorrectly sending custom or track events as page events. This
update corrects them back to the appropriate event types.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants