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

feat(oidc_auth): Add backend support for OIDC Auth #94

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

deo002
Copy link
Collaborator

@deo002 deo002 commented Dec 3, 2024

Changes

  • Added endpoints to update, delete and create user with oidc token.
  • Added role based access for admin apis
  • Changed /users/{id} endpoint to /users/{username} endpoint.
  • TODO: Add changelogs for /users endpoints
  • Set flag TranslateError in GORM Config to true to convert database-specific errors into GORM's own generalized errors: https://gorm.io/docs/error_handling.html#Dialect-Translated-Errors
  • Had to update Go from 1.20 to 1.22 because some packages required it.
  • Change JWT framework to letsstrat-go/jwx becase the JWKS extension for golang-jwt was of an incompliant license.

Submitter Checklist

  • Includes tests (if there is a feature changed/added)
  • Includes docs ( if changes are user facing)
  • I have tested my changes locally.

pkg/api/api.go Outdated Show resolved Hide resolved
pkg/auth/auth.go Outdated Show resolved Hide resolved
pkg/middleware/middleware.go Outdated Show resolved Hide resolved
pkg/auth/auth.go Outdated Show resolved Hide resolved
pkg/api/api.go Show resolved Hide resolved
pkg/middleware/middleware.go Outdated Show resolved Hide resolved
pkg/middleware/middleware.go Outdated Show resolved Hide resolved
@deo002 deo002 force-pushed the feat/auth_1 branch 2 times, most recently from 6bad174 to c2c1235 Compare December 13, 2024 08:59
@deo002 deo002 requested a review from GMishx December 13, 2024 09:17
Copy link
Member

@GMishx GMishx 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.

@deo002 deo002 force-pushed the feat/auth_1 branch 3 times, most recently from 44aa516 to 4acd525 Compare December 18, 2024 11:36
Copy link
Member

@avinal avinal left a comment

Choose a reason for hiding this comment

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

Few comments

.github/workflows/api-swagger.yml Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
cmd/laas/main.go Show resolved Hide resolved
pkg/middleware/middleware.go Outdated Show resolved Hide resolved
pkg/middleware/middleware.go Outdated Show resolved Hide resolved
@deo002 deo002 force-pushed the feat/auth_1 branch 4 times, most recently from d1cf2b3 to 8541110 Compare December 20, 2024 09:06
@GMishx
Copy link
Member

GMishx commented Dec 20, 2024

Found issues during testing:

  1. Default user role in DB is admin, the role based access middleware checks for ADMIN which fails.
    • Result: Admin user cannot perform admin tasks unless role updated in DB manually.
    • Suggested change: Make the role as enum or constant.
  2. Endpoint /users/oidc takes token as a parameter in body.
    • Result: User can imitate as a different user.
    • Suggested change: Use token only from the header.
  3. Authorization header is not parsed correctly.
    • Result: Authorization header should be Authorization: Bearer {jwt} which cannot be parsed by the middleware.
    • Suggested change: JWT token should be extracted after Bearer string from Authorization header.

Azure AD is not working at the moment, will continue further testing later :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants