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

feat(rest): Added creating user from REST and basic authentication #1832

Merged
merged 1 commit into from
Apr 20, 2023
Merged

feat(rest): Added creating user from REST and basic authentication #1832

merged 1 commit into from
Apr 20, 2023

Conversation

smrutis1
Copy link
Contributor

@smrutis1 smrutis1 commented Feb 8, 2023

Signed-off-by: Smruti Prakash Sahoo [email protected]

Please provide a summary of your changes here.

  • Added a password field in the user structure.
  • Added username and password based validation from REST endpoint.
  • Added one endpoint for creating user.

Note: The above endpoints are required for building new UI and initial step to get rid of Liferay.

  • Which issue is this pull request belonging to and how is it solving it? (Endpoints require for New-UI #1826)
  • Did you add or update any new dependencies that are required for your change? - No

Issue:

Suggest Reviewer

You can suggest reviewers here with an @mention.

How To Test?

How should these changes be tested by the reviewer?

  • Need to test creating /api/users endpoint for creating user.
  • And validating the authentication for that user from REST endpoint.

Have you implemented any additional tests?

Checklist

Must:

  • All related issues are referenced in commit messages and in PR

@smrutis1 smrutis1 changed the title feat(rest): Added basic username and password based authenticarion feat(rest): Added creating user from REST and basic authentication Feb 8, 2023
@smrutis1 smrutis1 added needs code review needs general test This is general testing, meaning that there is no org specific issue to check for labels Feb 8, 2023
@rudra-superrr
Copy link
Contributor

Three things that I observed during testing are:

  1. ExternalId/GID value is not being stored in the database and make this value as mandatory.
  2. Through this rest endpoint password field is getting added in the sw360users database which was not there before.
  3. Make all the fields used for user creation in body section as mandatory which is not the case right now.

@ag4ums ag4ums mentioned this pull request Feb 15, 2023
Open
@akapti akapti self-requested a review February 24, 2023 09:52
akapti
akapti requested changes Feb 27, 2023
View reviewed changes
Copy link
Contributor

@akapti akapti left a comment

Choose a reason for hiding this comment

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

kindly incorporate review comments.

backend/src/src-users/src/main/java/org/eclipse/sw360/users/db/UserDatabaseHandler.java Outdated Show resolved Hide resolved
.../org/eclipse/sw360/rest/resourceserver/security/basic/Sw360GrantedAuthoritiesCalculator.java Outdated Show resolved Hide resolved
...rc/main/java/org/eclipse/sw360/rest/resourceserver/security/basic/Sw360GrantedAuthority.java Outdated Show resolved Hide resolved
...java/org/eclipse/sw360/rest/resourceserver/security/basic/Sw360CustomUserDetailsService.java Outdated Show resolved Hide resolved
...java/org/eclipse/sw360/rest/resourceserver/security/basic/Sw360CustomUserDetailsService.java Outdated Show resolved Hide resolved
.../org/eclipse/sw360/rest/resourceserver/security/basic/Sw360GrantedAuthoritiesCalculator.java Outdated Show resolved Hide resolved
...main/java/org/eclipse/sw360/rest/resourceserver/security/basic/Sw360UserDetailsProvider.java Outdated Show resolved Hide resolved
...resource-server/src/main/java/org/eclipse/sw360/rest/resourceserver/Sw360ResourceServer.java Show resolved Hide resolved
...source-server/src/main/java/org/eclipse/sw360/rest/resourceserver/user/Sw360UserService.java Outdated Show resolved Hide resolved
...resource-server/src/main/java/org/eclipse/sw360/rest/resourceserver/user/UserController.java Outdated Show resolved Hide resolved
@smrutis1 smrutis1 force-pushed the feat/LoginMechanismFromRest branch from 62a00de to d62b0f1 Compare March 1, 2023 07:36
@smrutis1 smrutis1 force-pushed the feat/LoginMechanismFromRest branch 2 times, most recently from 7f4fd43 to c50c17a Compare March 15, 2023 04:59
@smrutis1
Copy link
Contributor Author

@rudra-superrr & @akapti Thank you for review and test. I have incorporated the changes. Kindly have a look once.

@rudra-superrr
Copy link
Contributor

Things that I observed during testing are:

  1. Make userGroup and externalId field mandatory in request structure/body.
  2. Getting 500 error code for some fields
    image

and 400 error code for some fields
image

Error code(400) should be same if any of the field is not added in body.

akapti
akapti requested changes Mar 30, 2023
View reviewed changes
Copy link
Contributor

@akapti akapti left a comment

Choose a reason for hiding this comment

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

Kindly incorporate the review comments.

backend/src/src-users/src/main/java/org/eclipse/sw360/users/UserHandler.java Outdated Show resolved Hide resolved
backend/src/src-users/src/main/java/org/eclipse/sw360/users/db/UserDatabaseHandler.java Outdated Show resolved Hide resolved
...java/org/eclipse/sw360/rest/resourceserver/security/basic/Sw360CustomUserDetailsService.java Outdated Show resolved Hide resolved
@smrutis1 smrutis1 force-pushed the feat/LoginMechanismFromRest branch from c50c17a to c45d1fb Compare March 31, 2023 11:56
@smrutis1
Copy link
Contributor Author

@rudra-superrr & @akapti Please have a look again

@smrutis1 smrutis1 force-pushed the feat/LoginMechanismFromRest branch from c45d1fb to a71e420 Compare March 31, 2023 14:32
akapti
akapti approved these changes Apr 1, 2023
View reviewed changes
Copy link
Contributor

@akapti akapti left a comment

Choose a reason for hiding this comment

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

Code looks good to me.
@rudra-superrr Can you please re-test it once?

@heliocastro heliocastro self-requested a review April 2, 2023 16:57
heliocastro
heliocastro approved these changes Apr 2, 2023
View reviewed changes
Copy link
Contributor

@heliocastro heliocastro left a comment

Choose a reason for hiding this comment

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

Good to go

@rudra-superrr
Copy link
Contributor

Hi @smrutis1,

  • Can you remove userGroup parameter from doc as this field is now not mandatory and user will be provided with user role by default.
  • Other thing, just change 500 to 400 error code for password field too.
    image

@smrutis1 smrutis1 force-pushed the feat/LoginMechanismFromRest branch from a71e420 to b484659 Compare April 5, 2023 04:10
@smrutis1
Copy link
Contributor Author

smrutis1 commented Apr 5, 2023

Hi @smrutis1,

  • Can you remove userGroup parameter from doc as this field is now not mandatory and user will be provided with user role by default.
  • Other thing, just change 500 to 400 error code for password field too.
    image

Done

@rudra-superrr
Copy link
Contributor

Testing was successful.

@rudra-superrr rudra-superrr added ready ready to merge and removed needs general test This is general testing, meaning that there is no org specific issue to check for labels Apr 5, 2023
@ag4ums
Copy link
Contributor

ag4ums commented Apr 5, 2023

@smrutis1, need to separate the PR from current implementation with a switch, else anyone in production servers can use this endpoint to login to backend

@smrutis1 smrutis1 force-pushed the feat/LoginMechanismFromRest branch from b484659 to 2e0732a Compare April 17, 2023 12:57
@smrutis1
Copy link
Contributor Author

@smrutis1, need to separate the PR from current implementation with a switch, else anyone in production servers can use this endpoint to login to backend

@ag4ums Added the flag in application.yml to disable/enable the endpoint. The user will get a Service is disabled message when it is disabled.

image

@ag4ums ag4ums added needs general test This is general testing, meaning that there is no org specific issue to check for and removed ready ready to merge labels Apr 19, 2023
@rudra-superrr
Copy link
Contributor

Testing was successful.
Getting the below message when service is disabled i.e. set to true.

image

@ag4ums ag4ums removed do not merge - нет! needs general test This is general testing, meaning that there is no org specific issue to check for labels Apr 20, 2023
ag4ums
ag4ums approved these changes Apr 20, 2023
View reviewed changes
Copy link
Contributor

@ag4ums ag4ums left a comment

Choose a reason for hiding this comment

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

Changes looks good

@ag4ums ag4ums merged commit af1e935 into eclipse-sw360:main Apr 20, 2023
@GMishx GMishx deleted the feat/LoginMechanismFromRest branch October 16, 2024 09:27
@GMishx GMishx mentioned this pull request Oct 16, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akapti akapti akapti approved these changes

@ag4ums ag4ums ag4ums approved these changes

@heliocastro heliocastro heliocastro approved these changes

Assignees

@akapti akapti

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@smrutis1 @rudra-superrr @ag4ums @heliocastro @akapti