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(postgraphiql): save headers to localStorage #1174

Merged
merged 4 commits into from
Nov 7, 2019

Conversation

phryneas
Copy link
Contributor

@phryneas phryneas commented Nov 6, 2019

It was pretty annoying to me that PostGraphiQL "forgets" the headers at every page refresh (we have schema polling disabled).
This will store the headers in localStorage as soon as they're valid JSON and retrieve them on component creation.

@benjie
Copy link
Member

benjie commented Nov 6, 2019

I understand the frustration, but this is another convenience vs security issue; storing JWTs and other private header data to LocalStorage opens an attack path that the users of GraphiQL might not expect. I'm very uncomfortable making storing it the default behaviour. Maybe we can make it opt-in either via a CLI/library option, or with a UI option like a checkbox "save these headers ℹ️" above the headers input? The info button should explain why it's not enabled by default.

@phryneas
Copy link
Contributor Author

phryneas commented Nov 6, 2019

I guess this checkbox would calm your concerns? :)

Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

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

Looks good to me; a little refactoring and we can merge :)

this.state.headersText = headersText;
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Please move this code to the state = property initialiser above, and simplify it, e.g.

    saveHeadersText: this._storage.get(STORAGE_KEYS.SAVE_HEADERS_TEXT) === 'true' ? true : false,
    headersText: this._storage.get(STORAGE_KEYS.HEADERS_TEXT) || "",

(Note headersText doesn't need to respect saveHeadersText here - if it's in local storage then we should display it anyway. The code later should ensure that it's not stored unless it's checked.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. You sure you want an empty string instead of the default '{\n"Authorization": null\n}\n'up until now?

Copy link
Member

Choose a reason for hiding this comment

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

Use the existing default :) The above comment is just an approximation.

@benjie
Copy link
Member

benjie commented Nov 7, 2019

(Also please merge the latest master into this branch; we do git squashing so you don't need to worry about keeping the history linear.)

@phryneas phryneas requested a review from benjie November 7, 2019 13:51
@phryneas
Copy link
Contributor Author

phryneas commented Nov 7, 2019

done :)

Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

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

Awesome; very useful feature 🙌

@benjie benjie changed the title feat(postgraphiql) save headers to localStorage feat(postgraphiql): save headers to localStorage Nov 7, 2019
@benjie benjie merged commit 37abcd3 into graphile:master Nov 7, 2019
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.

2 participants