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

Enable configuration of SAML authentication sources (#5512) #25132

Closed
wants to merge 1 commit into from
Closed

Enable configuration of SAML authentication sources (#5512) #25132

wants to merge 1 commit into from

Conversation

jrjake
Copy link

@jrjake jrjake commented Jun 8, 2023

This commit only allows administrators to configure a SAML authentication source. It does not include the logic for actually authenticating SAML users, which will come in the next patch.


I figured this part of the feature was big enough already so wanted to be kind to the reviewers :). This code, especially the validation, was based on the demo of the library I intend to use (recommended by Gitea maintainers in the original issue): https://github.com/russellhaering/gosaml2/blob/main/s2example/demo.go.

The SAML flow I intend to implement in a future patch is:

  1. User opens Service Provider (Gitea) in web browser
  2. User attempts to login to Gitea. Gitea detects the account is configured for SAML. Gitea redirects the user to the configured Identity Provider (IdP) Login URL.
  3. The IdP authenticates the user, signs a message verifying the user's identity, and redirects the user to Gitea with the signed message.
  4. Gitea will parse the signed message and verify that the message is from the IdP and valid using the configured IdP Issuer ID and IdP Certificate. If they are valid, Gitea logs the user in.
  5. If the user chooses to log out, Gitea will destroy the local session and redirect the user to the IdP Logout URL (if configured).

This commit only allows administrators to configure a SAML
authentication source. It does not include the logic for actually
authenticating SAML users, which will come in the next patch.
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 8, 2023
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 8, 2023
@jrjake jrjake mentioned this pull request Jun 8, 2023
@silverwind silverwind added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Jun 8, 2023
@silverwind
Copy link
Member

Looks good overall. I think many the login flow is quite similar to OAuth, so I guess much of that can be re-used.

@jrjake
Copy link
Author

jrjake commented Jun 8, 2023

Yes, thankfully it's a very simple protocol. I will admit I am unfamilar with Gitea's "norms" for merging, can I expect this to be merged with the promise of a future patch or should I keep adding to this pull request until it is fully complete?

@delvh
Copy link
Member

delvh commented Jun 8, 2023

can I expect this to be merged with the promise of a future patch or should I keep adding to this pull request until it is fully complete?

Depends on what you mean by should I keep adding to this PR:
If you mean merging the current main branch into this branch, then no, we will do that for you.
If you mean answering (and implementing) requested changes, then yes, you are primarily responsible for these types of things.

@jrjake
Copy link
Author

jrjake commented Jun 8, 2023

Oh, I apologize I did a horrible job at explaining what I meant there. I meant that this patch is just for configuring the Authentication Source in the Admin UI, and does not add the authentication code. Should I add the authentication code to this pull request and make it bigger, or keep this pull request smaller and make a second pull request for the authentication code?

@delvh
Copy link
Member

delvh commented Jun 8, 2023

Generally, smaller PRs are better as they are easier to review.
So, from my side, a follow-up PR is better.
I just skimmed over the code, would still say so.

@lunny
Copy link
Member

lunny commented Jun 9, 2023

Generally, smaller PRs are better as they are easier to review. So, from my side, a follow-up PR is better. I just skimmed over the code, would still say so.

Sorry, I don't think so. The feature should be basically finished. The SAML auth and the login flow is tight. It's better to put them in the same PR. And I think the feature is not a very big PR which should be less than 25 file changes.

@techknowlogick
Copy link
Member

Hey. I'm so sorry about the work you've done so far. Your PR reminded me that I've completed most of this a while ago already during a live stream coding session, and I just hadn't pushed the branch. Please see: #25165 for a nearly complete implementation of SAML auth.

@jrjake jrjake closed this Jun 9, 2023
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Sep 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants