Update GitHub Auth Backend #78
Conversation
|
@msessa @Lucretius @alexunwin PTAL |
| // Misc | ||
| import _ from 'lodash'; | ||
| import update from 'immutability-helper'; | ||
| import Avatar from 'material-ui/Avatar'; |
There was a problem hiding this comment.
you separate by comments but this is material ui
| import VaultObjectDeleter from '../../shared/DeleteObject/DeleteObject.jsx' | ||
|
|
||
| function snackBarMessage(message) { | ||
| let ev = new CustomEvent("snackbar", { detail: { message: message } }); |
There was a problem hiding this comment.
no point in declaring a var here
| base_url: undefined, | ||
| max_ttl: undefined, | ||
| ttl: undefined | ||
| } |
There was a problem hiding this comment.
what is this? set defaults from a config or put some defaults here. No need to make undefined as that is the default behavior of not existing.
There was a problem hiding this comment.
It's documentation. Reference for the immutability helper on what properties this object should actually contain.
There was a problem hiding this comment.
Schema should be kept separate
There was a problem hiding this comment.
It's only there to mirror Vault's schema for the config object. Vault-UI doesn't dictate any new schema.
There was a problem hiding this comment.
Right, it only exists and is only valid for the scope of this code.
| if (error.response.status !== 404) { | ||
| snackBarMessage(error); | ||
| } else { | ||
| this.setState({ teams: [] }); |
There was a problem hiding this comment.
what if this is a different status code?
There was a problem hiding this comment.
Then it will display the error out in the snack bar, 404 is a specific exception which in this case means the config hasn't been initialized
There was a problem hiding this comment.
Not in this case, Vault has some predefined return codes for its restful API. 404s are returned in this case and is the only status we need to handle.
| this.setState({ users: _.valuesIn(users) }); | ||
| }) | ||
| .catch((error) => { | ||
| if (error.response.status !== 404) { |
There was a problem hiding this comment.
again here.. 404 is limiting
There was a problem hiding this comment.
Yea I only want to handle 404s =P
| let policies = this.state.policies; | ||
| render() { | ||
| let renderListItems = () => { | ||
| let items = this.state.selectedTab === "team" ? this.state.teams : this.state.users; |
There was a problem hiding this comment.
double quotes...get that es-lintrc plugin
| tooltip="Delete" | ||
| onTouchTap={() => this.setState({ deleteUserPath: `${this.state.baseVaultPath}/map/${this.state.selectedTab}s/${item}` })} | ||
| > | ||
| {window.localStorage.getItem("showDeleteModal") === 'false' ? <ActionDeleteForever color={red500} /> : <ActionDelete color={red500} />} |
There was a problem hiding this comment.
why not use falsy here?
There was a problem hiding this comment.
The value we're comparing to is of type string, so we can't use a falsy in this case.
| <List> | ||
| <Subheader>Assigned Policies</Subheader> | ||
| <PolicyPicker | ||
| height="250px" |
| /> | ||
| <Subheader>Assigned Policies</Subheader> | ||
| <PolicyPicker | ||
| height="250px" |
| this.setState({ newConfig: update(this.state.newConfig, { ttl: { $set: e.target.value } }) }); | ||
| }} | ||
| /> | ||
| <div style={{ paddingTop: '20px', textAlign: 'center' }}> |
There was a problem hiding this comment.
css...should not be hardcoded. is there a reason?
|
ran changes locally... 👍 for functionality. Some minor stuff |
I’m leaving the css off of this PR, we’ll revisit on a cleanup PR
This PR brings the GitHub auth backend configuration to parity with the rest of auth backends.