Skip to content

Follow returnTo redirect when logging in via OAuth

Warren Gifford requested to merge oauth-returnto into main

Created by: arussellsaw

This PR fixes https://github.com/sourcegraph/sourcegraph/issues/15132

So this change has a very small surface area for a lot of time spent digging, but i suppose that's on purpose as i'm now (fairly) intimately familiar with how our OAuth providers work.

I'm not 100% happy with how this change is implemented, as it feels somewhat fragile and counterintuitive to have this logic live within the sign in page view logic, but it was the change with the smallest surface area.

some other approaches i considered were:

  • Refactoring the auth Provider interface to accept arguments when returning the authentication URL. This felt like a pretty large change with a lot of dependencies that i don't yet know about, and didn't feel appropriate to tackle in my first day.
  • Pulling the returnTo param from the Referrer header in https://github.com/sourcegraph/sourcegraph/blob/eceb1cd9065249fabf0b50936300a59e0de4cdb2/enterprise/cmd/frontend/auth/oauth/provider.go#L98 this feels like a similar surface area to the change i've made, but feels slightly more fragile to subsequent changes. If we'd prefer to go this route i'm happy to make this change. of course i'm happy to make any change as this is my job and that's what i'm supposed to do 😂

EDIT: we decided to move this to the backend https://github.com/sourcegraph/sourcegraph/pull/15439#discussion_r518054930

Merge request reports

Loading