Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support create token with headers #1455

Merged
merged 3 commits into from
May 31, 2024
Merged

Support create token with headers #1455

merged 3 commits into from
May 31, 2024

Conversation

coderzc
Copy link
Member

@coderzc coderzc commented May 14, 2024

Master Issue: #

Motivation

Currently, the tool that generates the token cannot attach headers. We should support it. For example, it makes sense to attach a kid field for JWKS.

Modifications

Support create token with headers

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Added integration tests for end-to-end deployment with large payloads (10MB)
  • Extended integration test for recovery after broker failure

Documentation

Check the box below.

Need to update docs?

  • doc-required

    (If you need help on updating docs, create a doc issue)

  • no-need-doc

    (Please explain why)

  • doc

    (If this PR contains doc changes)

@github-actions github-actions bot added the no-need-doc This pr does not need any document label May 14, 2024
@coderzc coderzc force-pushed the create_token_with_headers branch 2 times, most recently from 82a8401 to a506f4c Compare May 14, 2024 03:55
@coderzc coderzc force-pushed the create_token_with_headers branch from a506f4c to a7087a8 Compare May 14, 2024 04:04
mattisonchao
mattisonchao previously approved these changes May 30, 2024
pkg/cmdutils/token.go Outdated Show resolved Hide resolved
@zymap
Copy link
Member

zymap commented May 30, 2024

Copy link
Member

@zymap zymap left a comment

Choose a reason for hiding this comment

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

Seems we already support the API with header:

func (t *token) CreateToken(

what’s the difference between them? Maybe we can use the existing API to extend more options for the jwt claims?

@coderzc
Copy link
Member Author

coderzc commented May 30, 2024

Seems we already support the API with header:

func (t *token) CreateToken(

what’s the difference between them? Maybe we can use the existing API to extend more options for the jwt claims?

The CreateToken method must be with claims and Create will call CreateToken, I only pass headers params from Create to CreateToken

func (t *token) Create(algorithm algorithm.Algorithm, signKey interface{}, subject string,
expireTime int64) (string, error) {
var claims *jwt.MapClaims
if expireTime <= 0 {
claims = &jwt.MapClaims{
"sub": subject,
}
} else {
claims = &jwt.MapClaims{
"sub": subject,
"exp": jwt.NewNumericDate(time.Unix(expireTime, 0)),
}
}
return t.CreateToken(algorithm, signKey, claims, nil)
}

@coderzc coderzc force-pushed the create_token_with_headers branch from 199591f to cd82391 Compare May 31, 2024 01:09
@coderzc
Copy link
Member Author

coderzc commented May 31, 2024

@zymap I let cmd call the CreateToken method and revert the modifications to Create, PTAL

@@ -126,6 +135,8 @@ func create(vc *cmdutils.VerbCmd) {
"The expire time for a token. e.g. 1s, 1m, 1h")
set.BoolVar(&args.base64Encoded, "base64", false,
"The secret key is base64 encoded or not.")
set.StringVar(&args.headers, "headers", "",
Copy link
Member

Choose a reason for hiding this comment

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

There is a method set.StringToStringVar() to receive a map value from the command line. We can use that instead of parse the map value by ourselves.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, updated.

@zymap zymap merged commit c7fdea5 into master May 31, 2024
11 checks passed
@zymap zymap deleted the create_token_with_headers branch May 31, 2024 04:32
zymap pushed a commit that referenced this pull request Jun 5, 2024
### Motivation

Currently, the tool that generates the token cannot attach headers. We should support it. For example, it makes sense to attach a kid field for JWKS.

(cherry picked from commit c7fdea5)
zymap pushed a commit that referenced this pull request Jun 5, 2024
### Motivation

Currently, the tool that generates the token cannot attach headers. We should support it. For example, it makes sense to attach a kid field for JWKS.

(cherry picked from commit c7fdea5)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-need-doc This pr does not need any document
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants