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

Why is the Spring Authorization Server Release version used as serialVersionUID #1203

Closed
Chr3is opened this issue May 8, 2023 · 8 comments
Closed
Assignees
Labels
for: stackoverflow A question that's better suited to stackoverflow.com

Comments

@Chr3is
Copy link

Chr3is commented May 8, 2023

In org.springframework.security.oauth2.server.authorization.util.SpringAuthorizationServerVersion the serialVersionUID is defined which is used across all spring authorization server classes which implement the Serializable interface. However I do not unterstand the intention for that.

Lets assume these classes are used within a session store. For example the OAuth2Authorization. With every new release we would invalide these sessions due to a changed serialVersionUID and the resulting InvalidClassException when trying to deserialize the objects stored in the current session.

Wouldn't it be a better practice to provide an own serialVersionUID for every class which implements Serializable and only change it when real incompatibilities like removed fields where introduced? And provide a changelog for these changes?

Another question would be how am I supposed to extend the OAuth2Authorization which should be possible due to the fact that the class is not declared as final? There's no all args ctor, the fields are private and not protected. With an extended OAuth2Authorization it would be possible to overcome the serialVersionUID issue and provide an easier access to information stored within the attributes through own getters.

@Chr3is Chr3is added the type: enhancement A general enhancement label May 8, 2023
@jgrandja
Copy link
Collaborator

@Chr3is Questions are better suited to Stack Overflow. We prefer to use GitHub issues only for bugs and enhancements. Feel free to update this issue with a link to the re-posted question (so that other people can find it).

FYI, Spring Authorization Server follows how Spring Security assigns serialVersionUID using the release version. We consider the complete set of domain classes for a specific version not just one because compatibility needs to work as a whole for the specific version - not just it's smaller parts - if a change is made to one domain class only. You can consider JSON serialization instead of Java Object serialization for smoother upgrading between versions.

@jgrandja jgrandja self-assigned this May 18, 2023
@jgrandja jgrandja added for: stackoverflow A question that's better suited to stackoverflow.com and removed type: enhancement A general enhancement labels May 18, 2023
@Chr3is
Copy link
Author

Chr3is commented May 19, 2023

@jgrandja this is rather a discussion than a question. An enhancement could be to provide a "better" versioning for domain classes or at least to increment the serialVersionUID only if a real incompatibility was introduced. And this should not happen with a patch-release if you follow correct versioning. Spring Security Core (Release 5.8.2) has a serialVersionUID of 580L and not 582L.

One suggested enhancement is also to make org.springframework.security.oauth2.server.authorization.OAuth2Authorization better extendable with protected fields and a protected all args ctor. This class is used in one of the core interfaces e.g. OAuth2AuthorizationService and should be easy extendable for custom implementations. At the moment you're able to extend this class but you can't set the private fields like id, registeredClientId..

@jgrandja
Copy link
Collaborator

jgrandja commented May 19, 2023

@Chr3is

this should not happen with a patch-release

Thanks for catching that! That was a mistake on our end. It has now been fixed and pushed to all 3 branches.

OAuth2Authorization is fully extendable. OAuth2Authorization and OAuth2Authorization.Builder provide protected constructors.

protected all args constructor

That is one approach but we went with the approach where users should extend from OAuth2Authorization.Builder (and OAuth2Authorization) to provide their own custom attributes and there is full access to the existing attributes via the public and/or protected mutator methods within the Builder - fields do not need to be protected.

@Chr3is
Copy link
Author

Chr3is commented May 21, 2023

@jgrandja Thanks for fixing that!

I have no clue how an extended OAuth2Authorization.Builder could work and how the private fields, coming from the OAuth2Authorization, should be set in my extended OAuth2Authorization. Could you provide an example? That would be great. (sidenote: we use lombok for builder generation to avoid to much boilerplate code, but this won't work here at all)

import org.springframework.security.oauth2.server.authorization.OAuth2Authorization;
import org.springframework.security.oauth2.server.authorization.client.RegisteredClient;
import org.springframework.util.Assert;

public class MyOwnAuthorization extends OAuth2Authorization {

    private final String myOwnField;

    public MyOwnAuthorization(String myOwnField) {
        this.myOwnField = myOwnField;
    }

    public static MyOwnAuthorizationBuilder withRegisteredClient(RegisteredClient registeredClient) {
        Assert.notNull(registeredClient, "registeredClient cannot be null");
        return new MyOwnAuthorizationBuilder(registeredClient.getId());
    }

    public static class MyOwnAuthorizationBuilder extends OAuth2Authorization.Builder {
        private String myOwnField;

        protected MyOwnAuthorizationBuilder(String registeredClientId) {
            super(registeredClientId);
        }

        public MyOwnAuthorizationBuilder myOwnField(String myOwnField) {
            this.myOwnField = myOwnField;
            return this;
        }

        @Override
        public OAuth2Authorization build() {
            OAuth2Authorization authorization = super.build();
            MyOwnAuthorization myOwnAuthorization = new MyOwnAuthorization(myOwnField);
            // myOwnAuthorization.id = this.id

            //no idea how this is intended
            return myOwnAuthorization; //missing all fields from parent OAuth2Authorization and no possibility to set them
        }
    }
}

@jgrandja
Copy link
Collaborator

@Chr3is Apologies but OAuth2Authorization requires some changes to be fully extendable. OAuth2Authorization would need protected setters and OAuth2Authorization.Builder would need protected getters.

Can you provide more details on the type of attributes you need in your extended model? Could you not use OAuth2Authorization.attributes.

@Chr3is
Copy link
Author

Chr3is commented May 26, 2023

@jgrandja No worries!
In our case we extended the authorization with additional information like authorizedClaims, authTime, our representation of the authorizerequest (containing additional information like nonce, prompt,..) and so on. We don't want to use a generic map but use an explicit typesafe & understandable model. And this should be no problem if the OAuth2Authorization is fully extendable.

@jgrandja
Copy link
Collaborator

@Chr3is Can you please log a new issue for "Allow OAuth2Authorization to be extensible". Would you be interested in submitting a PR for this?

@Chr3is
Copy link
Author

Chr3is commented May 26, 2023

@jgrandja I'm going to create a new issue for that and yeah I can try to provide a PR for this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: stackoverflow A question that's better suited to stackoverflow.com
Projects
None yet
Development

No branches or pull requests

2 participants