Skip to content

Conversation

sfdctaka
Copy link
Contributor

@sfdctaka sfdctaka commented Jun 8, 2024

What does this PR do?

Created a class that generates 256-bit, CSPRNG-generated identity token associated with the local dev server. After the token is created it is saved in the file, config.json, which is a file sf CLI saves configuration in by default.

I have purposely omitted for now to call the method IdentityUtils.updateConfigWithIdentityToken() from any command until it finds an appropriate place to be called.

Created a class that generates 256-bit, CSPRNG-generated identity token associated with the local dev server. After the token is created it is saved in the file, lwr.config.json, if it exists.

I have purposely omitted for now to call the method IdentityUtils.createIdentityToken() from any command since there is no lwr.config.json created anywhere yet.

What issues does this PR fix or reference?

@W-15818521@

@sfdctaka sfdctaka requested a review from a team as a code owner June 8, 2024 03:58
@sfdctaka
Copy link
Contributor Author

@maliroteh-sf For now I went with your idea on handling the error case with config file. All of your comments are addressed. If @khawkins have another idea we can take care of it in this PR or in a later one. As long as you're not blocked.

@khawkins
Copy link
Collaborator

Can we rebase against main? It would be good to see these changes in relation to the dev server integration changes that came in from #49.

@sfdctaka sfdctaka force-pushed the webServerIdentity branch from 211963d to dba95d4 Compare June 12, 2024 18:32
return Promise.resolve(undefined);
}

public static async writeIdentityToken(token: string): Promise<void> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method could be private but when I do that I won't be able to stub it in the unit test.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is ok to leave it public.... it is a function in a utility library that could be on use in the future

Copy link
Contributor

Choose a reason for hiding this comment

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

I would however update the method signature to take an optional config param in addition to the token.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@maliroteh-sf I didn't add optional config in "write" because now we need to deal separate "configs" depending on "writing" to or "reading" from sf's configuration. Making config optional and delegate that responsibility to the called is certainly an approach that could have been taken. But with given inflexibility with Config's APIs and their shortcomings I think the safest way for now for us is to encapsulate the different configs(Config and ConfigAggregator) in respective "write" and "read" methods.

Comment on lines 28 to 31
const config = await LightningDevConfig.create({
isGlobal: false,
});
await config.read();
Copy link
Contributor

Choose a reason for hiding this comment

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

I would update the method signature to take an optional config param so that we don't have to re-read the config if it was already read before. Then you can check to see if config param was passed in or not and if not only then proceed with these lines. This way we can reduce disk access/read

sfdctaka and others added 2 commits June 20, 2024 10:44
Co-authored-by: Abdulsattar Mohammed <abdulsattarmohammed@salesforce.com>
@sfdctaka sfdctaka merged commit 412019c into salesforcecli:main Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants