Fix admin_display_duo_addon_settings? check

What does this MR do and why?

Related to https://gitlab.com/gitlab-org/gitlab/-/issues/523362

In this MR, we:

Fix admin_display_duo_addon_settings? to check for purchased add-ons from Fulfillment APIs instead of Cloud connector

Changelog: fixed EE: True

changed milestone to %17.10

added bugfunctional groupcustom models typebug labels

assigned to @mhamda

added devopsai-powered sectiondata-science labels

added pipelinetier-1 label

added backend label

Reviewer roulette

Category	Reviewer	Maintainer
backend	@morefice (UTC+1, same timezone as author)	@marc_shaw (UTC+8, 7 hours ahead of author)

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 Danger

added 1 commit

• cca1a856 - Fix admin_display_duo_addon_settings? check

Compare with previous version

requested changes

added 1 commit

• a1dd2f83 - Fix the scope to apply for_self_managed

Compare with previous version

@mhamda I had an unrelated question about the content of this check, so starting a new thread for this.

It seems odd to me that we even need to test for self-hosted models here. The GitLab Duo configuration page has GitLab Duo scope. Wouldn't that suggest that all we need to check for is whether some variation of the Duo Addon was purchased?

I see we also surface self-hosted settings here (assuming I'm looking at the right page):

This makes me wonder: if these settings specific to SHM like the AIGW URL textfield would live in a partial, the cleaner approach would be to:

1. Guard the entire endpoint / page with a duo_addon_purchased? check
2. Guard the SHM partial with an additional self_hosted_models_purchased? check

This would be another opportunity to remove an if-else check and provide a cleaner approach to which settings are guarded by which condition.

WDYT?

OK so frontend is not my strong suit @mhamda but I think it's this file that is being rendered here: https://gitlab.com/gitlab-org/gitlab/-/blob/adfd5a0556a2bf0d68ee5a00da7b796345836dca/ee/app/views/admin/gitlab_duo/configuration/index.html.haml

Looking at https://gitlab.com/gitlab-org/gitlab/-/blob/fbbd7ddb1ba1121ffbe6ebd9a20e9a492c5c19de/ee/app/helpers/admin/application_settings_helper.rb#L76 then, is there a way to hide Local AI Gateway URL unless SHM is available?

If we find a solution to this, we could do what I suggest above and properly segment these settings based on permissions.

1. Guard the entire endpoint / page with a duo_addon_purchased? check
2. Guard the SHM partial with an additional self_hosted_models_purchased? check

I am unsure what is meant by "additional self_hosted_models_purchased."

Instead, we should ensure that everything is guarded by a check for a valid duo_addon_purchased For self-hosted models, we specifically check for Duo Enterprise.

The challenge arises from the fact that this page includes a setting for GitLab Duo, which encompasses code suggestions, even without duo enterprise.

Since the page is accessible in two scenarios (code suggestions only and duo enterprise), it is crucial to verify both cases at the same level. While these settings are specific to self-hosted models, the current logic does not focus on self-hosted models; instead, it centers on code suggestions.

The proposed change is to eliminate the check for code suggestions when dealing with self-hosted environments, ensuring that we instead verify for Duo Enterprise, which is the supported add-on for self-hosted models.

@mkaeppler I created an issue here #523401 to discuss the necessary changes and determine ownership.

@mhamda Sorry, I may not have explained this very well and perhaps some of my assumptions are wrong.

Let me list my assumptions and you let me know where I misunderstand:

• This page does not guard self-hosted models specifically but Duo in general. There are 2 settings I can see that relate to SHM: Self-hosted AI models and Local AI Gateway URL.
• This page is about GitLab Duo settings in general, no matter if Enterprise or Pro. I should be able to navigate to it as soon as:
  • I am admin
  • I have Duo Pro or Enterprise.
• Once I open it, I should:
  • See SHM features when I'm on Duo Enterprise
  • Not see SHM features when I'm on Duo Pro

Is this correct? If so, I think we should change the logic (outside of using policies or not) to:

• In the controller, guard this endpoint with: admin & has_duo_pro_or_enterprise
• In the view rendered, guard the 2 SHM settings with admin & has_duo_enterprise

I don't think we need any check for Ai::Setting here. If anything, shouldn't it be the other way around i.e. have the Ai::Setting page be guarded by the Self-hosted AI models toggle? It seems wrong to allow the former to be accessed if the latter is set and vice versa.

Thanks, @mkaeppler, for explaining your thoughts; this is correct.

Is this correct? If so, I think we should change the logic (outside of using policies or not) to:

This statement is accurate; we are conflating two distinct aspects: Duo Pro and Enterprise.

In the controller, guard this endpoint with: admin & has_duo_pro_or_enterprise

This is what I am doing in this MR, we are checking if customers can access the page in general or not.

In the view rendered, guard the 2 SHM settings with admin & has_duo_enterprise

This is definitely a missing check in the initial iterations; we should display SHM toggles only for SHM users. We don't have a new design for that, but I belive it's better to move it out from this page to the self-hosted models page as a new tab.

Ai::Setting page be guarded by the Self-hosted AI models

The Ai::Setting page should also be protected by the Self-hosted AI models toggle. We need to ensure that users cannot access AI settings unless self-hosted models are in use. For instance, if we were to allow vendors like GitLab to toggle settings, we would be deviating from our current thoughts.

We should revise the index view to restrict access to the 2 SHM settings, making them available only to users with admin rights and those who have a duo enterprise purchase.

However, I believe this pertains to a separate issue as well. What do you think?

cc/ @julie_huang

This is what I am doing in this MR, we are checking if customers can access the page in general or not.

Is it though?

      if ::Ai::Setting.self_hosted?
        GitlabSubscriptions::AddOnPurchase.for_self_managed.for_duo_enterprise.active.any?

IIUC, this means that unless I have already added custom model config under the Self-hosted models tab, we will run down the else branch, which will also allow me to see SHM settings if I'm on Duo Pro:

else
  GitlabSubscriptions::AddOnPurchase.for_self_managed.for_duo_pro_or_duo_enterprise.active.any?

Is that the intended behavior?

@mkaeppler Let me explain

Current Issue:

As a customer who has purchased Duo Enterprise, I am unable to access the configuration page for Duo, while Duo Pro users can access it.

Proposed Fix:

• Modify the check from Duo Pro (Code Suggestions) to for_duo_enterprise if custom models are configured.
• If no custom model is configured, the page remains accessible since the else condition also checks for duo_enterprise.

Expected Behavior:

• For Duo Pro users: They do not have self-hosted models, so they will never trigger the first if condition. If they somehow have self-hosted models configured, their access will fail since they do not have a duo_enterprise purchase. Their access will depend solely on having the duo_pro add-on, which is handled in the else condition.
• For Duo Enterprise users: They will now have access to all relevant settings as expected.

Duo Enterprise customers see everything they should see.
Duo Pro customers currently see extra items—Self-hosted AI models and Local AI Gateway URL—which they should not have access to. These should either be properly guarded or extracted into a separate tab under the self-hosted model view.

This should be addressed separately, while this MR should focus on unblocking Duo Enterprise users from accessing the configuration page.

For Duo Pro users: They do not have self-hosted models, so they will never trigger the first if condition.

Right, which means they can access this page, including the 2 SHM settings it renders, which I am not sure what that would imply even (e.g. toggling that checkbox to enable SHM?) If that is OK, then this logic is fine I just think it's odd, but I'll trust your judgement here.

Aha, I see. There seems to be some confusion regarding the SHM checkbox.

Enabling the checkbox does not mean that SHM is being activated; instead, it indicates that users accepted that they can access certain models that are still in beta.

It’s important to note that the SHM functionality is available only for Duo Enterprise users.

I completely agree that it's unusual to see these two items presented together, and I believe they should not be displayed in this context. I would kindly ask @julie_huang to confirm if this is correct. If so, we should create an issue for this and consider moving them under the self-hosted models category.

Got it, thanks for clarifying!

I completely agree that it's unusual to see these two items presented together, and I believe they should not be displayed in this context. I would kindly ask @julie_huang to confirm if this is correct. If so, we should create an issue for this and consider moving them under the self-hosted models category.

That page is the right one for these configurations, note that we are centralising all AI settings into a single flow #478109 (closed), so it's not about having different settings pages, but the correct information displayed in that page.

Enabling the checkbox does not mean that SHM is being activated; instead, it indicates that users accepted that they can access certain models that are still in beta.

Yeah, that is a bit confusing still, even with the already improved wording.

---

Overall, what we should do here is fix this issue: a customer that has access to that feature should see the information. To change the UI is a vue change that will need to be addressed in a different MR either way.

@mkaeppler The beta models checkbox has been confusing for others as well. People think it toggles the entire Duo self-hosted feature. So there's definitely room for improvement with how we convey what this checkbox does.

Duo Pro customers currently see extra items—Self-hosted AI models and Local AI Gateway URL—which they should not have access to. These should either be properly guarded or extracted into a separate tab under the self-hosted model view.

@mhamda @mkaeppler @eduardobonet This part is already addressed in the frontend here — the beta models checkbox, AI logs and AIGW url configs are all guarded by a canManageSelfHostedModels prop from the backend. So users without an ultimate license or enterprise add-on should not be seeing these fields. If they are, let me know as it means something isn't working correctly.

Even though the Duo self-hosted configs are guarded, I think it would still be a great help to communicate that they are configs specific do Duo self-hosted features only. It would give the page more heirarchy/organisation. Is this what you mean @mhamda about "consider moving them under the self-hosted models category"?

In the Admin Area -> General settings page configs live in sections so it's easy to understand what areas each config is intended to affect. Sections might be overkill for the few configs that we have at the moment, but maybe wrapping the Duo self-hosted configs under its own header is enough to have the same effect?

CC: @timnoah @susie.bee

This part is already addressed in the frontend here — the beta models checkbox, AI logs and AIGW url configs are all guarded by a canManageSelfHostedModels prop from the backend.

Thank you, @julie_huang, for sharing this information. This means we don't need to address any issues in the front end.

Is this what you mean @mhamda about "consider moving them under the self-hosted models category"?

Yes, I would love to collect all self-hosted items from the self-hosted models section, but I will leave this to Tim. But this is another discussion as well.

This thread can now be resolved, as it's already answered the question about unintended access.

@julie_huang In the example and proposal you gave above "maybe wrapping the Duo self-hosted configs under its own header is enough to have the same effect?" Are you proposing that Self-Hosted settings would live under Admin area / General / Gitlab Duo features / GitLab Duo Self-Hosted? I am just not seeing that in the image you provided.

@mhamda @eduardobonet I'm also still a little confused by the boolean logic used here. If I rewrite this to be easier to reason through, it becomes:

     if self_hosted
       has_enterprise
     else
       has_pro_or_enterprise

If I flatten this out into its logical components, I get:

  (self_hosted AND has_enterprise) OR (has_pro OR has_enterprise)
= (self_hosted AND has_enterprise) OR has_enterprise OR has_pro

By the law of absorption, this is the same as:

has_enterprise OR has_pro

because the value of self_hosted is irrelevant for the result of this expression.

So this entire function could be simplified to just:

GitlabSubscriptions::AddOnPurchase.for_self_managed.for_duo_pro_or_duo_enterprise.active.any?

Or am I missing something else here?

"maybe wrapping the Duo self-hosted configs under its own header is enough to have the same effect?" Are you proposing that Self-Hosted settings would live under Admin area / General / Gitlab Duo features / GitLab Duo Self-Hosted?

@susie.bee sorry for the confusion — I included the above screenshot of the Admin Area / General settings page to highlight how it handles demarcation of configs. I was wondering if we could take inspiration from this and apply it to the Duo Settings page. For example if we applied the same concept of collapsible panels it could look like the below. It might require further UX input, but in my opinion this change already creates greater clarity and organisation around the relevance of those settings. WDYT? CC: @timnoah

Expanded Self-hosted configs	Collapsed Self-hosted configs

So this entire function could be simplified to just:

GitlabSubscriptions::AddOnPurchase.for_self_managed.for_duo_pro_or_duo_enterprise.active.any?

@mkaeppler, you're absolutely right about your logical simplification. I will create a new MR and ping you there.

left review comments

requested review from @mkaeppler and @eduardobonet

requested review from @mkaeppler

left review comments