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

create: make the default session secret default in all contexts #1123

Merged
merged 3 commits into from
Jul 28, 2024

Conversation

jordigh
Copy link
Contributor

@jordigh jordigh commented Jul 24, 2024

The problem here is that making it this optional meant that it wasn't supplied by the enterprise creation
function
. This resulted in an odd situation where the secret was required for the enterprise edition, even though it offers no additional security. Without this key, the enterprise code crashes.

The requirement to supply a secret key would make a Grist instance crash if you start in normal mode but switch to enterprise, as the enterprise creator does not supply a default secret key.

@jordigh jordigh force-pushed the jordigh/default-secret branch from 3ae3e7f to f12138a Compare July 24, 2024 19:17
@berhalak
Copy link
Contributor

berhalak commented Jul 25, 2024

Can you change the title? Or explain what it means?
Also, can you write a summary in the first comment, what is actually change here.

@berhalak berhalak self-requested a review July 25, 2024 13:15
@jordigh jordigh changed the title create: hard-code the default session secret even more create: make the default session secret default in all contexts Jul 25, 2024
@jordigh
Copy link
Contributor Author

jordigh commented Jul 25, 2024

Is that title better?

@berhalak
Copy link
Contributor

Is that title better?

Yes, thank you. And what does create: stands for?

app/server/lib/ICreate.ts Show resolved Hide resolved
app/server/lib/ICreate.ts Outdated Show resolved Hide resolved
@jordigh
Copy link
Contributor Author

jordigh commented Jul 25, 2024

Is that title better?

Yes, thank you. And what does create: stands for?

The general topic of the commit. This commit is about the create family of functions.

Each commit should describe what it's about so that reading git history is meaningful without reference to github or Phabricator. The commit messages should be useful on their own.

@berhalak
Copy link
Contributor

Is that title better?

Yes, thank you. And what does create: stands for?

The general topic of the commit. This commit is about the create family of functions.

Each commit should describe what it's about so that reading git history is meaningful without reference to github or Phabricator. The commit messages should be useful on their own.

Is this some general guideline? Can you share a link to it?

The problem here is that making it this optional meant that it wasn't
supplied by [the enterprise creation
function](https://github.com/gristlabs/grist-ee/blob/fb22d94878a539ec9f1085fa9ac12936ccb68dca/ext/app/server/lib/create.ts#L10).
This resulted in an odd situation where the secret was required for
the enterprise edition, even though it offers no additional security.
Without this key, the enterprise code crashes.

The requirement to supply a secret key would make a Grist instance
crash if you start in normal mode but switch to enterprise, as the
enterprise creator does not supply a default secret key.
@jordigh jordigh force-pushed the jordigh/default-secret branch from f12138a to 6d34286 Compare July 26, 2024 16:34
Copy link
Member

@paulfitz paulfitz left a comment

Choose a reason for hiding this comment

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

That looks right to me. Sorry the SaaS tied your hands for cleaning up more. Although the hard-coded secret has always been there and you are just moving it, could you add a comment to it summarizing our understanding of its significance?

app/server/lib/ICreate.ts Show resolved Hide resolved
@jordigh jordigh force-pushed the jordigh/default-secret branch 2 times, most recently from 6e239a8 to 37fe2c3 Compare July 26, 2024 17:37
Copy link
Contributor

@berhalak berhalak left a comment

Choose a reason for hiding this comment

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

Thanks.

app/server/lib/ICreate.ts Show resolved Hide resolved
@jordigh jordigh merged commit fea7c0b into main Jul 28, 2024
11 checks passed
@jordigh jordigh deleted the jordigh/default-secret branch July 28, 2024 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants