-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat: add LDAP authentication module with JWT issuance #2074
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: development
Are you sure you want to change the base?
Conversation
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.
Couple of Test cases to add
- LDAP destination does not exists
- . Bind request from an unauthorised user
- Binding fail due to invalid username/pass
- Binding Failure due to expired password
- BindSuccess but info retrieval failed
- App unable to generate JWT
- Using an expired JWT
internal/auth/ldapauth/ldapauth.go
Outdated
|
||
func (a *Authenticator) Authenticate(username, password string) (string, error) { | ||
// l, err := ldap.DialURL("ldap://" + a.cfg.Addr) | ||
l, err := a.dialFn("ldap://" + a.cfg.Addr) |
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.
Could you elaborate on what l
is ?
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.
Also, was wondering what possible validations we could have for a.cfg.Addr
?
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.
l is the LDAP connection (*ldap.Conn) returned by the dialer.
We should validate a.cfg.Addr for non-empty and proper host:port format using net.SplitHostPort.
internal/auth/ldapauth/ldapauth.go
Outdated
|
||
func New(cfg Config) *Authenticator { | ||
return &Authenticator{ | ||
cfg: cfg, |
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.
Is the config specific to the LDAP library being used ?
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.
The Config struct here is not directly from the go-ldap/ldap/v3 library — it's a custom wrapper defined within this module to keep the Authenticator decoupled from external configs.
This design allows flexibility — if we change or extend the backend (e.g. add StartTLS, change port or baseDN logic), we just update our config struct without impacting the rest of the app.
// Config holds LDAP server connection details and base search parameters.
// It is application-defined and not tied to the go-ldap library.
type Config struct {
Addr string
BaseDN string
BindDN string
BindPass string
}
internal/auth/ldapauth/ldapauth.go
Outdated
searchReq := ldap.NewSearchRequest( | ||
a.cfg.BaseDN, | ||
ldap.ScopeWholeSubtree, ldap.NeverDerefAliases, 0, 0, false, | ||
fmt.Sprintf("(uid=%s)", ldap.EscapeFilter(username)), | ||
[]string{"dn"}, | ||
nil, | ||
) | ||
|
||
sr, err := l.Search(searchReq) | ||
if err != nil || len(sr.Entries) != 1 { | ||
return "", fmt.Errorf("user not found") | ||
} | ||
|
||
userDN := sr.Entries[0].DN | ||
|
||
if err := l.Bind(userDN, password); err != nil { | ||
return "", fmt.Errorf("invalid credentials") | ||
} | ||
|
||
token := jwt.NewWithClaims(jwt.SigningMethodHS256, jwt.MapClaims{ | ||
"sub": username, | ||
"exp": time.Now().Add(1 * time.Hour).Unix(), | ||
}) | ||
|
||
signed, err := token.SignedString([]byte(a.cfg.JWTSecret)) |
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.
This should preferably be decomposed into smaller, testable sections
Hi @gizmo-rt 👋
Updated main.go + test cases as requested. @gizmo-rt I’ve added the requested edge case tests. Let me know if any further changes are needed. ✅ ✅ Added all suggested edge case tests including:
🧪 All tests pass locally. PR is ready for review. Thanks! |
@Umang01-hash @coolwednesday Is there any way or command I could use to run Code Quality checks on my local machine in the same way they are executed in the pipeline? |
Hey @mundele2004! To run these code qaulity checks you can run |
✅ Resolved all |
aeb2768
to
e324976
Compare
Final PR after cleanup and linting. Ready for review 🚀 |
✅ Resolved all golangci-lint issues . |
@mundele2004 Please address all the feedback/comments on the PR - don't mark them resolved without a response/change. |
Please check — I have fixed the linter issues such as whitespace, cuddled statements, etc. **$ golangci-lint run
|
@mundele2004 You can fix gci import order issues by installing and running the gci tool.
and then use |
@Umang01-hash I tried the steps you suggested, but those 2 issues are still not resolved. Please guide me on how to fix those errors. |
@Umang01-hash Everything is working fine now. ✅ |
Please tell me what I should do to fix this: |
Please tell me what I should do to fix this: |
1 similar comment
Please tell me what I should do to fix this: |
Pull Request Template
Description:
internal/auth/ldapauth
.Bind(username, password)
authentication against an LDAP server and issues a JWT token upon successful login.Breaking Changes (if applicable):
Additional Information:
github.com/go-ldap/ldap/v3
crypto/rand
,encoding/base64
) with HMAC SHA256 signing.mainFile/main.go
for demo andtestuser.ldif
for testing against a local OpenLDAP instance.Summary
This PR introduces a standalone LDAP authentication module (
internal/auth/ldapauth
) for the GoFr framework. It usesBind(username, password)
to validate users and issues a JWT on successful login.What's Included
ldapauth.go
: Core logic for LDAP authentication and JWT issuanceldapauth_test.go
: Unit tests with mock LDAP dialermain.go
: Demo login flow using/login
routetestuser.ldif
: Sample LDAP user for testingHow to Test
testuser.ldif
:curl -X POST http://localhost:8080/login
-H "Content-Type: application/json"
-d '{"username":"testuser", "password":"testpass"}'
Checklist:
goimport
andgolangci-lint
.