feat: add single sign on by saml by RawanMatar89 · Pull Request #420 · openedx/openedx-app-android

RawanMatar89 · 2025-02-10T07:00:54Z

Describtion:

as in iOS, we added a new button in sign in view named "Sign in with SSO" to be use for opening single sign on web view

TODO:

Revert previous Android SAML implementation to unify with iOS PR feat: Support for login and registration via a browser custom tab #371
Initial SAML support, tested and verified
YAML configuration documentation
Fixed conflicts with the develop branch
Initial review by RG team
Final review by RG team
Fix lint issues after the final review

Screenshots:

Before	After (both)	After (SSO only)

openedx-webhooks · 2025-02-10T07:00:58Z

Thanks for the pull request, @RawanMatar89!

This repository is currently maintained by @openedx/openedx-mobile-maintainers.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are 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.

Details

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.

github-advanced-security

detekt found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

e0d · 2025-02-14T14:23:29Z

@RawanMatar89 it looks like there are some failing tests and checks, can you have a look. We can review this when the checks are passing.

I also want to make sure we ensure this work meshes will with this:

#371

@xitij2000 can you chime in here?

xitij2000 · 2025-02-24T11:42:07Z

@RawanMatar89 it looks like there are some failing tests and checks, can you have a look. We can review this when the checks are passing.

I also want to make sure we ensure this work meshes will with this:

#371

@xitij2000 can you chime in here?

I am currently unable to test the code or have a deeper look; however, I do see some overlap. #371 adds a web-browser based flow that will initiate an OAuth flow with the LMS, delegating the actual login mechanism to whatever IdPs are set up at the LMS including any SAML-based ones. So from what I understand, what this PR is doing is already possible using the existing code.

OmarIthawi · 2025-03-13T08:28:32Z

I am currently unable to test the code or have a deeper look; however, I do see some overlap. #371 adds a web-browser based flow that will initiate an OAuth flow with the LMS, delegating the actual login mechanism to whatever IdPs are set up at the LMS including any SAML-based ones. So from what I understand, what this PR is doing is already possible using the existing code.

@xitij2000 good note that what I was thinking of when I looked at your WebView code. For a background, the original work has been done in the iOS app and is already merged since July 2024:

feat: add single sign on feature using SAML openedx-app-ios#447

This PR has gone stale a bit due to the review process and our internal prioritizations.

My recommendation is to keep the iOS and the Android in a very similar flow in regards to backend integration and YAML configurations.

@e0d @volodymyr-chekyrta How do you recommend going on with this?

volodymyr-chekyrta · 2025-03-14T08:54:10Z

I am currently unable to test the code or have a deeper look; however, I do see some overlap. #371 adds a web-browser based flow that will initiate an OAuth flow with the LMS, delegating the actual login mechanism to whatever IdPs are set up at the LMS including any SAML-based ones. So from what I understand, what this PR is doing is already possible using the existing code.

@xitij2000 good note that what I was thinking of when I looked at your WebView code. For a background, the original work has been done in the iOS app and is already merged since July 2024:

feat: add single sign on feature using SAML openedx-app-ios#447

This PR has gone stale a bit due to the review process and our internal prioritizations.

My recommendation is to keep the iOS and the Android in a very similar flow in regards to backend integration and YAML configurations.

@e0d @volodymyr-chekyrta How do you recommend going on with this?

Good question. I would like to see a similar approach and feature flags for both platforms.
It may be a good discussion for the Mobile: Product SubGroup Meeting.

RawanMatar89 · 2025-04-08T09:57:08Z

Plan for this PR:

Resolve Conflicts: First, I will address the existing conflicts in this PR to ensure everything is aligned correctly.
Revert Web base LMS Auth PR: To maintain consistency between Android and iOS code, I will re the changes from Web base LMS Auth PR for now.
Cover Missing Cases: After the rollback, I will review the changes from Web base LMS Auth PR and ensure that any cases not already covered in our PR are addressed.

This approach will help keep both the Android and iOS code versions in sync while maintaining any necessary improvements from Web base LMS Auth PR work.

cc: @xijit and @valimir

…penedx#371) This reverts commit 4566b1a

RawanMatar89 · 2025-05-25T07:14:39Z

dears, This PR is now ready for review.
Just to highlight, the approach used here is more advanced than the one in the reverted PR. With our implementation, both default and browser-based login options can coexist and be supported simultaneously. In contrast, the previous approach only allowed one login method to be available at a time.
Looking forward to your feedback!

