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

Adding Keycloak To Server #22

Merged
merged 29 commits into from
Dec 18, 2024
Merged

Adding Keycloak To Server #22

merged 29 commits into from
Dec 18, 2024

Conversation

niclasheun
Copy link
Contributor

This PR shall be basis for discussion. I would really appreciate your comments @Mtze @robertjndw

The main idea:

  • To implement fine-grained role management for Prompt
  • Similar to Artemis, I introduced the groups: Lecturer, Editor, Student
    • On creation of every course, the server sends requests to keycloak to create the groups

My main question: Which roles to create?

  • The big challenge is the following: We need access control on course and course phase level
    • Every course needs view / modify
    • Every course phase: view meta data, modify meta data, set student passed

We have two possibilities how to solve this

  1. Store in the server, which 'Main-Role' (Lecturer, Editor, Student) is allowed to do what
  2. Have fine grained roles in the keycloak
  • i.e. <course_phase_uuid:view>, <course_phase_uuid:modify>, ...
  • Advantage: These roles can be associated with groups + very easy modification / adaptability
  • Problem: A course with i.e. 6 phases x 3 rights per phases x 10 Courses blows the Baerer Token up pretty quickly.

Key changes include:

Keycloak Integration:

  • Added Keycloak middleware for route protection and role-based access control in server/course/router.go. [1] [2]
  • Implemented Keycloak client initialization and role/group management functions in server/keycloak package. [1] [2] [3] [4] [5]
  • Introduced Keycloak-related dependencies in server/go.mod. [1] [2] [3] [4]

Service Layer:

  • Added transaction handling and Keycloak role creation in CreateCourse function in server/course/service.go. [1] [2]

@@ -100,7 +102,7 @@ func createCourse(c *gin.Context) {
return
}

course, err := CreateCourse(c, newCourse)
course, err := CreateCourse(c, newCourse, userID)
if err != nil {
handleError(c, http.StatusInternalServerError, err)
Copy link
Member

Choose a reason for hiding this comment

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

I would not return the plain error here. This may leak information to the enduser/attacker. This applies to most errors in this file here.

I would recommend to log the error and return a more generic one

}

roleString := fmt.Sprintf("%s-%s-Lecturer", createdCourse.Name, createdCourse.SemesterTag.String)
err = CourseServiceSingleton.addUserToGroup(ctx, requesterID, roleString)
Copy link
Member

Choose a reason for hiding this comment

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

What happens to the database object when this call fails? It should be rolled back, otherwise we have a database entry without its respective roles

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is rolled back, as it happens within a transaction. If an error occurs, the function exits early and the transaction is never committed.

Copy link
Member

Choose a reason for hiding this comment

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

I highly doubt that this applies to actions which are executed on the Keycloak Groups and roles. Which my comment was actually referring to

@niclasheun niclasheun merged commit de39b1f into main Dec 18, 2024
5 of 6 checks passed
@niclasheun niclasheun deleted the keycloak-roles branch December 18, 2024 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants