feat: Support for login and registration via a browser custom tab #371

xitij2000 · 2024-07-30T16:54:08Z

This change adds support for logging in and registering a new account using the
browser. This can be useful for cases where the only way to log into the
instatance is via a custom third-party auth provider.

openedx-webhooks · 2024-07-30T16:54:13Z

Thanks for the pull request, @xitij2000!

What's next?

Please work through the following steps to get your changes ready for engineering review:

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

🔘 Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently maintained by @openedx/openedx-mobile-maintainers. Tag them in a comment and let them know that your changes are ready for review.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

navinkarkera

@xitij2000 Nice work! 👍

👍

I tested this: (tested login in both real hardware as well as emulator)
I read through the code
I checked for accessibility issues
Includes documentation

mphilbrick211 · 2024-10-24T21:01:12Z

@openedx/openedx-mobile-maintainers - is someone able to please take a look at this?

volodymyr-chekyrta · 2024-11-04T10:26:04Z

@openedx/openedx-mobile-maintainers - is someone able to please take a look at this?

I'll review it 👀
@xitij2000, could you change the target branch to develop and fix conflicts if they appear?
Thank you!

xitij2000 · 2024-11-04T11:12:40Z

@openedx/openedx-mobile-maintainers - is someone able to please take a look at this?

I'll review it 👀 @xitij2000, could you change the target branch to develop and fix conflicts if they appear? Thank you!

@volodymyr-chekyrta I've changed the target branch; however, I've had some issues running the develop branch before due to it often being reliant on unmerged PRs in edx-platform. So I might need your help with that.

Could you have a quick look and see if the general direction of the PR is okay or if bigger changes are needed so I can budget the time accordingly?

volodymyr-chekyrta · 2024-11-05T19:45:18Z

@openedx/openedx-mobile-maintainers - is someone able to please take a look at this?

I'll review it 👀 @xitij2000, could you change the target branch to develop and fix conflicts if they appear? Thank you!

@volodymyr-chekyrta I've changed the target branch; however, I've had some issues running the develop branch before due to it often being reliant on unmerged PRs in edx-platform. So I might need your help with that.

Could you have a quick look and see if the general direction of the PR is okay or if bigger changes are needed so I can budget the time accordingly?

@xitij2000 I checked the approach, and it is good 👍; I have a few comments according to the code, but it will be better to address them after resolving conflicts

xitij2000 · 2024-11-06T06:42:23Z

@openedx/openedx-mobile-maintainers - is someone able to please take a look at this?

I'll review it 👀 @xitij2000, could you change the target branch to develop and fix conflicts if they appear? Thank you!

@volodymyr-chekyrta I've changed the target branch; however, I've had some issues running the develop branch before due to it often being reliant on unmerged PRs in edx-platform. So I might need your help with that.
Could you have a quick look and see if the general direction of the PR is okay or if bigger changes are needed so I can budget the time accordingly?

@xitij2000 I checked the approach, and it is good 👍; I have a few comments according to the code, but it will be better to address them after resolving conflicts

I've rebased the code, but still need to test that everything works.

volodymyr-chekyrta

Minor changes, mostly move value to constant:

volodymyr-chekyrta · 2024-11-06T08:28:57Z

app/src/main/java/org/openedx/app/AppActivity.kt

+            if (data is Uri && data.scheme == BuildConfig.APPLICATION_ID && data.host == "oauth2Callback") {
+                return data.getQueryParameter("code")
+            }


Please move "oauth2Callback" and "code" to constants.

volodymyr-chekyrta · 2024-11-06T08:29:58Z

app/src/main/java/org/openedx/app/AppActivity.kt

+                        val bundle = Bundle()
+                        bundle.putString("auth_code", authCode)
+                        val fragment = SignInFragment()
+                        fragment.arguments = bundle
+                        addFragment(fragment)


Please use SignInFragment.newIntance(...) method

volodymyr-chekyrta · 2024-11-06T08:30:19Z

auth/src/main/java/org/openedx/auth/data/repository/AuthRepository.kt

+            grantType = ApiConstants.GRANT_TYPE_CODE,
+            clientId = config.getOAuthClientId(),
+            code = code,
+            redirectUri = "${config.getApplicationID()}://oauth2Callback"


oauth2Callback to constants

volodymyr-chekyrta · 2024-11-06T08:30:44Z

auth/src/main/java/org/openedx/auth/presentation/logistration/LogistrationFragment.kt

