-
Notifications
You must be signed in to change notification settings - Fork 18
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
feat: Add ability to set a shareId #267
Conversation
@amccloud this is still a WIP but I wanted to get your opinion on this approach before I finish things up with tests and documentation (and having to change some things after #266 is merged). Edit: I just came across SDK-386, which looks identical to what I'm trying to do here 😅 I can change |
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.
@anthony-j-castro yup, this is another version of https://goabstract.atlassian.net/browse/SDK-386
I think we should just update Client
to support both authentication options and update Endpoint to send both when it has both
@amccloud is this on the right track? I removed the ability for |
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.
@anthony-j-castro lgtm 👍
You can create a tagged pre-release to start to test this PR with UI (or possibly just link locally)
const tokenHeader = | ||
typeof token === "string" | ||
? { Authorization: `Bearer ${token}` } | ||
: { "Abstract-Share-Id": token && inferShareId(token) }; |
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.
We're removing the ability to pass a share ID as an accessToken
. Technically this would be a breaking change, but it's not documented for the public and I'm going to update our only usage in ui after a prerelease is cut.
@amccloud I've been testing this locally using a linked version and it's working well 👍 |
Co-authored-by: Andrew McCloud <[email protected]>
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 good to me 👍
Just two last things before this is ready:
- Don't forget to update docs
- And one suggestion below
### `shareId` | ||
|
||
This option can be used to pass a share identifier (`string`, [`ShareDescriptor`](/docs/abstract-api/#sharedescriptor), or [`ShareUrlDescriptor`](/docs/abstract-api/#shareurldescriptor)) to access objects associated with a public [share](/docs/abstract-api/#shares) without having to authenticate with an `accessToken`. | ||
|
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.
@amccloud here's my first attempt at documenting. Suggestions are welcome 🙃
We recently ran into an issue where some requests on private share link pages would fail because we were only allowing a single
accessToken
. For these share links,accessToken
was being set to the share ID, and the authentication header would never get configured.This PR adds a separate
shareId
option that can be set in addition to theaccessToken
, allowing both headers to exist on a request.