-
Notifications
You must be signed in to change notification settings - Fork 559
fix(auth): set organisation from job token when present #1934
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
base: develop
Are you sure you want to change the base?
Conversation
WalkthroughThe change updates the authentication process in the basic middleware by directly setting the organization ID from the job token instead of using a default value. Additionally, a logging statement has been introduced to record the job token verification details, including the organization ID and access level, thereby modifying the control flow in token processing. Changes
Sequence Diagram(s)sequenceDiagram
participant R as Request
participant A as HttpBasicApiAuth
participant J as JobToken
participant L as Logger
R->>A: HTTP request with job token
A->>J: Retrieve job token details
A->>A: Set context with job token's OrganisationID
A->>L: Log verification (OrganisationID & Access Level)
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
Addressed the organization ID mismatch in HTTP Basic Auth middleware by setting the ID from the job token rather than the default organization, though the job token branch may not be properly continuing the middleware chain.
- Modified
/backend/middleware/basic.go
to assign the correct organization ID using the job token. - Bug fix resolves policy lookup discrepancies caused by using the default org ID.
- Noted potential issue: the job token branch omits a call to
c.Next()
, which might interrupt request processing.
💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!
1 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/middleware/basic.go (2)
87-88
: Unnecessary return statement.The
return
statement on line 87 is superfluous since it's the last statement in the function and will be reached naturally when the function completes.- return
46-89
: Fixed access policy issue but consider improving variable naming.The core fix aligns with the PR objective of setting the organization ID from the job token rather than defaulting to the previously established organization ID. This resolves the authentication issue when using the access policy feature via API.
However, I noticed that variable names use both "organisation" (British English) and "organization" (American English) spellings inconsistently:
- The constant is
ORGANISATION_ID_KEY
(British)- The field is
jobToken.OrganisationID
(British)- But PR description uses "organization" (American)
For better code consistency, consider standardizing on one spelling throughout the codebase in a future PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/middleware/basic.go
(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
backend/middleware/basic.go (1)
backend/middleware/jwt.go (3)
ORGANISATION_ID_KEY
(326-326)ACCESS_LEVEL_KEY
(329-329)JOB_TOKEN_KEY
(330-330)
🔇 Additional comments (2)
backend/middleware/basic.go (2)
71-71
: Organization ID now set from job token directly.This change fixes the core issue where the organization ID was previously taken from the default organization, causing a mismatch. Now it correctly uses the organization ID from the verified job token.
74-74
: Good addition of debug logging.Adding this detailed log message will be helpful for troubleshooting authentication issues as it provides visibility into the job token verification process.
I discovered this bug while trying to use the access policy feature on the community edition.
When setting the policy through the api, all goes fine, but there is a mismatch between logged in org id and passed org when the github action is trying to retrieve the policy via its job-token.
The problem comes from this condition:
digger/backend/controllers/policies.go
Line 107 in 71aee64
This PR fixes it by making sure ORGANISATION_ID_KEY get set to the org id coming from the token, rather than the default org id
Summary by CodeRabbit