Skip to content

Expose refreshon parameter in authentication result #829

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jun 26, 2024
Merged

Expose refreshon parameter in authentication result #829

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
69abbcc
Expose refreshon parameter in auth result metadata
Avery-Dunn Jun 24, 2024
2af8552
Merge branch 'dev' of https://github.com/AzureAD/microsoft-authentica…
Avery-Dunn Jun 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AuthenticationResult.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ private ITenantProfile getTenantProfile() {
private final String scopes;

@Builder.Default
private final AuthenticationResultMetadata metadata = new AuthenticationResultMetadata();
private final AuthenticationResultMetadata metadata = AuthenticationResultMetadata.builder().build();

@Getter(value = AccessLevel.PACKAGE)
private final Boolean isPopAuthorization;
Expand Down
13 changes: 3 additions & 10 deletions msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AuthenticationResultMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package com.microsoft.aad.msal4j;

import lombok.AccessLevel;
import lombok.Builder;
import lombok.Getter;
import lombok.Setter;
import lombok.experimental.Accessors;
Expand All @@ -16,17 +17,9 @@
@Accessors(fluent = true)
@Getter
@Setter(AccessLevel.PACKAGE)
@Builder
public class AuthenticationResultMetadata implements Serializable {

private TokenSource tokenSource;

/**
* Sets default metadata values. Used when creating an {@link IAuthenticationResult} before the values are known.
*/
AuthenticationResultMetadata() {
}

AuthenticationResultMetadata(TokenSource tokenSource) {
this.tokenSource = tokenSource;
}
private Long refreshOn;
}
5 changes: 4 additions & 1 deletion msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/TokenRequestExecutor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,10 @@ private AuthenticationResult createAuthenticationResultFromOauthHttpResponse(
refreshOn(response.getRefreshIn() > 0 ? currTimestampSec + response.getRefreshIn() : 0).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this already exposed in AuthenticationResult? If it is why do we need it in the metadata as well? If this makes more sense in metadata, maybe remove it from here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

refreshOn is a field in AuthenticationResult but not the IAuthenticationResult interface, and the return type of our acquireToken methods is IAuthenticationResult. AuthenticationResult itself is a package-private, so other projects can only see the fields in IAuthenticationResult

For various historical design reasons IAuthenticationResult has most of the fields listed out individually but that's not very expandable: adding a new field to a public interface is considered a breaking change unless we also add a (potentially meaningless) default value, but that's considered a bad practice.

That's the reason the AuthenticationResultMetadata class was introduced: adding new fields to that is not a breaking change, and it can be a bucket for more niche things like refreshOn that most users will not need to worry about.

accountCacheEntity(accountCacheEntity).
scopes(response.getScope()).
metadata(new AuthenticationResultMetadata(TokenSource.IDENTITY_PROVIDER)).
metadata(AuthenticationResultMetadata.builder()
.tokenSource(TokenSource.IDENTITY_PROVIDER)
.refreshOn(response.getRefreshIn() > 0 ? currTimestampSec + response.getRefreshIn() : 0)
.build()).
build();

} else {
Expand Down