Skip to content
This repository has been archived by the owner on Feb 8, 2024. It is now read-only.

Use dedicated API for app FQDN resolving #284

Merged
merged 7 commits into from
May 4, 2021

Conversation

andrejtokarcik
Copy link
Contributor

Fixes gravitational/teleport#6551, based on gravitational/teleport#6449.

I tried to incorporate the recommendations from #235, feel free to adapt further according to your coding conventions.

@andrejtokarcik andrejtokarcik force-pushed the andrej/fix/double-CreateAppSession branch from c8f7962 to b38f36d Compare April 26, 2021 21:18
Copy link
Contributor

@kimlisa kimlisa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andrejtokarcik thank you for remembering about the recommendations and applying it here! :)

packages/teleport/src/AppLauncher/useAppLauncher.ts Outdated Show resolved Hide resolved
@@ -76,6 +76,7 @@ const cfg = {

api: {
aapSession: '/v1/webapi/sessions/app',
aapFqdnPath: '/v1/webapi/apps/:fqdn/:clusterId?/:publicAddr?',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't really like how we interchange between aap and app. Can we keep it app @alex-kovoy ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @kimlisa, lets keep it consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a general aap => app rename, hope that's alright!

packages/teleport/src/services/apps/apps.ts Outdated Show resolved Hide resolved
setAttempt({
status: 'failed',
statusText: err.message,
if (!state) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andrejtokarcik Could you update this function with below code and add some comments // comment here ?

export default function useAppLauncher() {
  const params = useParams<UrlLauncherParams>();
  const { attempt, setAttempt } = useAttempt('processing');

  React.useEffect(() => {
    resolveRedirectUrl(params)
      .then(url => {
        window.location.replace(url);
      })
      .catch((err: Error) => {
        setAttempt({
          status: 'failed',
          statusText: err.message,
        });
      });
  }, []);

  return {
    ...attempt,
  };
}

function resolveRedirectUrl(params: UrlLauncherParams) {
  const location = window.location;
  const port = location.port ? ':' + location.port : '';
  const state = getUrlParameter('state', location.search);

  // comment here
  if (!state) {
    return service.getAppFqdn(params).then(result => {
      const url = new URL(`https://${result.fqdn}${port}/x-teleport-auth`);
      if (params.clusterId) {
        url.searchParams.set('cluster', params.clusterId);
      }
      if (params.publicAddr) {
        url.searchParams.set('addr', params.publicAddr);
      }

      return url.toString();
    });
  }

  // comment here
  return service.createAppSession(params).then(result => {
    const url = new URL(`https://${result.fqdn}${port}/x-teleport-auth`);
    url.searchParams.set('state', state);
    url.hash = `#value=${result.value}`;
    return url.toString();
  });
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@andrejtokarcik andrejtokarcik force-pushed the andrej/fix/double-CreateAppSession branch from 7ef4ecc to 520d571 Compare May 3, 2021 18:25
@andrejtokarcik andrejtokarcik merged commit ab9934a into master May 4, 2021
@andrejtokarcik andrejtokarcik deleted the andrej/fix/double-CreateAppSession branch May 4, 2021 08:19
amha19 pushed a commit that referenced this pull request May 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't create multiple app sessions when launching an app
3 participants