search

dgrijalva / jwt-go

ARCHIVE - Golang implementation of JSON Web Tokens (JWT). This project is now maintained at:
https://github.com/golang-jwt/jwt
MIT License
10.78k stars 994 forks source link

non api breaking fix to MapClaims VerifyAudience #471

Closed aldas closed 3 years ago

aldas commented 3 years ago

This is non api breaking fix to MapClaims VerifyAudience problem. This could be release as patch version v3.2.1

See these comments; https://github.com/dgrijalva/jwt-go/issues/463#issuecomment-854975451 https://github.com/dgrijalva/jwt-go/issues/463#issuecomment-854979458

this does not change StandardClaims.Audience type as it not a actual problem for StandardClaims.VerifyAudience. Go type system will not allow slice to assigned to string. Problematic part is MapClaims.VerifyAudience which is handling intefaces

aldas commented 3 years ago

@dgrijalva please check this one

oxisto commented 3 years ago

@aldas Be aware, that this repository is in non-maintainance mode. There are currently discussions about migrating this popular library to a common organisation / repo (see #462 for details). Some interested parties, including myself, have set up such a repository on http://github.com/golang-jwt/jwt and we have fixed the issue in https://github.com/golang-jwt/jwt/pull/12. We are currently preparing everything for a smooth transition to the new import path and will release a fixed v3.2.1 version.

mrjrieke commented 3 years ago

@aldas Be aware, that this repository is in non-maintainance mode. There are currently discussions about migrating this popular library to a common organisation / repo (see #462 for details). Some interested parties, including myself, have set up such a repository on http://github.com/golang-jwt/jwt and we have fixed the issue in golang-jwt/jwt#12. We are currently preparing everything for a smooth transition to the new import path and will release a fixed v3.2.1 version.

Especially love that you kept history in your conversion giving respect to all those who put thought and hard work into the project before!

aldas commented 3 years ago

It would not take much from @dgrijalva to merge this little thing and have repo still unmaintained. I would gladly switch over to new repo but these API breaking changes are little bit NO-GO at the moment. If problem is mapclaims - why not fix it in patch version and do API breaking change (introduce []string in struct/method arguments) in v4.

ripienaar commented 3 years ago

Despite what you apparently believe. You have no actual RIGHT to any open source package or feeling anyone owes you anything.

If the maintainer finds himself unwilling to continue maintaining it that is 100% his prerogative.

It’s the nature of accepting open source dependencies. We are trying to keep moving to the new package as minimal impact as possible. But people will have to move it’s unavoidable.

aldas commented 3 years ago

I totally agree nor I am demanding anything - I have seen end of life discussing and accept that decision. I have taken my time to help even to make this fix easier by providing fix and tests suitable for patch release. If original maintainer has time he/she can do it easier and not worrying about API breaking concerns which take usually much effort in lib development. It tries to fix problems for a lot of people.

ripienaar commented 3 years ago

Then perhaps reconsider all caps proclamations about what is “NO-GO” when and where as that’s making quite a demand.

aldas commented 3 years ago

let me explain - NO-GO in sense that API breaking change to fix MapClaims.VerifyAudience internal problem is problematic as it causes/forces change to code and applications that work perfectly fine today with aud being string or VerifyAudience being required. This problem with casting and aud allowed to be array in JWT spec are 2 different topics and can be addressed in separate solutions/fixes.

oxisto commented 3 years ago

It would not take much from @dgrijalva to merge this little thing and have repo still unmaintained. I would gladly switch over to new repo but these API breaking changes are little bit NO-GO at the moment. If problem is mapclaims - why not fix it in patch version and do API breaking change (introduce []string in struct/method arguments) in v4.

Maybe I am missing something, but which part of the fix we maintain at golang-jwt/jwt is API breaking? We actually aim at not to break the API in the current v3 release cycle.

ripienaar commented 3 years ago

let me explain - NO-GO in sense that API breaking change to fix MapClaims.VerifyAudience internal problem is problematic as it causes/forces change to code and applications that work perfectly fine today with aud being string or VerifyAudience being required. This problem with casting and aud allowed to be array in JWT spec are 2 different topics and can be addressed in separate solutions/fixes.

I understand the breaking change. You still have no right to make all CAPS demands of how people choose to run their projects or not.

Ask nicely, explain why it’s important, propose alternatives. You have no entitlement to demand anything though of someone who is already ready to move on to something else

aldas commented 3 years ago

My apologies. Mistook that private function arguments change with MapClaims public one in quick scroll through.

oxisto commented 3 years ago

My apologies. Mistook that private function arguments change with MapClaims public one in quick scroll through.

No worries. The patch originally proposed by @Waterdrips here was actually API breaking, but we adjusted it in the new repo so that we can provide a drop-in-replacement. We have one final PR we want to look at with regards to documentation of migration and then we can (finally) release v3.2.1 on http://github.com/golang-jwt/jwt with the intention of being compatible, then later on introduce new features, either backwards-compatible on v3 or API-breaking on v4.

aldas commented 3 years ago

Please do not take as I want to diminish efforts of new maintained repository. As a non native English speaker I though "please check" and "does not take much" does not have that demanding sound to it. My only intention was to keep everyone efforts to overcome this bug minimal. Do not take it ("NO-GO") as I'm not wanting to use maintained repository (I'll definitely will be switching) but taking baby steps and going first with transparent patch release seemed too attractive not to try.