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.

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

Jump to bottom

Fix #1636: For API /auth/combinded assert credentialName #1637

Merged
merged 7 commits into from
May 6, 2024
Merged

Fix #1636: For API /auth/combinded assert credentialName #1637

merged 7 commits into from
May 6, 2024

Conversation

jandusil
Copy link
Contributor

No description provided.

@jandusil jandusil self-assigned this Apr 30, 2024
@jandusil jandusil linked an issue Apr 30, 2024 that may be closed by this pull request
For API /auth/combinded assert credentialName #1636
Closed
@jandusil jandusil marked this pull request as draft April 30, 2024 11:40
@jandusil
- fix np check
3c1ed52 
- add tests
@jandusil jandusil marked this pull request as ready for review April 30, 2024 11:59
romanstrobl
romanstrobl requested changes May 2, 2024
View reviewed changes
Copy link
Member

@romanstrobl romanstrobl left a comment

Choose a reason for hiding this comment

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

We should rather remove the parameter completely in my opinion, the parameter is ignored and should be take form the OTP.

@jandusil
Copy link
Contributor Author

jandusil commented May 2, 2024 •
edited
Loading

@zcgandcomp As the author of the issue I would need your response/comment please

@zcgandcomp
Copy link
Member

Hmm Roman is right. We can just remove it - it will still work (caller doesn't need to change the call immediately) and the functionality will not be affected - as the parameter is basically ignored.
We can take care of notifying the callers in the migration guide. So they can remove the parameter and, if necessary, use the correct one in the creation of the OTP.

@jandusil jandusil requested a review from romanstrobl May 6, 2024 06:49
zcgandcomp
zcgandcomp requested changes May 6, 2024
View reviewed changes
Copy link
Member

@zcgandcomp zcgandcomp left a comment

Choose a reason for hiding this comment

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

One enhancement of current possible NPE and one log message otherwise fine.

.../src/main/java/io/getlime/security/powerauth/app/nextstep/service/AuthenticationService.java Outdated Show resolved Hide resolved
.../src/main/java/io/getlime/security/powerauth/app/nextstep/service/AuthenticationService.java Outdated Show resolved Hide resolved
@jandusil jandusil requested a review from zcgandcomp May 6, 2024 08:21
zcgandcomp
zcgandcomp approved these changes May 6, 2024
View reviewed changes
Copy link
Member

@zcgandcomp zcgandcomp left a comment

Choose a reason for hiding this comment

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

We can have it in the migration guide - the parameter is required in the OTP creation. But whoever uses it should already know.

romanstrobl
romanstrobl approved these changes May 6, 2024
View reviewed changes
Copy link
Member

@romanstrobl romanstrobl left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the issue!

@jandusil jandusil merged commit df72894 into develop May 6, 2024
1 check passed
@jandusil jandusil deleted the issues/1636-valid-cred-name-combined-auth branch May 6, 2024 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@romanstrobl romanstrobl romanstrobl approved these changes

@zcgandcomp zcgandcomp zcgandcomp approved these changes

@banterCZ banterCZ Awaiting requested review from banterCZ

Assignees

@jandusil jandusil

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

For API /auth/combinded assert credentialName
3 participants
@jandusil @zcgandcomp @romanstrobl