投稿者 魔界の仮面弁士  (社会人) 投稿日時 2014/6/10 21:21:16
> 指摘通りに、Textbox1に「' OR 1 = 1 -- 」、Textbox2はデータベース上に無い値を入力してもログイン出来るようになっていました。

その問題もありますが、そもそも今のコードだと、
ユーザー名が「McDonald's」だった場合にエラーになります。
TextBox1=「'」TextBox2=「」でもダメでしょうね。

問題となっているのは「'」の文字なわけですが、この場合には、
  sql = "SELECT COUNT(ユーザーID) FROM ユーザー情報" _
     & " WHERE ユーザーID='" & Replace(id, "'""''") & "' AND パスワード='" & Replace(pass, "'""''") & "'"

のように処理することで、こうした問題を回避できます。

なお、こうしたパラメータを SqlCommand の Parameters 経由で渡すようにすれば、
データ型のチェックも行えるため、より安全・確実となります。



>>Sub Mainをスタートアップした後にログインフォームを呼ぶ。
> Sum Mainの方でログインフォームを呼ぶ設定に修正しましたが、扱いやすい理由がピンと来ません。
Sum ではなく Sub ですね。


> メインフォーム経由のログインフォームより、ログインフォームの方が良いのでは無いかと思います。
メインフォームをスタートアップにするのも、
ログインフォームをスタートアップにするのも、
どちらも適切ではない、と思います。(不正解ではありませんが)

私の提案は、いずれかのフォームをスタートアップにするのではなく、
フォーム非依存の Main プロシージャから始めるという処理方法です。

http://msdn.microsoft.com/ja-jp/library/ms235406.aspx
http://www.atmarkit.co.jp/fdotnet/dotnettips/524vb2005main/vb2005main.html
http://dobon.net/vb/dotnet/programing/makeentrypoint.html


というのも、現状のコードだと、Show/Hide/DialogResult/Close の使い分けが
適切ではないように思ったからです。

-------
現行のコードでは、まず最初にログインフォームを起動させていますよね。
ログインが完了したら、ログインフォームは不要なため、画面から消します。

ここまでは OK ですが、そのためにログインフォームを Hide しているのが
なんだか不自然に思えたのです。ログイン画面を度々「再表示」する必要が
あるのなら Hide を使うこともあるでしょうが、今回はそうでは無いですよね。

現在の UserID や UserName を管理しているという事情があるにしても、
提示されたコードでは、それらが Shared で宣言されているのですから、
ログインフォームをいつまでも残しておく必然性は無いはずです。

にも関わらず、Hide を使わざるを得なかったのは、ログインフォームが
スタートアップになっていたが故の弊害であろうと予測しました。

終了直前に、わざわざ Show → Close という「一瞬表示させてから閉じる」
という不自然な実装になっているのも同様の理由。

スタートアップフォームを閉じるとアプリが終了してしまうという問題を
どうにか回避させようと試行錯誤した結果ではないかと予想したのです。


そして何よりも、実際のメイン処理の画面となる fmBase や fMmain が、
モーダルフォームとして DialogResult で呼ばれているのが不自然です。
フォームが一つだけならば、本来はモードレスで十分なはずですから。


スタートアップの設定を見直すことで、これらの不自然さを解消できるかと。


> るきおさんが示したコードは新規でフォームを作成し、そのフォーム名を「UserInfo」にして
るきおさんは、
『IDとユーザー名を保存するためのクラスを定義します』
と書かれていますが、「フォームを作成」とは書いていないようです。


これは、『ユーザー情報を管理するためのクラス(あるいは構造体)』を
用意することを想定したものでしょう。その役目を、Form クラスに
兼任させることはできますが、それを意図したものでは無さそうです。


たとえば、「現在ログイン中のユーザー名」を管理するために、
Public Module Module1
 Public UserName As String
End Module
あるいは
といった、アプリケーション全体で利用可能な変数を用意したとします。
(クラスの Shared 変数でも可)


管理するのがユーザ名だけであれば、String 変数一つでも十分ですが、

・ログインに使うのは「ログインID」と「パスワード」だが、
 以降の画面では「ユーザーID」と「表示用ユーザー名」を利用したい。

などのように、1 つのアカウントに対して、複数の付随情報がある場合には、
その情報一つ一つに個別の変数を用意していると、管理が煩雑になってしまいます。

そこで、それらを一つの変数で管理できるよう、「UserInfoクラス」を
作成するということです。


>> もう一つは、そのIf 文を一体何のために記載しているのかです。
> 恐らくですが、
ご自身で記載されたコードなのに、その本人自らが「恐らく」といった
曖昧な理解ではまずいと思いますよ。(^^;

実装されたコード実装が正しいかどうかど、その処理をどういう目的で
記述したのかという『意図』は別物ですし。



>> その If 文の行の到達するまでの間、変数 RESULT の内容を書き換えるような
>> 処理はどこにも無いように見受けられます。だとすれば、この If 比較は、
>> 何を目的として設置されたのでしょうか。
> ユーザー名が無いデータを探していると思います。

『If ※RESULT※ IsNot Nothing Then』の部分は、
『「ユーザー名が無いデータ」が存在した場合』を
意図したコードということですね?

ということは、先の部分のコードは、下記のような意味になるのでしょうか。

Dim RESULT As Integer = 0   '初期値として「0」をセット。(……本来は String???) 
Dim SQL As String = ……          '問い合わせ用の SQL 文を作成 

'上記の SQL を実行するまえに、 
'下記の If チェックを実施する 

If ※RESULT※ IsNot Nothing Then  '←「ユーザー名が無いデータ」を探すための If 文??? 
 '「ユーザー名が無いデータ」だった場合に、下記の処理を実行。 
 FmLogin.UserID = id         '入力されたユーザーIDを保持。 
 FmLogin.UserName = RESULT   'ユーザー名として、初期値「0」をセット??? 
End If

'以下で、先の SQL を実行する 
Using conn As New SqlConnection(ConnectString)


説明された意図に従ってコメントをつけたつもりなのですが、
まだしっくり来ません…。説明された「意図」あるいは実際のコードの
いずれか(あるいは両方)が間違ってはいないでしょうか。