-
Notifications
You must be signed in to change notification settings - Fork 273
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
fix(enterprise): fix start interval when using access tokens #2211
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -70,27 +70,46 @@ export class EnterpriseApi { | |
return | ||
} | ||
this.enterpriseDomain = enterpriseDomain | ||
|
||
// Retrieve an authentication token | ||
const authToken = await this.readAuthToken() | ||
if (authToken && commandAllowed) { | ||
this.log.debug({ msg: `Refreshing auth token and starting refresh interval.` }) | ||
// Verify a valid token is present | ||
this.log.debug({ msg: `Refreshing auth token and trying to start refresh interval.` }) | ||
const tokenIsValid = await this.checkClientAuthToken(this.log) | ||
|
||
if (!tokenIsValid) { | ||
await this.refreshToken() | ||
// If the token is an Access Token and it's invalid we return. | ||
if (gardenEnv.GARDEN_AUTH_TOKEN) { | ||
throw new RuntimeError( | ||
"The provided access token is expired or has been revoked, please create a new one from the Garden Enterprise UI.", | ||
{} | ||
) | ||
} else { | ||
// Try to refresh an expired JWT | ||
// This will throw if it fails to refresh | ||
await this.refreshToken() | ||
} | ||
} | ||
// At this point we can be sure the user is logged in because we have | ||
// a valid token or refreshing the token did not go through. | ||
// TODO: Refactor to make a bit more robust (cc @emanuele and @thsig, you | ||
// know what I'm talking about.) | ||
this.isUserLoggedIn = true | ||
this.startInterval() | ||
// Start refresh interval if using JWT | ||
if (!gardenEnv.GARDEN_AUTH_TOKEN) { | ||
this.log.debug({ msg: `Starting refresh interval.` }) | ||
this.startInterval() | ||
} | ||
} | ||
} | ||
|
||
startInterval() { | ||
this.log.debug({ msg: `Will run refresh function every ${this.intervalMsec} ms.` }) | ||
this.intervalId = setInterval(() => { | ||
this.refreshToken().catch((err) => { | ||
this.log.debug(err) | ||
this.log.debug({ msg: "Something went wrong while trying to refresh the authentication token." }) | ||
this.log.debug({ msg: err.message }) | ||
}) | ||
}, this.intervalMsec) | ||
} | ||
|
@@ -106,9 +125,11 @@ export class EnterpriseApi { | |
const invalidCredentialsErrorMsg = "Your Garden Enteprise credentials have expired. Please login again." | ||
const token = await ClientAuthToken.findOne() | ||
|
||
if (!token) { | ||
throw new RuntimeError(invalidCredentialsErrorMsg, {}) | ||
if (!token || gardenEnv.GARDEN_AUTH_TOKEN) { | ||
this.log.debug({ msg: "Nothing to refresh, returning." }) | ||
return | ||
} | ||
|
||
if (isAfter(new Date(), sub(token.validity, { seconds: refreshThreshold }))) { | ||
try { | ||
const res = await this.get(this.log, "token/refresh", { | ||
|
@@ -207,26 +228,6 @@ export class EnterpriseApi { | |
return token | ||
} | ||
|
||
async logout() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we removing this? Note that the "other" logout function isn't used. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I moved the logic to be out of the |
||
const token = await ClientAuthToken.findOne() | ||
if (!token) { | ||
// Noop when the user is not logged in | ||
return | ||
} | ||
try { | ||
await this.post(this.log, "token/logout", { | ||
headers: { | ||
Cookie: `rt=${token?.refreshToken}`, | ||
}, | ||
}) | ||
|
||
await this.clearAuthToken() | ||
} catch (error) { | ||
this.log.error({ msg: "An error occurred while logging out." }) | ||
this.log.debug({ msg: JSON.stringify(error, null, 2) }) | ||
} | ||
} | ||
|
||
/** | ||
* If a persisted client auth token exists, deletes it. | ||
*/ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,8 @@ import getPort = require("get-port") | |
import { LogEntry } from "../logger/log-entry" | ||
import { InternalError } from "../exceptions" | ||
import { EnterpriseApi, AuthTokenResponse } from "./api" | ||
import { ClientAuthToken } from "../db/entities/client-auth-token" | ||
import { gardenEnv } from "../constants" | ||
|
||
/** | ||
* Logs in to Garden Enterprise if needed, and returns a valid client auth token. | ||
|
@@ -55,13 +57,22 @@ export async function login(enterpriseApi: EnterpriseApi, log: LogEntry): Promis | |
} | ||
|
||
export async function logout(enterpriseApi: EnterpriseApi, log: LogEntry): Promise<void> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function isn't used anywhere. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I updated the PR, I left out the |
||
const savedToken = await enterpriseApi.readAuthToken() | ||
// Ping platform with saved token (if it exists) | ||
if (savedToken) { | ||
log.debug("Local client auth token found, verifying it with platform...") | ||
if (await enterpriseApi.checkClientAuthToken(log)) { | ||
log.debug("Local client token is valid, no need for login.") | ||
} | ||
const token = await ClientAuthToken.findOne() | ||
if (!token || gardenEnv.GARDEN_AUTH_TOKEN) { | ||
// Noop when the user is not logged in or an access token is in use | ||
return | ||
} | ||
try { | ||
await enterpriseApi.post(log, "token/logout", { | ||
headers: { | ||
Cookie: `rt=${token?.refreshToken}`, | ||
}, | ||
}) | ||
|
||
await enterpriseApi.clearAuthToken() | ||
} catch (error) { | ||
log.error({ msg: "An error occurred while logging out." }) | ||
log.debug({ msg: JSON.stringify(error, null, 2) }) | ||
} | ||
} | ||
|
||
|
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.
Shouldn't this throw? If you're explicitly setting an access token that doesn't work you probably want an error.
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.
Possibly. I wanted to try a more conservative approach since it fails later, but I agree we can throw here.
I'll add it.