-
Notifications
You must be signed in to change notification settings - Fork 2
feat: add setRequestToken and fetchRequestToken methods
#900
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
susnux
wants to merge
3
commits into
main
Choose a base branch
from
feat/settoken
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
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
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
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,107 @@ | ||
| /** | ||
| * SPDX-FileCopyrightText: 2019 Nextcloud GmbH and Nextcloud contributors | ||
| * SPDX-License-Identifier: GPL-3.0-or-later | ||
| */ | ||
|
|
||
| import { emit, subscribe } from '@nextcloud/event-bus' | ||
| import { generateUrl } from '@nextcloud/router' | ||
|
|
||
| export interface CsrfTokenObserver { | ||
| (token: string): void | ||
| } | ||
|
|
||
| _subscribeToTokenUpdates() // TODO: remove once we drop support for Nextcloud 33.0.1 and before | ||
|
|
||
| /** | ||
| * Get current request token | ||
| * | ||
| * @return Current request token or null if not set | ||
| */ | ||
| export function getRequestToken(): string | null { | ||
| if (globalThis._nc_auth_requestToken) { | ||
| return globalThis._nc_auth_requestToken | ||
| } | ||
|
|
||
| if (globalThis.document) { | ||
| // for service workers or other contexts without DOM we need to safeguard this | ||
| return document.head.dataset.requesttoken ?? null | ||
| } | ||
| return null | ||
| } | ||
|
|
||
| /** | ||
| * Set a new CSRF token (e.g. because of session refresh). | ||
| * This also emits an event bus event for the updated token. | ||
| * | ||
| * @param token - The new token | ||
| * @fires Error - If the passed token is not a potential valid token | ||
| */ | ||
| export function setRequestToken(token: string): void { | ||
| if (!token || typeof token !== 'string') { | ||
| throw new Error('Invalid CSRF token given', { cause: { token } }) | ||
| } | ||
|
|
||
| if (globalThis._nc_auth_requestToken === token) { | ||
| // token is the same as before, no need to update and especially no need to notify the observers | ||
| return | ||
| } | ||
|
|
||
| globalThis._nc_auth_requestToken = token | ||
| if (globalThis.document) { | ||
| // For DOM environments we also set the token to the DOM, so it is available for legacy code | ||
| document.head.dataset.requesttoken = token | ||
| } | ||
|
|
||
| emit('csrf-token-update', { token, _internal: true }) | ||
| } | ||
|
|
||
| /** | ||
| * Fetch the request token from the API. | ||
| * This does also set it on the current context, see `setRequestToken`. | ||
| * | ||
| * @fires Error - If the request failed | ||
| */ | ||
| export async function fetchRequestToken(): Promise<string> { | ||
| const url = generateUrl('/csrftoken') | ||
|
|
||
| const response = await fetch(url) | ||
| if (!response.ok) { | ||
| throw new Error('Could not fetch CSRF token from API', { cause: response }) | ||
| } | ||
|
|
||
| const { token } = await response.json() | ||
| setRequestToken(token) | ||
| return token | ||
| } | ||
|
|
||
| /** | ||
| * Add an observer which is called when the CSRF token changes | ||
| * | ||
| * @param observer The observer | ||
| */ | ||
| export function onRequestTokenUpdate(observer: CsrfTokenObserver): void { | ||
| subscribe('csrf-token-update', async ({ token }) => { | ||
| try { | ||
| observer(token) | ||
| } catch (error) { | ||
| // we cannot use the logger as the logger uses this library = circular dependency | ||
| // eslint-disable-next-line no-console | ||
| console.error('Error updating CSRF token observer', error) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| /** | ||
| * Subscribe to token update events from server. | ||
| * | ||
| * @todo - This is legacy and not needed once all supported server versions use `setRequestToken` of this library. | ||
| */ | ||
| function _subscribeToTokenUpdates(): void { | ||
| // Listen to server event and keep token in sync | ||
| subscribe('csrf-token-update', ({ token, _internal }) => { | ||
| if (!_internal) { | ||
| // Only update the token if the event is not emitted from this library, otherwise we would end in a loop | ||
| setRequestToken(token) | ||
| } | ||
| }) | ||
| } | ||
This file was deleted.
Oops, something went wrong.
Oops, something went wrong.
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.
In the previous implementation it only used DOM API once to initiate the local state. Now every
getRequestTokencall gets it via DOM API.What about checking
_nc_auth_requestTokenfirst and only as a fallback get it from the document to init the local state?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.
Then we still have a problem if updated outside of this library (aka old server versions).
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 mean we can do so and hope to update server in time - but still wonder how much overhead the DOM API call would be here)
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.
What is the problem?
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.
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.
Doesn't
setTokenon server emit the event even on a very old server?nextcloud/server#17404
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.
Ah yes seems 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.
let me adjust this
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 adjusted the implementation to use the cached version if possible.
Also prevented an event loop
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.
(see fixup commit)