Add option to restrict from which e-mail domain can users sign-up trough OIDC. #2332

Open
lsde wants to merge 2 commits from lsde/vikunja:feature/oidc-email-domain-restriction into main
First-time contributor

This merge request adds an option to restrict the sign-up process for users using OpenID Connect (OIDC) to specific email domains. This feature will allow administrators to limit the type of accounts that can be created on their instance, ensuring a more secure environment for their users.

This merge request adds an option to restrict the sign-up process for users using OpenID Connect (OIDC) to specific email domains. This feature will allow administrators to limit the type of accounts that can be created on their instance, ensuring a more secure environment for their users.
Member

Hi lsde!

Thank you for creating a PR!

I've deployed the frontend changes of this PR on a preview environment under this URL: https://2332-feature-oidc-email-domain-restri--vikunja-frontend-preview.netlify.app

You can use this url to view the changes live and test them out.
You will need to manually connect this to an api running somewhere. The easiest to use is https://try.vikunja.io/.

This preview does not contain any changes made to the api, only the frontend.

Have a nice day!

Beep boop, I'm a bot.

Hi lsde! Thank you for creating a PR! I've deployed the frontend changes of this PR on a preview environment under this URL: https://2332-feature-oidc-email-domain-restri--vikunja-frontend-preview.netlify.app You can use this url to view the changes live and test them out. You will need to manually connect this to an api running somewhere. The easiest to use is https://try.vikunja.io/. This preview does not contain any changes made to the api, only the frontend. Have a nice day! > Beep boop, I'm a bot.
konrad requested changes 2024-05-07 13:56:05 +00:00
@ -53,6 +53,7 @@ type Provider struct {
OriginalAuthURL string `json:"-"`
AuthURL string `json:"auth_url"`
LogoutURL string `json:"logout_url"`
Domain string `json:"domain"`
Owner

Can you make this a string slice so that it's possible to add multiple domains?

Can you make this a string slice so that it's possible to add multiple domains?
Author
First-time contributor

done (the slicing is done at L44 though).

done (the slicing is done at [L44](https://kolaente.dev/lsde/vikunja/src/commit/9202e035ae1c7cbdc01e63260d0515e2d348a7f2/pkg/modules/auth/openid/providers.go#L144) though).
lsde marked this conversation as resolved
@ -192,2 +193,4 @@
}
//Check is the email address is at the domain configured for the provider, and return the error if not
if provider.Domain != "" && !strings.HasSuffix(cl.Email, provider.Domain) {
Owner

The comparison should include the @, otherwise it would be possible for a malicious attacker to use malicous-example.com when you only allowed example.com.

The comparison should include the `@`, otherwise it would be possible for a malicious attacker to use `malicous-example.com` when you only allowed `example.com`.
Author
First-time contributor

done here

done [here](https://kolaente.dev/lsde/vikunja/src/commit/9202e035ae1c7cbdc01e63260d0515e2d348a7f2/pkg/modules/auth/openid/openid.go#L195)
lsde marked this conversation as resolved
@ -427,4 +427,2 @@
}
// ErrNoOpenIDEmailProvided represents a "NoEmailProvided" kind of error.
type ErrOpenIDCustomScopeMalformed struct {
Owner

Why did you remove this error?

Why did you remove this error?
Author
First-time contributor

This error was just moved further down the code to be in the ascending order by error code. The error code for this was also fixed, as the code number was duplicate.

This error was just moved further down the code to be in the ascending order by error code. The error code for this was also fixed, as the code number was duplicate.
Owner

Ah makes sense, good catch!

Ah makes sense, good catch!
konrad marked this conversation as resolved
@ -534,1 +508,4 @@
}
// ErrOpenIDCustomScopeMalformed represents a "ErrOpenIDCustomScopeMalformed" kind of error.
type ErrOpenIDCustomScopeMalformed struct {
Owner

Please add this error to the docs.

Please add this error to the docs.
Author
First-time contributor

I'm not the autor of this, i guess you meant ErrCodeOpenIDEmailBadDomain.

I'm not the autor of this, i guess you meant ErrCodeOpenIDEmailBadDomain.
Owner

Yeah, the new error you added.

Yeah, the new error you added.
Author
First-time contributor

done here

done [here](https://kolaente.dev/lsde/vikunja/src/commit/9202e035ae1c7cbdc01e63260d0515e2d348a7f2/docs/content/doc/usage/errors.md?display=source#L48)
lsde marked this conversation as resolved
lsde force-pushed feature/oidc-email-domain-restriction from 6e747e19a8 to 9202e035ae 2024-05-07 16:12:15 +00:00 Compare
Author
First-time contributor

Updated the PR with the requested changes and rebased.

Updated the PR with the requested changes and rebased.
konrad requested changes 2024-05-07 16:45:12 +00:00
@ -194,0 +197,4 @@
splitEmail := strings.Split(cl.Email, "@")
foundGoodDomain := false
for domain, _ := range provider.Domains {
if strings.HasSuffix(splitEmail[len(splitEmail) -1], provider.Domains[domain]) {
Owner

Why the split instead of something like strings.HasSuffix(cl.Email, "@"+domain)?

Why the split instead of something like `strings.HasSuffix(cl.Email, "@"+domain)`?
Author
First-time contributor

no particular reason, fixed.

no particular reason, fixed.
lsde marked this conversation as resolved
@ -125,6 +125,11 @@ func getProviderFromMap(pi map[string]interface{}) (provider *Provider, err erro
logoutURL = ""
}
domains, ok := pi["domains"].(string)
Owner

This should be a list, not a string split by spaces.

This should be a list, not a string split by spaces.
Author
First-time contributor

fixed.

fixed.
lsde marked this conversation as resolved
@ -535,0 +534,4 @@
}
// ErrOpenIDEmailBadDomain represents a "OpenIDEmailBadDomain" kind of error.
type ErrOpenIDEmailBadDomain struct {
Owner

Can you include the wrong email domain in the error?

Can you include the wrong email domain in the error?
Author
First-time contributor

done

done
lsde marked this conversation as resolved
lsde force-pushed feature/oidc-email-domain-restriction from 298e12bc32 to 538653c363 2024-05-08 17:57:54 +00:00 Compare
lsde force-pushed feature/oidc-email-domain-restriction from 538653c363 to dc47c1dc9e 2024-05-08 20:31:40 +00:00 Compare
Author
First-time contributor

Updated the PR with the requested changes and rebased. (again)

Updated the PR with the requested changes and rebased. (again)
lsde force-pushed feature/oidc-email-domain-restriction from b0164e891f to 6a35cb9239 2024-05-09 06:15:41 +00:00 Compare
lsde force-pushed feature/oidc-email-domain-restriction from 6a35cb9239 to 07a4eabe78 2024-05-09 09:18:57 +00:00 Compare
lsde force-pushed feature/oidc-email-domain-restriction from 07a4eabe78 to a244a9506c 2024-05-09 09:53:32 +00:00 Compare
konrad reviewed 2024-05-15 21:36:20 +00:00
@ -325,0 +325,4 @@
# The list of e-mail domains that will be allowed to authenticate using OpenID Connect. If empty, any domain is allowed.
# Use this only if you want to restrict which users are allowed to authenticate using OpenID Connect.
domains:
- example.com
Owner

The .sample file will be copied as the default config file, that means by default, it will be restricted to @example.com emails. I think it's enough to leave it at that and expect people to read the config file, but worth keeping in mind.

The `.sample` file will be copied as the default config file, that means by default, it will be restricted to `@example.com` emails. I think it's enough to leave it at that and expect people to read the config file, but worth keeping in mind.
lsde marked this conversation as resolved
@ -194,0 +196,4 @@
if len(provider.Domains) > 0 {
foundGoodDomain := false
for domain := range provider.Domains {
if strings.HasSuffix(cl.Email, "@"+provider.Domains[domain]) {
Owner

Please change the loop to

		for _, domain := range provider.Domains {
			if strings.HasSuffix(cl.Email, "@"+domain) { 
Please change the loop to ``` for _, domain := range provider.Domains { if strings.HasSuffix(cl.Email, "@"+domain) {
lsde marked this conversation as resolved
@ -535,0 +535,4 @@
// ErrOpenIDEmailBadDomain represents a "OpenIDEmailBadDomain" kind of error.
type ErrOpenIDEmailBadDomain struct {
Email string
Owner

This should only include the domain, not the full email. We should not log emails as they are user-identifiable information.

This should only include the domain, not the full email. We should not log emails as they are user-identifiable information.
lsde marked this conversation as resolved
lsde force-pushed feature/oidc-email-domain-restriction from a244a9506c to 686569a368 2024-05-16 10:12:40 +00:00 Compare
lsde added 1 commit 2024-05-16 10:13:03 +00:00
Merge branch 'main' into feature/oidc-email-domain-restriction
Some checks failed
continuous-integration/drone/pr Build is failing
1fc8290bd1
Author
First-time contributor

Thanks for the code review, done requested changes, squashed and rebased.

Thanks for the code review, done requested changes, squashed and rebased.
konrad reviewed 2024-05-16 15:46:26 +00:00
konrad left a comment
Owner

Looks good, will test it properly when I'm back at my computer.

Looks good, will test it properly when I'm back at my computer.
Owner

Please fix the lint issues.

Please fix the lint issues.
Some checks failed
continuous-integration/drone/pr Build is failing
Required
Details
This pull request has changes conflicting with the target branch.
  • config.yml.sample
  • docs/content/doc/setup/openid-examples.md
  • docs/content/doc/setup/openid.md
  • docs/content/doc/usage/errors.md

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u feature/oidc-email-domain-restriction:lsde-feature/oidc-email-domain-restriction
git checkout lsde-feature/oidc-email-domain-restriction
Sign in to join this conversation.
No Reviewers
No Milestone
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: vikunja/vikunja#2332
No description provided.