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

Revoked Token Checking #30

Closed SaadArdati closed 4 years ago

SaadArdati commented 4 years ago

Fixes #26

Untested. What's the best way to test this?

Note: OpenID code and user_record.dart are taken from firebase_admin repo

SaadArdati commented 4 years ago

You could even potentially get away with not doing local token verification in that case since those checks will be performed by Firebase anyway.

Like I said, this supports backend serverless solutions. In my case, I'm making a REST api serverless container which requires token verification because anyone can make a request.

SaadArdati commented 4 years ago

You shouldn't touch the jwt class at all.

Would you like me to move the code to verify_token.dart?

cachapa commented 4 years ago

You could even potentially get away with not doing local token verification in that case since those checks will be performed by Firebase anyway.

Like I said, this supports backend serverless solutions. In my case, I'm making a REST api serverless container which requires token verification because anyone can make a request.

I think you misunderstood my point. When you request the user object from Firebase it will fail if the token is invalid for any reason, not just if it's been revoked. In that case you could spare the computational effort of performing local verification, with the possible cost of now knowing exactly why the token is invalid.

cachapa commented 4 years ago

Would you like me to move the code to verify_token.dart?

"Move" the code makes me think you want to have the http requests in verify_token, which is not what I said. You should find a way to get a handle to the UserGateway object and call getUser using the token you want to test.

SaadArdati commented 4 years ago

"Move" the code makes me think you want to have the http requests in verify_token, which is not what I said. You should find a way to get a handle to the UserGateway object and call getUser using the token you want to test.

Already did that locally, I meant the actual verification check because you said I shouldn't modify jwt

SaadArdati commented 4 years ago

I think you misunderstood my point. When you request the user object from Firebase it will fail if the token is invalid for any reason, not just if it's been revoked. In that case you could spare the computational effort of performing local verification, with the possible cost of now knowing exactly why the token is invalid.

I'd rather spare the network bandwidth. I suppose I'll move the checkRevoked code to the top of the method so that if you want to check if it's revoked, it performs the api request, then fails. Otherwise, it does local

cachapa commented 4 years ago

I think you misunderstood my point. When you request the user object from Firebase it will fail if the token is invalid for any reason, not just if it's been revoked. In that case you could spare the computational effort of performing local verification, with the possible cost of now knowing exactly why the token is invalid.

I'd rather spare the network bandwidth. I suppose I'll move the checkRevoked code to the top of the method so that if you want to check if it's revoked, it performs the api request, then fails. Otherwise, it does local

But the network request is the only way to know if the token has been revoked. I'm getting confused by this entire discussion.

SaadArdati commented 4 years ago

But the network request is the only way to know if the token has been revoked. I'm getting confused by this entire discussion.

I'm going to push my changes so far and you can see what I mean. Please respond to the review converseration as well

cachapa commented 4 years ago

I think this is going nowhere and I don't have time to continue this process. I think you're missing the fundamentals of how oauth works and what firedart already does. I also don't appreciate code from other projects simply being dumped here, much less without proper testing.

My proposal is that you either do the very simple implementation I described in the beginning, or drop the PR and I'll do it myself at some point in the future.

cachapa commented 4 years ago

After reconsidering this PR I'm coming to the conclusion that this is not the direction I want for this package. I don't feel it makes sense to implement the Admin API since we're basically replicating functionality that's already existing in the firebase_admin package.

This also means that I will probably be reverting the service credentials changes since they also don't make much sense from the user perspective.

SaadArdati commented 4 years ago

@cachapa May I have permission to modify my fork to admin only and rename it? I probably wont release it but if I someday wish to, may i?

cachapa commented 4 years ago

I would prefer you choose a completely different name to avoid confusing possible users.