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

feat: display the auth secret when creating an environment #219

Merged
merged 8 commits into from
Sep 2, 2021

Conversation

slimee
Copy link
Contributor

@slimee slimee commented Sep 1, 2021

Definition of Done

General

  • Write an explicit title for the Pull Request, following Conventional Commits specification
  • Test manually the implemented changes
  • Validate the code quality (indentation, syntax, style, simplicity, readability)

Security

  • Consider the security impact of the changes made

@@ -71,6 +71,7 @@ const preparePlan = ({
env,
prompts,
tokenBehavior,
additionnalStep,
Copy link
Contributor Author

@slimee slimee Sep 1, 2021

Choose a reason for hiding this comment

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

the diff in this file is to support the additionnalStep keyword, which adds a step at the end of the plan.

@@ -59,7 +59,9 @@ function errorIfStdRest(stds) {
}
}

function errorIfBadCommand({ commandClass, commandArgs, commandPlan }) {
function errorIfBadCommand({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the diff in this file is to support the 'additionnalStep' keyword, which adds a step at the end of the plan.

@@ -66,6 +66,7 @@ async function testCli({
commandClass,
commandArgs,
commandPlan: testCommandPlan,
additionnalStep,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the diff in this file is to support the additionnalStep keyword, which adds a step at the end of the plan.

@forest-bot
Copy link
Member

@slimee slimee requested a review from jeffladiray September 1, 2021 15:12
Copy link
Member

@jeffladiray jeffladiray left a comment

Choose a reason for hiding this comment

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

LGTM, but
image
FOREST_AUTH_SECRET seems a bit long compared to the one generated on the frontend.

That's not an issue, but for consistency, I would suggest to reduce the length to 48 (Just like the frontend does)

@@ -36,7 +38,7 @@ function EnvironmentManager(config) {
this.createEnvironment = async () => {
const authToken = authenticator.getAuthToken();

return agent
const environment = await agent
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we totally migrate this one to async/await ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why not!

@slimee slimee requested a review from jeffladiray September 2, 2021 14:27
@@ -7,10 +7,11 @@ class KeyGenerator {
crypto,
});
this.crypto = crypto;
this.length = 24;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a small comment on the fact that randomBytes(24).toString('hex') generate a 48 characters string might be useful here ...
Or maybe that's just me that is not super familiar with the crypto package 😅.

Node 15 introduce crypto.getRandomValues(), via the web crypto api that would have been better here :)

Copy link
Member

@jeffladiray jeffladiray left a comment

Choose a reason for hiding this comment

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

Look good, works as expected.

Feel free to ignore my comment if you don't find it useful, maybe that's just me :D

@slimee slimee merged commit 6320f5d into main Sep 2, 2021
@slimee slimee deleted the feat/environment-create-add-auth-secret branch September 2, 2021 14:47
forest-bot added a commit that referenced this pull request Sep 2, 2021
# [2.3.0](v2.2.8...v2.3.0) (2021-09-02)

### Features

* display the auth secret when creating an environment ([#219](#219)) ([6320f5d](6320f5d))
@forest-bot
Copy link
Member

🎉 This PR is included in version 2.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants