tractorcow commented 5 years ago

@Firesphere would love to discuss this with you before merging; See you on slack maybe?

Overview of changes in general order of most significant to least

Return type of all validate / create mutations has been consolidated and normalised. The standard response is now a MemberToken type with these fields:
- Member (the member)
- Token (string token)
- Valid (bool valid flag)
- Status (enum status code of the token)
- Code (int HTTP code that matches the status)
- Message (string message of humanised status code)
Multiple authenticator tokens per user; Allows multiple devices to have their own tokens.
Anonymous authentication is now shifted up into a custom Authenticator class; Support injectable authenticators into createToken mutation. Removed anonymous_allowed
When logging in anonymously, create a real member object to better integrate with silverstripe security logging / auditing practices.
Bump minimum dependency to php 7.1: strict_types are everywhere too.
Updated tests and documentation of course
Support multiple schemas (graphql 3.0 and above).
Relative path supported for JWT tokens
Updated renewal process to support separate token / refresh expiration dates
I gave every file my Love. Green ticks everywhere.

@Firesphere if you merge, I suggest branching master -> 1 first, as this is a new 2.x major change.

Firesphere commented 5 years ago

Could you make a minor change (I don't know, docs or something?) to ping CircleCI to run? I don't know why it didn't, but I've reset the PR settings back to default so it should work again.

Firesphere commented 5 years ago

Could you have a look at the CodeClimate and Scrutinizer issues? They're not super important or anything, but it's not a bad thing to keep an eye on :)

Firesphere commented 5 years ago

Could you have a look at the CodeClimate failures? They're not of high importance, but good to keep them low. Also, where did my previous comment go??!

Firesphere commented 5 years ago

Could you run PHP-CS-Fixer to fix the PSR-2 violations?

tractorcow commented 5 years ago

Great feedback so far. I'll get back later this week. :D

tractorcow commented 5 years ago

@Firesphere I've done a quick as guts refactor. All work is in separate commits for your review.

traits that were just used once I've inlined.

Removed unused JWTException.

I hope the getKeys() code suits you. It was kind of weird before.

I haven't done much except run the unit tests and phpcs. I'll need to come back and test more later.

Firesphere commented 5 years ago

Still confused why CircleCI won't run... but that refactor looks great!

Just a quick check, does this incorporate a possible fix fo #22 ?

Firesphere commented 5 years ago

Ah, this is why the tests don't run:

    branches:
      only:
        - master
        - develop
        - /feature.*/

in .circleci/config.yml

Mind removing those lines for me? Otherwise, you'd have to rebase for such a simple change ;)

tractorcow commented 5 years ago

22 would probably require a new pull request. I don't really have time at the moment I'm afraid.

tractorcow commented 5 years ago

Removed those lines.

codecov-io commented 5 years ago

Codecov Report

Merging #24 into master will not change coverage. The diff coverage is 75.39%.

@@            Coverage Diff            @@
##             master      #24   +/-   ##
=========================================
  Coverage     71.09%   71.09%           
  Complexity       72       72           
=========================================
  Files             8        8           
  Lines           211      211           
=========================================
  Hits            150      150           
  Misses           61       61

Impacted Files	Coverage Δ	Complexity Δ
src/Types/MemberTokenTypeCreator.php	`0% <0%> (ø)`	`2 <1> (ø)`	:arrow_down:
src/Mutations/CreateTokenMutationCreator.php	`70% <53.33%> (ø)`	`9 <9> (ø)`	:arrow_down:
src/Queries/ValidateTokenQueryCreator.php	`68% <63.63%> (ø)`	`8 <8> (ø)`	:arrow_down:
src/Helpers/HeaderExtractor.php	`80% <75%> (ø)`	`3 <3> (ø)`	:arrow_down:
src/Authentication/JWTAuthenticationHandler.php	`71.42% <83.33%> (ø)`	`9 <2> (ø)`	:arrow_down:
src/Authentication/JWTAuthenticator.php	`85.89% <85.48%> (ø)`	`25 <14> (ø)`	:arrow_down:
src/Mutations/RefreshTokenMutationCreator.php	`76.47% <90.47%> (ø)`	`14 <12> (ø)`	:arrow_down:
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 2a45ae2...f2b6ecb. Read the comment docs.

tractorcow commented 5 years ago

Oh I love the codeclimate output. :)

tractorcow commented 5 years ago

@Firesphere now that it's working, do you mind doing a bit of triaging on those codeclimate issues and tell me which I need to fix? I'd fix them all if you say, although I question whether the recommendations are ideal.

Firesphere commented 5 years ago

I'll try to have a look today, CodeClimate reports aren't that big a deal for me usually, as more often than not it's unrelated stuff (e.g. SilverStripe's use of statics ;) )

tractorcow commented 5 years ago

Yeah there's stuff there I can fix. I don't know why codeclimate hates having multiple returns (I prefer that over more nested code structures). I can do some work end of day to tidy it up though.

Firesphere commented 5 years ago

I don't see any major problems besides the two mentioned for maintainability. I'll give everything a run for its money somewhere this weekend :) And possibly push some minor doc changes or personal preferences :)

Firesphere commented 5 years ago

Oh I love the codeclimate output. :)

I think you mean CodeCov, but yeah, same, love that output back as a comment! :D

Firesphere commented 5 years ago

Ow, could you please comment on the open issues that are resolved in this PR as such with reference? Would make it easier for history (and for me to close them :) )

Firesphere commented 5 years ago

@Firesphere if you merge, I suggest branching master -> 1 first, as this is a new 2.x major change.

I think I'm going to go with a standard tagging, way too okay with these changes generally to even bother keeping the old start I made with 2.0 active. Tags should suffice and I can always branch off the tag later if needed :)

tractorcow commented 5 years ago

@Firesphere please check my latest commit. Sorry I didn't get the code perfect, but it is an improvement on the major style issues. I'm just time constrained sorry.

tractorcow commented 5 years ago

Hold on, don't merge! I've run into this problem with user-agent validation.

User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/74.0.3729.108 Safari/537.36

Means a weekend-update (or mid-day update) to chrome will break all your sessions. 74.0.3729.107 to 74.0.3729.108 means you need to re-login. =(

tractorcow commented 5 years ago

Fixed the thing I was worried about @Firesphere :)

Firesphere commented 5 years ago

Note to self: Look at this tonight and finally merge it in!

tractorcow commented 5 years ago

yeah that'd be great. :)

ec8or commented 5 years ago

Would be awesome! ;)

Firesphere / silverstripe-graphql-jwt

Major upgrade: Changes documented #24

22 would probably require a new pull request. I don't really have time at the moment I'm afraid.

Codecov Report