Gravatar OAuth in SwiftUI #359

etoledom · 2024-08-21T09:24:10Z

Description

This PR implements the Gravatar OAuth flow.

Testing Steps

Build and run the SwiftUI demo app
Go to Profile Editor with oauth demo screen.
insert an email and open the editor.
- Check that OAuth flow works as expected.

etoledom · 2024-08-21T09:34:51Z

Sources/GravatarUI/SwiftUI/OAuthSession/OAuthSession.swift

+enum OAuthError: Error {
+    case notConfigured
+    case couldNotCreateOAuthURLWithGivenSecrets
+    case couldNotGetCodeFromURL(String)
+    case oauthResponseError(String)
+    case unknown(Error)
+    case couldNotStoreToken(Error)
+}


This became a bit messy and I don't like it:

notConfigured: Secrets are not set up. This maybe could be a assertionFailure instead of throwing? The idea is to warn the developer.

couldNotCreateOAuthURLWithGivenSecrets this comes from using Encodable to add the query items to the URL. Probably if we add them manually we wouldn't need this error. And if this error happens, it's probably a developer mistake which should be catch on dev time (maybe assertion also?)

case couldNotGetCodeFromURL(String) error on parsing the callback URL to get the code. This could be a runtime error but not sure how could this be handled 🤔 .

oauthResponseError(String) Something went wrong at the backend. Needs to be handled in the UI.

unknown ¯_(ツ)_/¯ ... Probably on the UI also.

couldNotStoreToken(Error) Keychain error. I got errors here because the keychan was wrongly set up. But it was a dev issue. Probably there are instances that this could be a user issue, but again, not sure how this should be handled. Technically we have the token and the user should be able to edit the profile, so I don't think that throwing is a good option. The problem is that they will need to be authenticated again next time.

I would add the assertionFailures for notConfigured and couldNotCreateOAuthURLWithGivenSecrets but not necessarily replace the "throw" statements. We still need the throws for prod. Throws also allow us return non-optional values.

couldNotStoreToken(Error) Keychain error. I got errors here because the keychan was wrongly set up. But it was a dev issue. Probably there are instances that this could be a user issue, but again, not sure how this should be handled. Technically we have the token and the user should be able to edit the profile, so I don't think that throwing is a good option. The problem is that they will need to be authenticated again next time

As you said, since the user is able to continue I think we don't need to re-throw this one. This can be something we swallow inside a separate do { ... } catch {...} and handle just with an assertionFailure.

pinarol

Looks pretty good overall. Just dropping some non-critical comments and a question.

We need UI for this 2 case but it doesn't have to be in this PR.

we don’t have the token yet but we’ll try (we need an activity indicating UI)
we tried to get the token but we failed (we can reuse the ContentLoadingErrorView here as well)

Sources/GravatarUI/SwiftUI/OAuthSession/Keychain.swift

Sources/GravatarUI/SwiftUI/OAuthSession/OAuthSession.swift

pinarol · 2024-08-21T14:14:32Z

Sources/GravatarUI/SwiftUI/OAuthSession/OAuthSession.swift

+enum OAuthError: Error {
+    case notConfigured
+    case couldNotCreateOAuthURLWithGivenSecrets
+    case couldNotGetCodeFromURL(String)
+    case oauthResponseError(String)
+    case unknown(Error)
+    case couldNotStoreToken(Error)
+}


I would add the assertionFailures for notConfigured and couldNotCreateOAuthURLWithGivenSecrets but not necessarily replace the "throw" statements. We still need the throws for prod. Throws also allow us return non-optional values.

Sources/GravatarUI/SwiftUI/OAuthSession/OAuthSession.swift

pinarol · 2024-08-21T14:32:20Z

Sources/GravatarUI/SwiftUI/OAuthSession/OAuthSession.swift

