Skip to content
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

Merged
merged 2 commits into from
Jan 19, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion core/src/commands/logout.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import { Command, CommandParams, CommandResult } from "./base"
import { printHeader } from "../logger/util"
import dedent = require("dedent")
import { logout } from "../enterprise/auth"

export class LogOutCommand extends Command {
name = "logout"
Expand All @@ -32,7 +33,7 @@ export class LogOutCommand extends Command {
log.debug({ msg: `Logging out of ${garden.enterpriseApi?.getDomain()}` })
log.info({ msg: `Logging out of Garden Enterprise.` })
try {
await garden.enterpriseApi.logout()
await logout(garden.enterpriseApi, log)
log.info({ msg: `Succesfully logged out from Garden Enterprise.` })
} catch (error) {
log.error(error)
Expand Down
53 changes: 27 additions & 26 deletions core/src/enterprise/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Collaborator

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.

Copy link
Member Author

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.

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)
}
Expand All @@ -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", {
Expand Down Expand Up @@ -207,26 +228,6 @@ export class EnterpriseApi {
return token
}

async logout() {
Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

I moved the logic to be out of the EnterpriseApi class to maintain consistency with the login function

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.
*/
Expand Down
25 changes: 18 additions & 7 deletions core/src/enterprise/auth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -55,13 +57,22 @@ export async function login(enterpriseApi: EnterpriseApi, log: LogEntry): Promis
}

export async function logout(enterpriseApi: EnterpriseApi, log: LogEntry): Promise<void> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function isn't used anywhere.

Copy link
Member Author

@10ko 10ko Jan 19, 2021

Choose a reason for hiding this comment

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

I updated the PR, I left out the LogOutCommand

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) })
}
}

Expand Down