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

GUACAMOLE-805: Handle OpenID "id_token" parameter regardless of location in URL fragment. #407

Merged
merged 2 commits into from Jun 3, 2019

Conversation

Projects
None yet
4 participants
@mike-jumper
Copy link
Contributor

commented Jun 3, 2019

These changes add more robust handling of the id_token parameter. The URL fragment is automatically reformatted from the format used by OpenID to the format used by AngularJS, regardless of the location of id_token within that fragment.

The fragment is reformatted from #... to #/?... if all of the following are true:

  1. The fragment did not already start with #/ (an AngularJS path).
  2. The fragment did not already start with #? (which AngularJS will already handle as a list of parameters).
  3. The fragment contains an id_token parameter, either at the beginning or elsewhere within the parameter list.

All other fragments are left untouched.

mike-jumper added some commits Dec 8, 2018

GUACAMOLE-805: Only reformat a URL fragment that appears to be from O…
…penID Connect if the fragment is not already in a format consumable by AngularJS ("#?..." or "#/?...").

mike-jumper referenced this pull request in mike-jumper/guacamole-client Jun 3, 2019

@necouchman
Copy link
Contributor

left a comment

Looks fine to me.

@necouchman necouchman merged commit 0344ef3 into apache:master Jun 3, 2019

@mike-jumper mike-jumper deleted the mike-jumper:openid-token branch Jun 3, 2019

@alallier

This comment has been minimized.

Copy link

commented Jun 3, 2019

Let a comment on the JIRA issue but posting here as well.

I'm uniformed and ignorant of the release schedules on Guacamole but I see this fix is slated for 1.2.0. When is this planned to be released? I'm asking because I am unable to use OpenID because of this bug in current versions. This wouldn't get pushed up to a patch version at all?

@necouchman

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

@alallier: We do not have a planned release date for the 1.2.0 version - we haven't even nailed down the release date for 1.1.0.

@alallier

This comment has been minimized.

Copy link

commented Jun 3, 2019

That's fine. Is there anyway this can make it into 1.1.0?

@mike-jumper

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

No. The scope of 1.1.0 has been set. New issues will go to master only, for inclusion in the next release that isn't already in progress. Today, that release is 1.2.0.

@mike-jumper

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

... posting here as well.

Going forward, please do not double-post.

@alallier

This comment has been minimized.

Copy link

commented Jun 3, 2019

I apologize @mike-jumper for the double post.

While we are here though I have a follow up question about versioning. Since this PR addresses a large authentication problem preventing people from using OpenID why is it not in the next patch release? If you are setup for 1.1.0 now, then I would expect a patch to come out in 1.1.1 right after the release of 1.1.0 not having to wait until the next minor (feature based) release 1.2.0.

Or are you not doing patch releases?

I briefly read parts of this: https://www.mail-archive.com/dev@guacamole.apache.org/msg02298.html

@mike-jumper

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

Current process

At any given time, there are no more than two possible releases in flight:

  1. A release whose scope has been finalized (staging/*)
  2. A release whose scope has not yet been finalized (master)

Whether the project decides to release the next release as 1.1.1 rather than 1.2.0 is a decision to be made following 1.1.0. It would be best if you don't read into the version number any further than a cosmetic representation of the content and compatibility of the release; it does not affect timing.

Overall:

  • Releases are cut when possible. Timing depends only on testing and process and consensus, which all depend on the availability of the volunteers that contribute to the project, myself included. It has nothing to do with the version number.
  • Scope of the staging/* release is set in stone. Feature creep is a sure way to kill a release.
  • The version numbering will be ultimately decided when the scope of the next release is finalized. The decision of what will go into that release is not a decision to be made now. That decision happens after the next release.

Changes to the current process

As noted in the thread you link to, there has been occasional discussion of migrating to a process that would allow for bugfix releases. We don't currently have such a process. A lot of thought and preparation would need to go into that decision. It's not out of the question.

What you can do

Contribute. Be involved in the community.

A better place for this would have been the mailing list, where the rest of the community can be present in the discussion. Commenting only on JIRA or GitHub reaches a much smaller audience. Unless the discussion is extremely directly related to the development at hand, this will just drain the energy of the few that can see your comments. If this PR had not yet been merged and you found an issue with the change, this would have been a good place for that. If the merged code did not actually solve the issue and you believe the bug must be reopened, JIRA would be appropriate. It's not the place to discuss release policy.

If you have feedback regarding the next release, constructive ideas for changes to process, etc., the best place for that is the mailing list. The dev@ list is where all development discussion occurs, and that list will be the home of the thread that will be opened following 1.1.0 to discuss release scope.

http://guacamole.apache.org/release-procedures-part1/

@lukasmrtvy

This comment has been minimized.

Copy link

commented Jun 5, 2019

Huge impact for us -> We can not use OpenID authentication, this change should be backported as patch release asap.

@necouchman

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

Huge impact for us -> We can not use OpenID authentication, this change should be backported as patch release asap.

It won't be.

@mike-jumper

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

@lukasmrtvy, no, that is not our release process, nor is this the place to discuss what you would prefer our process to be. See above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.