Skip to content

Add OpenID Connect integration#142

Merged
alexashley merged 13 commits intomainfrom
express
Jul 7, 2021
Merged

Add OpenID Connect integration#142
alexashley merged 13 commits intomainfrom
express

Conversation

@alexashley
Copy link
Copy Markdown
Contributor

@alexashley alexashley commented Jun 29, 2021

Remaining work

  • add login/logout buttons
  • display authenticated username and maybe role
  • expose auth-related config
  • don't require login
  • tests for express integration
  • change server-side code to use ESM
  • update Dockerfile
  • set the next dev flag dynamically

Those first two will be a follow up, navigating to /login or /logout should work in the meantime. Auth is disabled by default.

Closes #140

Comment thread server.js Outdated
@alexashley alexashley marked this pull request as ready for review July 7, 2021 14:17
process.env.RODE_URL || "http://localhost:50051";

const fetch = (endpoint, method, body) => {
const fetch = ({ endpoint, method, body, accessToken }) => {
Copy link
Copy Markdown
Contributor

@pickjasmine pickjasmine Jul 7, 2021

Choose a reason for hiding this comment

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

Picky: would it be a good idea to keep the getRodeUrl() function and have that call out to config.get("rode.url") so if the config key ever changes, the change is only needed in one place?

The get/post/etc utils could also call to get this value since we're not calling anything other than Rode right now but that could change in the future, I suppose.

Copy link
Copy Markdown
Contributor Author

@alexashley alexashley Jul 7, 2021

Choose a reason for hiding this comment

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

I don't feel too strongly, but I think I'd rather have a wrapper around the get/ patch / del / post utils that is pre-configured to call Rode. That would eliminate a lot of these call sites for the config and maybe centralize some of the error handling.

Copy link
Copy Markdown
Contributor

@mrparkers mrparkers left a comment

Choose a reason for hiding this comment

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

One comment but otherwise LGTM.

Perhaps a future enhancement - it would be nice if the UI let the user know that a particular API call was unauthorized instead of displaying a generic server error. I don't expect this to happen in this PR though.

Comment thread Dockerfile Outdated
Co-authored-by: Michael Parker <michael@parker.gg>
@alexashley
Copy link
Copy Markdown
Contributor Author

Made some issues for follow up PRs: #155, #156

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.

Add support for OpenID Connect authn

3 participants