-
Notifications
You must be signed in to change notification settings - Fork 1
Add Oauth2 Authorization Code + Login Page #23
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
base: main
Are you sure you want to change the base?
Conversation
draft: looking for some feedback on design; assume things are not final |
@KCui0327 @ambroseling @elwincheng for thoughts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Where is logout functionality?
- I find it a little hard to follow the auth flow in code with no clear separation between the OAuth2 and session auth patterns mixed
- would help to make a diagram showing how the user interacts with the auth system and the control that is going on
|
||
import ( | ||
"context" | ||
"encoding/json" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused import
supervisor := NewSupervisor(redisAddr, consumerID, gpuType) | ||
manager := manage.NewDefaultManager() | ||
manager.SetAuthorizeCodeTokenCfg(manage.DefaultAuthorizeCodeTokenCfg) | ||
manager.MustTokenStorage(store.NewMemoryTokenStore()) // TODO: move to redis? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my understanding currently, if the API gateway crashes and restarts then users will need to get new tokens. Therefore, I would suggest moving to redis
func (a *App) jsonResponse(w http.ResponseWriter, statusCode int, response APIResponse) { | ||
w.Header().Set("Content-Type", "application/json") | ||
w.WriteHeader(statusCode) | ||
json.NewEncoder(w).Encode(response) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to handle error here
|
||
func (a *App) refresh(w http.ResponseWriter, r *http.Request) { | ||
fmt.Fprintf(w, "Hello, world!\n") | ||
// TODO: move this to a file? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move it to a file
return id, nil | ||
} | ||
|
||
func (a *App) getSession(session_id string) (string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use camel case
if strings.HasPrefix(authHeader, "Session ") { | ||
return strings.TrimPrefix(authHeader, "Session ") | ||
} else { | ||
if cookie, err := r.Cookie("session"); err == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing indent
wg sync.WaitGroup | ||
} | ||
|
||
func NewApp(redisAddr, gpuType string) *App { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the purpose of gpuType here?
@@ -1,47 +1,87 @@ | |||
package main | |||
|
|||
import ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's decouple some of the logic from this file to other new or existing files
) | ||
|
||
// creates a user session in redis | ||
func (a *App) CreateSession(uid string) (string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still need to add session expiration and proper session structure right?
No description provided.