Closed Kequc closed 6 years ago
Merging #132 into master will not change coverage. The diff coverage is
100%
.
@@ Coverage Diff @@
## master #132 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 18 18
Lines 798 798
=====================================
Hits 798 798
Hi @Kequc, thanks for engaging with openid-client.
There was not any way to make use of alternate redirect_uri's in the library.
Are you sure? You can have the strategy push any parameters down to authorize already...
strategy.authenticate({ params: { redirect_uri: 'https://example.com/cb2' } });
or am i missing something?
That test specifies the redirect_uri
in the Strategy
constructor, whereas I might use the same strategy for more than one authenticate()
.
yeah, but isn't that a matter of just calling authenticate in your callback the same way you do in your init route?
authenticate already takes options and those options' redirect_uri
is used in the callback part of the strategy.
like so:
strategy.authenticate(req, { redirect_uri: 'https://example.com/cb2' });
This is what i mean. See that the client instance here does not have a redirect_uri set. And yet the request and callback uses one that it figures out on the fly.
const { Issuer, Strategy } = require('openid-client')
const express = require('express');
const passport = require('passport');
const cookieParser = require('cookie-parser');
const cookieSession = require('cookie-session');
const app = express();
const users = new Map();
(async () => {
const { Client } = await Issuer.discover('https://guarded-cliffs-8635.herokuapp.com');
const client = new Client({
client_id: '4da2311d-0f14-407f-9f6d-53a5c5d51ea4',
client_secret: 'yD1X0e/q+3iZTRVIaW4tIDzzEuspWD1rgMfVpZcAr2G2Yr2+h3oBEyzG8Ksm/3ST',
});
client.CLOCK_TOLERANCE = 5;
passport.use('oidc', new Strategy({ client }, (tokenset, userinfo, done) => {
return done(null, userinfo);
}));
passport.serializeUser((user, done) => {
users.set(user.sub, user);
done(null, user.sub);
});
passport.deserializeUser((sub, done) => {
done(null, users.get(sub));
});
app.use(cookieParser());
app.use(cookieSession({ secret: 'foo', resave: false, saveUnitialized: true, cookie: { secure: false } }));
app.use(passport.initialize());
app.use('/init', (req, res, next) => {
// figure out which redirect_uri to use and use it on the fly
const redirect_uri = 'http://lvh.me:3000/oidc/cb';
passport.authenticate('oidc', { redirect_uri })(req, res, next);
});
app.use('/oidc/cb', (req, res, next) => {
// figure out which redirect_uri to use and use it on the fly
const redirect_uri = 'http://lvh.me:3000/oidc/cb';
passport.authenticate('oidc', { redirect_uri })(req, res, next);
}, (req, res) => {
res.json(req.user);
});
app.listen(3000);
})().catch((err) => {
console.error(err);
process.exit(1);
});
Note: the example client id and secret will expire in 24 hours and only this one redirect uri is actually whitelisted, but that's besides the point.
I would very much welcome your example snippets to support this PR. I'm not a passport poweruser so there's likely something i'm missing ;)
Sure, well for example.
const params = {
redirect_uri: `${myUrl}/login-callback`
};
passport.use('oidc', new Strategy({ client, params }, stratCallback));
app.get('/login', passport.authenticate('oidc', {
redirect_uri: `${myUrl}/login-callback`
}));
app.get('/login-callback', passport.authenticate('oidc', {
successRedirect: '/',
failureRedirect: '/login',
flashMessage: true
}));
app.get('/change-password', passport.authenticate('oidc', {
redirect_uri: `${myUrl}/change-password-callback`
}));
app.get('/change-password-callback', passport.authenticate('oidc', {
successRedirect: '/account',
failureRedirect: '/login',
flashMessage: true
}));
Here, /login
works.
However /change-password
throws an error invalid_grant
, if I change params.redirect_uri
to use ${myUrl}/change-password-callback
then /change-password
works and /login
throws an error invalid_grant
.
Upon inspection, the reason was that regardless of my redirect_uri
working, and causing redirection to the correct place. The details of the interaction had invalid data, the wrong redirect_uri
. My PR corrects this so that the redirect_uri
is set properly and eliminates the error message.
I checked the code and verified that the above works without the change in the PR by just specifying the redirect_uri in the callback too. So is this PR really needed?
app.get('/login', passport.authenticate('oidc', {
redirect_uri: 'http://lvh.me:3000/login-callback',
}));
app.get('/login-callback', passport.authenticate('oidc', {
redirect_uri: 'http://lvh.me:3000/login-callback'
}), (req, res) => {
res.json(req.user);
});
app.get('/change-password', passport.authenticate('oidc', {
redirect_uri: 'http://lvh.me:3000/change-password-callback',
}));
app.get('/change-password-callback', passport.authenticate('oidc', {
redirect_uri: 'http://lvh.me:3000/change-password-callback'
}), (req, res) => {
res.json(req.user);
});
I see. Quite a lot of time was spent trying to debug this problem, I wonder how it could be communicated better in the documentation.
Would you say this is a niche setup rather than something everyone should be aware of? A few more examples in the readme would be helpful, i'll come up with some. Does the existing way satisfy your needs?
It does in some ways feel like just another thing that can go wrong. The value being used redirect_uri
must be stored in two different locations and when they don't match it doesn't work. I can understand the functionality as a whole is quite complicated but I wouldn't sacrifice ease of use.
In my mind, and I'm sure I'm not as well versed on this as you are, having a single source of truth is generally better. If I were to change something, I'm not sure what I would change.
when they don't match it doesn't work
kind of the point, a client should be very exact / should know eaxctly the redirect_uri
it uses for the token endpoint call from where
I want to thank you for the contribution but since there's already a way to have one strategy instance handle multiple callback uris I'm going to close. With one follow up item, to provide more examples of the strategy being used, this case being one of them.
There was not any way to make use of alternate redirect_uri's in the library. This change adds the ability to set
redirect_uri
in theauthenticate()
options.This is in-line with the capabilities of oidc/openid.