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

Security-Tags: Permissions ebenfalls im HAL-Format? #78

Closed
rowe42 opened this issue Jan 16, 2018 · 18 comments
Closed

Security-Tags: Permissions ebenfalls im HAL-Format? #78

rowe42 opened this issue Jan 16, 2018 · 18 comments
Assignees
Milestone

Comments

@rowe42
Copy link
Owner

rowe42 commented Jan 16, 2018

Bisher habe ich die Security-Tags so implementiert, dass sie eine flache Liste von Permissions vom Backend holen:

[
    "RESOURCE2",
    "RESOURCE3",
    "RESOURCE1",
    "Default Resource"
]

(zukünftig entspricht das dann "administration_READ_Animal", "administration_WRITE_Animal" usw.)

In Issues #36 schlagt ihr vor, alle Requests im HAL-Format zu haben. Das sollte dann wohl auch für das Abholen der Permissions gelten, auch wenn diese nicht aus der Datenbank, sondern letztlich von KeyCloak kommen?

Das würde dann wohl so aussehen:

{
  "_embedded": {
    "permissions": [
       "RESOURCE2",
       "RESOURCE3",
       "RESOURCE1",
       "Default Resource"
    ]
  },
  "_links": {
    "self": {
      "href": "http://localhost:39146/permissions"
    },
    "profile": {
      "href": "http://localhost:39146/profile/permissions"
    },
    "search": {
      "href": "http://localhost:39146/permissions/search"
    }
  }
}

Richtig? Würde es die Links self, profile und search auch geben?

Würde das nämlich dann gleich entsprechend ändern wollen, dass die Tags diese Form verarbeiten können.

@xdoo @dragonfly28 @ejcsid

@xdoo
Copy link
Collaborator

xdoo commented Jan 16, 2018

Ja, wir sollten hier sch0n das HAL Format nehmen. ABER bitte nicht gleich implemntieren. Wir haben an zig Stellen das selbe Problem (bei Formularen und Listen). Das sollten wir überall gleich machen.

@rowe42
Copy link
Owner Author

rowe42 commented Jan 17, 2018

OK.
Was soll ich nicht gleich implementieren? Den Backend-Service? Oder das Frontend mit gemocktem Backend?
Letzteres würde ich schon gerne machen, sobald wir wissen, wie genau das HAL Format für die Permissions aussehen soll. Wie kommen wir denn dazu, dieses Format festzulegen? Welche Teile an meinem Vorschlag oben sind nicht korrekt oder diskussionswürdig?

@xdoo
Copy link
Collaborator

xdoo commented Jan 17, 2018

Es geht nicht darum das irgend etwas nicht korrekt ist. Es geht auch nicht darum wie das HAL Format aussieht (das ist spezifiziert). Es geht darum, wie wir es in der Oberfläche nutzen. Da kann natürlich jeder an seinem Teil herum basteln, danach wird es aber genau so aussehen.

Backend ist davon nicht betroffen.

@rowe42
Copy link
Owner Author

rowe42 commented Jan 17, 2018

OK, mir ist nicht klar, was du meinst. Ich mache jetzt erstmal nichts und überlasse es dir zu koordinieren, dass wir das "überall gleich machen".

@xdoo xdoo added this to the RefArch_1.0 milestone Jan 19, 2018
@rowe42
Copy link
Owner Author

rowe42 commented Jan 24, 2018

Ich habe das oben dargestellte Format mal hier hinterlegt:
http://demo7906595.mockable.io/permissions
und in Branch _#78 eingebaut.

