-
Notifications
You must be signed in to change notification settings - Fork 3
Cookies+nonce authentication #994
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
Merged
Merged
Changes from 3 commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
e367225
Implement getting REST nonce using account credentials
crazytonyli bc51ec1
Add Swift wrapper for cookies+nonce authentication
crazytonyli b9ebf4c
Add comments about WpAuthentication::Nonce
crazytonyli a5f4fb2
Assert the URLSession instance stores cookies
crazytonyli b8c153f
Try username & password authentication on any nonce errors
crazytonyli b834b53
Merge branch 'trunk' into cookies-nonce-authentication
crazytonyli b1942b7
Merge the Swift docker env into the existing wordpress image
crazytonyli 0154c20
Implement using username & password to make API calls
crazytonyli c666747
Less console output during building docker image
crazytonyli 1ebeeac
Update the installing swift script
crazytonyli 8155b69
Move Swift Linux test job to the end-to-end test group
crazytonyli c98be88
Fix swiftlint issues
crazytonyli 1ce4997
Default to test against WordPress v6.8.1
crazytonyli File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
48 changes: 48 additions & 0 deletions
48
native/swift/Tests/integration-tests/NonceAuthenticationTests.swift
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| import Foundation | ||
| import WordPressAPI | ||
| import WordPressAPIInternal | ||
| import Testing | ||
|
|
||
| @Suite | ||
| struct NonceAuthenticationTests { | ||
|
|
||
| @Test | ||
| func success() async throws { | ||
| let credentials = TestCredentials.instance() | ||
| let client = WordPressLoginClient(urlSession: .init(configuration: .ephemeral)) | ||
| let details = try await client.details(ofSite: credentials.siteUrl) | ||
| let api = try await client.authenticateTemporarily( | ||
| username: credentials.adminUsername, | ||
| password: credentials.adminAccountPassword, | ||
| details: details | ||
| ) | ||
| let loggedIn = try await api.users.retrieveMeWithEditContext().data.username | ||
| #expect(loggedIn == credentials.adminUsername) | ||
| } | ||
|
|
||
| @Test | ||
| func signInWithADifferentUser() async throws { | ||
| let credentials = TestCredentials.instance() | ||
| let client = WordPressLoginClient(urlSession: .init(configuration: .ephemeral)) | ||
| let details = try await client.details(ofSite: credentials.siteUrl) | ||
|
|
||
| // Given the URLSession is already signed in with the admin account. | ||
| let api = try await client.authenticateTemporarily( | ||
| username: credentials.adminUsername, | ||
| password: credentials.adminAccountPassword, | ||
| details: details | ||
| ) | ||
| let loggedIn = try await api.users.retrieveMeWithEditContext().data.username | ||
| #expect(loggedIn == credentials.adminUsername) | ||
|
|
||
| // When sign in with another account, an error should be returned. | ||
| await #expect(throws: NonceRetrievalError.AlreadyLoggedIn(username: credentials.adminUsername)) { | ||
| let _ = try await client.authenticateTemporarily( | ||
| username: credentials.authorUsername, | ||
| password: credentials.authorPassword, | ||
| details: details | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,13 +41,14 @@ echo "--- :wordpress: Setting up WordPress" | |
| wp core version --extra | ||
| wp --info | ||
|
|
||
| ADMIN_ACCOUNT_PASSWORD="strongpassword" | ||
| ## Install WordPress | ||
| wp core install \ | ||
| --url=localhost \ | ||
| --title=my-test-site \ | ||
| [email protected] \ | ||
| [email protected] \ | ||
| --admin_password=strongpassword \ | ||
| --admin_password="$ADMIN_ACCOUNT_PASSWORD" \ | ||
| --skip-email | ||
|
|
||
| ## Ensure URLs work as expected | ||
|
|
@@ -279,6 +280,7 @@ create_test_credentials () { | |
| admin_username="$ADMIN_USERNAME" \ | ||
| admin_password="$ADMIN_PASSWORD" \ | ||
| admin_password_uuid="$ADMIN_PASSWORD_UUID" \ | ||
| admin_account_password="$ADMIN_ACCOUNT_PASSWORD" \ | ||
| subscriber_username="$SUBSCRIBER_USERNAME" \ | ||
| subscriber_password="$SUBSCRIBER_PASSWORD" \ | ||
| subscriber_password_uuid="$SUBSCRIBER_PASSWORD_UUID" \ | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
WDYT about using
username/passwordhere and automatically getting thenonceprior to each request?We should still document that this is bad for performance, but as long as the username/password don't change the client won't become invalid unexpectedly which seems slightly better?
I don't feel strongly about this either way.
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.
That's how the original PR #327 implements it. It got pretty complicated because we need to refresh the nonce internally, and automatically retry the original API request after refreshing.
Considering the use case right now is using username and password to make one request, rather than all requests on the site, I thought it's better to keep it simple.
Uh oh!
There was an error while loading. Please reload this page.
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 was thinking we'd avoid the complexity by just assuming that the nonce is never valid and refreshing it prior to each request. That makes it simple but more expensive to use.
Still works great for our "we only plan to ever make one request" purpose, but ensures that if the client somehow persists for a while it doesn't "go bad" – it'll always 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.
I have implemented it in a different way in 0154c20. The implementation takes advantage of the existing auth mechanism, using the
DynamicAuthenticationProvidertrait. See the newtest_cookies_noncefunction.The implementation should just work if the nonce has expired. It should refresh the nonce and retry the request.
To summarize: We can now use the account username and password to create a
WpApiClient(backed byCookiesNonceAuthenticationProvider), which uses cookies+nonce authentication method. The API client instance is not affected by the nonce expiration time, which means it can be kept around for a long time, like your regularWpApiClientwith an application password. It stores the nonce internally and refreshes it when it's detected to have expired.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.
Looks cool – nothing else required to use it from Swift? Android should be ok
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 don't think there is anything extra needed for Swift. The
WordPressAPIalready has public APIs that can acceptCookiesNonceAuthenticationProvider. We can probably add a convenience initialiser toWordPressAPIto make the API more ergonomic.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'll approve the PR to unblock, feel free to add this now or later :)
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 plan to use the
authenticateTemporarilyadded in this PR. So, I'll merge the PR as it is. When we need to frequently use the account username & password to call the REST API from the apps, I'll see what kind of helpers are needed.