search

cachapa / firedart

A dart-native implementation of the Firebase Auth and Firestore SDKs
https://pub.dev/packages/firedart
Apache License 2.0
174 stars 62 forks source link

Enable proper platform-agnostic service account support. #21

Closed SaadArdati closed 4 years ago

SaadArdati commented 4 years ago

I have also created a test class, but I can't push it because it contains too much sensitive data relating to my database. Be rest assured that my tests passed successfully and I was able to modify my database properly as an admin.

I pulled pull request #18 and did all my changes. Moved everything to the auth package.

I would like to highlight the docs I wrote here: https://github.com/cachapa/firedart/pull/21/files#diff-c9ffac9237abe199618272addfb40eceR10-R33

SaadArdati commented 4 years ago

Not really sure how to make it pass the tests because those require sensitive information @cachapa

cachapa commented 4 years ago

Yeah I have to find another way to run those tests. I've ran them locally and it seems to be fine.

SaadArdati commented 4 years ago

@cachapa Will review all your requests. however: image

I reviewed dart documentations, and they all seem to agree exporting is the way to go, so i dont know...

SaadArdati commented 4 years ago

@cachapa

Given the size of the classes, you might be better off just dropping every class in a single service_account.dart.

I can't do that. The reason I split io and html is because dart can't have both loaded at the same time. This is why the export class exists. If I could drop them in the same class, I would, but this is how the dart team wants us to split io and html...

The export classes makes dart pick which dart file to load. If the same dart file contains both imports, it will crash.

SaadArdati commented 4 years ago

@cachapa

In fact, the more I understand your code, the less I feel there's a need for the entire service_account class collection. It seems to exist just as a helper class to load the contents of service-account.json, is that correct? If that's the case let's discuss the implementation a bit better before doing further changes. There's really no need for 3 layers of indirection just to supply a json string to the GRPC client.

So the problem is, when splitting between io and html, you inherently need 4 classes minimum.

One class acts as the platform-specific exporter, holding 1 export line. One class is for io, one for html, and one for unsupported. You could argue you can default to io, but that's not the proper way to do things, an unsupported class is proper etiquette imo.

And you need one class to be the Abstract implementation. That's 4 classes minimum + 5 for unsupported impl.

Not sure what else I could do instead.

SaadArdati commented 4 years ago

I think I'm going to approach this in a different way. An IO class only that looks for environment variable as a fallback and not html class. This should alleviate redundancy

SaadArdati commented 4 years ago

@cachapa Done, please review my changes.

cachapa commented 4 years ago

@cachapa Will review all your requests. however: image

I reviewed dart documentations, and they all seem to agree exporting is the way to go, so i dont know...

That's why the html client class contains annotations to ignore warnings. I'm honestly not sure what the best approach is here anymore, maybe your original one was "better".

cachapa commented 4 years ago

Thanks a lot for your updates, I'll review them soon.

SaadArdati commented 4 years ago

One option to solve that would be to simply provide two ready-made helper classes (one for io and one for html), the same way we also provide VolatileStore and VerboseClient. What do you think? I dislike this. The way I currently made it is by far the simplest from a user experience. You have one class and it handles the boilerplate between io/html for you, so you don't even have to worry about it.

Splitting it into helper classes will not fix the problem, it will bifurcate the implementation into more classes the user needs to be aware of. Helper classes also mean the user needs to look elsewhere and dig unnecessarily deeper into this to figure out that what they need is not inside ServiceAccount itself. Especially given your requirement for significantly simplified documentation.

I'm more of the thought that you can't have too much documentation. Users will be able to find exactly what they need swiftly. It's lack of documentation that bites you.

So given your requirement for simplified documentation, the implementation must, by all means necessary, require self-documenting code in which the classes are clear enough in their purpose that the user will achieve what they need swiftly.

If we split into helper classes, not only will you still have the same amount of boilerplate I currently have because you're substituting my boilerplate with helper classes (an eye for an eye), but also splitting implementation options across several classes. The way I currently have it means it's all centralized in service_account.dart, users need not look elsewhere.

My implementation, I believe, is the most ideal.

SaadArdati commented 4 years ago

@cachapa 1- done 2- we can't because that would require a Future to readAsString(). Unfortunately, the user will have to do this themselves. 3- done

What I can do is provide a static helper method... I'll try that I suppose

cachapa commented 4 years ago

Almost perfect. Can you check dartfmt?

$ dartfmt --dry-run --set-exit-if-changed .
lib/firestore/firestore_gateway.dart
lib/auth/service_account/io_access.dart
lib/auth/service_account/unsupported_access.dart
lib/auth/service_account/service_account.dart
lib/auth/firebase_auth.dart
example/admin.dart
cachapa commented 4 years ago

Thank you! Merged.

davidmartos96 commented 3 years ago

@cachapa @SwissCheese5 Hello! I've recently started looking for Dart solutions for the Firebase Admin SDK. I'm very interested in the aproach taken by this library, but I would like to use the auth feature implemented in this PR (authentication via service account). I've noticed that this has been reverted and @SwissCheese5 has started his own fork, but I haven't found any discussion/issue about that. Is there any reason why that functionality doesn't fit in firedart?

cachapa commented 3 years ago

The reasons were discussed in the PR itself, I believe. In summary, the PR was diverging too much from the current Firedart implementation in that it was using a lot of Google dependencies for the authentication.

While I'm not against Google dependencies per se (and in fact might be the correct long-term approach), it didn't fit the project at the time.

SaadArdati commented 3 years ago

To be a bit more specific: This project focuses on firebase clients rather than firebase backend/servers

davidmartos96 commented 3 years ago

@cachapa @SwissCheese5 Thanks for the clarification! I understand the desire to keep the client and server libraries separated, but maybe it could be beneficial to keep them both in a single repository with a common package that interacts with the Firebase APIs. As of now there is a lot of code in common between the fork and the main library. I think Dart has a lot of potential for server side code, so it would be great to try to avoid splitting to much the alternatives. There are already a couple of other libraries with very similar functionality to firedart. Thanks for the work on the packages to both of you guys!