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.

Sign up for GitHub

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

Jump to bottom

feat: custom client token ttl #3206

Merged
merged 11 commits into from
Jul 28, 2022
Merged

feat: custom client token ttl #3206

merged 11 commits into from
Jul 28, 2022

Conversation

grantzvolsky
Copy link
Contributor

@aeneasr Please note that I added swagger:ignore to the new fields in Client because otherwise the nullDuration fields broke the output of make sdk.

This change introduces a new endpoint that allows you to control how long client tokens last. Now you can configure the lifespan for each valid combination of Client, GrantType, and TokenType.

Part of #3157

Related issue(s)

Checklist

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change introduces a new feature.
  • I am following the contributing code guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security vulnerability.
    If this pull request addresses a security. vulnerability,
    I confirm that I got green light (please contact security@ory.sh) from the maintainers to push the changes.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added or changed the documentation.

Further Comments

@grantzvolsky grantzvolsky marked this pull request as draft July 28, 2022 02:45
grantzvolsky
grantzvolsky commented Jul 28, 2022
View reviewed changes
oauth2/oauth2_auth_code_test.go
@@ -45,6 +46,7 @@ import (
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"golang.org/x/oauth2"
goauth2 "golang.org/x/oauth2"
"golang.org/x/oauth2/clientcredentials"

"github.com/ory/fosite"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aeneasr The error is in CI as well, so we probably forgot to run quicktest after removing the // swagger:ignore directives

LINE 135 in this file:

--- FAIL: TestAuthCodeWithDefaultStrategy (9.01s)
    --- FAIL: TestAuthCodeWithDefaultStrategy/case=respects_client_token_lifespan_configuration (0.09s)
        --- FAIL: TestAuthCodeWithDefaultStrategy/case=respects_client_token_lifespan_configuration/case=custom-lifespans-active-jwt (0.09s)
            oauth2_auth_code_test.go:135:
                        Error Trace:    oauth2_auth_code_test.go:135
                                                                server.go:2047
                                                                server.go:2879
                                                                server.go:1930
                                                                asm_amd64.s:1581
                        Error:          Received unexpected error:
                                        json: cannot unmarshal string into Go struct field OAuth2Client.client.implicit_grant_access_token_lifespan of type models.NullDuration
                        Test:           TestAuthCodeWithDefaultStrategy/case=respects_client_token_lifespan_configuration/case=custom-lifespans-active-jwt
            oauth2_auth_code_test.go:124:
                        Error Trace:    oauth2_auth_code_test.go:124
                                                                oauth2_auth_code_test.go:407
                                                                oauth2_auth_code_test.go:460
                        Error:          Received unexpected error:
                                        Get "http://127.0.0.1:40185/?login_challenge=8339eb8eaae04cf69556fb80e8c7c86d": EOF
                        Test:           TestAuthCodeWithDefaultStrategy/case=respects_client_token_lifespan_configuration/case=custom-lifespans-active-jwt

@grantzvolsky grantzvolsky marked this pull request as ready for review July 28, 2022 13:15
github-advanced-security[bot]
github-advanced-security bot found potential problems Jul 28, 2022
View reviewed changes
test/e2e/oauth2-client/src/index.js
)

