Skip to content

Handle auth in front end #142

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

Merged
merged 8 commits into from
Mar 6, 2018
Merged

Handle auth in front end #142

merged 8 commits into from
Mar 6, 2018

Conversation

dpordomingo
Copy link
Contributor

@dpordomingo dpordomingo commented Feb 20, 2018

Fix #99

major changes:

  • new route on the front side: /auth:
    • it will receive the state and code from GitHub OAuth callback,
    • it will call through AJAX the (rename) backend /api/auth endpoint, and handle any possible error,
  • UI_DOMAIN is no longer needed in Helm charts

benefits:

  • the backend /api/auth endpoint will be standard in terms of our handler.RequestProcessFunc,
  • the backend will communicate with the front end as the rest of the app:
    FE → BE → response + status

possible improvements:

  • I realized that /auth route is not really-really needed because the auth thing could be handled by / itself if code is received.

Copy link
Contributor

@smacker smacker left a comment

Choose a reason for hiding this comment

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

  1. You can't read POST payload in javascript

So your idea FE->BE->reponse doesn't work.

  1. You don't handle errors from provider:
    https://tools.ietf.org/html/rfc6749#section-4.1.2.1

And we have to provide those errors to the user because those are problems between user&provider, not our app.

@dpordomingo
Copy link
Contributor Author

  • There is no post payload to read,
  • errors during auth phase are returned to the FE as usual.

@dpordomingo
Copy link
Contributor Author

dpordomingo commented Feb 21, 2018

It must be rebased over master and refactored considering #144

@bzz bzz requested a review from carlosms February 28, 2018 12:38
@dpordomingo dpordomingo assigned dpordomingo and unassigned smacker Mar 2, 2018
@dpordomingo dpordomingo changed the title [RFC] Handle auth errors in front end Handle auth in front end Mar 5, 2018
@dpordomingo dpordomingo requested a review from smacker March 5, 2018 09:28
@dpordomingo
Copy link
Contributor Author

dpordomingo commented Mar 5, 2018

PR rebased over current master.

This PR should not be merged till it can be coordinated with #201

PR now includes changes proposed by @smacker at #150:

It was also included some minors:

Copy link
Contributor

@smacker smacker left a comment

Choose a reason for hiding this comment

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

Oh. You cherry-picked my commits. Okay.

One small comment about error handling.

@@ -68,16 +68,18 @@ type GithubUser struct {
}

// MakeAuthURL returns string for redirect to provider
func (o *OAuth) MakeAuthURL(w http.ResponseWriter, r *http.Request) string {
func (o *OAuth) MakeAuthURL(w http.ResponseWriter, r *http.Request) (string, error) {
b := make([]byte, 16)
rand.Read(b)
state := base64.URLEncoding.EncodeToString(b)

session, _ := o.store.Get(r, "sess")
Copy link
Contributor

Choose a reason for hiding this comment

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

if you return (string, error), it makes sense to handle error here too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yesssss, you're totally right; thanks for spotting it!

@dpordomingo
Copy link
Contributor Author

Ready for a second pass @bzz @smacker

@dpordomingo dpordomingo merged commit d2dcb2e into src-d:master Mar 6, 2018
@dpordomingo dpordomingo deleted the auth branch March 6, 2018 17:10
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.

3 participants