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: add sso_auth parameter #45

Closed

Conversation

QuentinBtd
Copy link
Contributor

@QuentinBtd QuentinBtd commented Sep 6, 2023

Why ?

sso_auth parameter exists in Priutnl servers.
When set at true, it will require client to authenticate with single sign-on provider on each connection using web browser.
It could be usefull to have it. (And I need it, of course)

How ?

Adding it in provider.

Bonus

Update pritunl docker image version.

@QuentinBtd QuentinBtd marked this pull request as ready for review September 6, 2023 20:46
- Update the Pritunl Docker image version to `1.32.3602.80`

Signed-off-by: Quentin BERTRAND <[email protected]>
@QuentinBtd
Copy link
Contributor Author

Hello @disc 👋

@disc
Copy link
Owner

disc commented Sep 9, 2023

Hey @QuentinBtd, i appreciate your contribution, but I think it won't work without configuration of SSO settings for the whole pritunl instance (see the attached screenshot).
image

I started to work on the instance settings some time ago here () but haven't completed it yet.

@QuentinBtd
Copy link
Contributor Author

Do you think Pritunl will return an error if the provider send a JSON with sso_auth in, while SSO is not configured at the instance level?

I could try it with an instance where SSO is not configured 😃

@disc
Copy link
Owner

disc commented Sep 9, 2023

I meant it won't work without a configuration of SSO for the entire server, but the options sso_auth will be saved and will open browser each time with 404 page due to empty SSO settings.

image

This feature could not be useful if you manually setup SSO on the server but manage the sso_auth option with terraform, don't you think so?

@QuentinBtd
Copy link
Contributor Author

This feature could not be useful if you manually setup SSO on the server but manage the sso_auth option with terraform, don't you think so?

That's exactly what I want to do 👍

The SSO configuration is already present on my instance. My servers are provisioned via Terraform but I have to manually check sso_auth after each apply terraform because this action unchecks it 😢

internal/pritunl/server.go Outdated Show resolved Hide resolved
@@ -336,6 +336,12 @@ func resourceServer() *schema.Resource {
Optional: true,
Description: "Show server debugging information in output.",
},
"sso_auth": {
Copy link
Owner

Choose a reason for hiding this comment

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

Add test case for a new option

@disc
Copy link
Owner

disc commented Sep 9, 2023

This feature could not be useful if you manually setup SSO on the server but manage the sso_auth option with terraform, don't you think so?

That's exactly what I want to do 👍

The SSO configuration is already present on my instance. My servers are provisioned via Terraform but I have to manually check sso_auth after each apply terraform because this action unchecks it 😢

For

This feature could not be useful if you manually setup SSO on the server but manage the sso_auth option with terraform, don't you think so?

That's exactly what I want to do 👍

The SSO configuration is already present on my instance. My servers are provisioned via Terraform but I have to manually check sso_auth after each apply terraform because this action unchecks it 😢

Got you, lets add a test for the new option and fix small issues that I left.

- Modify the `SsoAuth` field to allow it to be omitted in the JSON representation

Signed-off-by: Quentin BERTRAND <[email protected]>
@disc disc added the duplicate This issue or pull request already exists label Sep 14, 2023
@disc
Copy link
Owner

disc commented Sep 14, 2023

@QuentinBtd I'm going to close this PR as a duplicate, because I've just merged another PR with a support of the same option sso_auth #48

@disc disc closed this Sep 14, 2023
@QuentinBtd
Copy link
Contributor Author

No problem
@SwissGipfel was more fast than me, or have more time, but thank you ❤️

@QuentinBtd QuentinBtd deleted the feat_add_sso_auth_parameter branch September 14, 2023 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants