-
Notifications
You must be signed in to change notification settings - Fork 4
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
Newspaper Archive auth endpoint - check supporter entitlement #1404
Conversation
return {}; | ||
} | ||
}; | ||
|
||
export const customMembersDataApiHandler = proxyApiHandler( |
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.
Could we use this function rather than having to export the authorization one?
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 had a go at this but it's quite hard to wrangle. Because it's a callback that doesn't return anything, we can give it our express Response object but we don't actually want to hijack what we return to the user
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 this looks good. I left a couple of bits of minor feedback to take or leave, see what you think.
server/routes/newspaperArchive.ts
Outdated
method: 'GET', | ||
headers: { | ||
...(await authorizationOrCookieHeader({ req, host })), | ||
'Content-Type': 'application/json', |
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.
Super minor point - but do we need to send a Content-Type
header here? My understanding is that it's needed as a request header when sending a request with a particular content type in the body (which isn't the case here as it's a GET).
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 you're right! Was copied from the other code that calls mdapi but doesn't seem necessary here. I've also copied the X_GU_ID_FORWARDED_SCOPE
which I'm assuming is important. I'll try test without it...
server/routes/newspaperArchive.ts
Outdated
return res.sendStatus(500); | ||
} | ||
|
||
const hasCorrectEntitlement = await checkSupporterEntitlement(req); |
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 wonder if this should be wrapped in a try/catch in case any of the underlying operations error (for example if the request to mdapi fails)? I guess in that case we'd want to set hasCorrectEntitlement
to false to be on the safe side.
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've wrapped the whole thing in a try catch, seems more accurate than eg telling a user they don't have a product in the case the mdapi call failed
Seen on PROD (merged by @andrewHEguardian 10 minutes and 7 seconds ago) Please check your changes! |
Current situation/background
We have an endpoint that authenticates a user, and sends them off to newspapers dot com. It doesn't check if they're actually entitled to go there.
What does this PR change?
This PR adds a check of the user's supporter entitlements - by sending a request to members data api's
user-attributes/me
endpoint.Next steps/further info
I've left 3 todos: