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.

Sign up for GitHub

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

Jump to bottom

Firebase Shell Plugin #331

Closed
wants to merge 2 commits into from
Closed

Firebase Shell Plugin #331

wants to merge 2 commits into from

Conversation

MukeshKumarBagaria
Copy link

@MukeshKumarBagaria MukeshKumarBagaria commented Jun 29, 2023 •
edited
Loading

Overview

Added support for Firebase Shell plugin to authenticate using 1Password.

Type of change

  • Created a new plugin
  • Improved an existing plugin
  • Fixed a bug in an existing plugin
  • Improved contributor utilities or experience

How To Test

To test the changes:

  1. Install the Firebase CLI on a machine with a browser.
  2. Run the command firebase login:ci to start the signin process.
  3. Visit the provided URL and log in using a Google account.
  4. Print the new refresh token.
  5. Store the output token securely in your CI system.
  6. Run Firebase CLI commands using the token with firebase <command> --token <token>.

Changelog

Authenticate Firebase CLI using 1Password for secure token storage.

Additional information

@MukeshKumarBagaria
Copy link
Author

This PR is in Progress It is not ready for review I'm working on it

@techcraver
Copy link
Contributor

Hello! Don't forget to also write an accompanying blog post on Hashnode with the tags 1Password and BuildWith1Password (not just putting #1Password in the text - but use the Hashnode tags in the CMS. :)

The full instructions are here.

@arunsathiya arunsathiya added waiting-on-reviewer signals that a certain PR is waiting for a review from a 1Password team member hashnode hackathon Ideas and inspiration for the hackathon running from June 1st - June 30th in-progress this PR is being worked on/comments are in the process of being addressed by the contributor and removed waiting-on-reviewer signals that a certain PR is waiting for a review from a 1Password team member labels Jun 29, 2023
@MukeshKumarBagaria
Copy link
Author

The blog link -https://mukeshbagaria.hashnode.dev/secured-firebase-cli-authentication-a-guide-to-protecting-your-firebase-cli-with-1password-shell-plugin

@MukeshKumarBagaria
Copy link
Author

I'm done with this plugin can you review it?

@arunsathiya arunsathiya added waiting-on-reviewer signals that a certain PR is waiting for a review from a 1Password team member and removed in-progress this PR is being worked on/comments are in the process of being addressed by the contributor labels Jun 30, 2023
@arunsathiya
Copy link
Contributor

Thanks for the heads up, @MukeshKumarBagaria. We'll get on this soon!

@hculea hculea requested a review from arunsathiya July 5, 2023 07:33
arunsathiya
arunsathiya suggested changes Jul 10, 2023
View reviewed changes
Copy link
Contributor

@arunsathiya arunsathiya left a comment

Choose a reason for hiding this comment

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

Thank you for your submission, @MukeshKumarBagaria. There are a couple of things to address before we can move forward, including writing the config file importer and the corresponding test function. Please give it a try and let us know if you have any questions along the way! 😄