+                            UrlUtils.openInBrowser(
+                                activity = context,
+                                apiHostUrl = viewModel.apiHostUrl,
+                                url = "/register",


Please move /register to constants

volodymyr-chekyrta · 2024-11-06T08:31:55Z

auth/src/main/java/org/openedx/auth/presentation/signin/SignInFragment.kt

+                    val authCode = arguments?.getString("auth_code")
+                    if (authCode is String && !state.loginFailure && !state.loginSuccess) {
+                        arguments?.remove("auth_code")


Please move arguments like "auth_code" to companion object constants.
e.g.

companion object { private const val ARG_COURSE_ID = "courseId" private const val ARG_INFO_TYPE = "info_type" private const val ARG_AUTH_CODE = "auth_code" fun newInstance(courseId: String?, infoType: String?): SignInFragment { val fragment = SignInFragment() fragment.arguments = bundleOf( ARG_COURSE_ID to courseId, ARG_INFO_TYPE to infoType ) return fragment } }

volodymyr-chekyrta · 2024-11-06T08:34:54Z

auth/src/main/java/org/openedx/auth/presentation/signin/compose/SignInView.kt

+                    if(state.isBrowserLoginEnabled) {
+                        onEvent(AuthEvent.SignInBrowser)


if(state.isBrowserLoginEnabled) -> if (state.isBrowserLoginEnabled)

volodymyr-chekyrta · 2024-11-06T08:35:33Z

auth/src/main/java/org/openedx/auth/presentation/signin/compose/SignInView.kt

+                    } else {
                    keyboardController?.hide()


Suggested change

} else {

keyboardController?.hide()

} else {

keyboardController?.hide()

volodymyr-chekyrta · 2024-11-06T08:36:51Z

auth/src/main/java/org/openedx/auth/presentation/sso/BrowserAuthHelper.kt

+            .appendQueryParameter("client_id", config.getOAuthClientId())
+            .appendQueryParameter("redirect_uri", "${activityContext.packageName}://oauth2Callback")
+            .appendQueryParameter("response_type", "code").build()


It probably will be good to move the params to constants as well

volodymyr-chekyrta · 2024-11-06T08:38:22Z

core/src/main/java/org/openedx/core/config/Config.kt

+    fun getApplicationID(): String {
+        return getString(APPLICATION_ID, "")
+    }
+


duplicates:

fun getAppId(): String { return getString(APPLICATION_ID, "") }

volodymyr-chekyrta · 2024-11-06T08:40:02Z

docs/how-tos/auth-using-browser.rst

+How to user Browser-based Login and Registration
+================================================
+


Just wanted to point out that I like this doc 👍

xitij2000 · 2024-11-07T09:08:24Z

@volodymyr-chekyrta I will allocate some time soon to go through these properly and update the PR. Thanks for the great feedback!

xitij2000 · 2024-11-28T11:54:21Z

@volodymyr-chekyrta I've updated the PR based on your feedback. Do have a look!

app/src/main/java/org/openedx/app/AppActivity.kt

app/src/main/java/org/openedx/app/di/ScreenModule.kt

auth/src/main/java/org/openedx/auth/presentation/logistration/LogistrationFragment.kt

auth/src/main/java/org/openedx/auth/presentation/logistration/LogistrationViewModel.kt

auth/src/main/java/org/openedx/auth/presentation/sso/BrowserAuthHelper.kt

volodymyr-chekyrta · 2024-12-02T09:12:56Z

@volodymyr-chekyrta I've updated the PR based on your feedback. Do have a look!

@xitij2000 thank you! I'll take a look.

Could you please check the errors in auth tests?

e: file:///home/runner/work/openedx-app-android/openedx-app-android/auth/src/test/java/org/openedx/auth/presentation/signin/SignInViewModelTest.kt:131:13 No value passed for parameter 'authCode'.
e: file:///home/runner/work/openedx-app-android/openedx-app-android/auth/src/test/java/org/openedx/auth/presentation/signin/SignInViewModelTest.kt:168:13 No value passed for parameter 'authCode'.
e: file:///home/runner/work/openedx-app-android/openedx-app-android/auth/src/test/java/org/openedx/auth/presentation/signin/SignInViewModelTest.kt:205:13 No value passed for parameter 'authCode'.
e: file:///home/runner/work/openedx-app-android/openedx-app-android/auth/src/test/java/org/openedx/auth/presentation/signin/SignInViewModelTest.kt:241:13 No value passed for parameter 'authCode'.
e: file:///home/runner/work/openedx-app-android/openedx-app-android/auth/src/test/java/org/openedx/auth/presentation/signin/SignInViewModelTest.kt:281:13 No value passed for parameter 'authCode'.

default_config/dev/config.yaml

volodymyr-chekyrta · 2024-12-02T11:49:25Z

The code looks good to me.
@marcotuts, would you like to check this feature from a product perspective before merging?

app/src/main/java/org/openedx/app/AppActivity.kt

This change adds support for logging in and registering a new account using the browser. This can be useful for cases where the only way to log into the instatance is via a custom third-party auth provider.

marcotuts · 2024-12-06T04:32:57Z

Would it be possible to clarify the usage for this, and if possible any screenshots of it, even if just from local dev?

I want to make sure I understand the flow here, is the configuration such that you are either using the native login / register (on by default?) or usign the browser mechanism?

I ask becasue we also merged in a secondary web login button that can be optionally configured on the native view to direct users to a browser based login (ex: for corporate logins or other options not available natively).

I still think the ability to choose a full browser login is a nimportant addition to the app, so I dont consider the two described features above as conflicting but I want to make sure I understand the changes and what the level of theming might be, or how the browser window renders (overlaps, embedded, etc).

Thanks @xitij2000 for this contribution!

xitij2000 · 2024-12-06T10:32:46Z

@marcotuts Sure! It will be hard to do it through screenshots since it a flow, but I will try to make a screencap and post it here.

In this particular implementation, the web-based login repalces the native login. So when you click on the login button, it opens a custom browser tab, which is essentially a full-screen browser window (AFAIK it will use your default browser) where the OAuth2 based login flow will be initiated. When the user completes the log in in this browser windows (it could be using a username and password or whatever third-party auth system the LMS is configured to use) it will redirect back to the app with the code and the app will fetch a login token from that.

From what I understand the feature you're describing adds the above login flow as an option during runtime rather than build time (which is what this PR does). I think if that is the case, we could simplify this PR to use that flow. Is my understanding correct?

xitij2000 · 2024-12-09T11:54:02Z

@marcotuts Here is a video of this at work:

Screen_recording_20241209_172251.webm

marcotuts · 2024-12-09T16:10:44Z

Thanks for the video! This looks great, thanks again for the contribution. It might be worth eventually exploring the embedded mode for this view but it seamlessly loads and returns to the app in the demo so that's 99% of what we need :D

product 👍

xitij2000 · 2024-12-09T18:41:57Z

Thanks for the video! This looks great, thanks again for the contribution. It might be worth eventually exploring the embedded mode for this view but it seamlessly loads and returns to the app in the demo so that's 99% of what we need :D

product 👍

I am not very knowledgeable about Android so I followed the advice I heard about WebViews for OAuth2 which instead recommended using custom tabs.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jul 30, 2024

itsjeyd mentioned this pull request Aug 1, 2024

[Product Pull Request] feat: Support web-based LMS OAuth openedx/platform-roadmap#345

Closed

1 task

navinkarkera approved these changes Aug 5, 2024

View reviewed changes

xitij2000 marked this pull request as ready for review August 13, 2024 13:51

This was referenced Aug 13, 2024

feat: Allow trusted apps to perform cookie login. openedx/edx-platform#35221

Merged

feat: Allow trusted apps to perform cookie login. open-craft/edx-platform#668

Merged

mphilbrick211 requested a review from a team August 22, 2024 20:39

navinkarkera mentioned this pull request Sep 13, 2024

feat: Support for login and registration via a browser custom tab open-craft/openedx-app-android#2

Merged

xitij2000 changed the base branch from main to develop November 4, 2024 11:01

xitij2000 force-pushed the kshitij/custom-auth-2 branch from b0b4165 to 92cb74e Compare November 6, 2024 06:41

volodymyr-chekyrta requested changes Nov 6, 2024

View reviewed changes

xitij2000 force-pushed the kshitij/custom-auth-2 branch from 4b5437b to 65b9c39 Compare November 28, 2024 11:48

github-advanced-security bot found potential problems Nov 28, 2024

View reviewed changes

volodymyr-chekyrta reviewed Dec 2, 2024

View reviewed changes

default_config/dev/config.yaml Outdated Show resolved Hide resolved

xitij2000 force-pushed the kshitij/custom-auth-2 branch from 9e22b77 to 189b3f4 Compare December 2, 2024 12:17

github-advanced-security bot found potential problems Dec 2, 2024

View reviewed changes

app/src/main/java/org/openedx/app/AppActivity.kt Fixed Show fixed Hide fixed

xitij2000 added 2 commits December 2, 2024 18:14

feat: Support for login and registration via a browser custom tab

8beb850

This change adds support for logging in and registering a new account using the browser. This can be useful for cases where the only way to log into the instatance is via a custom third-party auth provider.

fixup! feat: Support for login and registration via a browser custom tab

7b1f928

xitij2000 force-pushed the kshitij/custom-auth-2 branch from 189b3f4 to 7b1f928 Compare December 2, 2024 12:51

volodymyr-chekyrta approved these changes Dec 10, 2024

View reviewed changes

volodymyr-chekyrta merged commit 4566b1a into openedx:develop Dec 10, 2024
6 checks passed

xitij2000 deleted the kshitij/custom-auth-2 branch December 10, 2024 08:25

xitij2000 mentioned this pull request Dec 10, 2024

fix: Make SSO login use JWT for better compatibility open-craft/openedx-app-android#10

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Support for login and registration via a browser custom tab #371

feat: Support for login and registration via a browser custom tab #371

xitij2000 commented Jul 30, 2024 •

edited

Loading

openedx-webhooks commented Jul 30, 2024 •

edited

Loading

navinkarkera left a comment

mphilbrick211 commented Oct 24, 2024

volodymyr-chekyrta commented Nov 4, 2024

xitij2000 commented Nov 4, 2024

volodymyr-chekyrta commented Nov 5, 2024

xitij2000 commented Nov 6, 2024

volodymyr-chekyrta left a comment

volodymyr-chekyrta Nov 6, 2024

volodymyr-chekyrta Nov 6, 2024

volodymyr-chekyrta Nov 6, 2024

volodymyr-chekyrta Nov 6, 2024

volodymyr-chekyrta Nov 6, 2024

volodymyr-chekyrta Nov 6, 2024

volodymyr-chekyrta Nov 6, 2024

volodymyr-chekyrta Nov 6, 2024

volodymyr-chekyrta Nov 6, 2024

volodymyr-chekyrta Nov 6, 2024

xitij2000 commented Nov 7, 2024

xitij2000 commented Nov 28, 2024

volodymyr-chekyrta commented Dec 2, 2024

volodymyr-chekyrta commented Dec 2, 2024

marcotuts commented Dec 6, 2024

xitij2000 commented Dec 6, 2024

xitij2000 commented Dec 9, 2024 •

edited

Loading

marcotuts commented Dec 9, 2024 •

edited

Loading

xitij2000 commented Dec 9, 2024

		if(state.isBrowserLoginEnabled) {
		onEvent(AuthEvent.SignInBrowser)

		How to user Browser-based Login and Registration
		================================================

feat: Support for login and registration via a browser custom tab #371

feat: Support for login and registration via a browser custom tab #371

Conversation

xitij2000 commented Jul 30, 2024 • edited Loading

openedx-webhooks commented Jul 30, 2024 • edited Loading

What's next?

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

🔘 Let us know that your PR is ready for review:

Who will review my changes?

Where can I find more information?

When can I expect my changes to be merged?

navinkarkera left a comment

Choose a reason for hiding this comment

mphilbrick211 commented Oct 24, 2024

volodymyr-chekyrta commented Nov 4, 2024

xitij2000 commented Nov 4, 2024

volodymyr-chekyrta commented Nov 5, 2024

xitij2000 commented Nov 6, 2024

volodymyr-chekyrta left a 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

xitij2000 commented Nov 7, 2024

xitij2000 commented Nov 28, 2024

volodymyr-chekyrta commented Dec 2, 2024

volodymyr-chekyrta commented Dec 2, 2024

marcotuts commented Dec 6, 2024

xitij2000 commented Dec 6, 2024

xitij2000 commented Dec 9, 2024 • edited Loading

marcotuts commented Dec 9, 2024 • edited Loading

xitij2000 commented Dec 9, 2024

xitij2000 commented Jul 30, 2024 •

edited

Loading

openedx-webhooks commented Jul 30, 2024 •

edited

Loading

xitij2000 commented Dec 9, 2024 •

edited

Loading

marcotuts commented Dec 9, 2024 •

edited

Loading