feat: Support for login and registration via a browser custom tab #190
Conversation
openedx-webhooks
added
the
open-source-contribution
|
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:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
xitij2000
force-pushed
the
kshitij/custom-auth-2
branch
2 times, most recently
from
389f90f
to
dc9023c
Compare
There was a problem hiding this comment.
@xitij2000 Great work on this! It's working quite well when I tested it out. I just had a small comment on some debug code pushed. I think it would also be helpful to add some documentation/instructions either in the README or the Documentation.md about how to enable/setup the browser based authentication.
- I tested this: (Configured and built the app on my phone with browser based auth with @xitij2000 server running the LMS/CMS, and tested registration/login flows)
- I read through the code
-
I checked for accessibility issues - Includes documentation: Missing but flagged it
-
I made sure any change in configuration variables is reflected in the corresponding client'sconfiguration-securerepository.
| @@ -60,12 +69,16 @@ class AuthRepository( | |||
| } | |||
|
|
|||
| private suspend fun AuthResponse.processAuthResponse() { | |||
| Log.d("MMMMMMMM", error.toString()) | |||
| Log.d("MMMMMMMM", preferencesManager.accessToken) | ||
| Log.d("MMMMMMMM", preferencesManager.refreshToken) | ||
| Log.d("MMMMMMMM", preferencesManager.accessTokenExpiresAt.toString()) |
There was a problem hiding this comment.
Similarly some debugging code here
xitij2000
force-pushed
the
kshitij/custom-auth-2
branch
from
dc9023c
to
530e8c5
Compare
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.
xitij2000
force-pushed
the
kshitij/custom-auth-2
branch
from
530e8c5
to
b0b7bfb
Compare
|
@yusuf-musleh I've updated the PR to remove the debugging logs. |
|
@xitij2000 The new doc looks good to me! 👍 |
xitij2000
force-pushed
the
kshitij/custom-auth-2
branch
from
7fcd757
to
2790805
Compare
|
Hi @xitij2000! Just checking in on this. |
I'd love some feedback on the approach etc. If that seems OK I can update the PR and get it ready to merge. |
|
A few notes @xitij2000 on my end - This is good to go on the product review side, though I would suggest that this be set up as a configurable feature flag defaulted to being off. This will keep the native login / register as the default in the mobile applications. Additionally, there may be some need for conflict resolution and rebasing for the review to continue. @volodymyr-chekyrta can review again once these items are addressed (and any other items as well that came up in the review above that may be outstanding. ) thanks! |
Thanks for the review! The intention is definitely to keep the current login experience intact, and just allow this alternative flow for cases where it's needed. |
|
@xitij2000, could you please resolve conflicts so I can proceed with review? |
This PR is in the similar position as openedx/openedx-app-ios#65 (comment) I've tried rebasing but need more time that I have currently, since the rebased version is nto working. |
|
@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. |
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.