@volodymyr-chekyrta , @xitij2000 , @valimir

mphilbrick211 · 2025-06-20T20:21:34Z

Hi @openedx/openedx-mobile-maintainers! Would someone be able to please take a look at this?

mphilbrick211 · 2025-09-16T20:06:42Z

Hi @openedx/openedx-mobile-maintainers! Would someone be able to please take a look at this?

Friendly ping on this @openedx/openedx-mobile-maintainers

volodymyr-chekyrta · 2025-09-17T08:15:28Z

@RawanMatar89, could you please resolve conflicts on this PR? We will take it to review.
@IvanStepanok @PavloNetrebchuk fyi

OmarIthawi · 2025-09-27T20:25:36Z

@RawanMatar89 another ping for the rebase and conflict fixing.

This is still on our plate and we'll push for it to get merged.

Thanks @mphilbrick211 and @volodymyr-chekyrta for reviving it.

into SAML_SSO # Conflicts: # app/src/main/java/org/openedx/app/AppActivity.kt # auth/build.gradle # auth/src/main/java/org/openedx/auth/presentation/sso/BrowserAuthHelper.kt # build.gradle # gradle/wrapper/gradle-wrapper.properties

RawanMatar89 · 2025-10-02T12:39:31Z

@volodymyr-chekyrta, @OmarIthawi
Thanks for the reminder! I’ve resolved all conflicts, and the PR is now ready for review.

IvanStepanok

I’m worried about the SSO login flow here. In SSOWebContentScreen we read cookies without null checks and immediately split them,

openedx-app-android/core/src/main/java/org/openedx/core/ui/SSOWebContentScreen.kt

