Refactor Token Identification for Agnostic Token Revocation Service and Token Information API
Summary
As a follow-up to #443597 (closed), we'd like to refactor the idenfication of the token type. Currently, the pattern matching to identify the token type is duplicated in two places - Admin Token Information API and AgnosticTokenRevocationService.
Since we are about to extend the pattern matching to support additional token types, we should first consolidate the matching in some kind of helper module.
Implementation Plan
@nmalcolm suggested some approaches in the previous issue.
My suggested approach would be a slight variation of an concern-like pattern, though I'm open to the original approach as well:
The difference is that I'd like to pass the plaintext
token and the resolved_token
as parameters, instead of relying on an existing method/attribute/reader. The reason is that unlike in the AgnosticTokenService, in API::Admin::Token the token comes from a param
, and I would have to store the parameter in Token
first. I think this is fine for a service, but I'm unsure if we should to that in a REST API object.
# lib/agnostic_token_handler.rb
module AgnosticTokenHandler
def handle_plaintext(plaintext)
if plaintext.start_with?(Gitlab::CurrentSettings.current_application_settings.personal_access_token_prefix,
ApplicationSetting.defaults[:personal_access_token_prefix])
resolved_token = PersonalAccessToken.find_by_token(plaintext)
handle_personal_access_token(resolved_token)
elsif plaintext.start_with?(DeployToken::DEPLOY_TOKEN_PREFIX)
resolved_token = DeployToken.find_by_token(plaintext)
handle_deploy_token(resolved_token)
else
handle_not_found
end
end
def handle_personal_access_token(resolved_token)
raise 'Not implemented'
end
def handle_deploy_token(resolved_token)
raise 'Not implemented'
end
def handle_not_found
raise 'Not implemented'
end
end
# app/services/groups/agnostic_token_revocation_service.rb
module Groups # rubocop:disable Gitlab/BoundedContexts -- This service is strictly related to groups
class AgnosticTokenRevocationService < Groups::BaseService
include AgnosticTokenHandler
...
def execute
return error("Feature not enabled") unless Feature.enabled?(:group_agnostic_token_revocation, group)
return error("Group cannot be a subgroup") if group.subgroup?
return error("Unauthorized") unless can?(current_user, :admin_group, group)
handle_plaintext(plaintext)
end
# Implement our own handlers here
def handle_personal_access_token(resolved_token)
if user_has_group_membership?(resolved_token.user)
...
end
def handle_deploy_token(resolved_token)
handle_not_found unless resolved_token.group_type?
...
I prefer this approach over the other suggestions:
One way would be a simple resolver with an interface like
TokenResolver.new(plaintext).token
that just gives you a token / nil. That'd be useful to all the endpoints, revocation or not.
I really like the simplicity of this approach. However, we already have code paths that depend on the type of the token - so we would have to check again after resolving the token. Even though it would be simpler now (e.g. we could check based on the class instead of doing pattern matching on a string). Therefore, I prefer the suggested handler.
A third way would be some kind of revocation module where the logic to check whether to revoke is implemented by the specific classes.
That was actually my first thought, however I prefer the explicitness of the concern-like pattern above. I'm not sure if adding another level of indirection is helpful to (potentially) save a few lines of code.
Improvements
- Reduce code duplication
- When changes to token patterns occur, we only need to be update one place
Risks
Both APIs could break, however both have good test coverage and are behind a feature flag.
Involved components
app/services/groups/agnostic_token_revocation_service.rb
lib/api/admin/token.rb