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

FFS-2403: Token-based authentication #431

Merged
merged 15 commits into from
Feb 3, 2025
Merged

Conversation

allthesignals
Copy link
Contributor

@allthesignals allthesignals commented Jan 31, 2025

Ticket

Resolves FFS-2403.

Changes

Adds API Access Tokens, associates with a User, provides some methods to create and find/validate a user by token. Protects the invitations api with token auth.

Context for reviewers

We switched to a different method of encrypting the token so that we could search for the token itself.

  • User model has new boolean flag to indicate that it's a service account, is_service_account
  • User model generates API tokens through some method on the model or via a route
  • User model method that validates the authenticity of the token
  • API tokens generated by this model can be revoked (deleted).
  • Thorough unit tests

Acceptance testing

  • No acceptance testing needed
    • This change will not affect the user experience (bugfix, dependency updates, etc.)
  • Acceptance testing prior to merge
    • This change can be verified visually via screenshots attached below or by sending a link to a local development environment to the acceptance tester
    • Acceptance testing should be done by design for visual changes, product for behavior/logic changes, or both for changes that impact both.
  • Acceptance testing after merge
    • This change is hard to test locally, so we'll test it in the demo environment (deployed automatically after merge.)
    • Make sure to notify the team once this PR is merged so we don't inadvertently deploy the unaccepted change to production. (e.g. :alert: Deploy block! @ffs-eng I just merged PR [#123] and will be doing acceptance testing in demo - please don't deploy until I'm finished!)

Comment on lines 4 to 8
has_secure_password :access_token, validations: false

before_create do
self.access_token = SecureRandom.urlsafe_base64
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tdooner I'm not sure if this is right... but seems okay, I guess. has_secure_password makes available the authenticate_*** method and that's what I used to validate.

Comment on lines 13 to 15
def self.find_by_valid_access_token(token)
ApiAccessToken.all.find { |t| t.authenticate_access_token(token) }&.user
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what others think but this method was a bit hard to name because it both finds a user by a given token through validating that token against all tokens. Which seems slow. I'm not sure how else token lookup is supposed to work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like maybe you could do something fancier in SQL, i don't know.

Comment on lines 7 to 8
@cbv_flow_invitation = CbvInvitationService.new(event_logger).invite(cbv_flow_invitation_params, @current_user)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if the current_user helper is possible here... but that might hook into whatever Devise has got going on and I'm not sure if that's a can of worms or not...

@allthesignals allthesignals marked this pull request as ready for review January 31, 2025 20:11
@allthesignals allthesignals requested a review from tdooner January 31, 2025 20:12
Comment on lines +19 to +21
before do
request.headers["Authorization"] = "Bearer #{api_access_token.access_token}"
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wish there was a nicer helper for this kind of thing

Comment on lines 1 to 4
FactoryBot.define do
factory :api_access_token do
end
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Feel like I could just put this into the User factor to generate tokens as part of a trait

@allthesignals allthesignals changed the title FFS-2403: First pass at token-based authentication FFS-2403: Token-based authentication Feb 3, 2025
# Conflicts:
#	app/db/schema.rb
#	docs/app/rendered/database-schema.pdf
Copy link
Contributor

@tdooner tdooner left a comment

Choose a reason for hiding this comment

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

Looks good but we will need the terraform before deploying or else I'd bet the app won't start up.

app/.env Show resolved Hide resolved
app/app/models/api_access_token.rb Outdated Show resolved Hide resolved
app/app/models/user.rb Outdated Show resolved Hide resolved
@allthesignals
Copy link
Contributor Author

Created those env variables. I'll go ahead and merge.

@allthesignals allthesignals merged commit 0f4ae0e into main Feb 3, 2025
20 checks passed
@allthesignals
Copy link
Contributor Author

Ugh I misnamed something, fixing

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.

2 participants