Skip to content

Making it possible to run integration tests against any Firebase project #22

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 2 commits into from
May 11, 2017

Conversation

hiranya911
Copy link
Contributor

Currently integration tests are run against a specific Firebase project (admin-sdks-integration). This enables running integration tests against any Firebase project. This will allow external developers to run the test suite against their own projects/credentials.

See issue: #21

Copy link
Contributor

@bojeil-google bojeil-google left a comment

Choose a reason for hiding this comment

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

Looks good. Just a couple of minor stuff.

}

var apiKey = process.argv[2];
if (apiKey == undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

typeof apiKey === 'undefined'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

return serviceAccount.project_id;
}

function getApiKey() {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add javadoc for this function like you did for the ones above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link

@jshcrowthe jshcrowthe left a comment

Choose a reason for hiding this comment

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

Changes seem pretty straightforward. I'd address @bojeil-google's comments then this LGTM.

On a side note, with the hardcoded API key here being removed, is there any concern over that project being discovered and used maliciously?

Copy link
Contributor

@bojeil-google bojeil-google left a comment

Choose a reason for hiding this comment

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

They would need to get hold of the service account credentials which are not exposed here and can only be obtained via the Firebase Console.

@hiranya911
Copy link
Contributor Author

Thanks for the reviews. I've made the suggested changes. As @bojeil-google mentioned, it seems there's no risk in exposing the API key. See http://stackoverflow.com/questions/37482366/is-it-safe-to-expose-firebase-apikey-to-the-public for more details.

@hiranya911 hiranya911 merged commit 297f935 into master May 11, 2017
@hiranya911 hiranya911 deleted the hkj-intergration-tests branch May 11, 2017 17:09
lahirumaramba added a commit that referenced this pull request May 3, 2021
* Implement the App Check API
lahirumaramba added a commit that referenced this pull request May 6, 2021
* Refactor CryptoSigner (#21)

* Refactor Crypto Signer

* Introduce new CryptoSignerError type

* reorder imports

* PR fixes

* PR clean up

* feat(fac): Implement the App Check API (#22)

* Implement the App Check API

* Add AppCheck public API (#23)

* Add AppCheck public API

* Add AppCheck public api unit tests

* Add FAC Verify Token API (#26)

* (feat): Add FAC Verify token API

- Re-try with all the keys when kid is not present in the token header.
- Add JWKS key fetcher
- Add public API for FAC verify token

* Add ref docs and unit tests

* PR fixes

* Update api extractor report

* Add more tests for token-verifier

* export jwks key pairs

* PR fixes

* More PR fixes

* Update src/app-check/index.ts

Co-authored-by: Kevin Cheung <[email protected]>

Co-authored-by: Kevin Cheung <[email protected]>

* Add App ID

Co-authored-by: Kevin Cheung <[email protected]>
lahirumaramba added a commit that referenced this pull request May 10, 2021
* Refactor CryptoSigner (#21)

* Refactor Crypto Signer

* Introduce new CryptoSignerError type

* reorder imports

* PR fixes

* PR clean up

* feat(fac): Implement the App Check API (#22)

* Implement the App Check API

* Add AppCheck public API (#23)

* Add AppCheck public API

* Add AppCheck public api unit tests

* Add FAC Verify Token API (#26)

* (feat): Add FAC Verify token API

- Re-try with all the keys when kid is not present in the token header.
- Add JWKS key fetcher
- Add public API for FAC verify token

* Add ref docs and unit tests

* PR fixes

* Update api extractor report

* Add more tests for token-verifier

* export jwks key pairs

* PR fixes

* More PR fixes

* Update src/app-check/index.ts

Co-authored-by: Kevin Cheung <[email protected]>

Co-authored-by: Kevin Cheung <[email protected]>

* Add App ID

Co-authored-by: Kevin Cheung <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants