aliazmi68 commented 5 years ago

Implementation of Spring Security:

Authorization and authentication.
Definition of User roles.
Routes definition and restriction.

Implementation of Angular UI:

User Signup and Login features.
Implementation of admin panel for user listing, update and deletion.

nikit-srivastava commented 5 years ago

@aliazmi68 @maqboolayaz description is missing

maqboolkhan commented 5 years ago

@aliazmi68 @maqboolayaz @deepakgarg08 as per Deepak comment in https://github.com/dice-group/ida/pull/150 this branch is not working even without Docker. You guys must need to look into the problem and comment here if its working fine for you without any problem. Kindly do it ASAP

codecov-io commented 5 years ago

Codecov Report

Merging #121 into master will increase coverage by 2.06%. The diff coverage is 55.28%.

@@             Coverage Diff              @@
##             master     #121      +/-   ##
============================================
+ Coverage     43.06%   45.13%   +2.06%     
- Complexity      200      230      +30     
============================================
  Files            61       63       +2     
  Lines          1586     1870     +284     
  Branches        161      223      +62     
============================================
+ Hits            683      844     +161     
- Misses          858      946      +88     
- Partials         45       80      +35

Impacted Files	Coverage Δ	Complexity Δ
...main/java/upb/ida/provider/VennDiagramHandler.java	`6.66% <0%> (-1.03%)`	`1 <0> (ø)`
.../main/java/upb/ida/venndiagram/VennDataFilter.java	`0% <0%> (ø)`	`0 <0> (?)`
.../main/java/upb/ida/provider/GeoDiagramHandler.java	`3.33% <0%> (-0.12%)`	`1 <0> (ø)`
...a-ws/src/main/java/upb/ida/dao/UserRepository.java	`69.13% <0%> (+0.9%)`	`8 <0> (-1)`	:arrow_down:
...-ws/src/main/java/upb/ida/constant/IDALiteral.java	`100% <100%> (ø)`	`1 <1> (?)`
...-ws/src/main/java/upb/ida/provider/FdgHandler.java	`60% <100%> (-2.5%)`	`2 <0> (ø)`
...-ws/src/main/java/upb/ida/rest/UserController.java	`2.89% <14.28%> (-2.74%)`	`2 <1> (ø)`
...-ws/src/main/java/upb/ida/service/DataService.java	`16.66% <16.66%> (+4.16%)`	`1 <1> (ø)`	:arrow_down:
.../main/java/upb/ida/provider/SSBDiagramHandler.java	`5.66% <4.25%> (+0.66%)`	`1 <0> (ø)`	:arrow_down:
...src/main/java/upb/ida/provider/LoadDsMetadata.java	`61.11% <61.11%> (-8.12%)`	`2 <2> (ø)`
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update e646845...fa1c098. Read the comment docs.

deepakgarg08 commented 5 years ago

Variable name changed to "FUSEKI_URL"

On Tue, Jun 18, 2019 at 10:44 AM Clemens Damke notifications@github.com wrote:

@Cortys requested changes on this pull request.

In ida-ws/src/main/java/upb/ida/rest/UserController.java https://github.com/dice-group/ida/pull/121#discussion_r294676112:

