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

Merged
merged 1 commit into from
Jan 16, 2025

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
avinal
avinal previously requested changes Dec 18, 2024
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 5 times, most recently from 8541110 to 3f5dc62 Compare December 20, 2024 09:14
@deo002 deo002 requested review from GMishx and avinal December 20, 2024 09:21
@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 :-)

@deo002 deo002 requested a review from GMishx January 2, 2025 09:15
@GMishx
Copy link
Member

GMishx commented Jan 16, 2025

Tested, working as expected.

Just need to remove the unique constraint on the display_name field.

@GMishx GMishx removed the needs test label Jan 16, 2025
@deo002 deo002 requested review from GMishx and removed request for GMishx January 16, 2025 08:09
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.

LGTM

@GMishx GMishx dismissed avinal’s stale review January 16, 2025 09:34

Requested changes done.

@GMishx GMishx merged commit 556965f into fossology:main Jan 16, 2025
7 checks passed
@GMishx GMishx deleted the feat/auth_1 branch January 16, 2025 09:34
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.

3 participants