+        case let error as Keychain.KeychainError:
+            return .couldNotStoreToken(error)
+        default:
+            print("🛑 Error: \(type(of: error)) - \(error.localizedDescription)")


let's remove the "print" call. We can probably use the Apple’s os.log framework for logging, doesn't have to be in this PR though. 🤔

Sure, this was for debugging and see the types of the error thrown to be catch 👍
os.log is a good idea. It might help developers debug issues on their users.

Sources/GravatarUI/SwiftUI/OAuthSession/OAuthSession.swift

pinarol · 2024-08-21T14:44:48Z

Sources/GravatarUI/SwiftUI/OAuthSession/OAuthSession.swift

+
+extension OAuthError {
+    static func from(error: Error) -> OAuthError {
+        switch error {


We can catch "DecodingError"s and re-throw them after adding an assertionFailure maybe? This means our decoding keys or types are not matching the response, so the developer should fix it.

case let error as DecodingError: assertionFailure("Unable to decode the response. Error: \(error.description)") throw .decodingError(error)

Sources/GravatarUI/SwiftUI/AvatarPicker/AvatarPickerView.swift

pinarol · 2024-08-21T14:51:14Z

Sources/GravatarUI/SwiftUI/ProfileEditor/ProfileEditor.swift

+                case .avatarPicker:
+                    AvatarPickerView(model: .init(email: email, authToken: token))
+                }
+                Button("Log out (for testing only)") {


This is meant to be a temp Button I guess but I feel like it can be useful during development in the long run as well. So how about we wrap it with some kind of compiler flag and keep it?

#if DEMO #endif

Sounds good!

@pinarol - I tried this and the Build Conditions from the demo app won't be passed to the package, so there's no access to it from within the SDK.

I decided to move this button to the demo screen, previous to present the picker.

Now we will have a Log out button when we detect a session:

I see. That works too. 👍

wpmobilebot · 2024-08-22T12:55:18Z

📲 You can test the changes from this Pull Request in Gravatar UIKit Prototype Build by scanning the QR code below to install the corresponding build.

	App Name	Gravatar UIKit Prototype Build
	Build Number	1126
	Version	1.0
	Bundle ID	com.automattic.gravatar-sdk-demo-uikit.prototype-build
	Commit	`e3164d5`
	App Center Build	Gravatar SDK Demo - UIKit #57

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

wpmobilebot · 2024-08-22T12:55:25Z

📲 You can test the changes from this Pull Request in Gravatar SwiftUI Prototype Build by scanning the QR code below to install the corresponding build.

	App Name	Gravatar SwiftUI Prototype Build
	Build Number	1126
	Version	1.0
	Bundle ID	com.automattic.gravatar-sdk-demo-swiftui.prototype-build
	Commit	`e3164d5`
	App Center Build	Gravatar SDK Demo - SwiftUI #56

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

etoledom · 2024-08-22T12:59:52Z

Sources/GravatarUI/SwiftUI/ProfileEditor/ProfileEditor.swift

+        Task {
+            isAuthenticating = true
+            if !oauthSession.hasSession(with: email) {
+                _ = try? await oauthSession.retrieveAccessToken(with: email)


The possible errors are still not yet properly handled here, but we can do this alongside building the error screens to present these errors.

pinarol · 2024-08-22T13:01:16Z

Something caught my attention, I see the diff for my secrets file:

Seems like the path doesn't match the paths in the .gitignore

etoledom · 2024-08-22T13:01:42Z

@pinarol - I believe this is ready for another look 👀

etoledom · 2024-08-22T13:03:35Z

Something caught my attention, I see the diff for my secrets file:
Seems like the path doesn't match the paths in the `.gitignore`

You can delete this file.

This was an error when we renamed the demo apps. We left this file which is not supposed to be in source control inside the old folder name. Now it shows because I removed its entry from .gitignore.

You get this since this file still exist in your machine, but it's safe to be deleted 👍

pinarol

Looks good, just leaving some nits. I don't need to check again.

Demo/Demo/Gravatar-SwiftUI-Demo/DemoProfileEditorViewView.swift

Sources/GravatarUI/SwiftUI/OAuthSession/Keychain.swift

Sources/GravatarUI/SwiftUI/View+Additions.swift

pinarol · 2024-08-22T14:47:28Z

You get this since this file still exist in your machine, but it's safe to be deleted 👍

Oh I see. Cool.

etoledom added 4 commits August 20, 2024 16:48

Update .gitignore

642e9df

Delete Secrets.swift

9d6fd01

Update .gitignore

0ee8413

Adding Profile Editor view with oauth

b937229

etoledom commented Aug 21, 2024

View reviewed changes

pinarol reviewed Aug 21, 2024

View reviewed changes

etoledom added 6 commits August 22, 2024 12:02

Renaming SecureStorage password to secret

6e010fe

Showing loading indicator while asking for authorisation

8148366

Format

628f94f

Fix tests build

2dde0fd

Adding assertions

04d7882

Merge branch 'trunk' into etoledom/swiftui-oauth

5b0c93b

etoledom marked this pull request as ready for review August 22, 2024 12:48

etoledom requested a review from pinarol August 22, 2024 12:57

etoledom commented Aug 22, 2024

View reviewed changes

pinarol approved these changes Aug 22, 2024

View reviewed changes

Demo/Demo/Gravatar-SwiftUI-Demo/DemoProfileEditorViewView.swift Outdated Show resolved Hide resolved

Sources/GravatarUI/SwiftUI/OAuthSession/Keychain.swift Outdated Show resolved Hide resolved

Sources/GravatarUI/SwiftUI/View+Additions.swift Outdated Show resolved Hide resolved

etoledom added 3 commits August 23, 2024 09:18

Small review requests

2d4463c

Merge branch 'trunk' into etoledom/swiftui-oauth

30553a5

Add isPresented property to ProfileEditor

e3164d5

etoledom merged commit 33d1283 into trunk Aug 23, 2024
9 checks passed

etoledom deleted the etoledom/swiftui-oauth branch August 23, 2024 08:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gravatar OAuth in SwiftUI #359

Gravatar OAuth in SwiftUI #359

etoledom commented Aug 21, 2024 •

edited

Loading

etoledom Aug 21, 2024

pinarol Aug 21, 2024

pinarol Aug 21, 2024

pinarol left a comment

pinarol Aug 21, 2024

pinarol Aug 21, 2024

etoledom Aug 21, 2024

pinarol Aug 21, 2024

pinarol Aug 21, 2024

etoledom Aug 21, 2024

etoledom Aug 22, 2024

pinarol Aug 22, 2024

wpmobilebot commented Aug 22, 2024 •

edited

Loading

wpmobilebot commented Aug 22, 2024 •

edited

Loading

etoledom Aug 22, 2024

pinarol commented Aug 22, 2024

etoledom commented Aug 22, 2024

etoledom commented Aug 22, 2024 •

edited

Loading

pinarol left a comment

pinarol commented Aug 22, 2024

Gravatar OAuth in SwiftUI #359

Gravatar OAuth in SwiftUI #359

Conversation

etoledom commented Aug 21, 2024 • edited Loading

Description

Testing Steps

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pinarol 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

wpmobilebot commented Aug 22, 2024 • edited Loading

wpmobilebot commented Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

pinarol commented Aug 22, 2024

etoledom commented Aug 22, 2024

etoledom commented Aug 22, 2024 • edited Loading

pinarol left a comment

Choose a reason for hiding this comment

pinarol commented Aug 22, 2024

etoledom commented Aug 21, 2024 •

edited

Loading

wpmobilebot commented Aug 22, 2024 •

edited

Loading

wpmobilebot commented Aug 22, 2024 •

edited

Loading

etoledom commented Aug 22, 2024 •

edited

Loading