Allow using Ai query/subscription with ai_features token
What does this MR do and why?
This MR allows the ai_features
scoped personal access tokens to be used with GraphQL queries/mutations and subscriptions that are necessary for duo chat. This ai_features
scope token is documented in https://docs.gitlab.com/ee/user/profile/personal_access_tokens.html#personal-access-token-scopes and was designed to be used in our IDE extensions. Since we've recently introduced duo chat to our IDE extensions this means we also want to support duo chat with this token scope.
This change was quite a widespread change because this is the first time we've added support for token scopes in GraphQL. We had to introduce a few changes to make this work:
- At the top level of GraphQL queries and subscriptions we always allow
ai_features
scopes. This does not open up all GraphQL queries to use this because we are still validating the specific query types/mutations/fields for their individual scopes - We needed to introduce the ability to override the scopes at the type/object/field level and then add
ai_features
for all the types and fields used in our duo chat features
How to test
Enable the :graphql_minimal_auth_methods
feature flag:
Feature.enable(:graphql_minimal_auth_methods)
Feature.enable(:allow_ai_features_token_for_graphql_ai_features)
The following script will demonstrate:
- An
ai_features
scope token can call the duo chat API - An
api
scope token can call theupdateIssue
mutation - An
ai_features
scope token cannot call theupdateIssue
mutation
#api_scope_token=<GET_TOKEN>
#ai_features_scope_token=<GET_TOKEN>
curl 'http://127.0.0.1:3000/api/graphql' \
-H 'Content-Type: application/json' \
-H "Private-Token: $ai_features_scope_token" \
--data-raw '{"query":"mutation {\n aiAction(input: {chat: {content: \"Hello\"}}) {\n clientMutationId\n requestId\n }\n}","variables":null}'
echo
curl 'http://127.0.0.1:3000/api/graphql' \
-H 'Content-Type: application/json' \
-H "Private-Token: $api_scope_token" \
--data-raw '{"query":"mutation {\n updateIssue(input: {description: \"Correct update!\", projectPath:\"twitter/Typeahead.Js\", iid: \"7\"}) {\n clientMutationId\n issue {iid}\n }\n}","variables":null}'
echo
curl 'http://127.0.0.1:3000/api/graphql' \
-H 'Content-Type: application/json' \
-H "Private-Token: $ai_features_scope_token" \
--data-raw '{"query":"mutation {\n updateIssue(input: {description: \"Bad update!\", projectPath:\"twitter/Typeahead.Js\", iid: \"7\"}) {\n clientMutationId\n issue {iid}\n }\n}","variables":null}'
echo
Additionally you can read about how to test the GraphQL subscriptions in #455805 (closed) . But testing the duo chat functionality this way is tricky. So the easiest way to test the duo chat is to install one of the editor extensions (I was using Intellij IDEA GitLab Duo extension) and point it at your local GDK. But also this requires having all the AI stuff setup which is quite a process. But I have done this testing locally.
MR acceptance checklist
Please evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.
Screenshots or screen recordings
Screenshots are required for UI changes, and strongly recommended for all other merge requests.
Before | After |
---|---|
How to set up and validate locally
Numbered steps to set up and validate the change are strongly suggested.
Related to #455023 (closed)
Merge request reports
Activity
assigned to @DylanGriffith
- A deleted user
added backend label
9 Warnings This merge request is quite big (520 lines changed), please consider splitting it into multiple merge requests. 32243f7b: The commit body should not contain more than 72 characters per line. For more information, take a look at our Commit message guidelines. bd0f9f4e: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. 73e3e513: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. 1727307e: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. 9eb03142: The commit subject must start with a capital letter. For more information, take a look at our Commit message guidelines. 9eb03142: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. This merge request has more than 20 commits which may cause issues in some of the jobs. If you see errors like missing commits, please consider squashing some commits so it is within 20 commits. Do not add new controller specs. We are moving from controller specs to
request specs (and/or feature specs). Please add request specs under
/spec/requests
and/or/ee/spec/requests
instead.See &5076 for information.
1 Message CHANGELOG missing: If this merge request needs a changelog entry, add the
Changelog
trailer to the commit message you want to add to the changelog.If this merge request doesn't need a CHANGELOG entry, feel free to ignore this message.
Reviewer roulette
Category Reviewer Maintainer backend @rliu-gl
(UTC-4, 14 hours behind author)
@harsimarsandhu
(UTC+5.5, 4.5 hours behind author)
Please check reviewer's status!
Please refer to documentation page for guidance on how you can benefit from the Reviewer Roulette, or use the GitLab Review Workload Dashboard to find other available reviewers.
If needed, you can retry the
danger-review
job that generated this comment.Generated by
Dangeradded 1 commit
- dd0521e4 - Remove arguments from authorization as it is called from elsewhere
- Resolved by Dylan Griffith
- Resolved by Dylan Griffith
- Resolved by Dylan Griffith
mentioned in issue #455023 (closed)
- Resolved by Dylan Griffith
- Resolved by Dylan Griffith
- Resolved by Luke Duncalfe
- Resolved by Dylan Griffith
- Resolved by Dylan Griffith
The difference in the response for a
read_api
scoped token hitting a mutation between this branch andmaster
is the same, except there's anerrors
response inmaster
.master
:{"data":{"aiAction":null},"errors":[{"message":"The resource that you are attempting to access does not exist or you don't have permission to perform this action","locations":[{"line":2,"column":3}],"path":["aiAction"]}]}
this branch:
{"data":{"aiAction":null}}
I think it's because normally when a field doesn't pass its authorization, we don't raise a special exception (
GraphQL::ExecutionError
or any subclass) thatgraphql-ruby
transforms into an error response.BaseMutation
does, so we get that extra response.I ran out of time to test this idea overly much, but we may be able to have
BaseField
also raise the same exception if the field is onMutationType
:diff --git a/app/graphql/types/base_field.rb b/app/graphql/types/base_field.rb index 0a2c6639957d..79cf2ad23911 100644 --- a/app/graphql/types/base_field.rb +++ b/app/graphql/types/base_field.rb @@ -98,7 +98,11 @@ def constant_complexity? def field_authorized?(object, ctx) object = object.node if object.is_a?(GraphQL::Pagination::Connection::Edge) - authorization.ok?(object, ctx[:current_user], scope_validator: ctx[:scope_validator]) + return if authorization.ok?(object, ctx[:current_user], scope_validator: ctx[:scope_validator]) + + if owner == Types::MutationType + raise Gitlab::Graphql::Errors::ResourceNotAvailable, Gitlab::Graphql::Authorize::AuthorizeResource::RESOURCE_ACCESS_ERROR + end end # Historically our resolvers have used declarative permission checks only
Edited by Luke Duncalfe
- Resolved by Dylan Griffith
Thought
I know this is a very simple solution compared to what we have here, and it only accounts for mutations right now, but what if we just created a subclass
BaseAiMutation
?I tried experimenting with defining
permitted_scopes
in the mutation class itself, similar toauthorize
, but that doesn't get sent to theauthorized?
method.app/graphql/mutations/base_mutation.rb module Mutations class BaseMutation def self.authorized?(object, context) auth = ::Gitlab::Graphql::Authorize::ObjectAuthorization.new(:execute_graphql_mutation, permitted_scopes) end def self.permitted_scopes [:api] end end end
app/graphql/mutations/base_ai_mutation.rb module Mutations class BaseAiMutation < BaseMutation def self.permitted_scopes [:ai_features] end end end
Edited by Hinam Mehra
- Resolved by Dylan Griffith
added 2 commits
assigned to @.luke
- Resolved by Dylan Griffith
- Resolved by Dylan Griffith
Check that all subscriptions apart from AI are blocked with ai_features
- Resolved by Dylan Griffith
Confirm
ai_features
still don't work on other APIs that usefind_sessionless_user
mentioned in issue #455805 (closed)
added 3311 commits
-
b252374c...bb96b615 - 3306 commits from branch
master
- da946b7f - Draft: allow using Ai::Action with ai_features token
- 663ca04d - Remove arguments from authorization as it is called from elsewhere
- 35fe12a7 - Add ai_features to Ai::Action.errors
- f2464afe - Remove hack return request_id
- 681c9e61 - Move ai_features into GraphqlController, allow ai_features in channel
Toggle commit list-
b252374c...bb96b615 - 3306 commits from branch
added 1 commit
- a65614d7 - Open up rest of objects/fields for ai_features graphql
added 1 commit
- 64bf3048 - Add spec for validation ai_features token only works for AI subscription
added 302 commits
-
64bf3048...886e13db - 294 commits from branch
master
- b0ed3bb8 - Draft: allow using Ai::Action with ai_features token
- 1abb46e2 - Remove arguments from authorization as it is called from elsewhere
- 2f2b4da0 - Add ai_features to Ai::Action.errors
- fee106b2 - Remove hack return request_id
- 71dcf3a9 - Move ai_features into GraphqlController, allow ai_features in channel
- 5fade428 - Open up rest of objects/fields for ai_features graphql
- bd49985a - Add spec for validation ai_features token only works for AI subscription
- 39d4af53 - Fix graphql_name lint
Toggle commit list-
64bf3048...886e13db - 294 commits from branch
added 1 commit
- 693e031e - Explicitly only allow api scope tokens for mutation fields
- Resolved by Dylan Griffith
TODO: Need to move all mention of
ai_features
toee
overrides.
- Resolved by Dylan Griffith
- Resolved by Dylan Griffith