This repository has been archived by the owner on Feb 8, 2024. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 37
fowards path parameter to app access authentication #913
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zmb3
reviewed
Jun 25, 2022
hatched
approved these changes
Jun 27, 2022
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.
This looks good.
Could you add some more comments into this code as to why it's necessary though? Maybe a truncated version of our discussion offline?
avatus
force-pushed
the
michaelmyers/preserve-app-url
branch
from
June 27, 2022 17:43
6f00793
to
83d6ac1
Compare
kimlisa
reviewed
Jun 27, 2022
kimlisa
approved these changes
Jun 28, 2022
avatus
force-pushed
the
michaelmyers/preserve-app-url
branch
from
June 29, 2022 20:19
8ae7680
to
bf5c954
Compare
This was referenced Jul 1, 2022
avatus
added a commit
that referenced
this pull request
Jul 7, 2022
* fowards path parameter to app access authentication * add some comments and clearer naming convention * revert params change, single line comment
avatus
added a commit
that referenced
this pull request
Jul 7, 2022
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The Problem:
When a user navigates to an app url with a path included like https://app1.teleport.example.com/my/path without authentication, the path is lost during the authentication process and eventually just gets the root path instead.
This isn't a problem when launching from the Apps dashboard because 1, they are already authenticated, and 2, the launch button sends to the root path anyway. So the problem usually persists when someone is either sharing a link with a path included to a coworker, or when visiting a bookmark or whatever.
The current redirect URI only includes the host name (in this example above, it would be app1.teleport.example.com) because the /web/launch/:fqdn URI expects only the name of the app.
The Solution:
Because we cannot just simply pass the entire path to the redirect_uri, we have to include it as a query parameter. However, there is a bunch of redirects that happen between the Login screen and the final destination so we have to preserve that parameter the whole way through the process. Admittedly, most of this will happen in the api (not yet merged in). The code contained in this PR will help keep the
path
parameter during thex-teleport-auth
api calls. (seen here)Steps to reproduce:
Add an app to your teleport config if not already included
Make sure an app is running, for example
python3 -m http.server 8000
, and be sure to be unauthenticated.Visit
https://app1.your.teleport.url/example/path
In the current codebase, after authenticating, you'll lose the path and end up at
https://app1.your.teleport.url
.If you wanna check the teleport core side, make sure to checkout the branch