feat: renew expired access token

[jfatta]

• During setup (common initialization process for all commands), check if the access token is expired (according to the configuration property stored during login.)
• In that case, trigger the same login flow (with minor changes on the messages displayed to the user)
• Then move on with the requested command.

This way the user can login as part of the experience of the requested command instead of failing.

[Widcket]

@jfatta

[jfatta]

@jfatta

thanks. I'm adding a unit test for IsExpired() 😅

[cyx]

What do you think about doing 5 * time.Minute here and in the caller below doing int(accessTokenExpThreshold.Minutes()) ?

[jfatta]

yes! I always mess with it because time.* feels invalid for constants to me (which is not the case)

[cyx]

Looks great! Just a small idea on the expiration representation of int vs time.Duration

[woloski]

did you implement refresh token? using rt you can get a new at. No need for user interaction. You need to add scope offline_access for that

[cyx]

We were (me mostly) being paranoid that refresh tokens can last for super long. If we can scope those to a month max maybe it's fine?

[woloski]

yes that’s possible

…

On Fri, Feb 26, 2021 at 10:02 PM Cyril David ***@***.***> wrote: We were (me mostly) being paranoid that refresh tokens can last for super long. If we can scope those to a month max maybe it's fine? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#112 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABK6CAVBK6VTA522F5EMUDTBBAB5ANCNFSM4YI5AN3A> .

[jfatta]

@woloski @cyx if we agree that storing the refresh token on the config file is OK (given that asking permissions for macOS keychain might be inappropriate and not cross-platform), I can update this to:

• login stores AT and RT at config
• (every command setup) if AT is expired, get a new one with the RT
• if the previous step failed for some reason, trigger the login flow as today

makes sense?

[woloski]

can we use this https://github.com/zalando/go-keyring

…

On Mon, Mar 1, 2021 at 10:11 AM Jorge L. Fatta ***@***.***> wrote: @woloski <https://github.com/woloski> @cyx <https://github.com/cyx> if we agree that storing the refresh token on the config file is OK (given that asking permissions for macOS keychain might be inappropriate and not cross-platform), I can update this to: - login stores AT and RT at config - (every command setup) if AT is expired, get a new one with the RT - if the previous step failed for some reason, trigger the login flow as today makes sense? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#112 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABK6CAQFBB34K5OSP5NMEDTBOHBBANCNFSM4YI5AN3A> .

[jfatta]

@woloski yes, we can do that (we used a similar pkg for one of our internal projects too: https://github.com/99designs/keyring/).

My question was more related to the fact that accessing the keychain requires explicit user consent (a dialog appears the first time and with each new binary/updated), are we ok with this?

(I'll iterate on the solutions discussed here later this week)

[woloski]

yes, after login is ok. it will happen infrequently

…

On Mon, Mar 1, 2021 at 11:59 AM Jorge L. Fatta ***@***.***> wrote: @woloski <https://github.com/woloski> yes, we can do that (we used a similar pkg for one of our internal projects too: https://github.com/99designs/keyring/). My question was more related to the fact that accessing the keychain requires explicit user consent (a dialog appears the first time and with each new binary/updated), are we ok with this? (I'll iterate on the solutions discussed here later this week) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#112 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABK6CELX7NOBZ5V7XTRRN3TBOTTHANCNFSM4YI5AN3A> .