-
Notifications
You must be signed in to change notification settings - Fork 390
fix(auth): Use 'owner' token when communicating with Auth emulator #1085
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
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 a simpler implementation like the following:
- Change the
AuthorizedHttpClient
so child classes can override how it fetches tokens:
export class AuthorizedHttpClient extends HttpClient {
constructor(private readonly app: FirebaseApp) {
super();
}
protected getToken(): Promise<string> {
return this.app.INTERNAL.getToken().then((result) => {
return result.accessToken;
});
}
}
- Then in the Auth module extend the above class, and override
getToken()
:
class AuthHttpClient extends AuthorizedHttpClient {
constructor(app: FirebaseApp, private readonly useEmulator: boolean) {
super(app);
}
protected getToken(): Promise<string> {
if (this.useEmulator) {
return Promise.resolve('owner');
}
return super.getToken();
}
}
export abstract class AbstractAuthRequestHandler {
constructor(protected readonly app: FirebaseApp, useEmulator: boolean) {
this.httpClient = new AuthHttpClient(app, useEmulator);
}
}
@hiranya911 thanks for the comments! You're right that's much cleaner. |
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.
LGTM with a suggestion.
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.
Actually, can I ask you to add some test cases for this change?
Hiranya and Bassam asked for the same things
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.
Thank! LGTM with a couple of nits.
I know this was merged very recently, but when can we expect to see this in a tagged release? |
@nVitius we don't have a strict release cadence but if you look at the past we've released about once a month or so: This change will be included in the next release. If you want to use it right now you can always clone the |
@nVitius well you got lucky ... the |
* Use 'owner' token when communicating with Auth emulator * Unused import * Single quote * Simplify to address code review * Further simplify * Reduce diff * Stray comma * Remove circular import, add unit test * Final review feedback
Hey there! So you want to contribute to a Firebase SDK?
Before you file this pull request, please read these guidelines:
Discussion
Fixes #1077
Testing
Wrote a simple
test.js
script:When executed as normal:
When executed in the emulators:
API Changes
N/A