Closed endel closed 9 months ago
Hi @endel,
Thanks for your continued interest in using Grant, I really appreciate that.
The Grant: missing or misconfigured provider
error is there only for debugging purposes. The correct response in that case should have been a 404 Not Found
instead. But because with 404
you won't know if your server was configured correctly or not, I have decided to tell you that 'Your server is listening, and Grant is accessible, but maybe there is something wrong with your configuration'. And since it is almost impossible to configure Grant incorrectly to get to that error in particular, it almost certainly means that there is no such provider in your configuration.
Here is how a potential implementation may look like for Express:
var Grant = require('../../').express()
var grant = Grant(require('./config.json'))
express()
// ...
.use('/connect/:provider', (req, res, next) => {
Object.keys(grant.config).includes(req.params.provider)
? next()
: res.status(404).send('Not found')
})
.use(grant)
// ...
.listen(3000)
Similar implementation can be achieved for any of the provided handlers.
If you want to handle that error in some other way, other than a 404
, then what would be the difference between implementing this simple middleware, and implementing a dedicated handler just for that error or maybe updating your existing catch all error handler with the logic to handle this error in particular?
In the end whether you want a redirect to some other route or simply responding with a 404
seems like an implementation detail in your own layer. As mentioned here https://github.com/simov/grant/issues/194#issuecomment-703260509:
Basically this is the safest way to throw an error without making any assumptions about the environment.
Now when thinking about it again I don't know why the author of that thread considered having a middleware in front of Grant being a bit of a hack, so that I had to come up with that really weird implementation instead. There are many other cases where you may want to augment the Grant middleware with a little bit of code, for example recently we had the case of handling alternating domains, but even for builtin features that is required in some cases, like dynamic state overrides and the state transport itself.
Let me know if I am missing something about your setup in particular.
I spent a bit more time thinking about this and I wanted to elaborate a little bit more on what are we trying to fix with this proposed feature.
With this proposed feature we are trying to solve the issue of redirecting to the root /
path when the transport: 'querystring'
is being used (default) and a provider does not have a callback
set.
This covers:
callback
)callback
setTo fix 1. we can add a simple guard middleware before Grant and handle it accordingly:
.use('/connect/:provider', (req, res, next) => {
Object.keys(grant.config).includes(req.params.provider)
? next()
: res.status(404).send('Not found')
})
To fix 2. we can define a default callback
to use for all providers found in our configuration:
{
"defaults": {
"callback": "/default"
}
}
With this setup:
/connect/:provider
route will return 404 Not Found
callback
explicitly set will redirect to the /default
route instead, found in the defaults
There is one more case to cover and that is someone accessing the /connect/:provider/callback
route directly (outside of the OAuth flow):
.use('/connect/:provider/callback', (req, res, next) => {
Object.keys(grant.config).includes(req?.session?.grant?.provider)
? next()
: res.status(404).send('Not found')
})
Note that transport: 'session'
is not affected by this issue because having a callback
URL in that case is optional, and if missing it will return through the session object instead, and transport: 'state'
can only return through the state object.
Here is the full example:
{
"defaults": {
"origin": "http://localhost:3000",
"transport": "querystring",
"callback": "/default"
},
"google": {
"key": "APP_ID",
"secret": "APP_SECRET",
"scope": [
"openid"
]
}
}
var express = require('express')
var session = require('express-session')
var Grant = require('../../').express()
var grant = Grant(require('./config.json'))
express()
.use(session({secret: 'grant', saveUninitialized: true, resave: false}))
.use('/connect/:provider', (req, res, next) => {
Object.keys(grant.config).includes(req.params.provider)
? next()
: res.status(404).send('Not found')
})
.use('/connect/:provider/callback', (req, res, next) => {
Object.keys(grant.config).includes(req?.session?.grant?.provider)
? next()
: res.status(404).send('Not found')
})
.use(grant)
.get('/default', (req, res) => {
res.end(JSON.stringify(req.query, null, 2))
})
.listen(3000)
Navigating to:
http://localhost:3000/connect/twitter
- returns 404 Not Found
http://localhost:3000/connect/google/callback
- returns 404 Not Found
To sum it up:
defaults.callback
solves the issue of having a provider without an explicit callback
/connect/:provider
route is a perfectly valid solution to guard against providers not found in your grant.config
/connect/:provider/callback
endpoint solves the problem of someone accessing the callback route outside of the OAuth flow@endel let me know what you think
Thank you SO much for the detailed response - your first suggestion was exactly what I was looking for. The further advice is also immensely helpful 💙
Hi @simov,
I would like to be able to customize the default response location if a provider hasn't been configured yet. Currently, it is hardcoded to
"/"
:https://github.com/simov/grant/blob/c220e9ef812978608a4c05551b3f241e3e9da249/lib/response.js#L108
The reasoning is: I'd like to provide a friendly message when a provider wasn't configured, instead of showing this:
I'd like to send a PR to allow customizing this, would you mind pointing me a preferred direction to allow such customization?
(Thank you for your work again 🙏
grant
has been in my toolbelt for years, and it's really great!)