Bevor ich daraus einen Pull Request mache, hätte ich folgende Bitten/Fragen an @xdoo:

  1. Bitte schau dir sowohl den Mock als auch die Implementierung an, ob das so aus deiner Sicht korrekt ist. Stichwort "überall gleich machen".
  2. Ich habe hier das AnimadFormBehavior-Mixin verwendet. Aber eigentlich habe ich gar keine Form und brauche auch nicht alles im Behavior (insbesondere keine Toasts, keine Form-related Methoden usw.), sondern nur _loadFormData und die zugehörigen Properties url und data. Mein Vorschlag: Wollen wir das nicht in ein separates File animad-traverson-behavior (oder sogar: common-traverson-behavior) oder so ähnlich auslagern? Ich kann das gern machen, wenn du zustimmst.

@ejcsid
Copy link
Collaborator

ejcsid commented Jan 24, 2018

Ich fände es auch gut, wenn wir ein extra behavior für die Zugriffe auf das Backend hätten. Dann sollte aber auch der load und delete aus der Liste mit rein.

@xdoo
Copy link
Collaborator

xdoo commented Jan 24, 2018

zu 1.
Das Format passt. Ich denke den profil / search Link kann man raus lassen. Du schreibts ja oben, dass wir die Daten direkt aus KeyCloake bekommen. Aus meiner Sicht brauchen wir hier kein HAL Format künstlich zu erzeugen, da diese Daten ja nie die Security Komponente verlassen, oder (ausser über die API)? Wichtiger ist in diesem Kontext, dass die URL zum Laden dieser Daten im Response unserer Root URL zurück kommt.