plugins/firebase/access_token.go
return schema.CredentialType{
Name: credname.AccessToken,
DocsURL: sdk.URL("https://firebase.google.com/docs/cli#cli-ci-systems"),
ManagementURL: sdk.URL("https://firebase.google.com/docs/cli#cli-ci-systems"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove the ManagementURL because that URL is printed by the firebase login:ci command. If it's a standard link and if you are familiar with it, let's include that.

plugins/firebase/access_token.go
Secret: true,
Composition: &schema.ValueComposition{
Length: 53,
Prefix: "dummy_firebase_", // TODO: Check if this is correct
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is correct. This field is referring to the prefix like ghp_ in the case of GitHub Personal Access Tokens. Would you know if Firebase tokens have a similar prefix? If not, we can remove this field.

plugins/firebase/access_token.go
}

var defaultEnvVarMapping = map[string]sdk.FieldName{
"FIREBASE_TOKEN": fieldname.Token, // TODO: Check if this is correct
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove this TODO.

plugins/firebase/access_token.go
DefaultProvisioner: provision.EnvVars(defaultEnvVarMapping),
Importer: importer.TryAll(
importer.TryEnvVarPair(defaultEnvVarMapping),
TryfirebaseConfigFile(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we actually importing from a configuration file? If yes, could we include an example file at /test-fixtures path? Example from ngrok:

https://github.com/1Password/shell-plugins/blob/f57be66f86be90cf47450b1b0c4226c8c669c2a5/plugins/ngrok/test-fixtures/config.yml

plugins/firebase/access_token.go

// TODO: Check if the platform stores the Access Token in a local config file, and if so,
// implement the function below to add support for importing it.
func TryfirebaseConfigFile() sdk.Importer {
Copy link
Contributor

Choose a reason for hiding this comment

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

The config file importer doesn't seem written. Let's make sure to do that, and let's also test it by actually running op plugin init firebase. If the importer is properly written, there'd be a prompt to import those secrets.

plugins/firebase/access_token_test.go
func TestAccessTokenProvisioner(t *testing.T) {
plugintest.TestProvisioner(t, AccessToken().DefaultProvisioner, map[string]plugintest.ProvisionCase{
"default": {
ItemFields: map[sdk.FieldName]string{ // TODO: Check if this is correct
Copy link
Contributor

Choose a reason for hiding this comment

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

This TODO can be removed.

plugins/firebase/access_token_test.go
plugintest.TestProvisioner(t, AccessToken().DefaultProvisioner, map[string]plugintest.ProvisionCase{
"default": {
ItemFields: map[sdk.FieldName]string{ // TODO: Check if this is correct
fieldname.Token: "dummy_firebase_3bhfuelt31a99503j251bua8rov58m2example",
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefix to be fixed/removed once the correct prefix is known.

plugins/firebase/access_token_test.go
},
// TODO: If you implemented a config file importer, add a test file example in firebase/test-fixtures
// and fill the necessary details in the test template below.
"config file": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Config file importer test to be completed once the importer is written.

plugins/firebase/firebase.go
Name: "Firebase CLI",
Runs: []string{"firebase"},
DocsURL: sdk.URL("https://firebase.google.com/docs/cli"),
NeedsAuth: needsauth.IfAll(
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's skip auth if the command contains the arg --token because it takes precedence over envvars:

image

@arunsathiya arunsathiya added in-progress this PR is being worked on/comments are in the process of being addressed by the contributor and removed waiting-on-reviewer signals that a certain PR is waiting for a review from a 1Password team member labels Jul 10, 2023
arunsathiya
arunsathiya reviewed Jul 12, 2023
View reviewed changes
Copy link
Contributor

@arunsathiya arunsathiya left a comment

Choose a reason for hiding this comment

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

Hi @MukeshKumarBagaria, I noticed some changes since my last comment but they don't seem to address the previous suggestions. Is there something we can help with -- please let us know and we'd be happy to guide you on the next steps!

@arunsathiya arunsathiya self-requested a review July 12, 2023 11:51
@hculea
Copy link
Member

hculea commented Sep 19, 2023

Hey @MukeshKumarBagaria - reaching out again, is there anything that we can help with, to move this forward? 🚀

@hculea
Copy link
Member

hculea commented Sep 22, 2023

Closing this because of inactivity - please do feel free to re-open if you'd like to pick this back up @MukeshKumarBagaria

@hculea hculea closed this Sep 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arunsathiya arunsathiya Awaiting requested review from arunsathiya

Assignees

@MukeshKumarBagaria MukeshKumarBagaria

Labels
hashnode hackathon Ideas and inspiration for the hackathon running from June 1st - June 30th in-progress this PR is being worked on/comments are in the process of being addressed by the contributor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MukeshKumarBagaria @techcraver @arunsathiya @hculea