Skip to content
This repository has been archived by the owner on Nov 26, 2023. It is now read-only.

Laravel 8 support, Jira v3 API compatibility, Fix paginate request with query params, Fix naming collision with jwt-auth. #16

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

henritoivar
Copy link

Updated dependencies to support Laravel 8.
Updated Jira Paginator to use v3 API conventions.
Fixed query parameters being overwritten by default pagination parameters, when providing query parameters to $client->paginate.
Made guard name and middleware name more specific to avoid naming collision with tymon/jwt-auth. Ideally thiese two could be defined in the config?


protected function serializeDate(DateTimeInterface $date)
Copy link
Owner

Choose a reason for hiding this comment

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

Why is it needed?

Copy link
Author

Choose a reason for hiding this comment

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

The default date serialization format was changed in Laravel 7. See here: https://laravel.com/docs/7.x/upgrade#date-serialization

@@ -6,11 +6,11 @@
Route::post('installed', 'TenantController@installed')->name('installed');
Route::post('disabled', 'TenantController@disabled')->name('disabled');

Route::group(['middleware' => 'jwt'], function () {
Route::group(['middleware' => 'auth.atlassian'], function () {
Copy link
Owner

Choose a reason for hiding this comment

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

Is it a collision for you? We could rename it like this, but I want to understand whether this is a real issue.

Furthermore, we need to update README, so users will define this middleware instead.

Copy link
Author

@henritoivar henritoivar Nov 9, 2020

Choose a reason for hiding this comment

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

I managed a simpler solution for this by directly referencing the class. See commit 942c6cf

@@ -127,7 +127,7 @@ protected function registerFacades()
*/
protected function registerJWTGuard()
{
Auth::extend('jwt', function (Application $app, $name, array $config)
Auth::extend('jwt.atlassian', function (Application $app, $name, array $config)
Copy link
Owner

Choose a reason for hiding this comment

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

Do you experience a collision in a real project?

Copy link
Author

Choose a reason for hiding this comment

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

Hey, yes i have a project where i use tymondesigns/jwt-auth which also names it's guard "jwt". I have updated the pull-request to make this configurable: 942c6cf

@breart
Copy link
Owner

breart commented Nov 8, 2020

@henritoivar thanks for the contribution. If we add support for v3 API, v2 won't be longer supported. Would that be an issue?

I'll also add support for L7 in this PR since there is not a lot of changes between those.

…n header. This is causing the requests to redirect instead of returning json in case the validation does not pass.
… Atlassian Account ID from the sub property of the JWT claim instead.
…ame tag.

Directly reference middleware so user doesn't have to use exact alias in vendor file.
@henritoivar
Copy link
Author

I have added a few more adjustments:

  • dispatcher fire method has been removed in laravel 5.8, use dispatch instead
  • jira doesn't include user_key and user_id anymore in the request. See here: https://developer.atlassian.com/cloud/jira/platform/context-parameters/
  • Also atlassian callbacks do not include the Accept header with value application/json. This will make Laravel redirect in case validation does not pass. I have overriden this behaviour so validation messages are always returned as json.

@henritoivar
Copy link
Author

@henritoivar thanks for the contribution. If we add support for v3 API, v2 won't be longer supported. Would that be an issue?

I'll also add support for L7 in this PR since there is not a lot of changes between those.

I use only v3, so it's not a problem for me.

…same query then the second request returns data from the previous request.
@leganz
Copy link

leganz commented Jan 25, 2021

Are there any plans to integrate that PR into master? It would be pretty nice, cause i have laravel 8 project which would benefit from that :-)

@mystiquewolf
Copy link

@henritoivar Is it possible/reasonable for me to manually merge this pull request locally so i can use this in Laravel 8, is it stable enough in your opinion?

@henritoivar
Copy link
Author

@mystiquewolf I'm using this in production myself. So far no issues.

Until this gets merged you could use my fork (or fork my fork.. hehe) directly like this in your composer.json:

"require": {
  "brezzhnev/atlassian-connect-core": "dev-master",
  ...
},
"repositories": {
   {
       "type": "vcs",
       "url": "https://github.com/henritoivar/atlassian-connect-core"
   }
}

Also validate requests that are coming from Atlassian. We need to strip our base url to properly calculate QSH. See here: https://developer.atlassian.com/cloud/bitbucket/query-string-hash/

Actually validate JWT from requests sent by Atlassian. Also validate QSH for these requests.
@breart
Copy link
Owner

breart commented Aug 4, 2021

Sorry guys for abandoning this repo, I can't manage to find time to maintain and improve this package. I'm happy to invite someone as a collaborator on this project, making sure every change gets tested and follows the project coding style.

Please send an email to [email protected] if you're interested.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants