-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refresh authToken when it expires #192
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach seems reasonable, although there may be an alternative - see below.
So apparently, even if the Something like {
this.fusionAuthClient.login({
loginId: this.authContext.username,
password,
generateRefreshTokens: true,
})
}).then(() => {}) in sftp-service/src/classes/AuthenticationSession.ts Lines 47 to 50 in a9ca457
However, the Next step is to expirement with |
It is concerning that this service is requesting passwords from users! I don't know how other rclone backends handle OAuth, but I worry that the inspiration for doing so here came from the back-end. That was a compatibility shim I added to the back-end until the front-end was able to delegate its sign-in screen to FusionAuth - it is not the recommended approach, as user passwords should be exposed to as few services as possible, and ideally none of them. What I would have expected is for signing in to go through some kind of web browser flow, probably the device authorization grant, so that the user can provide their password exclusively to FusionAuth, as well as go through whatever MFA steps they have configured. Taking the responsibility of doing password and MFA into this app makes the process less secure and requires more code.
Unfortunately, it looks like you are doing FusionAuth-specific things. So long as you use their vendor-specific APIs rather than the standard OAuth/OIDC APIs, you'll have to continue to use their client library. I'd suggest moving away from using their vendor-specific APIs. |
I am sure you know this from looking at the code, but the passwords are still passed to the FusionAuth api directly not passed via the back-end. Now, I can't remember the specific challenges that Dan faced with trying to authenticate directly with FusionAuth without collecting passwords.
On this, I attempted avoid the FusionAuth typescript client in a bit to get this https://fusionauth.io/docs/v1/tech/apis/login#request
So at this point, looks like I am stuck with the FusionAuth dashboard again or maybe one really needs to change the authentication mechanism to move away from this login type completely. |
Yes, but my point is that this service should not have access to user passwords, either - only FusionAuth should. By accepting passwords, this service becomes a potential vector for compromising a user's credentials. I'm not saying that's likely, but it's an avoidable risk.
Using the FusionAuth APIs via direct HTTP calls would be the worst of both worlds, unfortunately. If you must use their proprietary APIs, you'll be best off with their client. To address the specific concern you have, from reading the docs, it appears that in order to get a refresh token via the login API you must pass your application ID:
|
@jasonaowen I can talk a bit about why we chose to implement login the way we did, though I think that will be best explained by voice at least initially (I do allude to some of the reasons here: #175 (comment) -- ultimately it is because rclone requires pre-configured credentials to connect to a remote host. It can be set up to use the I don't think that it will be within scope to develop support for oauth as part of this ticket. It does appear that |
Jason pinged me to point out that we missed a key line of his point!
@fenn-cs it looks like we may well have a solution! Could you take a look at that? |
So I replaced the existing login method-call with a custom client call that accepts However, this is what the response looks like :
Conclusion: Passing cc: @slifty |
e0fbb5f
to
d76e362
Compare
d76e362
to
4f6124c
Compare
ce0e123
to
88ca5f8
Compare
88ca5f8
to
f9d50e9
Compare
Access tokens have lifetimes that might end before the user wants to end their authenticated session. Hence, we need to refresh the user access token after an expiry detection. Resolves: #175 Signed-off-by: fenn-cs <[email protected]>
02cc614
to
2e89e19
Compare
Notes for testing
The short 6-minute expiration is for local testing only. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this, @fenn-cs! Very cool to see so much progress.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the progress on this! I identified some changes I think would make this implementation even better / safer!
2e89e19
to
4f20528
Compare
c853692
to
1ae60e5
Compare
47b9571
to
4df4750
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Phewf this is so close thank you for all the updates.
Some of my comments are style / naming suggestions which please feel free to take or leave. That said, there are some more important notes here:
- A small bug around time calculation (I think)
- We can't call
authContext.reject
when refreshing auth tokens
That second issue is a big one, and even led me to realize that there would be merit to extracting the auth token management from this class entirely (which I'm suggesting we do as a separate issue rather than addressing in this PR).
3f56d08
to
1c6f999
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for missing this originally -- let's make sure our log calls are using the right levels.
Signed-off-by: Fon E. Noel NFEBE <[email protected]>
1c6f999
to
c11bdd4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woohoo!!! Well done @fenn-cs thank you for all your excellent work on this.
The SFTP service needs some additional environment variables to be populated, as a result of some recent changes to how refresh tokens are used to generate auth tokens [1]. Issue #138 [1] PermanentOrg/sftp-service#192
The SFTP service needs some additional environment variables to be populated, as a result of some recent changes to how refresh tokens are used to generate auth tokens [1]. A few of these vriables are redundant [2], and that's why we use the same "source" variable to map them as late as possible in the provisioning. Eventually if the redundancy is removed from the sftp service we'll want to update the provisioner to stop populating the obsolete copies. Issue #138 [1] PermanentOrg/sftp-service#192
The SFTP service needs some additional environment variables to be populated, as a result of some recent changes to how refresh tokens are used to generate auth tokens [1]. A few of these vriables are redundant [2], and that's why we use the same "source" variable to map them as late as possible in the provisioning. Eventually if the redundancy is removed from the sftp service we'll want to update the provisioner to stop populating the obsolete copies. Issue #138 [1] PermanentOrg/sftp-service#192 [2] PermanentOrg/sftp-service#289
Access tokens have lifetimes that might end before the user wants to end their authenticated session. Hence, we need to refresh the user access token after an expiry detection.
Resolves: #175