vikunja
/
vikunja
6
43
Fork 71
Code Issues 150 Pull Requests 16 Packages Releases 32 Activity

Add support to login using identity from an identity-aware proxy #715

Closed
branchmispredictor wants to merge 9 commits from branchmispredictor/api:feature/identity-aware-proxy into main
pull from: branchmispredictor/api:feature/identity-aware-proxy
merge into: vikunja:main
Conversation 14 Commits 9 Files Changed 34 +3 -5
2 changed files with 3 additions and 5 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 213a7db661 - Show all commits

4
pkg/modules/auth/auth.go
View File

@ -228,8 +228,8 @@ func RenewToken(s *xorm.Session, c echo.Context) (token string, err error) {
}
return NewUserJWTAuthtoken(u)
}
if authProvider, ok := authProviders[claims.Type]; ok {
return "", echo.NewHTTPError(http.StatusBadRequest, models.Message{Message: "External auth types do not use JWT tokens."}
if _, ok := authProviders[claims.Type]; ok {
return "", echo.NewHTTPError(http.StatusBadRequest, models.Message{Message: "External auth types do not use JWT tokens."})
}
return "", echo.NewHTTPError(http.StatusBadRequest, models.Message{Message: "Invalid JWT token."})
}

4
pkg/modules/auth/identityawareproxy/identityawareproxy.go
View File

@ -21,8 +21,6 @@ import (
"time"
"code.vikunja.io/api/pkg/db"
"code.vikunja.io/api/pkg/config"
"code.vikunja.io/api/pkg/modules/auth"
"code.vikunja.io/api/pkg/user"
"github.com/dgrijalva/jwt-go"
@ -83,7 +81,7 @@ func newIAPUserJWTAuthClaims(u *user.User) (claims *auth.AuthClaims) {
func (p IAPAuthProvider) GetUser(c echo.Context, authClaims *auth.AuthClaims) (*user.User, error) {
// The IAP middleware already checked and created a user if needed, no need to regenerate them
// Just use the authClaims provided by the middleware
u = &user.User{
u := &user.User{
konrad commented 2021-02-14 10:58:54 +00:00
Outdated
Review
Copy Link

Is there a special reason this is a separate endpoint instead of reusing the existing token one? It seems like this will require a frontend change?

Is there a special reason this is a separate endpoint instead of reusing the existing token one? It seems like this will require a frontend change?
branchmispredictor commented 2021-05-17 19:56:54 +00:00
Outdated
Review
Copy Link

There's a problem with bootstrapping auth here. The way IAPs work is by setting an http header with claims to the downstream service (vikunja-api), however javascript or front-end code does not have access to http headers.

So on initial load, there is no cookie / vikunja-jwt set so the frontend does not see us as logged in. Similarly, the existing /user/token endpoint is also unavailable because it is behind the jwt middleware and requires a jwt cookie set. So this endpoint exists outside of the jwt middleware so it can be accessed from before a user is "logged in"

There's a problem with bootstrapping auth here. The way IAPs work is by setting an http header with claims to the downstream service (vikunja-api), however javascript or front-end code does not have access to http headers. So on initial load, there is no cookie / vikunja-jwt set so the frontend does not see us as logged in. Similarly, the existing `/user/token` endpoint is also unavailable because it is behind the jwt middleware and requires a jwt cookie set. So this endpoint exists outside of the jwt middleware so it can be accessed from before a user is "logged in"
konrad commented 2021-05-17 20:01:10 +00:00
Outdated
Review
Copy Link

So this is the endpoint to "translate" the http header from the IAP to a Vikunja token? Wouldn't it be possible to just extend the existing middleware to look for that http header? All that'd be left then would be to extend the frontend so it somehow checks if the user is logged in (maybe by calling /api/v1/user and see if there's a 200 response coming back and then setting the frontend state to "logged in"?)

So this is the endpoint to "translate" the http header from the IAP to a Vikunja token? Wouldn't it be possible to just extend the existing middleware to look for that http header? All that'd be left then would be to extend the frontend so it somehow checks if the user is logged in (maybe by calling `/api/v1/user` and see if there's a 200 response coming back and then setting the frontend state to "logged in"?)
branchmispredictor commented 2021-05-17 20:03:37 +00:00
Outdated
Review
Copy Link

What I can do, is abtract this away a bit and make this an /auth/externalprovider/loggedin endpoint, so any future external auth source might also reuse the same endpoint.

Was writing that before I saw the reply. Yes, this basically translates the http header from the IAP to a vikunja token. The only problem is that the jwt middleware will error out for /api/v1/user/* if the frontend does not provide an already valid jwt as a cookie

~~What I can do, is abtract this away a bit and make this an `/auth/externalprovider/loggedin` endpoint, so any future external auth source might also reuse the same endpoint.~~ Was writing that before I saw the reply. Yes, this basically translates the http header from the IAP to a vikunja token. The only problem is that the jwt middleware will error out for /api/v1/user/* if the frontend does not provide an already valid jwt as a cookie
konrad commented 2021-05-17 20:04:48 +00:00
Outdated
Review
Copy Link

How would that look in the frontend, would it call that endpoint every time on reload?

How would that look in the frontend, would it call that endpoint every time on reload?
branchmispredictor commented 2021-05-17 20:50:16 +00:00
Outdated
Review
Copy Link

Yes, or at least if not already logged in / if current jwt token is expired. Essentially it's similar to an open-id auth from the frontend's point of view except it 1) is hitting vikunja-backend directly instead of first hitting an external service and 2) it is automatically calling the endpoint instead of waiting for the openid button to be hit.

We can actually make num 2 optional, if for whatever reason we prefer being able to have both IAP login and local/openid logins too.

Yes, or at least if not already logged in / if current jwt token is expired. Essentially it's similar to an open-id auth from the frontend's point of view except it 1) is hitting vikunja-backend directly instead of first hitting an external service and 2) it is automatically calling the endpoint instead of waiting for the openid button to be hit. We can actually make num 2 optional, if for whatever reason we prefer being able to have both IAP login and local/openid logins too.
konrad commented 2021-05-17 20:54:21 +00:00
Outdated
Review
Copy Link

Then I'd prefer modifying the existing jwt middleware to accept a jwt token or an IAP header if no token is provided (and move all the logic to create a new user etc there as well). That way, we won't need the new endpoint and can modify the frontend only a bit to make a call to /user every time it is loaded, regardless if it has a jwt token in localstorage or not. I think that would make the IAP a lot more transparent to the frontend.

Then I'd prefer modifying the existing jwt middleware to accept a jwt token or an IAP header if no token is provided (and move all the logic to create a new user etc there as well). That way, we won't need the new endpoint and can modify the frontend only a bit to make a call to `/user` every time it is loaded, regardless if it has a jwt token in localstorage or not. I think that would make the IAP a lot more transparent to the frontend.
branchmispredictor commented 2021-05-17 21:10:15 +00:00
Outdated
Review
Copy Link

Okay, I'll give it a shot.

Okay, I'll give it a shot.
branchmispredictor commented 2021-05-17 21:35:11 +00:00
Outdated
Review
Copy Link

Following up just to make sure I capture the implicit decisions here, so you want for the backend:

  1. If a valid jwt token is presented from the frontend, use that (e.g. ignore the IAP header)
  2. If no valid jwt token exists, create one from the IAP header. In the backend, treat this newly generated jwt token the same as if it was provided by the frontend

In the frontend:

  1. Call /user/token on load -> will get the jwt token generated from the IAP header
Following up just to make sure I capture the implicit decisions here, so you want for the backend: 1. If a valid jwt token is presented from the frontend, use that (e.g. ignore the IAP header) 2. If no valid jwt token exists, create one from the IAP header. In the backend, treat this newly generated jwt token the same as if it was provided by the frontend In the frontend: 1. Call /user/token on load -> will get the jwt token generated from the IAP header
konrad commented 2021-05-18 10:02:52 +00:00
Outdated
Review
Copy Link

Almost, I wouldn't generate a jwt token from the backend at all but instead use the IAP header to authenticate the user.

That would look like this in the middleware:

  1. If a valid jwt token is presented from the frontend, use that
  2. If a valid IAP header is present, use that as a means to authenticate the user in place of a jwt token. This includes creating the user internally if none exists yet. There may be a few places which would need to be a bit more abstracted to not only rely on a jwt token to get the user information.

The frontend would then always call user to figure out if the user is authenticated or not. Currently it only does that if a jwt token is present in local storage.

Almost, I wouldn't generate a jwt token from the backend at all but instead use the IAP header to authenticate the user. That would look like this in the middleware: 1. If a valid jwt token is presented from the frontend, use that 2. If a valid IAP header is present, use that as a means to authenticate the user in place of a jwt token. This includes creating the user internally if none exists yet. There may be a few places which would need to be a bit more abstracted to not only rely on a jwt token to get the user information. The frontend would then always call [`user`](https://try.vikunja.io/api/v1/docs#tag/user/paths/~1user/get) to figure out if the user is authenticated or not. Currently it only does that if a jwt token is present in local storage.
ID: authClaims.UserID,
Email: authClaims.UserEmail,
konrad commented 2020-11-26 19:39:55 +00:00
Outdated
Review
Copy Link

What if there's more than one Key? Is that even possible? In that case, should we still just use the first one or return a different error?

What if there's more than one Key? Is that even possible? In that case, should we still just use the first one or return a different error?
Username: authClaims.UserUsername,