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 UI boilerplate with sign up, sign in and ability to list clusters #140

Merged
merged 23 commits into from
Jul 30, 2021

Conversation

jmorganca
Copy link
Contributor

@jmorganca jmorganca commented Jul 30, 2021

This looks to be a pretty hefty PR, but much of the code is generated or boilerplate. The three main additions:

  • A next.js UI under ./internal/ui
  • Modifications to ./internal/registry to serve the UI as static website
  • GRPC-gateway to translate GRPC requests to REST/JSON for browsers to use (namely our UI)

This fixes #123 in passing

Screenshots

Below are some screenshots @BruceMacD @mchiang0610

Screen Shot 2021-07-29 at 10 45 53 PM

Screen Shot 2021-07-29 at 10 45 58 PM

Screen Shot 2021-07-29 at 11 11 05 PM

Screen Shot 2021-07-29 at 10 48 30 PM

Screen Shot 2021-07-29 at 10 48 34 PM

Screen Shot 2021-07-29 at 10 48 43 PM

Screen Shot 2021-07-29 at 11 01 50 PM

@jmorganca jmorganca requested a review from BruceMacD July 30, 2021 02:24
Expires: expires,
HttpOnly: true,
Path: "/",
SameSite: http.SameSiteStrictMode,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case I opted to not make the cookies Secure - which means they work over plaintext http. While this is much easier for self-hosted setups who don't want to deal with the certificate issue, it might be better if we just force everything to be https. Thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Any way we can check if there is a certificate and set the secure flag based on that? Otherwise I think it is better to force it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. I'll commit as-is and then create an issue to enforce secure cookies and redirect http to https automatically: #143 in case users don't set this up on their side

This may have a few issues with usability (e.g. accessing via port forwarding) we should double check so will treat it as it's own issue for now vs amending that here

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good to me

@@ -38,3 +49,87 @@ func (h *Http) WellKnownJWKs(w http.ResponseWriter, r *http.Request) {
[]jose.JSONWebKey{pubKey},
})
}

func (h *Http) loginRedirectMiddleware(next http.Handler) http.Handler {
Copy link
Contributor Author

@jmorganca jmorganca Jul 30, 2021

Choose a reason for hiding this comment

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

This middleware is used to avoid "flashes of content" by doing server-side redirects to the /login route based on auth info (instead of client side redirects, which is what most products do, and is very ugly).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Love this

@jmorganca jmorganca changed the title Add UI boilerplate with sign up, sign in and listing clusters Add UI boilerplate with sign up, sign in and ability to list clusters Jul 30, 2021
@@ -429,6 +433,26 @@ func NewToken(db *gorm.DB, userId string, token *Token) (secret string, err erro
return
}

func ValidateAndGetToken(db *gorm.DB, in string) (*Token, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is extracted from grpc.go and unmodified so it can be shared with the loginRedirectMiddleware below

Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't need to be part of this PR, but we should add some tests for this token validation too at some point

@BruceMacD
Copy link
Collaborator

nit: Should the UI be under ./internal/registry/ui where it is specifically a UI for the registry? Not a big deal though.

Copy link
Collaborator

@BruceMacD BruceMacD left a comment

Choose a reason for hiding this comment

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

Lets go

@@ -429,6 +433,26 @@ func NewToken(db *gorm.DB, userId string, token *Token) (secret string, err erro
return
}

func ValidateAndGetToken(db *gorm.DB, in string) (*Token, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't need to be part of this PR, but we should add some tests for this token validation too at some point

@@ -38,3 +49,87 @@ func (h *Http) WellKnownJWKs(w http.ResponseWriter, r *http.Request) {
[]jose.JSONWebKey{pubKey},
})
}

func (h *Http) loginRedirectMiddleware(next http.Handler) http.Handler {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Love this

Expires: expires,
HttpOnly: true,
Path: "/",
SameSite: http.SameSiteStrictMode,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any way we can check if there is a certificate and set the secure flag based on that? Otherwise I think it is better to force it.

]

return (
<div className="h-screen bg-gray-50 overflow-hidden flex">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we split the sidebars and header out to separate components?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so, but only if we re-use them elsewhere. Otherwise the thinking is to keep it one (albeit big) Layout component.

if (process.browser && cookies.login) {
router.replace("/")
return <></>
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: this } isn't correctly aligned 👀

</div>
<div className="my-2.5">
<label htmlFor="password" className="block text-sm font-medium text-gray-700">
Password
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should have a confirm password here too and do some client side validation to make sure they match.

id="password"
name="password"
type="password"
autoComplete="current-password"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure we want auto-complete here

rpc CreateUser(CreateUserRequest) returns (User) {
option (google.api.http) = {
post: "/v1/users"
body: "*"
Copy link
Collaborator

Choose a reason for hiding this comment

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

These body fields are wild-carded because we are looking at request params?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, to my knowledge this tells the grpc gateway to map http body params to grpc params

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.

Http logging
2 participants