app.use(
session({
secret: '804cd9c9-b447-4df0-b9f0-3126893d3a8e',
secret: "804cd9c9-b447-4df0-b9f0-3126893d3a8e",

Check failure

Code scanning / CodeQL

Hard-coded credentials

The hard-coded value "804cd9c9-b447-4df0-b9f0-3126893d3a8e" is used as [key](1).
test/e2e/oauth2-client/src/index.js
const credentials = {
client: {
id: req.query.client_id,
secret: req.query.client_secret
secret: req.query.client_secret,

Check warning

Code scanning / CodeQL

Sensitive data read from GET request

[Route handler](1) for GET requests uses query parameter as sensitive data.
test/e2e/oauth2-client/src/index.js
const credentials = {
client: {
id: req.query.client_id,
secret: req.query.client_secret
secret: req.query.client_secret,

Check warning

Code scanning / CodeQL

Sensitive data read from GET request

[Route handler](1) for GET requests uses query parameter as sensitive data.
test/e2e/oauth2-client/src/index.js
const credentials = {
client_id: req.query.client_id,
client_secret: req.query.client_secret
client_secret: req.query.client_secret,

Check warning

Code scanning / CodeQL

Sensitive data read from GET request

[Route handler](1) for GET requests uses query parameter as sensitive data.
test/e2e/oauth2-client/src/index.js
Comment on lines 47 to +55
session({
secret: '804cd9c9-b447-4df0-b9f0-3126893d3a8e',
secret: "804cd9c9-b447-4df0-b9f0-3126893d3a8e",
resave: false,
saveUninitialized: true,
cookie: {
secure: false,
httpOnly: true
}
})
httpOnly: true,
},
}),

Check warning

Code scanning / CodeQL

Clear text transmission of sensitive cookie

Sensitive cookie sent without enforcing SSL encryption
grantzvolsky and others added 8 commits July 28, 2022 18:07
@grantzvolsky @aeneasr
feat: custom client token ttl
63a1247 
This change introduces a new endpoint that allows you to control how long client tokens last. Now you can configure the lifespan for each valid combination of Client, GrantType, and TokenType.

Part of #3157
@aeneasr
chore: code review
6972ecd
@grantzvolsky @aeneasr
code review: remove unused code; fix swagger type
fd7b587
@aeneasr
fix: correct types
5c33a92
@aeneasr
ignore prettier
3a8b4bc
@aeneasr
chore: format
afd56d4
@aeneasr
u
8a8ceb6
@aeneasr
u
69e9890
@aeneasr aeneasr force-pushed the feat/custom-ttl-per-oauth2-client branch from a5b948c to 69e9890 Compare July 28, 2022 16:08
aeneasr
aeneasr approved these changes Jul 28, 2022
View reviewed changes
internal/testhelpers/oauth2.go Outdated
"PUT",
adminTS.URL+client.ClientsHandlerPath+"/"+clientID+"/lifespans",
bytes.NewBuffer(b),
"application/x-www-form-urlencoded",
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be application/json? I don't think it really matters because we use a "dumb" json decoder, but would be good to fix anyways

@codecov
Copy link

codecov bot commented Jul 28, 2022

Codecov Report

Merging #3206 (8b64c6d) into master (ed6eb30) will decrease coverage by 0.20%.
The diff coverage is 70.51%.

@@            Coverage Diff             @@
##           master    #3206      +/-   ##
==========================================
- Coverage   79.55%   79.34%   -0.21%     
==========================================
  Files         112      111       -1     
  Lines        7971     8070      +99     
==========================================
+ Hits         6341     6403      +62     
- Misses       1225     1256      +31     
- Partials      405      411       +6
Impacted Files Coverage Δ
cmd/server/handler.go 63.76% <ø> (ø)
cmd/token_user.go 13.69% <0.00%> (ø)
driver/registry_base.go 87.14% <ø> (-0.05%) ⬇️
x/oauth2cors/cors.go 89.47% <ø> (+0.38%) ⬆️
x/sqlx.go 53.70% <ø> (-26.30%) ⬇️
client/client.go 74.74% <62.85%> (-6.51%) ⬇️
client/handler.go 77.77% <68.96%> (-1.42%) ⬇️
persistence/sql/migratest/exptected_data.go 100.00% <100.00%> (ø)
persistence/sql/persister_oauth2.go 81.01% <0.00%> (-0.85%) ⬇️
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

@aeneasr aeneasr merged commit 9544c03 into master Jul 28, 2022
@aeneasr aeneasr deleted the feat/custom-ttl-per-oauth2-client branch July 28, 2022 19:12
grantzvolsky added a commit that referenced this pull request Jul 31, 2022
@grantzvolsky @aeneasr @tricky42
feat: custom client token ttl (#3206)
4538353 
This change introduces a new endpoint that allows you to control how long client tokens last. Now you can configure the lifespan for each valid combination of Client, GrantType, and TokenType.

See #3157

Co-authored-by: aeneasr <3372410+aeneasr@users.noreply.github.com>
Co-authored-by: Andreas Bucksteeg <andreas@bucksteeg.de>
grantzvolsky added a commit that referenced this pull request Aug 1, 2022
@grantzvolsky @aeneasr @tricky42
feat: custom client token ttl (#3206)
8d9a0e1 
This change introduces a new endpoint that allows you to control how long client tokens last. Now you can configure the lifespan for each valid combination of Client, GrantType, and TokenType.

See #3157

Co-authored-by: aeneasr <3372410+aeneasr@users.noreply.github.com>
Co-authored-by: Andreas Bucksteeg <andreas@bucksteeg.de>
aeneasr added a commit that referenced this pull request Aug 1, 2022
@grantzvolsky @aeneasr @tricky42
feat: custom client token ttl (#3206)
1e68614 
This change introduces a new endpoint that allows you to control how long client tokens last. Now you can configure the lifespan for each valid combination of Client, GrantType, and TokenType.

See #3157

Co-authored-by: aeneasr <3372410+aeneasr@users.noreply.github.com>
Co-authored-by: Andreas Bucksteeg <andreas@bucksteeg.de>
aeneasr added a commit that referenced this pull request Aug 18, 2022
@grantzvolsky @aeneasr @tricky42
feat: custom client token ttl (#3206)
943d01a 
This change introduces a new endpoint that allows you to control how long client tokens last. Now you can configure the lifespan for each valid combination of Client, GrantType, and TokenType.

See #3157

Co-authored-by: aeneasr <3372410+aeneasr@users.noreply.github.com>
Co-authored-by: Andreas Bucksteeg <andreas@bucksteeg.de>
aeneasr added a commit that referenced this pull request Sep 5, 2022
@grantzvolsky @aeneasr @tricky42
feat: custom client token ttl (#3206)
2b8edad 
This change introduces a new endpoint that allows you to control how long client tokens last. Now you can configure the lifespan for each valid combination of Client, GrantType, and TokenType.

See #3157

Co-authored-by: aeneasr <3372410+aeneasr@users.noreply.github.com>
Co-authored-by: Andreas Bucksteeg <andreas@bucksteeg.de>
aeneasr added a commit that referenced this pull request Sep 7, 2022
@grantzvolsky @aeneasr @tricky42
feat: custom client token ttl (#3206)
9ef671f 
This change introduces a new endpoint that allows you to control how long client tokens last. Now you can configure the lifespan for each valid combination of Client, GrantType, and TokenType.

See #3157

Co-authored-by: aeneasr <3372410+aeneasr@users.noreply.github.com>
Co-authored-by: Andreas Bucksteeg <andreas@bucksteeg.de>
@sja sja mentioned this pull request Sep 16, 2022
Closed
6 tasks
@kotyara85
Copy link

@grantzvolsky Is there gonna be a cli integration for this?
Thanks

@grantzvolsky
Copy link
Contributor Author

@kotyara85 I'm not aware of a plan to implement this in the cli. Feel free to open a feature request; it could be a good first issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aeneasr aeneasr aeneasr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@grantzvolsky @kotyara85 @aeneasr @tricky42