Lines 171 to 176 in 2207571

    
           fun getCookie(siteName: String?, cookieName: String?): String? { 
        
               var cookieValue: String? = "" 
        
               val cookieManager = CookieManager.getInstance() 
        
               val cookies = cookieManager.getCookie(siteName) 
        
               val temp = cookies.split(";".toRegex()).dropLastWhile { it.isEmpty() }

so the very first onPageFinished can throw an NPE, and even when it doesn’t we concatenate header/payload/signature without the separating dot.

openedx-app-android/core/src/main/java/org/openedx/core/ui/SSOWebContentScreen.kt

Lines 125 to 126 in 2207571

    
           val jwtToken = getCookie(url, "edx-jwt-cookie-header-payload")  + getCookie(url, "edx-jwt-cookie-signature") 
        
           onWebPageUpdated(jwtToken)

SSOWebContentFragment treats any non-empty result as success and pops the stack,

openedx-app-android/core/src/main/java/org/openedx/core/presentation/global/webview/SSOWebContentFragment.kt

Lines 43 to 45 in 2207571

    
           if (token.isNotEmpty()){ 
        
               setFragmentResult("requestKey", bundleOf("bundleKey" to token)) 
        
               requireActivity().supportFragmentManager.popBackStack()

and SignInFragment then hands ssoLogin the requestKey instead of the token from the bundle.

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

Lines 47 to 48 in 2207571

    
           setFragmentResultListener("requestKey") { requestKey, bundle -> 
        
               viewModel.ssoLogin(token = requestKey)

Combined, this means ssoLogin ends up with garbage and the SSO attempt can never finish successfully.

IvanStepanok · 2025-10-02T13:39:09Z

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

+    }
+    if (state.isSSOLoginEnabled) {
+        Spacer(modifier = Modifier.height(18.dp))
+        if (state.isLoginRegistrationFormEnabled) {


if (!state.isLoginRegistrationFormEnabled) {

According to iOS implementation we need to hide this title in case when isLoginRegistrationFormEnabled = true

IvanStepanok · 2025-10-02T13:45:14Z

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

+                        buttonWidth
+                            .testTag("btn_sso")
+                            .fillMaxWidth(),
+                    text = stringResource(id = coreR.string.core_sso_sign_in),


We are ignoring SSO_BUTTON_TITLE parameter on config file here

IvanStepanok · 2025-10-02T13:51:14Z

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

    }

-    viewModel { (courseId: String?, infoType: String?, authCode: String) ->
+    val lang = Resources.getSystem().configuration.locales[0].language


lang is lowercased here but in
SSO_BUTTON_TITLE:
AR: "الدخول عبر SSO"
EN: "Sign in with SSO"

parameters are uppercased. Can’t check this because in current implementation we are ignoring SSO_BUTTON_TITLE

IvanStepanok · 2025-10-02T13:56:06Z

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

+    fun ssoLogin(token: String) {
+        logEvent(AuthAnalyticsEvent.USER_SIGN_IN_CLICKED)
+
+


We need to add the same logic as on default Login where we have a clean calendar and synchronize FCM if users are changed.

IvanStepanok · 2025-10-02T13:59:39Z

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

+        _uiState.update { it.copy(showProgress = true) }
+        viewModelScope.launch {
+            try {
+                interactor.ssoLogin("JWT $token")


I cant check this, but looks like we will return here duplicates of "JWT". Like "JWTJWT token1323231232".

IvanStepanok · 2025-10-02T14:04:17Z

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

+        if (preferencesManager.accessToken.isBlank() ||
+            preferencesManager.refreshToken.isBlank()){
+            preferencesManager.accessToken = jwtToken
+            preferencesManager.refreshToken = jwtToken


Do we need to update accessTokenExpiresAt here?

RawanMatar89 · 2025-12-29T20:06:28Z

@IvanStepanok, I think all issues resolved, could you please check

IvanStepanok

LGTM

PavloNetrebchuk

Also, please check the detekt warnings

PavloNetrebchuk · 2026-01-06T09:50:46Z

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

+    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
+        super.onViewCreated(view, savedInstanceState)
+        setFragmentResultListener("requestKey") { _, bundle ->
+            val token = bundle.getString("bundleKey")


Please move "requestKey" and "bundleKey" to companion object. Reuse it for SSOWebContentFragment.kt too

PavloNetrebchuk · 2026-01-06T09:53:57Z

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

+    fun ssoClicked(fragmentManager: FragmentManager) {
+        router.navigateToSSOWebContent(
+            fm = fragmentManager,
+            title = resourceManager.getString(org.openedx.core.R.string.core_sso_sign_in),


move org.openedx.core.R to imports

PavloNetrebchuk · 2026-01-06T09:56:26Z

core/src/main/java/org/openedx/core/presentation/global/webview/SSOWebContentFragment.kt

+                    onWebPageUpdated = {
+                        val token = it
+                        if (token.isNotEmpty()){
+                            setFragmentResult("requestKey", bundleOf("bundleKey" to token))


use consts for "requestKey" and "bundleKey"

PavloNetrebchuk · 2026-01-06T10:23:01Z

default_config/dev/config.yaml

+
+SSO_BUTTON_TITLE:
+  AR: "الدخول عبر SSO"
+  EN: "Sign in with SSO"


I think for more consistency it would be better to set the strings in the resources strings.xml file

How do you propose this to happen @PavloNetrebchuk? Given that there's more than one button in the same app which isn't very practical to maintain in the context of strings.xml.

But we are showing only 1 button in 1 language at the same time. Could you describe this flow in more detail, I don't fully understand the reason for this decision

PavloNetrebchuk · 2026-01-06T10:29:36Z

core/src/main/java/org/openedx/core/ui/SSOWebContentScreen.kt

+    var cookieValue: String? = ""
+    if (siteName != null && cookieName != null) {
+        val cookieManager = CookieManager.getInstance()
+        val cookies = cookieManager.getCookie(siteName)


If the request fails, the cookies will return null and the application will crash with a NullPointerException. We need to handle the error and display a message.

…her fixes

RawanMatar89 · 2026-01-25T15:36:05Z

Issues resolved ✔️ 🙏🏻

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Feb 10, 2025

RawanMatar89 marked this pull request as ready for review February 10, 2025 08:42

mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Feb 11, 2025

github-advanced-security bot found potential problems Feb 13, 2025

View reviewed changes

volodymyr-chekyrta mentioned this pull request Feb 14, 2025

feat: Support web-based LMS OAuth. openedx/openedx-app-ios#65

Closed

mphilbrick211 removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Feb 14, 2025

RawanMatar89 added 2 commits April 14, 2025 22:29

revert: Support for login and registration via a browser custom tab (o…

eb3f8a9

…penedx#371) This reverts commit 4566b1a

feat: add saml sso

135a305

RawanMatar89 force-pushed the SAML_SSO branch from 4d834d9 to 135a305 Compare April 14, 2025 20:15

mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label May 12, 2025

mphilbrick211 moved this from Waiting on Author to Needs Tests Run or CLA Signed in Contributions May 12, 2025

mphilbrick211 removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label May 15, 2025

mphilbrick211 moved this from Needs Tests Run or CLA Signed to Ready for Review in Contributions May 15, 2025

mphilbrick211 requested a review from a team May 15, 2025 18:20

mphilbrick211 added the inactive PR author has been unresponsive for several months label Sep 16, 2025

mphilbrick211 removed the inactive PR author has been unresponsive for several months label Sep 24, 2025

RawanMatar89 changed the title ~~feat: add single sing on by saml~~ feat: add single sign on by saml Sep 29, 2025

fix: resolve conflicts and unit test

fa5f13a

RawanMatar89 force-pushed the SAML_SSO branch from 28dcbf7 to fa5f13a Compare October 2, 2025 12:49

volodymyr-chekyrta requested a review from IvanStepanok October 2, 2025 12:50

fix: unit test issues

2207571

IvanStepanok reviewed Oct 2, 2025

View reviewed changes

mphilbrick211 moved this from Ready for Review to In Eng Review in Contributions Nov 6, 2025

RawanMatar89 added 3 commits December 7, 2025 11:51

fix: sso login flow and other fixes

316ebc4

fix: fix conflicts with develop

4e1ce2c

fix: sso login unit tests

e2badfd

OmarIthawi requested a review from IvanStepanok January 3, 2026 09:25

IvanStepanok requested a review from PavloNetrebchuk January 3, 2026 13:30

IvanStepanok reviewed Jan 3, 2026

View reviewed changes

PavloNetrebchuk reviewed Jan 6, 2026

View reviewed changes

RawanMatar89 added 2 commits January 24, 2026 11:42

fix: sso login flow handle cookies error and display a message and ot…

833d138

…her fixes

fix: fixing detekt issues

25b1a00

	fun getCookie(siteName: String?, cookieName: String?): String? {
	var cookieValue: String? = ""

	val cookieManager = CookieManager.getInstance()
	val cookies = cookieManager.getCookie(siteName)
	val temp = cookies.split(";".toRegex()).dropLastWhile { it.isEmpty() }

	val jwtToken = getCookie(url, "edx-jwt-cookie-header-payload") + getCookie(url, "edx-jwt-cookie-signature")
	onWebPageUpdated(jwtToken)

	if (token.isNotEmpty()){
	setFragmentResult("requestKey", bundleOf("bundleKey" to token))
	requireActivity().supportFragmentManager.popBackStack()

	setFragmentResultListener("requestKey") { requestKey, bundle ->
	viewModel.ssoLogin(token = requestKey)

		fun ssoLogin(token: String) {
		logEvent(AuthAnalyticsEvent.USER_SIGN_IN_CLICKED)

Conversation

RawanMatar89 commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describtion:

TODO:

Screenshots:

Uh oh!

openedx-webhooks commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-advanced-security bot left a comment

Choose a reason for hiding this comment

Uh oh!

e0d commented Feb 14, 2025

Uh oh!

xitij2000 commented Feb 24, 2025

Uh oh!

OmarIthawi commented Mar 13, 2025

Uh oh!

volodymyr-chekyrta commented Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RawanMatar89 commented Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RawanMatar89 commented May 25, 2025

Uh oh!

mphilbrick211 commented Jun 20, 2025

Uh oh!

mphilbrick211 commented Sep 16, 2025

Uh oh!

volodymyr-chekyrta commented Sep 17, 2025

Uh oh!

OmarIthawi commented Sep 27, 2025

Uh oh!

RawanMatar89 commented Oct 2, 2025

Uh oh!

IvanStepanok left a comment

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!

RawanMatar89 commented Dec 29, 2025

Uh oh!

IvanStepanok left a comment

Choose a reason for hiding this comment

Uh oh!

PavloNetrebchuk left a comment • 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!

RawanMatar89 commented Jan 25, 2026

Uh oh!

Reviewers

Assignees

Labels

RawanMatar89 commented Feb 10, 2025 •

edited

Loading

openedx-webhooks commented Feb 10, 2025 •

edited

Loading

volodymyr-chekyrta commented Mar 14, 2025 •

edited

Loading

RawanMatar89 commented Apr 8, 2025 •

edited

Loading

PavloNetrebchuk left a comment •

edited

Loading