feat(join-tokens): [44794] API changes for managing GitHub Join Tokens by nicholasmarais1158 · Pull Request #54374 · gravitational/teleport

nicholasmarais1158 · 2025-04-29T09:21:00Z

Description

This change prepares the Web API to support creating and editing GitHub Join Tokens from the Join Tokens page.

Issue: #44794

changelog: Fixed an issue causing Join Token expiries to be overwritten when editing a token

Changes

Expose github-specific fields from GET /webapi/tokens - this is required to populate the edit form
Support "token" in /webapi/yaml/parse/:kind - this is required to allow detection of unsupported fields
Use empty time.Time for token expiry in POST /webapi/tokens - also fixes a bug related to the expiry being overwritten on edit

nicholasmarais1158

A few questions.

nicholasmarais1158 · 2025-04-29T09:24:46Z

+	if r.Method == "PUT" {
+		// if using the PUT route, tokenId will be present
+		// in the X-Teleport-TokenName header
+		tokenId = r.Header.Get(HeaderTokenName)


Is this a current pattern, or something legacy?

I'm not sure - it strikes me as a little odd - @avatus may have more context?

we added it here specifically so someone could not get the secret token name from the request param in logs as JoinTokens are meant to be secret

#42402 (comment) context

Thanks for the context @avatus.

It makes sense for the delete endpoint, given the id is included in the path, but this endpoint doesn't use path params (POST|PUT /webapi/tokens).

Currently the Web UI doesn't use the PUT endpoint, should the header approach be retained or should we use a value from the request body instead?

Request body is fine

Thanks.

Removed in 99dcad0.

nicholasmarais1158 · 2025-04-29T09:25:08Z

+	// If this is an edit, then overwrite the metadata to retain the existing fields
+	if existingToken != nil {
+		token.SetMetadata(existingToken.GetMetadata())


Is this a valid operation to perform? Is it safe to copy the metadata unchanged to the new token? Does it mess with the revision mechanism?

Yeah - that's fine. The revision mechanism is actually designed to be used this way. When performing an action and providing the revision, the backend checks that the provided revision matches what is currently in the backend. You have introduced a change in behaviour here (e.g it'll now perform optimistic locking, which it didn't perform previously as the revision was wiped) but this is probably a good thing since it'll reject edits where the resource has changed since it was fetched.

I wouldn't expect the revision to matter much if we are still calling UpsertToken below though.

I wouldn't expect the revision to matter much if we are still calling UpsertToken below though.

Yeah - imho, we should eventually split this webapi endpoint to a seperate create/update and call the appropriate underlying Create or Update. At the moment, you can accidentally overwrite an existing token when creating a new one. But I think that's best left out of scope for this PR.

nicholasmarais1158 · 2025-04-29T10:19:37Z

+			// Skip enterprise-only methods
+			if method == types.JoinMethodTPM || method == types.JoinMethodSpacelift {
+				t.Skipf("Skipping %s, as it's enterprise-only", method)
+			}


Is there an enterprise-mode that can be used to prevent having to skip enterprise-only features?

An error is returned when creating TMP and Spacelift tokens.

You can do something similar to:

modules.SetTestModules(t, &modules.TestModules{ TestBuildType: modules.BuildEnterprise, TestFeatures: modules.Features{ Cloud: true, }, }

But since this modifies global state, it means that the test can't be marked parallel.

Thanks.

Done in 2b90f08.

strideynet

I think this is nearly there - I've identified a few nits but I'm also waiting for some more context before approving.

strideynet · 2025-04-29T10:46:33Z

 	}
 }

+func TestGetGithubTokens(t *testing.T) {


Would it be possible to extend the existing TestGetTokens test table rather than introduce a new test here?

Done in e7fd9a5.

strideynet · 2025-04-29T10:49:03Z

+			// Skip enterprise-only methods
+			if method == types.JoinMethodTPM || method == types.JoinMethodSpacelift {
+				t.Skipf("Skipping %s, as it's enterprise-only", method)
+			}


You can do something similar to:

modules.SetTestModules(t, &modules.TestModules{ TestBuildType: modules.BuildEnterprise, TestFeatures: modules.Features{ Cloud: true, }, }

But since this modifies global state, it means that the test can't be marked parallel.

strideynet · 2025-04-29T10:51:31Z


-	if editing && tokenId != req.Name {
-		return nil, trace.BadParameter("renaming tokens is not supported")
+	var tokenId string


Nit: I'd simplify this by removing the else branch and initialising the variable with the value we'd set in the else.

Done in 98c1c72.

strideynet · 2025-04-29T10:56:05Z

+	require.Equal(t, map[string]string{
+		"test-key": "test-value",
+	}, editedToken.GetMetadata().Labels)
+}


Let's add an assertion that the expiry time isn't modified as well so we can prove that we've fixed the bug reported in #54306

Added in 1311d91.

strideynet · 2025-04-29T12:15:15Z

+	}
+
+	var existingToken types.ProvisionToken
+	if tokenId != "" {


I have a hunch that by this point, we definitely have a value for tokenId, so we can omit this if statement.

Removed in 98c1c72.

Reverted in f0254a6. If req.Name is empty we want to avoid fetching an existing token.

avatus · 2025-04-29T16:56:31Z

will the updates to this endpoint support web UIs of version n-1 making this request still? for example, if this goes out in 17.5, will a webUI served by 17.4 fail when making this request to a 17.5 proxy?

nicholasmarais1158 · 2025-04-30T09:28:44Z

will the updates to this endpoint support web UIs of version n-1 making this request still? for example, if this goes out in 17.5, will a webUI served by 17.4 fail when making this request to a 17.5 proxy?

@avatus Yes, it's backwards compatible.

Out of interest, given the Proxy serves the Web UI, how would the situation of an older Web UI and new Proxy come about?

avatus · 2025-04-30T14:47:59Z

given the Proxy serves the Web UI, how would the situation of an older Web UI and new Proxy come about?

wonderful question. There are many users that do rolling upgrades on their clusters. For example, if i have 4 proxies and i upgrade 2 of them to N+1, if a user gets served the web UI from proxy N, their requests during their session lifetime may get routed to a proxy N api, or proxy N+1.

Its an edge case, but not as edge-y as it may sound. Sometimes, its unavoidable, but we want to keep it top of mind when we can to make sure as many features work between new/old proxy and new/old web UI.

You can read a bit more about it here

backport-bot-workflows · 2025-05-01T13:51:38Z

@nicholasmarais1158 See the table below for backport results.

Branch	Result
branch/v16	Failed
branch/v17	Create PR

#54374) * Return github-specific config from `GET /webapi/tokens` * Support "token" in `/webapi/yaml/parse/:kind` * Use empty time.Time for token expiry (`POST /webapi/tokens`) * Cover enterprise token types in tests * Cover github tokens in existing tests * Tweak handling of `tokenId` * Check expiry is not overwritten * Revert removing tokenId check * Remove use of `X-Teleport-TokenName` header # Conflicts: # lib/web/join_tokens.go # lib/web/join_tokens_test.go # lib/web/yaml.go

…54454) * feat(join-tokens): [44794] API changes for managing GitHub Join Tokens (#54374) * Return github-specific config from `GET /webapi/tokens` * Support "token" in `/webapi/yaml/parse/:kind` * Use empty time.Time for token expiry (`POST /webapi/tokens`) * Cover enterprise token types in tests * Cover github tokens in existing tests * Tweak handling of `tokenId` * Check expiry is not overwritten * Revert removing tokenId check * Remove use of `X-Teleport-TokenName` header # Conflicts: # lib/web/join_tokens.go # lib/web/join_tokens_test.go # lib/web/yaml.go * nit: Fix comment

nicholasmarais1158 added 3 commits April 29, 2025 09:47

Return github-specific config from GET /webapi/tokens

05de19a

Support "token" in /webapi/yaml/parse/:kind

0943a83

Use empty time.Time for token expiry (POST /webapi/tokens)

413cde5

nicholasmarais1158 added machine-id backport/branch/v16 backport/branch/v17 labels Apr 29, 2025

nicholasmarais1158 self-assigned this Apr 29, 2025

nicholasmarais1158 added the no-changelog Indicates that a PR does not require a changelog entry label Apr 29, 2025

nicholasmarais1158 commented Apr 29, 2025

View reviewed changes

nicholasmarais1158 removed the no-changelog Indicates that a PR does not require a changelog entry label Apr 29, 2025

nicholasmarais1158 mentioned this pull request Apr 29, 2025

feat(join-tokens): [44794] Add Join Token form for GitHub #54308

Merged

nicholasmarais1158 marked this pull request as ready for review April 29, 2025 09:36

github-actions Bot requested review from klizhentas and zmb3 April 29, 2025 09:36

github-actions Bot added the size/md label Apr 29, 2025

nicholasmarais1158 commented Apr 29, 2025

View reviewed changes

strideynet reviewed Apr 29, 2025

View reviewed changes

strideynet requested a review from avatus April 29, 2025 12:10

strideynet reviewed Apr 29, 2025

View reviewed changes

nicholasmarais1158 added 5 commits April 30, 2025 09:37

Cover enterprise token types in tests

2b90f08

Cover github tokens in existing tests

e7fd9a5

Tweak handling of tokenId

98c1c72

Check expiry is not overwritten

1311d91

Merge branch 'master' into nickmarais/feat/44794-github-join-tokens-api

7696729

Revert removing tokenId check

f0254a6

nicholasmarais1158 requested a review from strideynet April 30, 2025 10:51

Remove use of X-Teleport-TokenName header

99dcad0

Merge branch 'master' into nickmarais/feat/44794-github-join-tokens-api

6c18ccb

boxofrad approved these changes May 1, 2025

View reviewed changes

avatus approved these changes May 1, 2025

View reviewed changes

public-teleport-github-review-bot Bot removed request for klizhentas and zmb3 May 1, 2025 13:22

nicholasmarais1158 added this pull request to the merge queue May 1, 2025

Merged via the queue into master with commit 8ba33a5 May 1, 2025
42 checks passed

nicholasmarais1158 deleted the nickmarais/feat/44794-github-join-tokens-api branch May 1, 2025 13:49

nicholasmarais1158 mentioned this pull request May 1, 2025

[v17] feat(join-tokens): [44794] API changes for managing GitHub Join Tokens #54450

Merged

nicholasmarais1158 mentioned this pull request May 1, 2025

[v16] feat(join-tokens): API changes for managing GitHub Join Tokens #54454

Merged

Conversation

nicholasmarais1158 commented Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Uh oh!

nicholasmarais1158 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

strideynet Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

strideynet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicholasmarais1158 Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avatus commented Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicholasmarais1158 commented Apr 30, 2025

Uh oh!

avatus commented Apr 30, 2025

Uh oh!

Uh oh!

backport-bot-workflows Bot commented May 1, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

nicholasmarais1158 commented Apr 29, 2025 •

edited

Loading

strideynet Apr 29, 2025 •

edited

Loading

nicholasmarais1158 Apr 30, 2025 •

edited

Loading

avatus commented Apr 29, 2025 •

edited

Loading