zu 2.
Hatte ich auch schon überlegt. Aus meiner Sicht spricht da aber vor allem dagegen, dass wir hier ja immer mit einem Callback arbeiten müssen. In diesen Callbacks machen wir (zumindest in der Form / Relation Mixin auch immer sehr spezifische Sachen. Viel sparen kann man da zu einem klassischen fetch nicht. Ich denke man sollte auch beachten, dass mehrere Mixins, die voneinander abhängen die Sache einen nicht leichter durchschauen lassen. Das macht vor allem bei der Fehlersuche Probleme. Ich würde tendenziell bei einem klassische fetch bleiben, der genau das spezifische Problem löst.

@rowe42
Copy link
Owner Author

rowe42 commented Jan 25, 2018

zu 1.: OK ich schaue mir an, wie ich die URL im Backend in der Root URL unterkriege. Soll ich dafür ein eigenes Issue anlegen oder dieses hier offen lassen?

Außerdem Frage dazu: Haben wir schon ein Issue dafür, wie und wo wir die Root-URL im Frontend verarbeiten? Das linke Menü wird ja in app aufgebaut, allerdings bräuchte ich die Permissions-URL bereits in index.html. Oder sollen wir die Root-URL mehrmals aufrufen, dann könnte man es auch jeweils in der View machen, wo heute auch die URL hinterlegt ist... Kann für diese Fragestellung und Aufgabe gern auch ein weiteres Issue aufmachen, wenn wir dafür noch keines haben.

@xdoo
Copy link
Collaborator

xdoo commented Jan 25, 2018 via email

@rowe42
Copy link
Owner Author

rowe42 commented Jan 25, 2018

Zu 2. oben: ich verstehe dass wir keine voneinander abhängige Behaviours wollen.

Was du mit dem "klassischen fetch" meinst, verstehe ich nicht so ganz. Das loadData im FormBehavior tut doch genau was ich brauche, warum sollte ich das duplizieren?
Ich würde den Code dann eher so belassen, dass ich weiterhin den FormBehavior verwende ok?

@xdoo
Copy link
Collaborator

xdoo commented Jan 25, 2018

Ja. Dein Fall ist halt keine klassische Form, aber finde ich jetzt nicht so dramatisch das Mixin trotzdem zu verwenden. Das ist ja tatsächlich relativ einfach gestrickt. Bitte beachten, dass ich da im zuge von #123 ein paar Änderungen vorgenommen habe, die allerdings keine Auswirkungen auf die Funktionalität, sondern nur auf den Aufruf haben.

@rowe42
Copy link
Owner Author

rowe42 commented Jan 26, 2018

@xdoo Noch eine Frage dazu: Du sagst oben: "Wichtiger ist in diesem Kontext, dass die URL zum Laden dieser Daten im Response unserer Root URL zurück kommt.".

Wenn man aber so drüber nachdenkt, sollten die Permissions doch gar nicht vom animad_admin_service zurückgeliefert werden. Denn, wenn wir nun mehrere Microservices hätten, würden wir doch nicht wollen, dass jeder auch /permissions anbietet?
Stattdessen sollten wir doch einen extra Microservice animad_user_service haben (Repo dazu gibt es ja schon), der dann /permissions in seinem Root bietet, der aber im API-Gateway über einen anderen Pfad ausgeliefert wird? Und ggf. wäre dieser MS dann gar nicht Bestandteil der animad-Anwendung, sondern ein generischer MS, der im OpenShift bereitgestellt und von allen genutzt wird?

Frage 1: Siehst du das auch so oder verstehe ich da was falsch?
Frage 2: Ist die Erwartung, dass der API-Gateway seinerseits auch wieder eine Root-URL ausliefert, die dann die einzelnen MS als Links aufführt? Und wenn ja, wie erreicht man diese Root-URL? "/" würde ja wohl zur index.html (also Polymer-Oberfläche) weiterleiten...

@rowe42
Copy link
Owner Author

rowe42 commented Jan 29, 2018

Den Frontend-Parts habe ich jetzt unter Nutzung animad-form-behavior in Pull Request #141 eingecheckt.

@xdoo die beiden Fragen oben habe ich aber weiterhin.

@rowe42
Copy link
Owner Author

rowe42 commented Jan 30, 2018

Zu Frage 2 oben von mir habe ich mal einen Branch _#78 im API-Gateway-Projekt erstellt, der eine solche Root-URL bedient. Allerdings baue ich die URLs von Hand und damit dupliziert zur application.yml, was nicht sehr schön ist - aber notfalls akzeptabel wäre, wenn wir es generieren. Trotzdem: Geht das irgendwie eleganter? Im Internet habe ich bislang nichts dazu gefunden und lediglich HAL-Support ergänzen bewirkt das leider nicht (es gibt dann zwar /, aber nur mit dem profile-Link).

@rowe42
Copy link
Owner Author

rowe42 commented Jan 31, 2018

Für die Frage mit der Root-URL habe ich nun den separaten Issue #145 erstellt.

@rowe42
Copy link
Owner Author

rowe42 commented Jan 31, 2018

Ich würde gerne das Format der Permissions-JSON noch umstellen und das _embedded rauslassen. Das ist aus meiner Sicht nicht ganz korrekt, da es ja keine Objekte sind, die da eingebettet sind. Und außerdem implementiert es sich leichter im user_service.
Sieht dann also so aus:

{
  "permissions": [
     "RESOURCE2",
     "RESOURCE3",
     "RESOURCE1",
     "Default Resource"
  ],
  "_links": {
    "self": {
      "href": "http://localhost:39146/permissionsv2"
    }
  }
}

(das ist die Ausgabe des neuen Mocks, den ich dafür unter http://demo7906595.mockable.io/permissionsv2 erstellt habe)

Habe es in der Art auch schon in Branch _#78 in admin_html5 und user_service eingecheckt.

dragonfly28 pushed a commit that referenced this issue Jan 31, 2018
@dragonfly28
Copy link
Contributor

im "self" müsste auch noch die Host-adresse von mockable stehen

@rowe42
Copy link
Owner Author

rowe42 commented Jan 31, 2018

Ok, habe ich geändert. Schaut jetzt so aus:
{ permissions: [ "RESOURCE2", "RESOURCE3", "RESOURCE1", "Default Resource" ], _links: { self: { href: "http://demo7906595.mockable.io/permissionsv2" } } }
Ich schließe das Issue.

@rowe42 rowe42 closed this as completed Jan 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants