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

Add Github auth for queues ui #3782

Merged
merged 6 commits into from
Feb 16, 2022
Merged

Conversation

davemcorwin
Copy link
Contributor

Changes proposed in this pull request:

  • Configures Github authentication for the Queues UI Enable Github authentication for queues ui #3780
  • Some refactoring to partly disentangle the queues ui from the rest of the app, at least some configuration
  • use FederalistLocal GH org and OAuth apps for local development

security considerations

Allowing Github authentication for queues UI based on membership in federalist-admins GH org, same as admin interface.

Other thoughts

For the config that has been updated, the architecture reflects where I think we should be going with configuration.

  1. The environment (process.env) should be source of truth so we can easily configure on CG, Docker, and local.
  2. IFF the app is running on CG, map CG json variables (VCAP_APPLICATION and VCAP_SERVICES) to environment variables. Typically this would be with a .profilefile, but since the Queues UI isn't deployed all by it's lonesome quite yet, we useenv.js` to do this.
  3. If necessary, flesh out (and maybe validate) the configuration from the environment into js (config.js) for easy consumption.

@davemcorwin davemcorwin requested a review from a team February 15, 2022 23:42
@davemcorwin davemcorwin self-assigned this Feb 15, 2022
@davemcorwin
Copy link
Contributor Author

must fix tests....

Copy link
Contributor

@svenaas svenaas left a comment

Choose a reason for hiding this comment

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

I've looked this over and don't have any suggestions, but this is my first time looking at the bull-board code. @apburnes, maybe you should have final approval on this one?

Note that if we do approve it we'll want to decide whether it gets merged before #3768 goes into main.

Copy link
Contributor

@apburnes apburnes left a comment

Choose a reason for hiding this comment

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

👍

@apburnes apburnes self-requested a review February 16, 2022 20:17
@apburnes
Copy link
Contributor

apburnes commented Feb 16, 2022

👍

Tests aren't running because we aren't pulling in redis creds for bull-board.

if (process.env.NODE_ENV === 'production') {
const cfenv = require('cfenv'); // eslint-disable-line global-require
const appEnv = cfenv.getAppEnv();
const {
Copy link
Contributor

@apburnes apburnes Feb 16, 2022

Choose a reason for hiding this comment

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

I cannot hydrate these when running the tests locally. We use the config/test.js convention for other parts of the app but bull-board is a bit different since it may also be broken out into its own entity through something like the monorepo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, i started going down a bit of a yak-shave in my head for how we should handle env vars and config...

@davemcorwin davemcorwin merged commit 5b9ca00 into staging Feb 16, 2022
@davemcorwin davemcorwin deleted the 3780-github-auth-for-queues-ui branch February 16, 2022 22:56
davemcorwin added a commit that referenced this pull request Jun 1, 2022
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.

None yet

3 participants