Skip to content

Make OAuth2 authorization code expire - #40766

Merged
julien-nc merged 13 commits into
masterfrom
enh/noid/oauth2-code-expiration
Oct 5, 2023
Merged

Make OAuth2 authorization code expire#40766
julien-nc merged 13 commits into
masterfrom
enh/noid/oauth2-code-expiration

Conversation

@julien-nc

@julien-ncjulien-nc commented Oct 4, 2023

Copy link
Copy Markdown
Member

Oauth authorization codes now expire 10 minutes after being created.
To know if a row in oauth2_access_tokens is an authorization code or an access token, we keep track of the number of delivered access tokens. If no token was delivered, it means the row still contains an authorization code (in the encrypted_token column).

  • Refuse to deliver an access token from an expired authorization code (and immediately delete the related DB entry)
  • Expired authorization codes are cleaned up every month
  • Fix: A refresh token could be used as an authorization code (with the authorization_code grant type). This is no longer possible
  • A few tests have been implemented

@julien-ncjulien-nc added this to the Nextcloud 28 milestone Oct 4, 2023
github-advanced-security[bot]
github-advanced-securityAI found potential problems Oct 4, 2023
View reviewed changes
Comment threadapps/oauth2/lib/BackgroundJob/CleanupExpiredAuthorizationCode.php Fixed
Comment threadapps/oauth2/lib/Db/AccessToken.php
/** @var string */
protected $encryptedToken;
/** @var int */
protected $codeCreatedAt;

Check notice

Code scanning / Psalm

PropertyNotSetInConstructor

Property OCA\OAuth2\Db\AccessToken::$codeCreatedAt is not defined in constructor of OCA\OAuth2\Db\AccessToken or in any methods called in the constructor
Comment threadapps/oauth2/lib/Db/AccessToken.php
/** @var int */
protected $codeCreatedAt;
/** @var int */
protected $tokenCount;

Check notice

Code scanning / Psalm

PropertyNotSetInConstructor

Property OCA\OAuth2\Db\AccessToken::$tokenCount is not defined in constructor of OCA\OAuth2\Db\AccessToken or in any methods called in the constructor
Comment threadapps/oauth2/lib/Migration/Version011603Date20230620111039.php Fixed
@julien-nc
julien-ncforce-pushed the enh/noid/oauth2-code-expiration branch from 2a63356 to 4cb31e1CompareOctober 4, 2023 09:54
nickvergessen
nickvergessen reviewed Oct 4, 2023
View reviewed changes
Comment threadapps/oauth2/lib/BackgroundJob/CleanupExpiredAuthorizationCode.php Fixed
Comment threadapps/oauth2/lib/Migration/Version011603Date20230620111039.php Fixed
Comment threadapps/oauth2/lib/Migration/Version011603Date20230620111039.php Outdated
Comment threadapps/oauth2/lib/Migration/Version011603Date20230620111039.php
if (!$table->hasColumn('code_created_at')) {
$table->addColumn('code_created_at', Types::BIGINT, [
'notnull' => true,
'default' => 0,

@nickvergessennickvergessenOct 4, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
'default' => 0,
'unsigned' => true,

Default 0 is automatically, unsigned will allow more numbers

@julien-ncjulien-ncOct 4, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Why not explicitly setting the default to 0? I didn't know about it, maybe others don't know either.

@nickvergessennickvergessenOct 4, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also fine 👍🏼

Comment threadapps/oauth2/lib/Migration/Version011603Date20230620111039.php
Comment threadapps/oauth2/lib/BackgroundJob/CleanupExpiredAuthorizationCode.php
@nickvergessen

nickvergessen commented Oct 4, 2023

Copy link
Copy Markdown
Member

OpenAPI command also needs to be executed

provokateurin
provokateurin approved these changes Oct 4, 2023
View reviewed changes

@provokateurinprovokateurin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

API changes LGTM

Comment threadapps/oauth2/lib/Controller/OauthApiController.php Outdated
@julien-nc
julien-ncforce-pushed the enh/noid/oauth2-code-expiration branch from efde8a9 to 3e9e57bCompareOctober 4, 2023 18:02
@julien-nc
julien-nc enabled auto-merge October 4, 2023 18:02
@julien-nc
julien-ncforce-pushed the enh/noid/oauth2-code-expiration branch 2 times, most recently from 5efcbd5 to b2808afCompareOctober 5, 2023 08:03
@julien-nc
make oauth2 authorization code expire after 10 minutes
807f173
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@julien-nc
add tests for oauth2 authorization code expiration
2995b09
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@julien-nc
cleanup access tokens that are still in authorization state and that …
7bba410
…have expired
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@julien-nc
refuse oauth authorization code if a token has already been delivered…
1ab45ba
… (active token)
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@julien-nc
delete oauth access token when receiving a code that has expired
779e1d5
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@julien-nc
add test for refusing to get an oauth token from a code when we're no…
ddfc124
…t in authorization state
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@julien-nc
add db index on oauth2_access_tokens's (token_count, created_at)
e944980
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@julien-nc
rename oauth2_access_token's created_at to code_created_at
c6da994
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@julien-nc
adjust oauth tests
32f984c
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@julien-nc
adjust oauth app
d2bc483
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@julien-nc
update autoload files
da63d3c
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@julien-nc
update OpenAPI specs
98c8a46
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@julien-nc
adjust phpdoc types in OauthApiController
d56950a
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@julien-nc
julien-ncforce-pushed the enh/noid/oauth2-code-expiration branch from b2808af to d56950aCompareOctober 5, 2023 12:24
@julien-nc
julien-nc merged commit b4fec29 into masterOct 5, 2023
@julien-nc
julien-nc deleted the enh/noid/oauth2-code-expiration branch October 5, 2023 13:16
@solracsfsolracsf mentioned this pull request Jan 18, 2024
Closed
This was referenced Jan 22, 2024
Merged
Merged
@joshtrichardsjoshtrichards mentioned this pull request Aug 1, 2024
Open
@joshtrichardsjoshtrichards mentioned this pull request Mar 17, 2025
Open
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nickvergessennickvergessennickvergessen left review comments
@AndyScherzingerAndyScherzingerAndyScherzinger approved these changes
@provokateurinprovokateurinprovokateurin approved these changes
@juliusknorrjuliusknorrAwaiting requested review from juliusknorr
@ChristophWurstChristophWurstAwaiting requested review from ChristophWurstChristophWurst is a code owner

Assignees

No one assigned

Labels

3. to reviewWaiting for reviewsenhancementfeature: authentication

Projects

None yet

Milestone

Nextcloud 28

Development

Successfully merging this pull request may close these issues.

5 participants

@julien-nc@nickvergessen@AndyScherzinger@provokateurin@github-advanced-security