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

Feature CORS-RFC1918 Support #4305

Merged
merged 17 commits into from Sep 20, 2022

Conversation

pklitscher
Copy link
Contributor

Description

Adding CORS-RFC1918 Support to Auth and Storage.

Allows emulated firebase features to be accessed at localhost:#### from a site that is exposed via tunnel such as localtunnel or ngrok. See issue #4227 & https://wicg.github.io/private-network-access/#headers

Change adds access-control-allow-private-network=true header to api responses when access-control-request-private-network is present.

This change should be replaced in favor of a expressjs/cors option if adopted. See expressjs/cors#236

If PR accepted please consider also adding access-control-allow-private-network=true header to RTDB and Firestore.

Scenarios Tested

Attempt to create a user/sign in, and upload file when site is exposed via a tunnel.
Setup

  1. Setup a firebase project with hosting and emulators
  2. Replace index.html in public folder with below
  3. Emulate project
  4. Expose site via tunnel using localtunnel/localtunnel lt -p 5000 -s firebase-emulator-cors-test
  5. Navigate to https://firebase-emulator-cors-test.loca.lt (or url output by localtunnel)

Test Authentication

  1. Click "Create User"
  2. Verify user was created (see console & emulator ui)
  3. Click "Authenticate User"
  4. Verify user was authenticated (see console)

Test Storage

  1. Click Choose files
  2. Verify file was uploaded (see console & emulator ui)
<!DOCTYPE html>
<html lang="en">
<head>
  <title>App</title>
</head>
<body>
  
  <body>
    <div>
      <button id="createUser">Create User</button>
      <button id="authenticateUser">Authenticate User</button>
      <input type="file" id="files" name="files[]" multiple />
      
    </div>
    <script type="module">
      
      import { initializeApp } from 'https://www.gstatic.com/firebasejs/9.6.7/firebase-app.js'
    
      import { 
        getAuth, 
        connectAuthEmulator, 
        signInWithEmailAndPassword,
        createUserWithEmailAndPassword,
      } from 'https://www.gstatic.com/firebasejs/9.6.7/firebase-auth.js'
      
      import { 
        getStorage, 
        ref, 
        uploadBytesResumable, 
        getDownloadURL,
        connectStorageEmulator
      } from "https://www.gstatic.com/firebasejs/9.6.7/firebase-storage.js";


      const email = "name@fake.com"
      const password = "password"

      const firebaseConfig = {
        apiKey: "AIzaSyDrzYTAKem9ut-r_iV-vIDZggnsjvttfQM",
        authDomain: "fir-emulator-test-40bf3.firebaseapp.com",
        projectId: "fir-emulator-test-40bf3",
        storageBucket: "fir-emulator-test-40bf3.appspot.com",
        messagingSenderId: "1033607461892",
        appId: "1:1033607461892:web:4bfee036d7c11e4f7a4951"
      };

      const app = initializeApp(firebaseConfig);
      const auth = getAuth()
      const storage = getStorage();

      connectAuthEmulator(auth, "http://localhost:9099" );
      connectStorageEmulator(storage, "localhost", 9199)
        
      async function signOut () {
        console.log("Signing out");
        await auth.signOut()
        console.log("Signed out");
      }
      async function authenticateUser () {
        await signOut()
        console.log("Signing in");
        const credential = await signInWithEmailAndPassword(auth, email, password)
        console.log("Signed in", credential)
      }
      async function createUser () {
        console.log('Creating User');
        const credential = await createUserWithEmailAndPassword(auth, email, password)
        console.log("User created", credential)
      }

      auth.onAuthStateChanged(console.log)

      function uploadFile (e) {
        console.log(e);
        const file = e.srcElement.files[0];

        const storageRef = ref(storage, `files/${file.name}`);
        const uploadTask = uploadBytesResumable(storageRef, file);

        uploadTask.on('state_changed', 
          (snapshot) => {
            const progress = (snapshot.bytesTransferred / snapshot.totalBytes) * 100;
            console.log("State: ", snapshot.state)
            console.log('Upload is ' + progress + '% done');
          }, 
          (error) => {
            // Handle unsuccessful uploads
          }, 
          () => {
            getDownloadURL(uploadTask.snapshot.ref).then((downloadURL) => {
              console.log('File available at', downloadURL);
            });
          }
        );
      }

      
      document.getElementById('createUser').addEventListener('click', createUser, true);
      document.getElementById('authenticateUser').addEventListener('click', authenticateUser, true);
      document.getElementById('files').addEventListener('change', uploadFile, true);
    </script>
  </body>
</body>
</html>

Sample Commands

N/A

@google-cla
Copy link

google-cla bot commented Mar 16, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

Copy link
Contributor

@bkendall bkendall left a comment

Choose a reason for hiding this comment

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

This seems pretty reasonable to me. Could you take a look at the comments and re-request a review? I'll re-review and then make sure someone who knows the storage paths better than me takes a look (probably @yuchenshi ) :)

Could you also add an entry to the CHANGELOG? Something like

