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

feat: Use isEnterprise from endpoint version response to determine permissions #2422

Merged
merged 2 commits into from
Aug 4, 2023

Conversation

novakzaballa
Copy link
Contributor

@novakzaballa novakzaballa commented Jul 12, 2023

Thanks for submitting a PR! Please check the boxes below:

  • I have run pre-commit to check linting
  • I have filled in the "Changes" section below?
  • I have filled in the "How did you test this code" section below?

Changes

Use isEnterprise attribute from API /version endpoint response to determine permissions.

How did you test this code?

Tested with both self-hosted version

@vercel
Copy link

vercel bot commented Jul 12, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 3, 2023 2:25pm
flagsmith-frontend-preview ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 3, 2023 2:25pm
flagsmith-frontend-staging ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 3, 2023 2:25pm

@github-actions
Copy link
Contributor

github-actions bot commented Jul 12, 2023

Uffizzi Preview deployment-30748 was deleted.

@dabeeeenster dabeeeenster changed the title Use isEnterprise from enpoint version response to determine permissions feat: Use isEnterprise from endpoint version response to determine permissions Jul 13, 2023
if (!Utils.getFlagsmithHasFeature('plan_based_access')) {
if (
!Utils.getFlagsmithHasFeature('plan_based_access') &&
global.flagsmithVersion.is_enterprise
Copy link
Member

Choose a reason for hiding this comment

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

If I'm not mistaken this logic is incorrect, I think we allow features such as audit in open source

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @kyle-ssg. @matthewelwell, could you please confirm our business policy regarding the self-hosted free plan? If we have some features enabled for that case, I think we could return true for those in the same block of code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Audit shouldn't be allowed in open source (although it has been to this point). I think therefore this change is correct @kyle-ssg ?

Copy link
Member

Choose a reason for hiding this comment

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

Ok, yeah after talking with Ben:

  • Self hosted = same as SaaS without a plan
  • EE = same as enterprise plan

I think this means we can remove the plan_based_access flag entirely in this PR.

Copy link
Contributor Author

@novakzaballa novakzaballa Jul 28, 2023

Choose a reason for hiding this comment

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

@kyle-ssg @matthewelwell I have a couple of questions in this regard:

  • In the case of self-hosted (free) AFAIK we should allow unlimited users (invites), while in SaaS free we only allow 1 user. Currently, in the self-hosted free version, we allow unlimited invites for new users, which can be admins or regular users since RBAC is enabled. After these changes, we will only allow admins, since RBAC will be disabled. Is that OK?
  • If that’s the case should we change this description https://github.com/Flagsmith/self-hosted/blob/ed4a044ddf165e7ec5017cb0dc53e661c3e04b6e/README.md?plain=1#L27 ?
  • Other behaviors, like limiting the number Projects to only 1, and in general all the non-free plan features that were enabled before for self-hosted free will no longer be available, I guess we are aware of all these changes right?

@matthewelwell
Copy link
Contributor

@novakzaballa this is now released in v2.61.0 of the flagsmith images (flagsmith/flagsmith and flagsmith/flagsmith-private-cloud). You should be able to test it using those.

Copy link
Contributor

@matthewelwell matthewelwell left a comment

Choose a reason for hiding this comment

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

This looks good to me.

I'm keen to get @kyle-ssg's opinion here too.

Also, one thing this has highlighted is that this means that all organisations in a given enterprise installation will have all functionality. Previously these controls were implemented on a per organisation basis via subscriptions that users created in their own installations. Keen to get @dabeeeenster's thoughts here?

@dabeeeenster
Copy link
Contributor

This looks good to me.

I'm keen to get @kyle-ssg's opinion here too.

Also, one thing this has highlighted is that this means that all organisations in a given enterprise installation will have all functionality. Previously these controls were implemented on a per organisation basis via subscriptions that users created in their own installations. Keen to get @dabeeeenster's thoughts here?

That's fine

@kyle-ssg
Copy link
Member

kyle-ssg commented Aug 1, 2023

Added 1 comment to PR. In regards to controlling this per org, if that's important to the business I think ultimately we should just return plan data for those orgs, then the FE never has to distinguish between installations and it all comes down to the plan.

@matthewelwell
Copy link
Contributor

I think ultimately we should just return plan data for those orgs

This is what we were doing previously though isn't it?

@kyle-ssg
Copy link
Member

kyle-ssg commented Aug 1, 2023

Not sure, but if we were then that sounds better than detecting their installation type.

@novakzaballa
Copy link
Contributor Author

Not sure, but if we were then that sounds better than detecting their installation type.
@kyle-ssg @matthewelwell If you take a look at the code changes this PR ended up only replacing !Utils.getFlagsmithHasFeature('plan_based_access') with global.flagsmithVersion?.backend.is_enterprise
image

@novakzaballa
Copy link
Contributor Author

Now, in addition to the above comment, what I understand from @kyle-ssg comment is that we should remove these lines that return true if it is a self-hosted enterprise from the getPlanPermission and the getPlansPermission functions, and instead return 'enterprise' in the getPlanName function when global.flagsmithVersion?.backend.is_enterprise is true, is it correct?

@matthewelwell matthewelwell merged commit edf38ac into main Aug 4, 2023
11 checks passed
@matthewelwell matthewelwell deleted the chore/handle-new-version-endpoint-response branch August 4, 2023 13:47
This was referenced Aug 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
front-end Issue related to the React Front End Dashboard
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants