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

Realm Web: Refresh access token #3020

Merged
merged 9 commits into from
Jul 10, 2020
Merged

Conversation

kraenhansen
Copy link
Member

What, How & Why?

This fixes the first part of #2965 (refreshing tokens upon the first 401 response.

Note: This should be rebased on v10 once the current base has been merged.

☑️ ToDos

  • 📝 Changelog entry
  • 📝 Compatibility label is updated or copied from previous entry
  • 🚦 Tests
  • 📝 Public documentation PR created or is not necessary
  • 💥 Breaking label has been applied or is not necessary

@kraenhansen kraenhansen requested review from kneth and steffenagger July 1, 2020 11:05
@kraenhansen kraenhansen self-assigned this Jul 1, 2020
@kraenhansen kraenhansen force-pushed the kh/web/refresh-access-token branch 2 times, most recently from dfbdae6 to 99cc607 Compare July 1, 2020 13:40
@kraenhansen kraenhansen force-pushed the kh/web/more-credentials branch from 6d12656 to 98e6ade Compare July 1, 2020 13:45
@kraenhansen kraenhansen force-pushed the kh/web/refresh-access-token branch from 99cc607 to 87ca3a9 Compare July 1, 2020 13:45
},
{ bar: "baz" },
]);
// Login with an aanonymous user
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Login with an aanonymous user
// Login with an anonymous user

this._accessToken = accessToken;
this.storage.accessToken = accessToken;
} else {
throw new Error("Expected a 'access_token' in the response");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
throw new Error("Expected a 'access_token' in the response");
throw new Error("Expected an 'access_token' in the response");

@@ -124,6 +88,8 @@ export class DefaultNetworkTransport implements NetworkTransport {
contentType.startsWith("application/json")
) {
throw new MongoDBRealmError(
request.method,
request.url,
response.status,
response.statusText,
await response.json(),
Copy link
Contributor

Choose a reason for hiding this comment

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

(void comment) Is this actually awaited before thrown? I'm more & more impressed with TS.

Copy link
Member Author

@kraenhansen kraenhansen Jul 2, 2020

Choose a reason for hiding this comment

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

Is what awaited? (or is your "void comment" because I forced pushed a change? 😺 )

const credentials = Credentials.anonymous();
await app.logIn(credentials, false);
// Send a request (which will fail)
try {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this try/catch could be simplified with chai-as-promised (not tested though):

expect(app.functions.foo({ bar: "baz" })).eventually.throws(MongoDBRealmError)
                .property("message", "Request failed (POST http://invalid): invalid session (status 401)");

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I agree, I did have a look at it when writing this. I didn't like the way they're loading in that plugin.

const { access_token: accessToken } = response;
if (typeof accessToken === "string") {
this._accessToken = accessToken;
this.storage.accessToken = accessToken;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why store it in 2 places? Is this._accessToken to allow continued use in different tabs?

Copy link
Member Author

Choose a reason for hiding this comment

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

this.storage persists it to local storage. It's very frequently used and I don't want to hit localStorage on every request to the server, so the user stores its token as a property as well, which is hydrated from the storage at its convenience.

user &&
retries === 0 &&
err instanceof MongoDBRealmError &&
err.statusCode === 401
Copy link
Contributor

Choose a reason for hiding this comment

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

Could there be a case where the endpoint returns 403 (forbidden) from an outdated token - if a user has been granted new/extended access, but the client has not updated its access token yet?
(I guess this depends on how much of the access rights are stored in the JWT).

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so ... it seems to me only 401 are returned when a refresh of the access token would resolve the issue.

fetchAndParse<RequestBody extends any, ResponseBody extends any>(
request: Request<RequestBody>,
): Promise<ResponseBody>;
fetchWithCallbacks<RequestBody extends any>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering why fetch with parsing is promise-based, and fetch without parsing (or limited parsing) is callback-based?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because when Realm JS is using this, its easier to consume from JS if a handler can be passed when invoked instead of having to call methods on the returned promise. Makes sense?

@kraenhansen kraenhansen force-pushed the kh/web/refresh-access-token branch from 87ca3a9 to 743b71d Compare July 6, 2020 12:58
@kraenhansen kraenhansen force-pushed the kh/web/more-credentials branch 2 times, most recently from 97777a8 to 65e2025 Compare July 8, 2020 08:53
@kraenhansen kraenhansen force-pushed the kh/web/refresh-access-token branch from 743b71d to 97731f8 Compare July 8, 2020 08:55
@kraenhansen
Copy link
Member Author

I just published realm-network-transport v0.6.0 off this branch.

@kraenhansen kraenhansen force-pushed the kh/web/more-credentials branch from 65e2025 to a9e52ca Compare July 10, 2020 13:44
Base automatically changed from kh/web/more-credentials to v10 July 10, 2020 14:03
@kraenhansen kraenhansen force-pushed the kh/web/refresh-access-token branch from abd3a52 to 0719c04 Compare July 10, 2020 14:13
@kraenhansen kraenhansen merged commit cc11c88 into v10 Jul 10, 2020
@kraenhansen kraenhansen deleted the kh/web/refresh-access-token branch July 10, 2020 14:34
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants