Skip to content
This repository has been archived by the owner on Feb 12, 2020. It is now read-only.

[Feature] Add JWT support #27

Merged
merged 18 commits into from
Aug 26, 2019
Merged

[Feature] Add JWT support #27

merged 18 commits into from
Aug 26, 2019

Conversation

freddy5566
Copy link
Member

@freddy5566 freddy5566 commented Aug 25, 2019

Description

as title add jwt support for auth management
Fixes #13

Type of Change

  • Bug fix (Issue / Bug fixed | Non-breaking)
  • Feature (New feature | Non-breaking)
  • Breaking Change
  • Refactor
  • add jwt middleware
  • add infrastructure layer for communicate between outside world and our app

Test Cases

  1. run app in local
  2. send post request with form studentID and password
  3. it will get token then copy it to jwt and check out the result
  4. send get request with query string targetStudentID year semester to get courses
  5. send get request with query string targetStudentID to get curriculum semester
  • /login
  • /auth/curriculums/courses
  • /auth/curriculums/semesters

@freddy5566 freddy5566 added the feature New feature or request label Aug 25, 2019
app.go Outdated Show resolved Hide resolved
Copy link
Contributor

@DevilTea DevilTea left a comment

Choose a reason for hiding this comment

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

It looks very good.

Copy link

@thelebdev thelebdev left a comment

Choose a reason for hiding this comment

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

You should note that documenting your functions is an extremely important step in your code. Especially in Golang since the syntax isn't as readable as other languages. You can take Go's own documentation as reference and keep that in mind.

authMiddleware, err := middleware.NewAuthMiddleware()
if err != nil {
c.Status(500)
log.Fatal("JWT Error:" + err.Error())

Choose a reason for hiding this comment

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

log.Fatal will crash your code. According to their documentation, it will log.Print then it will os.Exit(1). You shouldn't crash your code in the case of a failed Middleware Authentication

Copy link
Member Author

Choose a reason for hiding this comment

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

so in here I just use the regular print?

Choose a reason for hiding this comment

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

Your common log.Printf will work, and whenever you purposefully want to break the execution of your code, use panic or os.Exit(1)

Copy link
Member Author

Choose a reason for hiding this comment

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

I've fix it with regular print

result, err := loginController.Login()

if err != nil {
log.Panicln("failed to fetch login cookie")

Choose a reason for hiding this comment

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

log.Panicln will print your message then call the panic function, which, in summary, will also crash your program. I don't see the need to crash the program in the case of any malfunctions occurring in the login function

Copy link
Member Author

Choose a reason for hiding this comment

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

same as here, do I just use the regular print? Is there any rule that I have to fellow.

Choose a reason for hiding this comment

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

Yep. Pretty much the same as the previous comment.
I think for the meantime this is okay, but we will have to tackle it in the Issue concerning Error Handling.

@freddy5566
Copy link
Member Author

@thelebdev I will open another issue to document this project. Is there any detail that I gotta know?

@thelebdev
Copy link

@jamfly we can open the issue for now and then we'll go through the details. Maybe adopt some sort of convention

@freddy5566
Copy link
Member Author

@thelebdev I've opened issue #28, could you mend up some detail in that issue? I believe we need some sort of function/interface/struct doc convention.

@freddy5566 freddy5566 merged commit bc34be3 into master Aug 26, 2019
@freddy5566 freddy5566 deleted the feature/jwt-support branch August 26, 2019 07:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Add JWT support
4 participants