Skip to content

feat: fetch line email - #4

Draft
lipowen wants to merge 1 commit into
DMCPartners:masterfrom
kaikhq:feature/fetch-email
Draft

lipowen wants to merge 1 commit into
DMCPartners:masterfrom
kaikhq:feature/fetch-email

Conversation

@lipowen

@lipowen lipowen commented Feb 4, 2023

Copy link
Copy Markdown

No description provided.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

The new email fetch path needs correct scope, safer JSON parsing, and more robust HTTP/error handling to avoid breaking authentication flows.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR extends the OmniAuth::Strategies::Line strategy to include a user email in the returned OmniAuth info hash by verifying the id_token against LINE’s /oauth2/v2.1/verify endpoint and merging the resulting email into raw_info.

Changes:

  • Add email to the OmniAuth info hash.
  • Implement an email helper that calls LINE’s token verification endpoint and parses the returned email.
  • Update raw_info memoization to inject the fetched email into the cached profile payload.
File summaries
File Description
lib/omniauth/strategies/line.rb Adds email retrieval/merging into raw_info and exposes it via info[:email].
Review details

Suppressed comments (1)

lib/omniauth/strategies/line.rb:54

  • Avoid JSON.load when parsing remote/untrusted JSON, since it can enable object creation via create_additions in some configurations. JSON.parse is safer for HTTP responses.
        @raw_info = JSON.load(access_token.get('v2/profile').body)
  • Files reviewed: 1/1 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines 26 to 30
name: raw_info['displayName'],
image: raw_info['pictureUrl'],
description: raw_info['statusMessage']
description: raw_info['statusMessage'],
email: raw_info["email"]
}
Comment on lines +33 to +41
def email
params = {
id_token: @id_token,
client_id: client.id
}

response = Net::HTTP.post_form(URI("https://api.line.me/oauth2/v2.1/verify"), params)
JSON.load(response.body)["email"]
end

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

The new email-fetching path can raise/break callbacks and introduces security/compatibility issues (e.g., JSON.load on remote data and present? without an ActiveSupport dependency).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

lib/omniauth/strategies/line.rb:40

  • email can raise and break the callback: it posts even when @id_token is nil/empty, doesn’t check for non-success HTTP responses, and uses JSON.load on a remote response body. It also relies on Net::HTTP/URI without ensuring the stdlib is loaded. Consider making this method resilient and returning nil on failure.
        response = Net::HTTP.post_form(URI("https://api.line.me/oauth2/v2.1/verify"), params)
        JSON.load(response.body)["email"]

lib/omniauth/strategies/line.rb:56

  • present? is an ActiveSupport extension and isn’t provided by Ruby stdlib or this gem’s declared dependencies, so this can raise NoMethodError in non-Rails consumers. Also, JSON.load on remote data is riskier than JSON.parse (can enable object creation via json_class).
        return @raw_info if @raw_info.present?

        @raw_info = JSON.load(access_token.get('v2/profile').body)
        @raw_info["email"] = email
        @raw_info
  • Files reviewed: 1/1 changed files
  • Comments generated: 3
  • Review effort level: Lite


extra do
hash = {}
hash[:id_token] = access_token['id_token'] if access_token['id_token'].present?
Comment on lines +61 to +68
def build_access_token
verifier = request.params["code"]
get_token_params = {:redirect_uri => callback_url}.merge(token_params.to_hash(:symbolize_keys => true))
result = client.auth_code.get_token(verifier, get_token_params, deep_symbolize(options.auth_token_params))
@id_token = result.params["id_token"]

return result
end
Comment on lines 24 to 30
info do
{
name: raw_info['displayName'],
image: raw_info['pictureUrl'],
description: raw_info['statusMessage']
description: raw_info['statusMessage'],
email: raw_info["email"]
}

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

The current implementation introduces runtime-breaking present? calls (ActiveSupport dependency), plus brittle email fetching/error handling that can break the auth callback and lacks test coverage for the new email behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (4)

lib/omniauth/strategies/line.rb:41

  • email performs a network call without requiring net/http/uri, uses JSON.load, and has no guardrails for missing id_token, non-2xx responses, or JSON parse failures—any of which can raise and break the auth callback. It also always calls the verify endpoint even when email scope isn’t requested, adding latency for most installs.
      def email
        params = {
          id_token: @id_token,
          client_id: client.id
        }

        response = Net::HTTP.post_form(URI("https://api.line.me/oauth2/v2.1/verify"), params)
        JSON.load(response.body)["email"]
      end

lib/omniauth/strategies/line.rb:47

  • present? is an ActiveSupport extension and will raise NoMethodError in non-Rails usage (this gem doesn’t depend on ActiveSupport). Use a plain Ruby emptiness check when deciding whether to expose :id_token.
        hash = {}
        hash[:id_token] = access_token['id_token'] if access_token['id_token'].present?

        hash

lib/omniauth/strategies/line.rb:55

  • raw_info uses present? (ActiveSupport-only) and JSON.load (less safe than JSON.parse). This can crash in non-Rails apps and also enables JSON object deserialization if JSON additions are enabled.
      def raw_info
        return @raw_info if @raw_info.present?

        @raw_info = JSON.load(access_token.get('v2/profile').body)
        @raw_info["email"] = email

lib/omniauth/strategies/line.rb:68

  • build_access_token re-implements omniauth-oauth2 token exchange logic, increasing drift risk vs upstream. Prefer calling super and only extracting/storing the id_token.
      def build_access_token
        verifier = request.params["code"]
        get_token_params = {:redirect_uri => callback_url}.merge(token_params.to_hash(:symbolize_keys => true))
        result = client.auth_code.get_token(verifier, get_token_params, deep_symbolize(options.auth_token_params))
        @id_token = result.params["id_token"]

        return result
      end
  • Files reviewed: 1/1 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment on lines 24 to 30
info do
{
name: raw_info['displayName'],
image: raw_info['pictureUrl'],
description: raw_info['statusMessage']
description: raw_info['statusMessage'],
email: raw_info["email"]
}
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.

2 participants