- Adds appropriate headers to Auth and Storage emulators when accessing them via a tunnel (#4227).

src/emulator/auth/server.ts Outdated Show resolved Hide resolved
src/emulator/storage/server.ts Outdated Show resolved Hide resolved
.eslintrc.js Outdated Show resolved Hide resolved
.prettierrc.js Outdated Show resolved Hide resolved
pklitscher and others added 4 commits March 17, 2022 08:34
Co-authored-by: Bryan Kendall <bryan.a.kendall@gmail.com>
Co-authored-by: Bryan Kendall <bryan.a.kendall@gmail.com>
endOfLine rule was temporarily changed for prettier/prettier to allow tests to pass on Windows + VS Code. Reverting to original repo settings
@pklitscher
Copy link
Contributor Author

Hey team. Anything holding this one up? Did you need anything more from me?

@yuchenshi yuchenshi enabled auto-merge (squash) June 2, 2022 02:26
@yuchenshi yuchenshi dismissed bkendall’s stale review June 2, 2022 02:27

all comments resolved

Copy link
Contributor

@bkendall bkendall left a comment

Choose a reason for hiding this comment

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

I've suggested the fix for the tests, but LGTM!

auto-merge was automatically disabled August 23, 2022 11:48

Head branch was pushed to by a user without write access

@pklitscher
Copy link
Contributor Author

Hey @bkendall or @yuchenshi just wondering if you can help as to why these tests are failing? I can't seem to see any information in the details and I am not too sure what I have wront. Sorry first time doing PR here.

@bkendall
Copy link
Contributor

@pklitscher looks like the formatter is failing. Running npm run format will fix it.

You should be able to run npm test locally and it'll do all the same checks.

@pklitscher
Copy link
Contributor Author

Thank you @bkendall

I am hitting two issues.

  1. If I run npm run format it will try remove every carriage return which formats every file. I don't think you'll acccept that PR haha. Appears to be related to Windwos vs Max https://stackoverflow.com/questions/53516594/why-do-i-keep-getting-delete-cr-prettier-prettier? Have you experienced this before? Only way I can get it to pass is by editing the eslint config with "prettier/prettier": ["error", { endOfLine: "auto" }] which I know you don't want. I originally ignored these errors when I did the PR as it had nothing to do with the changes.

If I change the rule and run npm test I am then getting this error (I didn't have this when I did the PR):

  1. When it tries to run prettier --check '**/*.{md,yaml,yml}' it is erroring with No files matching the pattern were found: "'**/*.{md,yaml,yml}'". I havne't touched any of this so not sure why it's throwing an error. Have run npm install to make sure had correct versions.

Sorry for my lack of knowledge here - not sure where I am going wrong and don't want to touch how your linting and tests are setup.

@bkendall
Copy link
Contributor

You may be able to make the eslint change, run the formatter, then revert the eslint change. Alternatively, I think you may just have to add a newline to the CHANGELOG file - that might get you past that error.

I'm not sure what the differences are here, unfortunately - most of the development environments I deal with are linux or macOS, so this kind of issue I don't hit regularly.

I may be able to take your branch, fix the formatting and submit it. Would that be alright?

@pklitscher
Copy link
Contributor Author

@bkendall I think I cleared the issue with CHANGELOG by adding a heading and a newline as you mentioned.

Still seems to be failing on some of the integration tests so I am happy for you to grab the branch and submit.

Apologies for the hassle for such a small change. Thank you for your help.

@codecov-commenter
Copy link

codecov-commenter commented Sep 16, 2022

Codecov Report

Base: 56.99% // Head: 56.99% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (d4df6be) compared to base (fb6dbba).
Patch coverage: 50.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4305      +/-   ##
==========================================
- Coverage   56.99%   56.99%   -0.01%     
==========================================
  Files         292      292              
  Lines       19508    19516       +8     
  Branches     3902     3904       +2     
==========================================
+ Hits        11119    11123       +4     
- Misses       7452     7456       +4     
  Partials      937      937              
Impacted Files Coverage Δ
src/emulator/storage/server.ts 14.06% <0.00%> (-0.94%) ⬇️
src/emulator/auth/server.ts 71.78% <100.00%> (+0.47%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@bkendall bkendall merged commit 3b86ba2 into firebase:master Sep 20, 2022
christhompsongoogle pushed a commit that referenced this pull request Sep 30, 2022
* Add support for cors rfc1918

* Remove null from next()

Co-authored-by: Bryan Kendall <bryan.a.kendall@gmail.com>

* Remove null from next()

Co-authored-by: Bryan Kendall <bryan.a.kendall@gmail.com>

* Revert incorrect linting changes

endOfLine rule was temporarily changed for prettier/prettier to allow tests to pass on Windows + VS Code. Reverting to original repo settings

* Update CHANGELOG.md

* Update src/test/emulators/auth/rest.spec.ts

Co-authored-by: Bryan Kendall <bryan.a.kendall@gmail.com>

* Update CHANGELOG.md

* Update CHANGELOG

* Update src/emulator/storage/server.ts

Co-authored-by: Yuchen Shi <yuchenshi@google.com>

* Update CHANGELOG.md

Co-authored-by: Bryan Kendall <bryan.a.kendall@gmail.com>
Co-authored-by: Yuchen Shi <yuchenshi@google.com>
Co-authored-by: Bryan Kendall <bkend@google.com>
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.

None yet

4 participants