@@ -36,14 +43,16 @@ @CrossOrigin(origins = "*", allowCredentials = "true") @RequestMapping("/user") public class UserController {

static String dbUrl = System.getenv("IDA_CHATBOT");

Please use a more meaningful variable name, e.g. FUSEKI_URL.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dice-group/ida/pull/121?email_source=notifications&email_token=AFSNP2JL4M6R4WLFB77BVB3P3COATA5CNFSM4GRIKNUKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB32RNWA#pullrequestreview-250943192, or mute the thread https://github.com/notifications/unsubscribe-auth/AFSNP2NKHGGB4Z5QEBRFKWTP3COATANCNFSM4GRIKNUA .

deepakgarg08 commented 5 years ago

well, for fuseki host and port, I will say whatever user dataset you set will work. for example, if you put http://fuseki:3030/demo then it will work fine... provided like user dataset exists, in a similar way if demo name dataset exists, then this will work perfectly. so as per my understanding no changes are required in this case. Else another alternative can be, to set two environmental variables may be...one for host and port and another for the dataset. Please let me know, do I need to change something or not?

On Tue, Jun 18, 2019 at 8:23 PM Clemens Damke notifications@github.com wrote:

@Cortys requested changes on this pull request.

Great! The PR seems to be mostly fine. I also verified that it works with our Docker stack.

I only have some minor comments that need to be resolved before this can be merged:

@deepakgarg08 https://github.com/deepakgarg08 The application assumes that the dataset is part of FUSEKI_URL (e.g. http://fuseki:3030/user), it should only contain the host and port, i.e. http://fuseki:3030. Please separate those aspects so that FUSEKI_URL can be used throughout the entire application and not just the user dataset.

@aliazmi68 https://github.com/aliazmi68 The UI was changed to always make room for the chatbox, even if it is not shown. This change makes the collapsible chatbox useless, since collapsing it does not make any more room for the data. Please fix this.

Once those two points are resolved, I think we can merge this.

@maqboolkhan https://github.com/maqboolkhan Can you test this PR on your machine as well?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dice-group/ida/pull/121?email_source=notifications&email_token=AFSNP2OQAEV3KG665Z6TOZTP3ER35A5CNFSM4GRIKNUKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB35DLBA#pullrequestreview-251278724, or mute the thread https://github.com/notifications/unsubscribe-auth/AFSNP2NINAAJMZU3ZY6XDP3P3ER35ANCNFSM4GRIKNUA .

Cortys commented 5 years ago

@deepakgarg08 Yes it will work with another dataset name but that is not the issue. The problem is that it only works with a single dataset. We want to store all data as RDF triples not just the user data and thus should use multiple datasets to separate different data domains. The FUSEKI_URL should not contain any dataset name, instead appropriate dataset names should be appended to it.

deepakgarg08 commented 5 years ago

Ok, Shall I create a new environment variable which take name of dataset for example user, demo etc?

On Tue, Jun 18, 2019 at 8:47 PM Clemens Damke notifications@github.com wrote:

@deepakgarg08 https://github.com/deepakgarg08 Yes it will work with another dataset name but that is not the issue. The problem is that it only works with a single dataset. We want to store all data as RDF triples not just the user data and thus should use multiple dataset to separate different data domains. The FUSEKI_URL should not contain any dataset name, instead appropriate dataset names should be appended to it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dice-group/ida/pull/121?email_source=notifications&email_token=AFSNP2ICTLD5RT4VS6EDRDTP3EUV5A5CNFSM4GRIKNUKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODX7TEFA#issuecomment-503263764, or mute the thread https://github.com/notifications/unsubscribe-auth/AFSNP2I2LMIAZPDBJRFUMVDP3EUV5ANCNFSM4GRIKNUA .

Cortys commented 5 years ago

@deepakgarg08 No, the dataset name is not part of the environment. The same ida-ws process will use different datasets in different parts of the application. The user management code will use the user dataset, other parts of the application might use other datasets. For now it should be fine if you just hardcode the dataset name (user in your case) and build the URL you need out of the FUSEKI_URL variable and the dataset name you want to use.

deepakgarg08 commented 5 years ago

Changed, you can take pull request now, and test it.

On Tue, Jun 18, 2019 at 8:58 PM Clemens Damke notifications@github.com wrote:

@deepakgarg08 https://github.com/deepakgarg08 No, the dataset name is not part of the environment. The same ida-ws process will use different datasets in different parts of the application. The user management code will use the user dataset, other parts of the application might use other datasets. For now it should be fine if you just hardcode the dataset name ( user in your case) and build the URL you need out of the FUSEKI_URL variable and the dataset name you want to use.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dice-group/ida/pull/121?email_source=notifications&email_token=AFSNP2PMRMNKTC2DQZ54PGLP3EV5BA5CNFSM4GRIKNUKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODX7UBRI#issuecomment-503267525, or mute the thread https://github.com/notifications/unsubscribe-auth/AFSNP2MHJCBCZQWZC2OPTEDP3EV5BANCNFSM4GRIKNUA .

deepakgarg08 commented 5 years ago

hi, I have no info about this file. ida-ws/src/main/java/upb/ida/smtp/EmailForSignup.java https://github.com/dice-group/ida/pull/121#discussion_r296441596: you can delete this file ida-ws/src/main/java/upb/ida/repository/UserRepository.java https://github.com/dice-group/ida/pull/121#discussion_r296441752: don't delete this file, its being used ida-ws/src/main/java/upb/ida/rest/LoginController.java https://github.com/dice-group/ida/pull/121#discussion_r296441904:

On Sat, Jun 22, 2019 at 1:01 PM maq notifications@github.com wrote:

@maqboolkhan requested changes on this pull request.

@deepakgarg08 https://github.com/deepakgarg08 @aliazmi68 https://github.com/aliazmi68 I have left some comments, please compliance them And remove all the necessary files or commented code if not needed! There are one or two files which I believe not in use anymore (Backend file, I also mentioned them in comments)

And @maqboolayaz https://github.com/maqboolayaz I would you to review backend code and comment here if everything is fine!

In ida-ws/src/main/java/upb/ida/rest/LoginController.java https://github.com/dice-group/ida/pull/121#discussion_r296441904:

@@ -0,0 +1,70 @@ +package upb.ida.rest; + +import java.util.HashMap; +import java.util.Map; + +import javax.servlet.http.HttpServletRequest; + +import org.springframework.beans.factory.annotation.Autowired;

Are we really using this file???

In ida-chatbot/src/app/components/user/user.component.html https://github.com/dice-group/ida/pull/121#discussion_r296442145:

<mat-menu #actionsmenu="matMenu">

<button mat-icon-button color="primary" (click)="startEdit(row.username, row.firstname, row.lastname)">

editEdit

<button mat-icon-button color="warn" (click)="deleteItem(row.username)">

deleteDelete

<tr mat-header-row *matHeaderRowDef="displayedColumns">

<tr mat-row *matRowDef="let row; columns: displayedColumns;">

you dont need === 0 part here

In ida-chatbot/src/app/components/user/user.component.ts https://github.com/dice-group/ida/pull/121#discussion_r296442333:

dataSource = new MatTableDataSource();

showSpinner = false;

constructor(private restservice: RestService, private userservice: UserService, private router: Router, public dialog: MatDialog , public snackBar: MatSnackBar) {}

ngOnInit() {

this.isAdmin();

}

startEdit(username: string, firstname: string, lastname: string) {

const dialogRef = this.dialog.open(UpdateDialogComponent, {

data: {username: username, firstname: firstname, lastname: lastname}

});

dialogRef.afterClosed().subscribe(result => {

error handling is missing in the whole file

In ida-chatbot/src/app/components/user/user.component.ts https://github.com/dice-group/ida/pull/121#discussion_r296442348:

}

if(counter == 5){

counter = 0;

if(user.userrole == "USER"){

users.push(user);

}

user = {

'firstname':'',

'lastname':'',

'username':'',

'userrole':'',

'password':'',

};

}

}

if (returnResp.status === false) {

if (! returnResp.status)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dice-group/ida/pull/121?email_source=notifications&email_token=AFSNP2MJIVDNSP2UYNPXQSDP3YA75A5CNFSM4GRIKNUKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB4K52MY#pullrequestreview-253091123, or mute the thread https://github.com/notifications/unsubscribe-auth/AFSNP2KJYXA5YABCEF2IF5DP3YA75ANCNFSM4GRIKNUA .

deepakgarg08 commented 5 years ago

Hi, all extra files and extra commented code removed

On Mon, Jul 1, 2019 at 11:43 AM Ayaz Maqbool notifications@github.com wrote:

@maqboolayaz commented on this pull request.

In ida-ws/src/main/resources/application.properties https://github.com/dice-group/ida/pull/121#discussion_r298961949:

@@ -2,3 +2,25 @@ logging.level.root=INFO spring.devtools.restart.enabled=true

server.servlet.contextPath=/ida-ws + +# =============================== +# = DATA SOURCE +# =============================== +# Set here configurations for the database connection +#spring.data.uri=http://localhost:3030

Remove the neo4j db configurations from here.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dice-group/ida/pull/121?email_source=notifications&email_token=AFSNP2K6EJX3YSVVLH3L2JLP5HGUBA5CNFSM4GRIKNUKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB5C453Q#pullrequestreview-256233198, or mute the thread https://github.com/notifications/unsubscribe-auth/AFSNP2MKFMNQ4TP7E7CJG3TP5HGUBANCNFSM4GRIKNUA .

deepakgarg08 commented 5 years ago

Now you can merge

maqboolkhan commented 5 years ago

cc @Cortys

Cortys commented 5 years ago

@maqboolkhan This feature is already merged into our demo branch. We could also merge it into master but that would decrease our test coverage since no tests have been written yet for the new user management code.

@nikit91 Should we merge this in now and add the tests in a separate PR next semester or delay the merge?

dice-group / ida-pg

Feat/springsecurity #121

Codecov Report

@Cortys requested changes on this pull request.

And @maqboolayaz https://github.com/maqboolayaz I would you to review backend code and comment here if everything is fine!

Are we really using this file???

you dont need === 0 part here

error handling is missing in the whole file

@maqboolayaz commented on this pull request.