Skip to content

Conversation

rayluo
Copy link
Contributor

@rayluo rayluo commented May 10, 2019

Add remove_account() API, and it will signout the account from the app family. This PR will close #12 .

Most of the implementations are refactoring the underlying cache helpers. To review high level "what does it do" logic, please focus on only 50 lines starting from here (half of them are inline documentation).

@rayluo
Copy link
Contributor Author

rayluo commented May 13, 2019

Thanks @SomkaPe for reviewing this PR promptly. I'm also documenting the summary of the code review conversations below for future reference.

[5/10 3:19 PM] Petro Somka
is there any api review pr for it?
i mean is there some description of logic
​[5/10 3:22 PM] Ray Luo
API review PR for the remove_account() behavior is the internal PR 775 I sent out weeks ago, about the sign-out-of-app or sign-out-of-family.
Now this actual implementation do the sign out of family.
​[5/10 3:22 PM] Petro Somka
ok

​[5/10 3:28 PM] Petro Somka
why do you not remove idTokens in _sign_out() ?
​[5/10 3:32 PM] Ray Luo
I believe id_token are intrinsically more close to accounts, so I only do it in _forget_me().
​[5/10 3:33 PM] Petro Somka
_forget_me() and _sign_out() are private , and only removeAccount() is public which just call _forget_me()
[5/10 3:34 PM] Ray Luo
Yes
In that sense, whether removing IDT happens in signout() or in forget_me(), does not really matter right now.
It would matter in the future if our entire org agree to officially provide public api for signout() and forget_me(). At that time we would need to precisely define and agree on their behaviors.

​[5/10 3:31 PM] Petro Somka
one more question if you remove account realm - independently It also means that get Accounts api returns accounts realm independently as well , correct ?
​[5/10 3:33 PM] Ray Luo
Yes, I believe get_account should return all accounts regardless of their realms.
[5/10 3:35 PM] Petro Somka
but not returning same account twice if there are few account in the cache with diff realms 
(1 liked)

@rayluo rayluo merged commit 5f0befb into dev May 17, 2019
@rayluo rayluo deleted the signout-family branch May 17, 2019 22:31
@rayluo rayluo mentioned this pull request May 22, 2019
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.

token cache: support logout
2 participants