Skip to content

fix: minor stuff#1562

Merged
daffl merged 1 commit into
feathersjs:masterfrom
vonagam:fix-minor-stuff
Sep 21, 2019
Merged

fix: minor stuff#1562
daffl merged 1 commit into
feathersjs:masterfrom
vonagam:fix-minor-stuff

Conversation

@vonagam

@vonagam vonagam commented Sep 16, 2019

Copy link
Copy Markdown
Member

Minor stuff:

  1. Moved @feathers/* dependencies from devDependencies to dependencies if their types are present in a package build.

  2. Moved this line up, since no need to call getPayload and getTokenOptions if their result is irrelevant.

  3. Here used location as is, without prepending slash, as it will be stripped anyway.

  4. In @feathersjs/configuration allow calling without an argument and use app!. instead of (app as Application)..

  5. No need to service && here since line before will throw if service is undefined, also in types service is non-nullable.

Note:

  1. @feathersjs/commons seems to be stuck at 4.3.0 while everyone else is already at 4.3.2 (it skipped 4.3.1 too).

@daffl
daffl merged commit 42c13e2 into feathersjs:master Sep 21, 2019
@daffl

daffl commented Sep 21, 2019

Copy link
Copy Markdown
Member

Sweet, all makes sense, thank you! I was actually wondering why all other dependencies always get updated but it's basically when a dependent package has a new version and @feathersjs/commons doesn't depend on anything so it doesn't need to get updated.

@vonagam

vonagam commented Sep 21, 2019

Copy link
Copy Markdown
Member Author

Ah... i get it now. It is because of this file. Because almost everything depends on @feathersjs/feathers when you update this file - and you do it every new version regardless if @feathersjs/feathers had any changes - almost everything gets new version. Except @feathersjs/commons because it does not depend on any feathers packages.

@vonagam
vonagam deleted the fix-minor-stuff branch September 21, 2019 19:46
@bertho-zero

bertho-zero commented Sep 23, 2019

Copy link
Copy Markdown
Contributor

In my opinion it should rather be peerDependencies to favor the version installed by the developer and avoid duplicates.

@jalbersdorfer

jalbersdorfer commented Jan 23, 2020

Copy link
Copy Markdown
Contributor

Hi, your 2nd change breaks my implementation.
I use it to concretize a jwt authentication as following:

I login to my application using Username + Password (LocalStrategy).
This Strategy loads a list of Tenants the logged in user has access to.
Then the User can choose a specific tenant.
Now I use the JWT Token I got from the LocalStrategy to login via JWTStrategy and do a kind of a re-authentication to obtain a new token.

And there's the point. In getPayload, I check whether I have to recreate the token and delete the old one from authResult if so. Which causes in turn that a new token with new Payload gets created.

With the current (your) Implementation, I have no clue how I could achieve the same, since getPayload will never again get called for JWTStrategy or any derived.

@daffl

daffl commented Jan 23, 2020

Copy link
Copy Markdown
Member

I think it is valid to change this back if you can add a test that illustrates your use case.

@vonagam

vonagam commented Jan 23, 2020

Copy link
Copy Markdown
Member Author

You can also extend default AuthenticationService with functionality that you want. For example, if i understand correctly, your case can be achieved by:

class MyAuthenticationService extends AuthenticationService {
  async create (data, params) {
    const authResult = await super.create(data, params);
    // do your checks of authResult here
    return authResult;
  }
}

Or even hooks may work (and be more feathers-way) since create is a standard feathers method and authResult will be in context.result in after hooks.

@jalbersdorfer

Copy link
Copy Markdown
Contributor

Thanks guys for your quick response and thank you @vonagam for your hint about overriding the create method in my own AuthenticationService. I already had a derivate of it in order to extend getPayload. I definitively get your point from the performance optimization standpoint, but the outcome for my scenario is all but nice nor clean.

So I want step back a little and thank @daffl for your support about moving back. @vonagam, look at it in a way of consistency. Until now, for every AuthenticationStrategy getPayload got called. Now only for Strategies not authenticating themselfe with a token getPayload gets called.

And from this perspective I also would appreciate to change this back to the consistent behavior.
What are your thoughts about this?

@vonagam

vonagam commented Jan 24, 2020

Copy link
Copy Markdown
Member Author

Why are you against overriding create (or using hook which is better if it is possible) for your use-case while ok with overriding getPayload?

@jalbersdorfer

Copy link
Copy Markdown
Contributor

because that's exactly what I do - I want to modify/change the Payload.

@vonagam

vonagam commented Jan 24, 2020

Copy link
Copy Markdown
Member Author

One decides whenever to use old token or create new one in authenticate (by returning authResult with or without accessToken). So in your case you may want to consider custom auth strategy as an appropriate solution.

For me getPayload simply returns a payload for a newly created token and the method should not be expected to modify authResult or params. Especially since it is called in parallel with getTokenOptions and if you are ok with both of them modifying their arguments you will end up with race conditions...

@jalbersdorfer

jalbersdorfer commented Jan 24, 2020

Copy link
Copy Markdown
Contributor

Yes, you are right! - It Works! Thanks!
It is perfectly clean and nice. It is not prone to race condition, either. Just a perfect solution.
I was, for some reason fixed to the idea of having to overwrite getPayload to modify the payload.

EliSadaka pushed a commit to yusernetwork/authentication-oauth that referenced this pull request Oct 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants