feat: Support web-based LMS OAuth. #65
Conversation
| .padding(.vertical, 40) | ||
| if viewModel.forceWebLogin { | ||
| // On first load, we should bring the user right to the web login | ||
| // interface rather than showing this view. |
There was a problem hiding this comment.
What do you think about this: instead of this boolean forceWebLogin which special-cases one login method, could we have a setting called presumeLoginMethod or forceLoginMethod which can optionally be set to one of the LoginMethod enum values?
Whether you call it presumeLoginMethod or forceLoginMethod depends on whether you allow the user to go "Back" and choose a different method, or if this is the only option available to them. Though I suppose the "force" could be achieved simply by disabling all but one of the login methods.
There was a problem hiding this comment.
I'm not sure how I feel about this suggestion-- for two reasons. The first is that there's a rendering effect I'm concerned about here. The forceWebLogin method is there not only to bring the user to the web authentication page, but to make sure that, on initial load, the sign in page isn't drawn. There needs to be some way to signal to the view not to render the button or text if we're going to be opening the Safari browser to perform the login. Otherwise you get this visual glitch where you launch the app, see the sign-in page, and then the browser opens, which can be alarming and confusing-- I know that if I saw that as a user, I'd wonder if I pressed something I shouldn't or something happened I didn't want.
What you suggest could notionally be compatible with that first reason, but it would still require special casing the web login because, presumably, any other login method would have its interfaces written into the Swift view, and thus would need to be hidden if we're presuming the web login.
The second reason I'm not sure about your suggestion is that I'm not sure any other login methods, other than direct login, are actually viable outside of a web view, or wouldn't otherwise require some other complex changes to the UI. For example if you were to sign in with Google, you'd still need to visit the LMS login page to do the same dance we're doing for the client with Auth0. Same with any SAML provider, as far as I can tell-- the existing login view assumes that the user will be logging into the LMS using a post to Django directly, but nearly all external authentication methods will require visiting some intermediary web page and getting redirected back, requiring a Safari window, or else some tighter, complex iOS integration I'm unaware of.
For reference, I initially attempted to do the integration following Auth0's guide on integrating with iOS. However I realized as I was working with it that while I could authenticate with Auth0, the tokens it would provide me only worked with their servers directly. I couldn't get a token for the LMS without visiting the LMS and getting redirected from it to a login page provided by Auth0. There might be some other way to do it but it escapes me for now, and I suspect this is the way it's intended to be done. After all, all the MFEs at present just redirect to the LMS for login and then redirect you back, rather than presenting a streamlined login view.
|
@bradenmacdonald I've responded to one comment and addressed the other. Today I attempted to work on the tests, but it looks like I can't run them properly on non-Apple silicon. The tests rely on autogenerated mocks which need to be refreshed in order to update them and run, and the Podfile installs a dependency for generating these-- SwiftyMocky, which is prebuilt for Apple Silicon only. I attempted to compile it for |
Kelketek
changed the title
|
@bradenmacdonald Unmarking as WIP because although it probably can't be merged as-is, it does need upstream feedback from RaccoonGang before it can be moved forward. cc @IvanStepanok @volodymyr-chekyrta If either of you has some time :) Thanks! |
There was a problem hiding this comment.
Hi, thank you for your efforts and input!
And I apologize for the delayed review.
This feature looks quite complex to me, so let's take it step by step.
Could we please start with changing the target branch to develop, I know now it's a bit confusing compared to the legacy repos, but we would like to follow the git flow with the main and develop branches.
I'm going to describe this approach in the repos asap or we'll do it as part of the OEP-64.
| func login(viewController: UIViewController) async { | ||
| /* OAuth web login. Used when we cannot use the built-in login form, | ||
| but need to let the LMS redirect us to the authentication provider. | ||
|
|
||
| An example service where this is needed is something like Auth0, which | ||
| redirects from the LMS to its own login page. That login page then redirects | ||
| back to the LMS for the issuance of a token that can be used for making | ||
| requests to the LMS, and then back to the redirect URL for the app. */ | ||
| self.safariDelegate = WebLoginSafariDelegate(viewModel: self) | ||
| oauthswift = OAuth2Swift( | ||
| consumerKey: config.oAuthClientId, | ||
| consumerSecret: "", // No secret required | ||
| authorizeUrl: "\(config.baseURL)/oauth2/authorize/", | ||
| accessTokenUrl: "\(config.baseURL)/oauth2/access_token/", | ||
| responseType: "code" | ||
| ) | ||
|
|
||
| oauthswift!.allowMissingStateCheck = true | ||
| let handler = SafariURLHandler( | ||
| viewController: viewController, oauthSwift: oauthswift! | ||
| ) | ||
| handler.delegate = self.safariDelegate | ||
| oauthswift!.authorizeURLHandler = handler | ||
|
|
||
| // Trigger OAuth2 dance | ||
| guard let rwURL = URL(string: "\(Bundle.main.bundleIdentifier ?? "")://oauth2Callback") else { return } | ||
| oauthswift!.authorize(withCallbackURL: rwURL, scope: "", state: "") { result in | ||
| switch result { | ||
| case .success(let (credential, _, _)): | ||
| Task { | ||
| self.webLoginAttempted = true | ||
| let user = try await self.interactor.login(credential: credential) | ||
| self.analytics.setUserID("\(user.id)") | ||
| self.analytics.userLogin(method: .oauth2) | ||
| self.router.showMainScreen() | ||
| } | ||
| // Do your request | ||
| case .failure(let error): | ||
| self.webLoginAttempted = true | ||
| self.isShowProgress = false | ||
| self.errorMessage = error.localizedDescription | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This function can be problematic for several reasons.
- We are trying to encapsulate all navigation and ViewControllers logic inside of the Router entity.
This helps us to free ourselves from UIKit, navigation logic and be able to change the navigation framework at any time and write tests easily.
We could play with it and ask Router to provide us with configured SafariURLHandler instance or something like that. - I recommend converting the closure style function
oauthswift!.authorize(withCallbackURL: rwURL, scope: "", state: "") { result in
to async style function
await oauthswift!.authorize(...)orawait oauthswift!.authorizeTask(...)
we can reach it with some extension function and wrapoauthswift!.authorizeto the withCheckedThrowingContinuation call. - It's better to remove
Task {}creating from the async function as it makes it hard/impossible to test with Unit testing. - Last but not least, could you please extend the
SignInViewModelTests.swiftwith tests for your new functions? This is a crucial part of the application's stability.
I highly recommend running thegenerateAllMocks.shscript before you start creating tests. This script creates all mocks for tests if its need.
If you need any help, hit me on Slack or email me 📪
There was a problem hiding this comment.
Thanks, @volodymyr-chekyrta ! This is great feedback. I'll ping you as I make progress on it, as this appears to be the bulk of the changes to be made.
| self.validator = validator | ||
| self.webLoginAttempted = false |
There was a problem hiding this comment.
Is it needed to initialize webLoginAttempted with false on init if the variable is already initialized var webLoginAttempted: Bool = false?
There was a problem hiding this comment.
Nope-- that got left in when I was trying a few things. I'll remove it.
volodymyr-chekyrta
added
the
enhancement
| if viewModel.forceWebLogin { | ||
| // On first load, we should bring the user right to the web login | ||
| // interface rather than showing this view. | ||
| // | ||
| // If that login fails or the user escapes back, they'll be brought | ||
| // back to the view where any error message will be shown. | ||
| Task { | ||
| await webLogin() | ||
| } | ||
| } |
There was a problem hiding this comment.
In my mind, a more SwiftUI way will put this code to the View .onAppear {}
What do you think about it?
There was a problem hiding this comment.
@volodymyr-chekyrta I'm quite new to Swift, SwiftUI, and its idioms. This is my first Swift code, in fact :)
So, I couldn't tell you for sure. But I've looked up the docs, and I think you're probably right! I'll add this.
@Kelketek if you have a non-Apple Silicon chip, try this |
|
@Kelketek 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. |
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @Kelketek! What's next?Please work through the following steps to get your changes ready for engineering review: 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo 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:
🔘 Get a green buildIf 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 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:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
|
@e0d No, no product ticket was created for this work. It was discovered as necessary for a client when preparing a copy of the app for them. We did get the build working for them with these customizations, but due to other snags there is no anticipation of continuing development on this at this time. Still, this is likely to be a fairly important thing to have for any team which is using anything other than direct auth to the platform (so, pretty much all SSO users). It could be rolled into this existing ticket, which this fix would solve. To whoever that person is, there's one other bug that showed up later: It doesn't look like this implementation actually lets you log out. Likely, what is needed is the ability to clear all Safari state for the app in order to perform the logout. However I'm unfamiliar with the ecosystem to know how to do this off-hand (this is my first foray into Swift.) @xitij2000 worked on a similar fix for Android. I believe it has the same features and limitations. |
Kelketek
force-pushed
the
web_oauth
branch
3 times, most recently
from
ee46d1a to
14ad52e
Compare
|
A few notes @Kelketek 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! |
|
@Kelketek, could you please resolve conflicts so I can proceed with review? |
|
Hi @volodymyr-chekyrta ! We're currently discussing internally how to handle the budget for finishing this PR. For context, we've now had two different clients who have needed this but haven't yet secured enough from either of them to get it across the finish line. We're trying to figure out what to do about it and how to get buy-in for the finishing work. I'll update you as soon as I have a prediction on when we'll be able to rebase this and address your notes. |
Makes sense to me 👍 |
* Merge pull request #42 from edx/password-input-view-eye feat: eye icon for password field * chore: added SecureInputView to project * Merge pull request #44 from edx/clear-cookies-by-logout fix: clear cookies by logout * Merge pull request #47 from edx/reduce-grid-spacing style: reduce grid spacing * chore: Fix outdated course dates alert title truncation issue (#53) * Merge pull request #57 from edx/remaining-size-in-gb-or-mb chore: size in GB or MB * fix: Remove urls from headings of End User lisence agreements (#65) * fix: move padding to right place * chore: fixed double space --------- Co-authored-by: Anton Yarmolenko <[email protected]> Co-authored-by: Shafqat Muneer <[email protected]>
|
Had a conversation with Zemeel Hassan from Blend-ed who is considering taking over this PR on behalf of one of their clients. Recording here, where we walk a little bit through the code and some of the issues that he's running into with the rebase (and which I'd previously run into on previous rebases) |
Hey! I have started working on rebasing it to the develop branch. I have some doubts which I'll have to clear prior to drafting the PR. How should I proceed? |
@zameel7 Go ahead and ask them here! I'm sure @volodymyr-chekyrta will be able to answer your questions about creating the replacement PR and can likely answer some questions about the changes that have been made since this original version. |
|
Hi @zameel7 @Kelketek, please let me know if you have any questions. I'll do my best to help you. The only doubt I have is that we need to double-check that we do not duplicate the behavior introduced at #447. |
|
@zameel7 The merged PR that @volodymyr-chekyrta just linked might actually solve what this PR was attempting to do incidentally. Could you build that version of the app and check if it works for you? If it does, we could close this PR and be done here :) |
|
@Kelketek Sure, let me check that. I wonder how I missed the context of that PR😀. |
|
@zameel7 That's what I don't know. I would suspect it might, since I would think the SAML auth would turn into a more native grant on behalf of the platform once activated via web auth, but the only way to know would be to try it out. It might not. But even if not, starting from that branch will already give you a leg up in setting up external auth. |
|
I tested it and it works. For the SSO URL, I gave the URL that the OAuth swift used to build for authorise and for the SSO FINISHED URL, I gave the redirect callback url to the mobile app(I have a doubt here). |
Description
This pull request makes it possible to use the LMS's web login rather than the app's login form. This is necessary in the case that the client institution is using a custom authentication provider, and must piggy-back off the LMS's authentication flow to avoid writing a whole new custom login interface for these providers.
Testing instructions
The best way to test this is to use the version of the app built for Yam education.
Deadline
None
Author Concerns
One thing that's not working quite right in our client's application build is logout. This is because while the existing logout does, in fact, expire the LMS token, it doesn't log out the user from Auth0, the provider they're using. This means hitting the sign in button will log you right back in without prompting you for the password once more. It turns out that on the LMS, the logout button has a 'next' variable to a view that also logs the user out from Auth0. I've not yet determined if this is something we should also add as a tuneable variable-- having some custom logout URL. I'd have to see how that was implemented first.
My suspicion is that we should actually change what the LMS is doing rather than adding some additional logout action into the app code, but I'm not yet sure. cc @Cup0fCoffee
Reviewers