Feature/fun 509 oauth2 token handler

[btamas]

Related to https://oat-sa.atlassian.net/browse/FUN-509

Goal is to implement https://oauth2.thephpleague.com/authorization-server/refresh-token-grant/
The different between the required oauth2 request and the original OAT JWT token request is:
Request:

• refresh token is called refresh_token in the request
• request format is FormData instead of json
• client_id and grant_type should be sent, but this is handled with the existing refreshTokenParameters

Response:

• access token is called access_token in the response
• refresh token is sent in the response and it is called refresh_token
• expires_in is sent in the response and this is the TTL of the access token in seconds

These differences could be handled with some conditions, so I made the decision to do not create a new token handler, but modify the existing one.

Test:

• run tests and check them, because they covers everything

(I tested it with @peetya on his local environment, where this modification will be necessary)

Now, it is not possible to test in a working environment, because it is just a small part of big refactoring.

[github-actions]

Coverage Report

Totals
Statements:	90.61% ( 2684 / 2962 )
Methods:	94.2% ( 779 / 827 )

[oatymart]

Goal is to implement https://oauth2.thephpleague.com/authorization-server/refresh-token-grant/

I'd like to know what is the real goal, what problem will it solve for us? Is it more secure? More convenient? ... Is it because we could scope the requests with client_id or scope, somehow preventing cheating?

The other main difference I notice from reading the code is that we'll receive back expires_in - but I added a util last year which had a similar purpose, to extract TTL from the existing JWT response payload. It's convenient to receive the named value from the server but is the result any different?

[krampstudio]

In the Solar test runner we've even started to use another approach (integrated with env management) where the refresh token is never exposed directly but set using an HTTPOnly cookie, to prevent leaking it. Maybe we should align on the mechanism to use.

[jsconan]

The code looks ok, and the unit tests are passing. I have no issue with the proposal, but yes, looking at the other comments I agree we should align on the solution. We need to apply a unified way otherwise we'll have different implementations for each consumer ;)

[btamas]

Goal is to implement https://oauth2.thephpleague.com/authorization-server/refresh-token-grant/

I'd like to know what is the real goal, what problem will it solve for us? Is it more secure? More convenient? ... Is it because we could scope the requests with client_id or scope, somehow preventing cheating?

During GCP migration of the scoring project, a new environment management system will be used and there will be a dedicated auth server. This server will do the auth related tasks (token creation, password check) and it will be separated from the BE server, that sends session related configurations. This auth server will use oauth2, what was a decision earlier, but manual scoring is unique, because we should support user/password login. To implement it in the oauth2 context, we have to follow the standard. This standard contains the client_id, scope and grant_type. Oauth2 can return with any token, without specific requirement, but in our projects, we will use JWT, so we can use JWT token modules.

The other main difference I notice from reading the code is that we'll receive back expires_in - but I added a util last year which had a similar purpose, to extract TTL from the existing JWT response payload. It's convenient to receive the named value from the server but is the result any different?

I also have problem with a redundant expires_in <-> jwt token exp, but the oauth2 protocol send the token ttl, so we should use it.

[btamas]

In the Solar test runner we've even started to use another approach (integrated with env management) where the refresh token is never exposed directly but set using an HTTPOnly cookie, to prevent leaking it. Maybe we should align on the mechanism to use.

I agree with you, but this is not part of this task. We will modify the store later, but this task solves only the GCP migration related issues.

[btamas]

@krampstudio @jsconan @oatymart Do you need any other info to approve this PR?

[oatymart]

Code looks clean and simple. Tests are passing and cover almost everything.

• New code is covered by tests (if applicable)
• Tests are running successfully (old and new ones) on my local machine (if applicable)
• New code is respecting code style rules
• New code is respecting best practices
• Feature is working correctly on my local machine (if applicable)
• Acceptance criteria are respected
• Pull request title and description are meaningful
• Pull request's target is not master
• Documentation is updated according to changes (if applicable)

[jsconan]

The code is ok and passes the unit tests.
In the end, we would still need to unify the APIs, but as per the discussion above, the added feature should be acceptable.

Note: tested only via unit tests.

Review Checklist

• New code is covered by tests (if applicable)
• Tests are running successfully (old and new ones) on my local machine (if applicable)
• New code is respecting code style rules
• New code is respecting best practices
• New code is not subject to concurrency issues (if applicable)
• Feature is working correctly on my local machine (if applicable) - Tested only via unit test
• Acceptance criteria are respected
• Pull request title and description are meaningful
• Pull request's target is not master
• Commits are following conventional commits
• Commits messages are meaningful
• Commits are atomic

[github-actions]

Version

Target Version	1.19.0
Last version	1.18.2

There are 0 BREAKING CHANGE, 1 feature, 0 fix