fix: redirect unauthorized git users to login screen #10995
Conversation
| @@ -91,7 +91,7 @@ export const SignInForm: FC<React.PropsWithChildren<SignInFormProps>> = ({ | |||
| </div> | |||
| )} | |||
|
|
|||
| {Boolean(info) && Boolean(error) && ( | |||
| {Boolean(info) && ( | |||
There was a problem hiding this comment.
@aslilac just want to double-check we don't need this error check here.
coderd/userauth.go
Outdated
| httpapi.Write(ctx, rw, http.StatusUnauthorized, codersdk.Response{ | ||
| Message: "You aren't a member of the authorized Github organizations!", | ||
| }) | ||
| httpmw.RedirectToLogin(rw, r, &url.URL{Path: "/login"}, "You aren't a member of the authorized Github organizations!") |
There was a problem hiding this comment.
if we're ok with this pattern, I can redirect similarly on the other oauth errors.
There was a problem hiding this comment.
One issue with this function is that it sets ?redirect=${currentPath}, which we don't want to do. If you could change the httpmw.RedirectToLogin function to accept a nil *http.Request and avoid adding the redirect query param then it should solve the problem
coderd/userauth.go
Outdated
| httpapi.Write(ctx, rw, http.StatusUnauthorized, codersdk.Response{ | ||
| Message: "You aren't a member of the authorized Github organizations!", | ||
| }) | ||
| httpmw.RedirectToLogin(rw, r, &url.URL{Path: "/login"}, "You aren't a member of the authorized Github organizations!") |
There was a problem hiding this comment.
| httpmw.RedirectToLogin(rw, r, &url.URL{Path: "/login"}, "You aren't a member of the authorized Github organizations!") | |
| httpmw.RedirectToLogin(rw, r, nil, "You aren't a member of the authorized Github organizations!") |
dashboardURL defaults to /login if nil
| @@ -62,7 +62,7 @@ export const useSingleSignOnSection = () => { | |||
| // The redirect on success should be back to the login page with a nice message. | |||
| // The user should be logged out if this worked. | |||
| encodeURIComponent( | |||
| `/login?info=Login type has been changed to ${loginTypeMsg}. Log in again using the new method.`, | |||
| `/login?message=Login type has been changed to ${loginTypeMsg}. Log in again using the new method.`, | |||
There was a problem hiding this comment.
this'll evaluate to "%2Flogin%3Fmessage%3DLogin%20type%20...", which I'm guessing is what we want but this diff doesn't give much context. if it was fine before though it's probably ok!
| @@ -91,9 +91,9 @@ export const SignInForm: FC<React.PropsWithChildren<SignInFormProps>> = ({ | |||
| </div> | |||
| )} | |||
|
|
|||
| {Boolean(info) && Boolean(error) && ( | |||
| {Boolean(message) && ( | |||
There was a problem hiding this comment.
| {Boolean(message) && ( | |
| {message && ( |
nit: technically this cast is superfluous
| @@ -63,7 +63,7 @@ export interface SignInFormProps { | |||
| isSigningIn: boolean; | |||
| redirectTo: string; | |||
| error?: unknown; | |||
| info?: string; | |||
| message?: string; | |||
There was a problem hiding this comment.
if this was message?: ReactNode;...
| @@ -24,7 +24,8 @@ export const LoginPageView: FC<LoginPageViewProps> = ({ | |||
| const redirectTo = retrieveRedirect(location.search); | |||
| // This allows messages to be displayed at the top of the sign in form. | |||
| // Helpful for any redirects that want to inform the user of something. | |||
| const info = new URLSearchParams(location.search).get("info") || undefined; | |||
| const message = | |||
| new URLSearchParams(location.search).get("message") || undefined; | |||
There was a problem hiding this comment.
...then you wouldn't need this || undefined!
https://www.loom.com/share/cfb530114a1e421e8ee388adc83e5f7a?sid=efce074f-034f-43e4-9132-9fc863fb658b
resolves #10902