Skip to content
This repository has been archived by the owner on Mar 29, 2022. It is now read-only.

Properly fill in auth object for Android #89

Merged
merged 2 commits into from
May 14, 2017
Merged

Properly fill in auth object for Android #89

merged 2 commits into from
May 14, 2017

Conversation

dotdoom
Copy link
Contributor

@dotdoom dotdoom commented May 14, 2017

The "auth" object is later used to do permission checks by the means of targaryen library [1].
I don't know the semantics of the token that JavaScript client sends, but [3] is for what Android sends in, and, given that JavaScript clients were tested to work correctly, I assume the format is different for some reason. The idea of this pull request is to bring the Android token in line with the official documentation at [2].

Why not make this change to targaryen library instead?
Well, the "auth" object supplied to targaryen is supposed to be of the format described in [2] rather than being protocol-bound, so firebase-server, as a protocol handler, is a more appropriate place to put an adapter in.

Fixes #88.

[1] https://github.com/goldibex/targaryen
[2] https://firebase.google.com/docs/reference/security/database/#auth
[3] #88

The "auth" object is later used to do permission checks by the means of targaryen library [1].
I don't know the semantics of the token that JavaScript client sends, but [3] is for what Android sends in, and, given that JavaScript clients were tested to work correctly, I assume the format is different for some reason. The idea of this pull request is to bring the Android token in line with the official documentation at [2].

Why not make this change to targaryen library instead?
Well, the "auth" object supplied to targaryen is supposed to be of the format described in [2] rather than being protocol-bound, so firebase-server, as a protocol handler, is a more appropriate place to put an adapter in.

[1] https://github.com/goldibex/targaryen
[2] https://firebase.google.com/docs/reference/security/database/#auth
[3] #88
@coveralls
Copy link

coveralls commented May 14, 2017

Coverage Status

Coverage decreased (-0.3%) to 97.01% when pulling 574ae3c on dotdoom:patch-1 into 541800e on urish:master.

index.js Outdated
data = decodedToken.d;
} else {
data = {
uid: decodedToken.sub,
Copy link
Owner

Choose a reason for hiding this comment

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

per #88, wasn't it user_id instead of sub ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right.
Both have the same value, but sub is a standard JWT claim, while user_id is firebase-specific.
What if I use sub as a fallback for user_id?

@urish
Copy link
Owner

urish commented May 14, 2017

thanks, please have a look at my comment

@dotdoom
Copy link
Contributor Author

dotdoom commented May 14, 2017

Thanks for taking a look. Sorry for the diminished coverage: unfortunately, the token encryption in firebase-token-generator is bound to the generator that enforces .d field, and can not be reused to generate an Android token mentioned in #88.
Copy-pasting token encryption algorithm is possible, however I'm not sure you'd like to see such code in this repo.

@coveralls
Copy link

coveralls commented May 14, 2017

Coverage Status

Coverage decreased (-0.3%) to 97.0% when pulling 942967f on dotdoom:patch-1 into 541800e on urish:master.

@urish urish merged commit 9a50b2c into urish:master May 14, 2017
@urish
Copy link
Owner

urish commented May 14, 2017

awesome, thanks!

@urish
Copy link
Owner

urish commented May 18, 2017

Released as 0.10.1

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants