Skip to content

Use secure rules for Firebase Storage Integration tests #5643

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 19 commits into from
May 20, 2020

Conversation

schmidt-sebastian
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian commented May 19, 2020

This changes the Storage Integration Test to use Firebase Auth with Email/Password sign in. I also removed some duplicate tests.

Addresses internal b/155892305

#no-changelog

@@ -136,37 +151,8 @@ - (void)testUnauthenticatedGetMetadata {
[self waitForExpectations];
}

- (void)testUnauthenticatedUpdateMetadata {
Copy link
Contributor Author

@schmidt-sebastian schmidt-sebastian May 19, 2020

Choose a reason for hiding this comment

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

There is already an update metadata test.

@@ -205,27 +191,8 @@ - (void)testDeleteWithNilCompletion {
[self waitForExpectations];
}

- (void)testUnauthenticatedSimplePutData {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test also exists twice.

@paulb777 paulb777 force-pushed the mrschmidt/storagetests branch from 3ebb919 to c773b50 Compare May 19, 2020 21:32
var storage: Storage!
static var once = false

override class func setUp() {
FirebaseApp.configure()
app = FirebaseApp.app()
auth = Auth.auth(app:app)
auth.signIn(withEmail: "[email protected]", password: "testing") { result, error in
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need to block on this?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. Good catch!

Copy link
Contributor

@maksymmalyhin maksymmalyhin left a comment

Choose a reason for hiding this comment

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

LGTM on CI green

@@ -705,6 +716,16 @@ class StorageIntegration: XCTestCase {
waitForExpectations()
}

private func signInAndWait() {
let expectation = self.expectation(description: #function)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if self can be omitted here?

Copy link
Member

Choose a reason for hiding this comment

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

nope. Variable used within its own initial value

*
* You can define these access rights in the Firebase Console of your project.
*/

NSString *const kTestUser = @"[email protected]";
NSString *const kTestPassword = @"testing";
Copy link
Contributor

Choose a reason for hiding this comment

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

BTW are public credentials an issue from b/155892305 perspective? We can move credentials to secrets (e.g. like Auth do).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are planning to do so as well.

@ncooke3
Copy link
Member

ncooke3 commented May 20, 2020

Some of the Storage Swift Integration tests are now failing since they test with unauthenticated access. What should we do about the unauthenticated tests?

@paulb777
Copy link
Member

paulb777 commented May 20, 2020

@ncooke3 Yep - Working on it now trying to figure out why the tests are still failing on CI, even though they succeed locally. Any ideas or suggestions appreciated.

@ncooke3
Copy link
Member

ncooke3 commented May 20, 2020

@paulb777 Hmm does it have anything to do with the fact that the rules have changed for the storage container on the firebase console? When I run them locally, any current tests that does any writing fails because it is unauthenticated and the rules on the console have since changed to only allow writes if authenticated. Maybe this is what is making the CI fail?

@paulb777
Copy link
Member

@ncooke3 Yes - the rules have changed, but the tests should now be signing in to authenticate

@paulb777 paulb777 force-pushed the mrschmidt/storagetests branch from 72fc6c3 to d026ea1 Compare May 20, 2020 18:49
@schmidt-sebastian schmidt-sebastian merged commit c2fbc77 into master May 20, 2020
@schmidt-sebastian schmidt-sebastian deleted the mrschmidt/storagetests branch May 20, 2020 19:38
@firebase firebase locked and limited conversation to collaborators Jun 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants