chore(dependencies)!: update grant from 4.x to 5.x #1989

wolfgangwalther · 2020-06-09T12:41:25Z

This updates @feathersjs/authentication-oauth to use the new version of grant (5.x). (#1961)

Among other stuff, the new grant version offers the possibility to pass options to the underlying http client, which is used for the request for profile_url. In my project I need to add proxy settings to this request.

This should go together with a corresponding PR for the docs, as some things have changed with grant. This still needs to be done.

Unfortunately I was not able to test those changes in a real project. I tried installing the @feathersjs/authentication-oauth package from my own git repo instead of npm, but this didn't work out well, because of the monorepo structure.
Because the unit tests don't implement a full oauth flow, I guess this should still be tested somehow. Especially the "getting the profile" part, because that was not tested but mocked before already and has completely changed now.
Any ideas on that, how to proceed here?

wolfgangwalther · 2020-06-09T12:43:25Z

packages/authentication-oauth/test/express.test.ts

@@ -40,12 +48,4 @@ describe('@feathersjs/authentication-oauth/express', () => {
      delete app.get('authentication').oauth.redirect;
    }
  });
-
-  it('oauth/test/authenticate with error', async () => {


Not sure what to do with that test. The thrown error was only implemented in the mocked getProfile function. Not sure what that was supposed to test in the first place.

Since getProfile doesn't exist anymore, I just removed the test for now.

wolfgangwalther · 2020-06-09T12:45:43Z

packages/authentication-oauth/src/express.ts

-    });
-
-    authApp.get('/:name/callback', (req: any, res: any) => {
-      res.redirect(`${path}/connect/${req.params.name}/callback?${qs.stringify(req.query)}`);


The new grant version mounts at /prefix instead of /prefix/connect - so redirecting is not needed anymore, but the request can just be passed on to the next handler.

wolfgangwalther · 2020-06-09T12:47:00Z

packages/authentication-oauth/src/index.ts

+      prefix,
+      origin: `${protocol}://${host}`,
+      transport: 'session',
+      response: ['tokens', 'raw', 'profile']


['tokens', 'raw'] should be the default - not sure whether tokens would be enough?

profile is added, to make grant make the request to profile_url.

wolfgangwalther · 2020-06-09T12:47:33Z

packages/authentication-oauth/src/strategy.ts

@@ -1,5 +1,4 @@
 // @ts-ignore
-import getProfile from 'grant-profile/lib/client';


grant-profile is now merged into grant itself.

daffl · 2020-06-09T16:18:04Z

Thank you for the pull request, this is great! Since these are definitely breaking changes, could you do me a favour and resubmit it against the v5 branch at https://github.com/feathersjs/feathers/tree/dove?

To test the changes in an application, in the Feathers repository you can do

cd packages/authentication-oauth
npm run compile
npm link

Then in your project

npm link @feathersjs/authentication-oauth

The same steps work for any other modules that changed.

As for getting the profile, I think the getProfile method should stay as it is but just return the data.profile that Grant includes now. The other problem is that I don't think this change allows to authenticate with an existing access token which is very important for usage in e.g. mobile applications.

One way to improve the tests would be to create a test oAuth application, get an access token via an HTTP request and run the test with that. It wouldn't test the browser flow though of course.

wolfgangwalther · 2020-06-09T16:38:56Z

So I rebased on dove, force-pushed and changed the base branch for this PR - I guess that should do it in that regard?

Keeping getProfile: I guess I expected that and actually I agree ;)

The other problem is that I don't think this change allows to authenticate with an existing access token which is very important for usage in e.g. mobile applications.

Can you elaborate? Whats "this change" here - the whole update of grant or just the "removing getProfile" part?

Thanks for the hint about npm link. That sounds useful, but won't be as straightforward in my case, I guess: My application runs in docker containers - for development as well. So "global node_modules" is different for my application and the feathers repo. But I'll figure something out with that.

nathanbrizzee · 2020-07-17T02:30:17Z

Hi @daffl , any idea when this pull request might be available? I have a desperate need behind our corporate ntlm proxy. All web access is proxied so I can't get any of the OAuth grant to work unless I am outside our proxy server. The current 4.x branch works fine as long as I am not behind a proxy. Otherwise, It simply times out. I've tried the monkey patch suggestion by the author simov/grant#93 , but I can't get it to work either. Any help would be greatly appreciated.

daffl · 2020-07-17T03:05:53Z

Because of the breaking changes this pull request requires a major release of which upgrading Grant is only a minor part. There is no release date for v5 other than before the end of the year and there is a lot that still needs to be done. You are welcome to book an office hour to see if there are ways to get things working for you before.

wolfgangwalther · 2020-07-17T08:50:16Z

As for getting the profile, I think the getProfile method should stay as it is but just return the data.profile that Grant includes now. The other problem is that I don't think this change allows to authenticate with an existing access token which is very important for usage in e.g. mobile applications.

So I brought back getProfile. Does that solve the "access token" problem you are referencing, @daffl, or do you have something else in mind here?

One way to improve the tests would be to create a test oAuth application, get an access token via an HTTP request and run the test with that. It wouldn't test the browser flow though of course.

Will look into this.

wolfgangwalther · 2020-07-17T09:54:33Z

The build is failing - but I have no idea how that error might be related to my changes. I don't know TypeScript well, though.

daffl · 2020-10-17T18:09:38Z

Thank you again @wolfgangwalther, sorry it took so long! I got the build fixed and it is now merged with the dove branch. I will do some more testing and figure out how to bring back authenticating with existing oAuth access tokens and get it out with another v5.x pre-release.

BREAKING CHANGES: Updates Grant to the latest major version (see https://github.com/simov/grant/blob/master/MIGRATION.md)

wolfgangwalther commented Jun 9, 2020

View reviewed changes

wolfgangwalther changed the base branch from master to dove June 9, 2020 16:30

wolfgangwalther and others added 3 commits October 16, 2020 20:37

chore(dependencies)!: update grant from 4.x to 5.x

67a756f

bring back getProfile

1964864

Update packages files

2bf05ea

daffl merged commit 37f7a4a into feathersjs:dove Oct 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(dependencies)!: update grant from 4.x to 5.x #1989

chore(dependencies)!: update grant from 4.x to 5.x #1989

wolfgangwalther commented Jun 9, 2020

wolfgangwalther Jun 9, 2020

wolfgangwalther Jun 9, 2020

wolfgangwalther Jun 9, 2020

wolfgangwalther Jun 9, 2020

daffl commented Jun 9, 2020

wolfgangwalther commented Jun 9, 2020

nathanbrizzee commented Jul 17, 2020 •

edited

Loading

daffl commented Jul 17, 2020

wolfgangwalther commented Jul 17, 2020

wolfgangwalther commented Jul 17, 2020

daffl commented Oct 17, 2020

		@@ -1,5 +1,4 @@
		// @ts-ignore
		import getProfile from 'grant-profile/lib/client';

chore(dependencies)!: update grant from 4.x to 5.x #1989

chore(dependencies)!: update grant from 4.x to 5.x #1989

Conversation

wolfgangwalther commented Jun 9, 2020

wolfgangwalther Jun 9, 2020

Choose a reason for hiding this comment

wolfgangwalther Jun 9, 2020

Choose a reason for hiding this comment

wolfgangwalther Jun 9, 2020

Choose a reason for hiding this comment

wolfgangwalther Jun 9, 2020

Choose a reason for hiding this comment

daffl commented Jun 9, 2020

wolfgangwalther commented Jun 9, 2020

nathanbrizzee commented Jul 17, 2020 • edited Loading

daffl commented Jul 17, 2020

wolfgangwalther commented Jul 17, 2020

wolfgangwalther commented Jul 17, 2020

daffl commented Oct 17, 2020

nathanbrizzee commented Jul 17, 2020 •

edited

Loading