Skip to content
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

Ensure access tokens can only access specific allowed routes #827

Merged
merged 3 commits into from Jul 29, 2022

Conversation

knolleary
Copy link
Member

This removes verifyTokenOrSession as we want to be more deliberate and explicit over what routes can be accessed by a token, rather than a login session.

We have stories related to user-generated access tokens. When we come to them, we will need to carefully open it back up.

There are three sets of end points that need to be token-accessible:

  • /api/v1/user - used as part of nr-auth to get a user's identify when logging into node-red editor
  • /api/v1/teams/<teamid>/projects - used by Project Nodes to get a list of projects in the team. I've added logic to verify the token being used belongs to a project owned by the team it is accessing.
  • /api/v1/devices/<device>/live/* - used by device agent

I have verified all of those routes behave themselves with this change.

We want to be more intentional over what routes tokens are allowed
to access.
@knolleary knolleary requested review from hardillb and Steve-Mcl and removed request for hardillb July 29, 2022 15:09
Copy link
Contributor

@Steve-Mcl Steve-Mcl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR looks far better than just checking the user is undefined

Question: when verifying using token, does the request.session.User get populated now or will it still be empty? I ask as I use its absence of the User property to limit the contents of the project listing in the API (Line 118 const limitResponse = ... in forge/routes/api/team.js)

@knolleary
Copy link
Member Author

Tokens are not (currently) associated with a user. At that point in the code, request.session will be defined because it has passed through authentication, but request.session.User will not be defined because it is token based auth.

It would technically be more accurate to use as the check request.session.ownerType === 'project' as that is more explicit. I'll update the PR shortly.

@knolleary
Copy link
Member Author

Change pushed. This is good for final review/merge.

@hardillb hardillb merged commit 8444124 into block-unverified-user Jul 29, 2022
@hardillb hardillb deleted the tighten-token-handling branch July 29, 2022 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants