-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add prompt to authenticate with application passwords #23726
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
Conversation
|
|
||
| if #available(iOS 16.4, *) { | ||
| scrollView.scrollBounceBehavior(.basedOnSize, axes: [.vertical]) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jkmassel I didn't use the AccessibilityScrollView in your PR, because dynamic size seems working fine without it. Let me know if I missed anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a problem – we have a lot more room to work with here
|
| App Name | WordPress Alpha |
|
| Configuration | Release-Alpha | |
| Build Number | pr23726-97a28ad | |
| Version | 25.4.2 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | 97a28ad | |
| App Center Build | WPiOS - One-Offs #10977 |
|
| App Name | Jetpack Alpha |
|
| Configuration | Release-Alpha | |
| Build Number | pr23726-97a28ad | |
| Version | 25.4.2 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | 97a28ad | |
| App Center Build | jetpack-installable-builds #10017 |
| } | ||
| } | ||
|
|
||
| #Preview { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you get it to work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope 🤦♂️
| @MainActor | ||
| private func migrate() async { | ||
| guard let url = try? blog.getUrlString() else { | ||
| let error = NSLocalizedString("applicationPasswordMigration.error.siteUrlNotFound", value: "Cannot find the current site's url", comment: "Error message when the current site's url cannot be found") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(nit) Move to the bottom to Strings (applies to all other instances too)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 53aaea9
| } | ||
|
|
||
| static func description(localizedFeatureName: String) -> String { | ||
| let format = NSLocalizedString("applicationPasswordRequired.description", value: "Application passwords are a more secure way to connect to your self-hosted site, and enable support for features like %@.", comment: "Description for the prompt to upgrade to Application Passwords. The first argument is the name of the feature that requires Application Passwords.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"are a more secure way" – as opposed to what? I'd probably leave it out as it raises questions about the alternative options. I don't think it's inherently more secure than cookies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's compared to what the app currently uses to authenticate: xmlrpc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep that's right – it's way more secure than XMLRPC
Modules/Package.swift
Outdated
| "WordPressFlux", | ||
| "WordPressShared", | ||
| "WordPressUI", | ||
| "WordPressAppUI", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest adding a new auth-related module for these new screens like we had with WordPressAuthentificator as opposed to a broad "AppUI". For shared components, we already have WordPressUI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed in ce8d2f8.
ce8d2f8 to
97a28ad
Compare
|
⬆️ Rebased without code changes. |


Note
This PR is built on top of #23715.
The main change in this PR is taken from #23572, which is opening up "Application Passwords" entry (and upcoming "Users") to all self-hosted sites, with a prompt (
RestApiUpgradePrompt) for application password authentication if the site is not authenticated with application password.Here is a walkthrough video:
Simulator.Screen.Recording.-.iPhone.16.-.2024-10-30.at.21.22.21.mp4
Regression Notes
Potential unintended areas of impact
What I did to test those areas of impact (or what existing automated tests I relied on)
What automated tests I added (or what prevented me from doing so)
PR submission checklist:
RELEASE-NOTES.txtif necessary.Testing checklist: