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: OpenID Connect for Self Hosted Instance with God-Mode Implementation #3341

Closed

Conversation

torbenraab
Copy link

@torbenraab torbenraab commented Jan 10, 2024

This PR closes #1319 because it was more work to pull the current develop branch into the old branch than to just rebase the code from it into this one.
I tried to clean up the commit as good as possible.
This PR enables Authentication via OpenID Connect for Self-Hosted Instances. It can be configured via the Environment Variables (here it is also possible to do a Autodiscovery for the Endpoints if you set the issuer) or via the new God-Mode.
It also enables Auto-SignIn for OIDC so that the users don't have to click anything and are redirected directly if they aren't signed in yet. This can also be switched on or off via the God-Mode Interface.
Futhermore it also implements to be logged out to the End-Session Endpoint of the OpenID Provider.

It matches the user based on the email address. If a new user is created the username is set based on the preferred_username from the Identity Provider.

It has proven to work with Authentik and Keycloak.

Update 28.01.2024
As this will not be merged soon I provide a repository where the feature is implemented. Please look at torbenraab/plane. I try to keep the changes as minimal as possible and update the preview and master branch accordingly including the docker image builds and guides for self hosting.

sriramveeraghanta and others added 28 commits December 30, 2023 21:33
fix: project settings form not loading new data while switching between projects
…ry (makeplane#3309)

error of table not being defined while getting getBoundingClientRect()
and solve other TS issues

- Added ResolvedPos import from @tiptap/pm/model
- Updated setCellsBackgroundColor function parameter type to string
- Declared ToolboxItem type for toolbox items
- Modified columnsToolboxItems and rowsToolboxItems to use the ToolboxItem type
- Updated createToolbox function parameters to specify Element or null for triggerButton and ToolboxItem[] for items
- Added ts-expect-error comment above the toolbox variable declaration
- Updated update method parameter type to readonly Decoration[]
- Changed destructuring assignment of hoveredTable and hoveredCell in updateControls method to use Object.values and reduce method
- Added null check for this.table in updateControls method
- Wrapped the code that updates columnsControl and rowsControl with null checks for each control
- Replaced ts-ignore comments with proper dispatch calls in selectColumn and selectRow methods

Co-authored-by: sriram veeraghanta <veeraghanta.sriram@gmail.com>
* chore: add proper message for cycle/ module having start & end date but isn't active yet.

* fix: infinite loader after updating workspace settings.

* fix: user profile icon dropdown doesn't closes automatically.

* style: fix inconsistent padding in cycle empty state.

* chore: remove multiple `empty state` in labels settings and improve add label logic.

* style: fix inconsistent padding in project label, integration and estimates empty state.

* style: fix integrations settings breadcrumb title.

* style: add proper `disabled` styles for email field in profile settings.

* style: fix cycle layout height.
* refactor: updated preloaded function for the list view quick add

* fix: resolved bug in the assignee dropdown

* chore: issue sidebar link improvement

* fix: resolved subscription store bug

* chore: updated preloaded function for the kanban layout quick add

* chore: resolved issues in the list filters and component

* chore: filter store updated

* fix: issue serializer changed

* chore: quick add preload function updated

* fix: build error

* fix: serializer changed

* fix: minor request change

* chore: resolved build issues and updated the prepopulated data in the quick add issue.

* fix: build fix and code refactor

* fix: spreadsheet layout quick add fix

* fix: issue peek overview link section updated

* fix: cycle status bug fix

* fix: serializer changes

* fix: assignee and labels listing

* chore: issue modal parent_id default value updated

* fix: cycle and module issue serializer change

* fix: cycle list serializer changed

* chore: prepopulated validation in both list and kanban for quick add and group header add issues

* chore: group header validation added

* fix: issue response payload change

* dev: make cycle and module issue create response simillar

* chore: custom control link component added

* dev: make issue create and update response simillar to list and retrieve

* fix: build error

* chore: control link component improvement

* chore: globalise issue peek overview

* chore: control link component improvement

* chore: made changes and optimised the issue peek overview root

* build-error: resolved build erros for issueId dependancy from issue detail store

* chore: peek overview link fix

* dev: update state nullable rule

---------

Co-authored-by: gurusainath <gurusainath007@gmail.com>
Co-authored-by: NarayanBavisetti <narayan3119@gmail.com>
Co-authored-by: pablohashescobar <nikhilschacko@gmail.com>
…e#3326)

* fix: estimate order not maintained in create/ update modal.

* fix: estimate points mutation on update.
…plane#3330)

* fix cycle creation and active cycle map

* minor fix in cycle store

* create cycle breaking fix

* replace last possible bits of router.push with Link

---------

Co-authored-by: Rahul R <rahulr@Rahuls-MacBook-Pro.local>
…eplane#3327)

Bumps [tj-actions/changed-files](https://github.com/tj-actions/changed-files) from 38 to 41.
- [Release notes](https://github.com/tj-actions/changed-files/releases)
- [Changelog](https://github.com/tj-actions/changed-files/blob/main/HISTORY.md)
- [Commits](tj-actions/changed-files@v38...v41)

---
updated-dependencies:
- dependency-name: tj-actions/changed-files
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* chore: mobile configs

* chore: mobile configurations changed

* chore: removed the slack id

* chore: reversed google client id
* fix: jira importer validations

* dev: update validation for cloud hostname

* dev: update the function to be used externally

* dev: update codeql workflow

* dev: update repository selection api
* dev: update jira summary endpoints

* dev: update jira project key validations

* dev: updated key length
* dev: dropdown key down custom hook added

* chore: plane ui dropdowns updated

* chore: cycle and module tab index added in modals

* chore: view and page tab index added in modals

* chore: issue modal tab indexing added

* chore: project modal tab indexing added

* fix: build fix

* build-error: build error in pages new structure and reverted back to old page structure

---------

Co-authored-by: gurusainath <gurusainath007@gmail.com>
@torbenraab
Copy link
Author

This PR resolves #413

res = requests.post(url, headers=headers, data=data)

data = res.json()
print(data)

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information

This expression logs [sensitive data (secret)](1) as clear text.
@pablohashescobar
Copy link
Collaborator

Hey guys, as this got quite some traction over the course of just 2 days, I just want to leave some words. I won't comment on the commercial part as @Floppy012 already did better than I possibly could.

First things first, our most sincere apologies to you for the several missteps we took. We should have been more deliberate and candid in our responses later if not from the beginning. Not in our defense, but we simply didn't know enough. We still don't. As mentioned above, we will do our best to find a way to make your contribution work.

If security is you real concern than it would be an option to validate the signature of the token with the public key and sign the request with private keys from the backend. The standard allows for such things.

It is our biggest concern. As a heads-up, we are in the middle of a SOC 2 Type 2 exercise and we want to be very careful with the roadmap of a feature, not just checking boxes for now. You are right. The standard does allow for token signatures, but we are also worried about things like token hijacking, CSRF, and federation. All of them, I am sure you will agree, are non-trivial problems. We just frankly don't know enough and why are investing in experts, despite being a very-early-stage start-up, who can tell us better.

Not to say, as a company I was willing to sponsor the project for the feature richness that it gave for essentially no money. But if there is a subscription involved for just SSO and other features that we don't need, that would be the part of the story where you look for something different or implement the things for yourself.

I hear you, but I also assure you our stance for building and shipping hasn't changed nor will it change. We have built for things that you and others have asked for, so it is unlikely that we will build "features that we don't need" just because we are considering making SSO a paid feature.

Again, just bear with us while we work earnestly to land this debate and know that we will build features that a significant majority find useful.

@Floppy012
Copy link

How do you mean? We are an AGPL 3.0 and Commercial Open Source company. We will have to have plane-ee if we want to monetize features at all. Please throw some light on this.

I'm by far no expert on that matter. With AGPL you must provide source code even for your SaaS project (network access). If you run SaaS and provide a free tier then free tier users must be able to download the code of that software. If your SaaS runs the code from the plane-ee repo then any free user must have access to that repository because of the AGPL. Providing a different repository URL based on the tier is probably not enough unless SaaS runs the code from this repo for free tier users and a whole separate instance (running plane-ee) for paying users.

And even then there is still the possibility that someone just pays for the lowest plan, downloads the source code from the plane-ee repo, and re-publishes it on GitHub (which is ethically questionable but from the license point of view doable).

My point is, that from my understanding a plane-ee repo would be more effort than it benefits you unless you plan on keeping the repo private which would be a violation of the AGPL.

As said, I'm no expert and not a lawyer but if you haven't done already I suggest having a lawyer look at this situation.

@bitbay
Copy link

bitbay commented Jan 15, 2024

First of all, seems like a current/hot topic, so i just wanted to share some thoughts on it.
Security should be of course the main priority for every software project - and the latest industry trends point to "federated identity", most relevant of them being "OpenID Connect" (OIDC) with "Open Authorization" (OAuth 2.1).
Connecting third-party systems to an identity server implementing these protocols to get a user's identity is already standardized. Doing so in the "back" (server to server) is already considered safe enough with "known clients".

  1. configure a new client app in Your identity server (ie. register a callback URI)
  2. redirect the user to the IDServer if You need a "session" (need to know who the user is)
  3. !let the IDS handle authentication!
  4. get user data on the callback endpoint

Obviously, it is more complex behind the scenes, but what i wanted to emphasize is step 3. If the app itself (client) delegates the authZ part to an external service, efectively decoupling it, You can safely change the implementation behind it.
People mentioned keycloak already, i just would like to add ory.sh, authelia, authentik, zitadel to the self-hosted list - not to mention google, github and other third-party enterprise identity providers.

I hope it is clear that it's better to adopt existing standards, and not reinvent no wheels - and maybe the fact that giving Your users options will make them happy, which in turn will make them grow.

My +1 on a "composable" identity system, either using plane's own user directory, or connecting to an external one via OIDC/OAuth.

@pablohashescobar
Copy link
Collaborator

How do you mean? We are an AGPL 3.0 and Commercial Open Source company. We will have to have plane-ee if we want to monetize features at all. Please throw some light on this.

I'm by far no expert on that matter. With AGPL you must provide source code even for your SaaS project (network access). If you run SaaS and provide a free tier then free tier users must be able to download the code of that software. If your SaaS runs the code from the plane-ee repo then any free user must have access to that repository because of the AGPL. Providing a different repository URL based on the tier is probably not enough unless SaaS runs the code from this repo for free tier users and a whole separate instance (running plane-ee) for paying users.

We double checked our understanding. Here is how the licensing works, which is also typical for any commercial open source company that publishes its code under AGPL.

When we publish code under AGPL, the licensee, meaning you, has to share the source code if they modify the code and provide a SaaS as detailed in AGPL Section 13. We, as the licensor and the author of the code and software, have no obligation to publish new code under AGPL. We have the option to publish all or part of any new code under AGPL, but we can choose other licenses as well. We can't actually violate our own license, because the conditions only run downstream. I hope that helps explain how this all works.

My point is, that from my understanding a plane-ee repo would be more effort than it benefits you unless you plan on keeping the repo private which would be a violation of the AGPL.

Licensing questions aside, our job, as stewards of the project, is to be clear about which code is under AGPL and which is not, to make both the Community and Enterprise editions stand on their own, and to be sure our community members have a good experience working with us. So we're going to focus on that.

@thefiredragon
Copy link

@torbenraab I had tested your oidc implementation, and it was not easy to find out that you automatically add to oidc_issuer URL the /.well-known/openid-configuration ending automatically.

You should remove this or make a note for this

@torbenraab
Copy link
Author

@thefiredragon Thank you for testing! I will improve the function to detect if the user already added the slug. Is there anything else that could be done better in your opinion?

@thefiredragon
Copy link

thefiredragon commented Jan 19, 2024

@torbenraab I had noticed to leave oidc_issuer empty the exception won't work correctly.
And starting plane will throw into an error that oidc_issuer is not provided.
Also with correct values for manual setup.
But I'm using podman-compose , so I can't say it's an issue.

@almereyda
Copy link

Regarding the oidc_issuer, this variable would preferably be called oidc_discovery. I've seen this in other implementations, that issuer is used for discovery, and it always confuses me.

But I can sense what the intent is behind the naming, but also behind auto-appending a discovery URL. Since OIDC discovery is a protocol extension, this variable should either (1) only act for oidc_issuer and provide an eventual oidc_discovery sibling, as they can be considered mutually exclusive, or (2) only try the discovery endpoint, if no sufficiently complete OIDC configuration is known.

@thefiredragon
Copy link

thefiredragon commented Jan 23, 2024

Another question here, will this PR merged soon or OIDC implemented?
When not it's okay and we don't need to wait for it.
We're looking for plane since october last year.
It's really a missing and important feature for our usage.
Best regards
Dan

@torbenraab
Copy link
Author

Hey @thefiredragon
As of the above comments the feature will not be implemented soon and only as a paid feature. I'm currently working on maintaining a fork that includes a OIDC Implementation as we need it ourselves and wanted to keep the latest version of plane. A big part is that we need SSO for our running services. We are willing to pay if it is required but as long as it is not available for plane, we will definitly maintain a fork.

General
</div>
<Command.Item onSelect={closePalette} className="focus:outline-none">
<Link href={`/${workspaceSlug}/settings`}>

Check warning

Code scanning / CodeQL

Client-side URL redirect

Untrusted URL redirection depends on a [user-provided value](1).
Members
</div>
<Command.Item onSelect={closePalette} className="focus:outline-none">
<Link href={`/${workspaceSlug}/settings/members`}>

Check warning

Code scanning / CodeQL

Client-side URL redirect

Untrusted URL redirection depends on a [user-provided value](1).
Billing and Plans
</div>
<Command.Item onSelect={closePalette} className="focus:outline-none">
<Link href={`/${workspaceSlug}/settings/billing`}>

Check warning

Code scanning / CodeQL

Client-side URL redirect

Untrusted URL redirection depends on a [user-provided value](1).
Integrations
</div>
<Command.Item onSelect={closePalette} className="focus:outline-none">
<Link href={`/${workspaceSlug}/settings/integrations`}>

Check warning

Code scanning / CodeQL

Client-side URL redirect

Untrusted URL redirection depends on a [user-provided value](1).
Import
</div>
<Command.Item onSelect={closePalette} className="focus:outline-none">
<Link href={`/${workspaceSlug}/settings/imports`}>

Check warning

Code scanning / CodeQL

Client-side URL redirect

Untrusted URL redirection depends on a [user-provided value](1).
Comment on lines +54 to +56
href={`/${workspaceSlug}/projects/${notification.project}/${
notification.data.issue_activity.field === "archived_at" ? "archived-issues" : "issues"
}/${notification.data.issue.id}`}

Check warning

Code scanning / CodeQL

Client-side URL redirect

Untrusted URL redirection depends on a [user-provided value](1).
}}
href={`/${workspaceSlug}/projects/${project.id}/settings`}

Check warning

Code scanning / CodeQL

Client-side URL redirect

Untrusted URL redirection depends on a [user-provided value](1).
Go to issues
</Button>
//TODO: Create a new component called Button Link to handle such scenarios
<Link href={`/${workspaceSlug}/projects/${projectId}/issues`}>

Check warning

Code scanning / CodeQL

Client-side URL redirect

Untrusted URL redirection depends on a [user-provided value](1).
image={emptyIntegration}
primaryButton={{
text: "Configure now",
onClick: () => router.push(`/${workspaceSlug}/settings/integrations`),

Check warning

Code scanning / CodeQL

Client-side URL redirect

Untrusted URL redirection depends on a [user-provided value](1).
};

return (
<a href={href} target={target} onClick={_onClick} {...rest}>

Check warning

Code scanning / CodeQL

Client-side URL redirect

Untrusted URL redirection depends on a [user-provided value](1).
@theparthacus
Copy link
Collaborator

Hey @thefiredragon As of the above comments the feature will not be implemented soon and only as a paid feature. I'm currently working on maintaining a fork that includes a OIDC Implementation as we need it ourselves and wanted to keep the latest version of plane. A big part is that we need SSO for our running services. We are willing to pay if it is required but as long as it is not available for plane, we will definitly maintain a fork.

stepping in for @pablohashescobar, thank you for the commitment to this project and the fork. when we make custom SSO and OIDC available on a paid plan, we will reach out to you first.

flirting with the idea of a one-time fee for SSO and other features valid for one major version. will keep everyone posted here.

@torbenraab torbenraab marked this pull request as draft January 28, 2024 22:04
@torbenraab
Copy link
Author

Hey @theparthacus & @pablohashescobar, thanks for your kind words. Looking forward to your OpenID Implementation. I updated the Description of the PR accordingly and will maintain the fork at least until you provide the implementation.

Thanks for all the comments on this PR. I will not maintain this directly as the fork is maintained.
Thanks for your interest and I hope I can help. If there are any issues just reach out to me.

@CLAassistant
Copy link

CLAassistant commented Feb 8, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
9 out of 11 committers have signed the CLA.

✅ aaryan610
✅ sriramveeraghanta
✅ Palanikannan1437
✅ anmolsinghbhatia
✅ rahulramesha
✅ prateekshourya29
✅ NarayanBavisetti
✅ pablohashescobar
✅ mguptahub
❌ JiginJayaprakash
❌ torbenraab
You have signed the CLA already but the status is still pending? Let us recheck it.

@theparthacus theparthacus self-assigned this Mar 18, 2024
@gkaskonas
Copy link

So how far away are we from having any kind of SSO?

@torbenraab
Copy link
Author

So how far away are we from having any kind of SSO?

If you are waiting for the official implementation I cannot say anything. If you are happy with a community implementation please take a look at the update posted in PR description.

MMore added a commit to ssotax/ssotax that referenced this pull request Apr 19, 2024
@makeplane makeplane locked and limited conversation to collaborators Apr 25, 2024
@makeplane makeplane unlocked this conversation Apr 25, 2024
@makeplane makeplane locked as too heated and limited conversation to collaborators Apr 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet