Skip to content

Migrate authentication to LabsPlatformSwift #600

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

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

jonathanmelitski
Copy link
Contributor

LabsPlatformSwift is a new package that can handle the authentication flow with the Penn Labs Platform server. It handles the general authentication flow and all network-related requests to the Penn Labs servers (all network requests have been rewritten).

The package has a lot of functionality. Analytics are at the crux of this package but are for a future PR.

Copy link
Member

@JHawk0224 JHawk0224 left a comment

Choose a reason for hiding this comment

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

This is amazing!

For the status code comments, I don't think they need to be changed, I just wanted to point them out.

On an overall note, I see when logging in that the ProgressView we had inserted over the login sheet is no longer there (action is still disabled though preventing stale web request - ty). Is it possible we can add it back? Ik it's Penn's fault for being slow, but otherwise there's no indication something is happening.

Also, when testing, I just logged in but it brought be to the Login page after a successful login. Then clicking again doesn't do anything, see below video. Not sure why it's happening, but should be fixed.

weird-behavior.mov

Note that I am not done testing, so I will be continuing to add comments, but I thought I would share this review for now.

if successful {
UserDefaults.standard.setLastLogin()
}
UserDefaults.standard.storeCookies()
Copy link
Member

Choose a reason for hiding this comment

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

I'm assuming we don't need to store cookies anymore and LabsPlatform handles everything?

Copy link
Contributor Author

@jonathanmelitski jonathanmelitski Apr 14, 2025

Choose a reason for hiding this comment

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

actually an incredible catch, forgot that HTTPCookieStorage doesn't persist through restarts, which is what LabsPlatform is currently doing. Yes it is handled by LabsPlatform (albeit evidently not to a full capacity) and will fix. Btw I'm going to PR my changes to LabsPlatformSwift just to add a reference.

@@ -146,8 +147,9 @@ class RootViewController: UIViewController, NotificationRequestable, ShowsAlert
func clearAccountData() {
Copy link
Member

Choose a reason for hiding this comment

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

What's the difference between this and the AuthManager's clearAccountData?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe it's copied and pasted. I don't know if it would be incredibly difficult to remove RootViewController from the project. Really just the applicationDidEnterForeground function, moving it to an onChange of scenePhase.

@jonathanmelitski
Copy link
Contributor Author

jonathanmelitski commented Apr 14, 2025

This is amazing!

For the status code comments, I don't think they need to be changed, I just wanted to point them out.

On an overall note, I see when logging in that the ProgressView we had inserted over the login sheet is no longer there (action is still disabled though preventing stale web request - ty). Is it possible we can add it back? Ik it's Penn's fault for being slow, but otherwise there's no indication something is happening.

Also, when testing, I just logged in but it brought be to the Login page after a successful login. Then clicking again doesn't do anything, see below video. Not sure why it's happening, but should be fixed.

weird-behavior.mov
Note that I am not done testing, so I will be continuing to add comments, but I thought I would share this review for now.

my thought/initial guess, the client id you're using isn't setup for the auth flow that I've been using. Oversight on my part, and it reminds me that this will need to be fixed prior to deployment.

@JHawk0224
Copy link
Member

This is amazing!
For the status code comments, I don't think they need to be changed, I just wanted to point them out.
On an overall note, I see when logging in that the ProgressView we had inserted over the login sheet is no longer there (action is still disabled though preventing stale web request - ty). Is it possible we can add it back? Ik it's Penn's fault for being slow, but otherwise there's no indication something is happening.
Also, when testing, I just logged in but it brought be to the Login page after a successful login. Then clicking again doesn't do anything, see below video. Not sure why it's happening, but should be fixed.
weird-behavior.mov
Note that I am not done testing, so I will be continuing to add comments, but I thought I would share this review for now.

my thought/initial guess, the client id you're using isn't setup for the auth flow that I've been using. Oversight on my part, and it reminds me that this will need to be fixed prior to deployment.

Ok I will test again when I get a chance.

@JHawk0224
Copy link
Member

JHawk0224 commented Apr 14, 2025

This is amazing!
For the status code comments, I don't think they need to be changed, I just wanted to point them out.
On an overall note, I see when logging in that the ProgressView we had inserted over the login sheet is no longer there (action is still disabled though preventing stale web request - ty). Is it possible we can add it back? Ik it's Penn's fault for being slow, but otherwise there's no indication something is happening.
Also, when testing, I just logged in but it brought be to the Login page after a successful login. Then clicking again doesn't do anything, see below video. Not sure why it's happening, but should be fixed.
weird-behavior.mov
Note that I am not done testing, so I will be continuing to add comments, but I thought I would share this review for now.

my thought/initial guess, the client id you're using isn't setup for the auth flow that I've been using. Oversight on my part, and it reminds me that this will need to be fixed prior to deployment.

Also re adding ProgressView for authentication with penn, it should be really easy. Basically just changing AuthWebView.swift in LabsPlatformSwift to use

ZStack {
    AuthWebViewRepresentable(url: url, redirect: redirect, isLoading: $isLoading, completion: callback)

    if isLoading {
        Color.black.opacity(0.1)
            .ignoresSafeArea()

        ProgressView()
            .tint(nil)
            .scaleEffect(1.6)
            .frame(width: 100, height: 100)
            .background(.thickMaterial)
            .cornerRadius(16)
    }
}

@jonathanmelitski
Copy link
Contributor Author

jonathanmelitski commented May 9, 2025

See PR in LabsPlatformSwift. Also I sent working clientID via Slack

@jonathanmelitski
Copy link
Contributor Author

if isLoading {
        Color.black.opacity(0.1)
            .ignoresSafeArea()

        ProgressView()
            .tint(nil)
            .scaleEffect(1.6)
            .frame(width: 100, height: 100)
            .background(.thickMaterial)
            .cornerRadius(16)
    }

This is oddly challenging, there's a SwiftUI safety/optimization where if the webview isn't the foremost view, it disables it updating.

@jonathanmelitski jonathanmelitski requested a review from JHawk0224 May 9, 2025 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants