GUACAMOLE-1322: Include SAML Plugin in Docker Image

[petzsch]

Addressing the issue of missing SAML Plugin in Docker Image.

Please include in next release. Thank You!

[petzsch]

added another commit to fix the whitespace issue hopefully.

[tworcester]

Code looks good! You need to re-do all of your commits to have the issue tag at the front, like this: GUACAMOLE-1322: fixed whitespaces

[petzsch]

I wasn't aware of that. I assume the commits would get squashed on merge anyway.
I've done a rebase of the last 3 commits. Hopefully that was the intended solution.

[mike-jumper]

You need to re-do all of your commits to have the issue tag at the front, like this: GUACAMOLE-1322: fixed whitespaces

Also, please do not use a commit message like "fixed whitespaces". There just isn't any context there. Like a code comment, a good commit message should describe the nature of the change, not the literal change:

https://www.codelord.net/2015/03/16/bad-commit-messages-hall-of-shame/

For example, after seeing your commit, I still had no idea what whitespace issue you were referring to or why anything needed to be fixed. I looked at the content of the commit itself and was able to see for myself, but that sort of information should already be apparent from the message.

With something like:

GUACAMOLE-1322: fixed whitespaces

A developer looking through the git history is left wondering what happened. If something is wrong with the content of your change, the developer goes to the relevant commit message to understand your reasoning for that change, and here that would be absent.

On the other hand, something like:

GUACAMOLE-1322: Correct indentation of SAML property assignments to match established code style.

is very clear. If (somehow) the change didn't achieve what it aimed at achieving, that would be apparent even weeks to months down the line.

No commit or change is too trivial to warrant a good message.

[petzsch]

@mike-jumper I agree with your arguments and am always willing to improve. Before I do another rebase and alter my "fixed whitespaces" commit message, are the other ones I used somewhat OK (are they still within an acceptable range)?

[mike-jumper]

@mike-jumper I agree with your arguments and am always willing to improve. Before I do another rebase and alter my "fixed whitespaces" commit message, are the other ones I used somewhat OK (are they still within an acceptable range)?

The other messages are better, for sure. If you'd like to improve those messages:

• Avoid shorthand like "ext" and "env" for "extension" and "environment variable". It's unnecessarily obfuscated for something that's intended to be documentation.
• Avoid describing what you're modifying by its filename alone - git already tells us this, and someone reading through the git history may not be as familiar with the purpose of each file. Describing things at a high level is better than saying "Update something.foo include BAR cfg"

For example, consider:

GUACAMOLE-1322: Update build-guacamole.sh - include saml ext.
GUACAMOLE-1322: Update start.sh include SAML env

vs.

GUACAMOLE-1322: Build and include SAML authentication extension within Docker image.
GUACAMOLE-1322: Add Docker environment variables for configuring SAML.

[petzsch]

stupid typo: sorry for the hassle.

[petzsch]

I've just been testing the changes and for some reason the .jar file for saml is not present in the docker image:

guacamole@aaf35270b1e9:/opt/guacamole/saml$ ls -al
total 8
drwxr-xr-x 2 root root 4096 Jun 3 09:17 .
drwxr-xr-x 1 root root 4096 Jun 3 09:17 ..

If someone spots the problem, any help would be appreciated.
Will update, if I find it myself.

Until that's solved: This pull request is not ready to be merged!

[petzsch]

this should fix it.

[petzsch]

Commit Messages have been corrected as requested. Please continue with the review. I'll remove the Draft status.

The docker image I've build for my usecase seems to run. Still working on getting the SAML integration with my coportate IdP to work.

Hopefully the documentation is right about this:

Often the metadata file contains most of the required properties for SAML authentication and the other parameters are not required.

[petzsch]

From my side the changes worked for my deployment together with the IdP that my customer uses. If any more changes are needed, please let me know. Otherwise a merge before the next release would be appreciated. 🥇

[jbpaux]

were you able to implement requested changes @petzsch ?

[petzsch]

were you able to implement requested changes @petzsch ?

Very sorry for the long radio silence. I'm currently involved in other projects.
If you want to pick up on my work, feel free to do so.

Do let me know if you need permissions on my patch-1 branch on the forked repo.

[jbpaux]

ok @petzsch I can have a look on it the weekend :)

[jbpaux]

I still can have a look if you give perms 😅

[petzsch]

I still can have a look if you give perms 😅

I just added you to the project.

[jbpaux]

I've implemented the requested changes @mike-jumper :)

[petzsch]

is this still being monitored?

[mike-jumper]

Yep, I just got real busy toward the end of last year. I'll try to look over this later today.

[JackWolfard]

What's the status of this PR? Will it be included as a feature in 1.5.0?

[jbpaux]

I've rebased on master and removed commits dealing with adding the saml jar since it's already included with all other sso plugins. should be good @mike-jumper 🤞

[necouchman]

@JackWolfard:

What's the status of this PR?

As you can see from the messages, it's being actively worked between the person who submitted the PR and the reviewer.

Will it be included as a feature in 1.5.0?

I could not say with certainty one way or the other, but, as it's currently active, I would say the likelihood that it'll end up in the next release is pretty high.

[trident25]

I've found a bug in this pull request - captured here: https://issues.apache.org/jira/browse/GUACAMOLE-1570

I think the logic when deciding to load the SAML extension is incomplete. @petzsch