-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
validate player secrets on public API requests #125
Conversation
let filename = this.getFilename(res); | ||
res.blob().then(fileBlob => { | ||
var url = URL.createObjectURL(fileBlob); | ||
var a = document.createElement('a'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it really the best way to temporarily create an a-tag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, we should be consistent with using 'let' and 'var'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we may have talked about this, I couldn't actually get it to download using window.assign() or window.open() it would just display the file on the screen
|
router.get('/model/:id', async (ctx) => { | ||
const matchID = ctx.params.id; | ||
//authorise | ||
router.use('/:matchID/', async (ctx, next) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I understand it correctly that routes above this router.use()
call are not authenticated and only the ones belows actually are?
If yes, I am not sure if this is a good solution. This will definitly lead to confusion if someone wants to add another endpoint in the future... Would it be possible to attach the middleware only to certain routes (and make this explicit)? Or alternatively, could you add something like a whitelist, such that the middleware by default wants to authorize everything and only the routes in the whitelist are not considered?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be that the routes which match the path '/:matchID/'
use the authorisation.
the syntax is router.use(path, callback)
where the path is either a string or an array of paths.
It might be better to use '/:matchID/*'
(which acts the same but might be a bit clearer)
or to explicitly list all the complete paths: ['/:matchID/players', '/:matchID/model/', ... ]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see. Cool! I think '/:matchID/*'
would be good. Then the logic is that everything specific to a game should be authenticated, which sounds reasonable to me.
Just for clearity, could you move the router.use()
statement above all the route definitions? And could you please mention in the comment that the middleware only applies to the '/:matchID/*'
routes, please?
@matthewejones Just had a manual test of this PR. Works great! I love it! |
done:
Validate player secrets for the following public api requests:
players
model
download
download/text
Download buttons use get request
todo:
Resolves #70