Skip to content
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

Authenticator - Various revisions #688

Merged
merged 14 commits into from
Apr 26, 2024
Merged

Conversation

dfeinzimer
Copy link
Collaborator

@dfeinzimer dfeinzimer commented Apr 16, 2024

Resolves various issues found during review of #392

@dfeinzimer dfeinzimer added the enhancement New feature or request label Apr 16, 2024
@dfeinzimer dfeinzimer self-assigned this Apr 16, 2024
@@ -48,9 +38,9 @@ struct AuthenticationApp: App {
// - Integrated Windows Authentication (IWA)
// - Client Certificate (PKI)
.authenticator(authenticator)
.environmentObject(authenticator)
Copy link
Collaborator Author

@dfeinzimer dfeinzimer Apr 16, 2024

Choose a reason for hiding this comment

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

While considering this feedback I noticed that this environment object appears to go unused in the example.

Without a good use for the suggested environment value it seems like it might be better to just remove it altogether.

@@ -18,19 +18,9 @@ import ArcGIS

@main
struct AuthenticationApp: App {
@ObservedObject var authenticator: Authenticator
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

AuthenticationApp owns the Authenticator instance so this should be a StateObject as noted here.

)
// Sets authenticator as ArcGIS and Network challenge handlers to handle authentication
// challenges.
ArcGISEnvironment.authenticationManager.handleChallenges(using: authenticator)
Copy link
Collaborator Author

@dfeinzimer dfeinzimer Apr 16, 2024

Choose a reason for hiding this comment

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

With authenticator as a StateObject, this causes a purple runtime warning. As a result, I've moved this call to handleChallenges into the task below.

Comment on lines -16 to -18
Group {
HomeView()
}
Copy link
Collaborator Author

@dfeinzimer dfeinzimer Apr 16, 2024

Choose a reason for hiding this comment

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

Removes the unneeded Group in response to this feedback

@dfeinzimer dfeinzimer marked this pull request as ready for review April 16, 2024 18:49
@dfeinzimer dfeinzimer marked this pull request as draft April 16, 2024 19:07
@@ -73,7 +66,7 @@ struct AuthenticationApp: App {
// // The scheme of the redirect URL is also specified in the Info.plist file.
// redirectURL: URL(string: "authexample://auth")!
// )
//}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Uncommenting the extension but leaving the property within commented prevents us from violating the swiftlint comment_spacing rule.

@dfeinzimer dfeinzimer marked this pull request as ready for review April 16, 2024 21:29
@dfeinzimer dfeinzimer merged commit bd111c1 into v.next Apr 26, 2024
@dfeinzimer dfeinzimer deleted the df/authenticatorRevisions branch April 26, 2024 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants